From 0b3e188f06e32698a5ff439faf117d7913ca8b55 Mon Sep 17 00:00:00 2001 From: Ayush Pahwa Date: Thu, 2 Jan 2025 15:05:05 +0700 Subject: [PATCH 1/2] chore: flexible return for test payload extraction func (#38443) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description The `getTestPayloadFromCollectionData` extracts the `testPayload` from passed `collectionData` and returns an empty string if no information is present. This PR updates the function definition to allow different default value based on the function call. Fixes #37742 ## Automation /test sanity ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 26ea6e2b17c9a31e836ced4e3787a0723db63680 > Cypress dashboard. > Tags: `@tag.Sanity` > Spec: >
Wed, 01 Jan 2025 16:34:44 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit - **Bug Fixes** - Enhanced function handling for undefined collection data - Improved key iteration in action execution utilities - **Documentation** - Added JSDoc comment to clarify function behavior --- .../src/ce/utils/actionExecutionUtils.ts | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/app/client/src/ce/utils/actionExecutionUtils.ts b/app/client/src/ce/utils/actionExecutionUtils.ts index ea4a379236a5..675d31ded0ba 100644 --- a/app/client/src/ce/utils/actionExecutionUtils.ts +++ b/app/client/src/ce/utils/actionExecutionUtils.ts @@ -9,6 +9,7 @@ import { getCurrentEnvironmentDetails } from "ee/selectors/environmentSelectors" import type { Plugin } from "api/PluginApi"; import { get, isNil } from "lodash"; import type { JSCollectionData } from "ee/reducers/entityReducers/jsActionsReducer"; +import { objectKeys } from "@appsmith/utils"; export function getPluginActionNameToDisplay(action: Action) { return action.name; @@ -20,7 +21,7 @@ export const getActionProperties = ( ) => { const actionProperties: Record = {}; - Object.keys(keyConfig).forEach((key) => { + objectKeys(keyConfig).forEach((key) => { const value = get(action, key); if (!isNil(value)) { @@ -69,7 +70,7 @@ export function getActionExecutionAnalytics( datasourceId: datasourceId, isMock: !!datasource?.isMock, actionId: action?.id, - inputParams: Object.keys(params).length, + inputParams: objectKeys(params).length, source: ActionExecutionContext.EVALUATION_ACTION_TRIGGER, // Used in analytic events to understand who triggered action execution }; @@ -98,17 +99,24 @@ export function isBrowserExecutionAllowed(..._args: any[]) { return true; } -// Function to extract the test payload from the collection data +/** + * Function to extract the test payload from the collection data + * @param [collectionData] from the js Object + * @param [defaultValue=""] to be returned if no information is found, + * returns an empty string by default + * @returns stored value from the collectionData + * */ export const getTestPayloadFromCollectionData = ( collectionData: JSCollectionData | undefined, + defaultValue = "", ): string => { - if (!collectionData) return ""; + if (!collectionData) return defaultValue; const activeJSActionId = collectionData?.activeJSActionId; const testPayload: Record | undefined = collectionData?.data ?.testPayload as Record; - if (!activeJSActionId || !testPayload) return ""; + if (!activeJSActionId || !testPayload) return defaultValue; - return (testPayload[activeJSActionId] as string) || ""; + return testPayload[activeJSActionId] as string; }; From 51f3d0ebdfb5a67301fdf03acaccd4cbfa9f9bbf Mon Sep 17 00:00:00 2001 From: Nidhi Date: Thu, 2 Jan 2025 14:56:50 +0530 Subject: [PATCH 2/2] chore: Added support for artifact type in git redis utils (#38418) --- .../server/constants/ArtifactType.java | 6 +- .../appsmith/server/git/GitRedisUtils.java | 49 +++++++--- .../git/central/CentralGitServiceCEImpl.java | 96 ++++++++++++------- .../server/git/fs/GitFSServiceCEImpl.java | 63 ++---------- 4 files changed, 109 insertions(+), 105 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ArtifactType.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ArtifactType.java index a9c4df2ce48d..3075ad27e65e 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ArtifactType.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ArtifactType.java @@ -7,5 +7,9 @@ public enum ArtifactType { APPLICATION, PACKAGE, - WORKFLOW + WORKFLOW; + + public String lowerCaseName() { + return this.name().toLowerCase(); + } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java index e0d61cdc192b..40e903568f8c 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java @@ -1,6 +1,7 @@ package com.appsmith.server.git; import com.appsmith.external.git.constants.GitSpan; +import com.appsmith.server.constants.ArtifactType; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; import com.appsmith.server.helpers.RedisUtils; @@ -26,17 +27,18 @@ public class GitRedisUtils { /** * Adds a baseArtifact id as a key in redis, the presence of this key represents a symbolic lock, essentially meaning that no new operations * should be performed till this key remains present. - * @param baseArtifactId : base id of the artifact for which the key is getting added. - * @param commandName : Name of the operation which is trying to acquire the lock, this value will be added against the key + * + * @param key : base id of the artifact for which the key is getting added. + * @param commandName : Name of the operation which is trying to acquire the lock, this value will be added against the key * @param isRetryAllowed : Boolean for whether retries for adding the value is allowed * @return a boolean publisher for the added file locks */ - public Mono addFileLock(String baseArtifactId, String commandName, Boolean isRetryAllowed) { + public Mono addFileLock(String key, String commandName, Boolean isRetryAllowed) { long numberOfRetries = Boolean.TRUE.equals(isRetryAllowed) ? MAX_RETRIES : 0L; - log.info("Git command {} is trying to acquire the lock for application id {}", commandName, baseArtifactId); + log.info("Git command {} is trying to acquire the lock for identity {}", commandName, key); return redisUtils - .addFileLock(baseArtifactId, commandName) + .addFileLock(key, commandName) .retryWhen(Retry.fixedDelay(numberOfRetries, RETRY_DELAY) .onRetryExhaustedThrow((retryBackoffSpec, retrySignal) -> { if (retrySignal.failure() instanceof AppsmithException) { @@ -49,13 +51,16 @@ public Mono addFileLock(String baseArtifactId, String commandName, Bool .tap(Micrometer.observation(observationRegistry)); } - public Mono addFileLock(String defaultApplicationId, String commandName) { - return addFileLock(defaultApplicationId, commandName, true); + public Mono addFileLock(String baseArtifactId, String commandName) { + String key = generateRedisKey(ArtifactType.APPLICATION, baseArtifactId); + return addFileLock(key, commandName, true); } - public Mono releaseFileLock(String defaultApplicationId) { + public Mono releaseFileLock(String baseArtifactId) { + String key = generateRedisKey(ArtifactType.APPLICATION, baseArtifactId); + return redisUtils - .releaseFileLock(defaultApplicationId) + .releaseFileLock(key) .name(GitSpan.RELEASE_FILE_LOCK) .tap(Micrometer.observation(observationRegistry)); } @@ -64,33 +69,47 @@ public Mono releaseFileLock(String defaultApplicationId) { * This is a wrapper method for acquiring git lock, since multiple ops are used in sequence * for a complete composite operation not all ops require to acquire the lock hence a dummy flag is sent back for * operations in that is getting executed in between + * + * @param artifactType * @param baseArtifactId : id of the base artifact for which ops would be locked * @param isLockRequired : is lock really required or is it a proxy function * @return : Boolean for whether the lock is acquired */ - // TODO @Manish add artifactType reference in incoming prs. - public Mono acquireGitLock(String baseArtifactId, String commandName, Boolean isLockRequired) { + public Mono acquireGitLock( + ArtifactType artifactType, String baseArtifactId, String commandName, Boolean isLockRequired) { if (!Boolean.TRUE.equals(isLockRequired)) { return Mono.just(Boolean.TRUE); } - return addFileLock(baseArtifactId, commandName); + String key = generateRedisKey(artifactType, baseArtifactId); + + return addFileLock(key, commandName, true); } /** * This is a wrapper method for releasing git lock, since multiple ops are used in sequence * for a complete composite operation not all ops require to acquire the lock hence a dummy flag is sent back for * operations in that is getting executed in between + * + * @param artifactType * @param baseArtifactId : id of the base artifact for which ops would be locked * @param isLockRequired : is lock really required or is it a proxy function * @return : Boolean for whether the lock is released */ - // TODO @Manish add artifactType reference in incoming prs - public Mono releaseFileLock(String baseArtifactId, boolean isLockRequired) { + public Mono releaseFileLock(ArtifactType artifactType, String baseArtifactId, boolean isLockRequired) { if (!Boolean.TRUE.equals(isLockRequired)) { return Mono.just(Boolean.TRUE); } - return releaseFileLock(baseArtifactId); + String key = generateRedisKey(artifactType, baseArtifactId); + + return redisUtils + .releaseFileLock(key) + .name(GitSpan.RELEASE_FILE_LOCK) + .tap(Micrometer.observation(observationRegistry)); + } + + private String generateRedisKey(ArtifactType artifactType, String artifactId) { + return artifactType.lowerCaseName() + "-" + artifactId; } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java index 460be19c7813..970860a9b268 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java @@ -394,13 +394,16 @@ protected Mono checkoutReference( String baseArtifactId = baseGitMetadata.getDefaultArtifactId(); final String finalRefName = gitRefDTO.getRefName().replaceFirst(ORIGIN, REMOTE_NAME_REPLACEMENT); + ArtifactType artifactType = baseArtifact.getArtifactType(); - GitArtifactHelper gitArtifactHelper = - gitArtifactHelperResolver.getArtifactHelper(baseArtifact.getArtifactType()); + GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); GitHandlingService gitHandlingService = gitHandlingServiceResolver.getGitHandlingService(gitType); Mono acquireFileLock = gitRedisUtils.acquireGitLock( - baseArtifactId, GitConstants.GitCommandConstants.CHECKOUT_BRANCH, addFileLock); + baseArtifact.getArtifactType(), + baseArtifactId, + GitConstants.GitCommandConstants.CHECKOUT_BRANCH, + addFileLock); Mono checkedOutArtifactMono; // If the user is trying to check out remote reference, create a new reference if it does not exist already @@ -445,12 +448,12 @@ protected Mono checkoutReference( return acquireFileLock .then(checkedOutArtifactMono) .flatMap(checkedOutArtifact -> gitRedisUtils - .releaseFileLock(baseArtifactId, addFileLock) + .releaseFileLock(artifactType, baseArtifactId, addFileLock) .thenReturn(checkedOutArtifact)) .onErrorResume(error -> { log.error("An error occurred while checking out the reference. error {}", error.getMessage()); return gitRedisUtils - .releaseFileLock(baseArtifactId, addFileLock) + .releaseFileLock(artifactType, baseArtifactId, addFileLock) .then(Mono.error(error)); }) .tag(GitConstants.GitMetricConstants.CHECKOUT_REMOTE, FALSE.toString()) @@ -581,9 +584,9 @@ protected Mono createReference( GitArtifactMetadata baseGitMetadata = baseArtifact.getGitArtifactMetadata(); GitAuth baseGitAuth = baseGitMetadata.getGitAuth(); GitArtifactMetadata sourceGitMetadata = sourceArtifact.getGitArtifactMetadata(); + ArtifactType artifactType = baseArtifact.getArtifactType(); - GitArtifactHelper gitArtifactHelper = - gitArtifactHelperResolver.getArtifactHelper(baseArtifact.getArtifactType()); + GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); GitHandlingService gitHandlingService = gitHandlingServiceResolver.getGitHandlingService(gitType); ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO(); @@ -604,7 +607,10 @@ protected Mono createReference( } Mono acquireGitLockMono = gitRedisUtils.acquireGitLock( - baseGitMetadata.getDefaultArtifactId(), GitConstants.GitCommandConstants.CREATE_BRANCH, FALSE); + artifactType, + baseGitMetadata.getDefaultArtifactId(), + GitConstants.GitCommandConstants.CREATE_BRANCH, + FALSE); Mono fetchRemoteMono = gitHandlingService.fetchRemoteChanges(jsonTransformationDTO, baseGitAuth, TRUE); Mono createBranchMono = acquireGitLockMono @@ -675,7 +681,9 @@ protected Mono createReference( }) .flatMap(newImportedArtifact -> gitRedisUtils .releaseFileLock( - newImportedArtifact.getGitArtifactMetadata().getDefaultArtifactId(), TRUE) + artifactType, + newImportedArtifact.getGitArtifactMetadata().getDefaultArtifactId(), + TRUE) .then(gitAnalyticsUtils.addAnalyticsForGitOperation( AnalyticsEvents.GIT_CREATE_BRANCH, newImportedArtifact, @@ -683,7 +691,7 @@ protected Mono createReference( .onErrorResume(error -> { log.error("An error occurred while creating reference. error {}", error.getMessage()); return gitRedisUtils - .releaseFileLock(baseGitMetadata.getDefaultArtifactId(), TRUE) + .releaseFileLock(artifactType, baseGitMetadata.getDefaultArtifactId(), TRUE) .then(Mono.error(new AppsmithException(AppsmithError.GIT_ACTION_FAILED, "checkout"))); }) .name(GitSpan.OPS_CREATE_BRANCH) @@ -747,10 +755,10 @@ protected Mono deleteGitReference( GitArtifactMetadata baseGitMetadata = baseArtifact.getGitArtifactMetadata(); GitArtifactMetadata referenceArtifactMetadata = referenceArtifact.getGitArtifactMetadata(); + ArtifactType artifactType = baseArtifact.getArtifactType(); GitHandlingService gitHandlingService = gitHandlingServiceResolver.getGitHandlingService(gitType); - GitArtifactHelper gitArtifactHelper = - gitArtifactHelperResolver.getArtifactHelper(baseArtifact.getArtifactType()); + GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); // TODO: write a migration to shift everything to refName in gitMetadata final String finalRefName = referenceArtifactMetadata.getRefName(); @@ -766,7 +774,7 @@ protected Mono deleteGitReference( .flatMap(isBranchProtected -> { if (!TRUE.equals(isBranchProtected)) { return gitRedisUtils.acquireGitLock( - baseArtifactId, GitConstants.GitCommandConstants.DELETE, TRUE); + artifactType, baseArtifactId, GitConstants.GitCommandConstants.DELETE, TRUE); } return Mono.error(new AppsmithException( @@ -786,7 +794,7 @@ protected Mono deleteGitReference( return gitHandlingService .deleteGitReference(jsonTransformationDTO) .flatMap(isReferenceDeleted -> gitRedisUtils - .releaseFileLock(baseArtifactId, TRUE) + .releaseFileLock(artifactType, baseArtifactId, TRUE) .thenReturn(isReferenceDeleted)) .flatMap(isReferenceDeleted -> { if (FALSE.equals(isReferenceDeleted)) { @@ -829,7 +837,9 @@ protected Mono deleteGitReference( referenceArtifactMetadata.getRefName(), baseArtifactId); - return gitRedisUtils.releaseFileLock(baseArtifactId, TRUE).then(Mono.error(error)); + return gitRedisUtils + .releaseFileLock(artifactType, baseArtifactId, TRUE) + .then(Mono.error(error)); }) .name(GitSpan.OPS_DELETE_BRANCH) .tap(Micrometer.observation(observationRegistry)); @@ -1190,8 +1200,8 @@ private Mono commitArtifact( boolean isSystemGenerated = commitDTO.getMessage().contains(DEFAULT_COMMIT_MESSAGE); - GitArtifactHelper gitArtifactHelper = - gitArtifactHelperResolver.getArtifactHelper(baseArtifact.getArtifactType()); + ArtifactType artifactType = baseArtifact.getArtifactType(); + GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); GitHandlingService gitHandlingService = gitHandlingServiceResolver.getGitHandlingService(gitType); GitArtifactMetadata baseGitMetadata = baseArtifact.getGitArtifactMetadata(); GitArtifactMetadata branchedGitMetadata = branchedArtifact.getGitArtifactMetadata(); @@ -1214,6 +1224,7 @@ private Mono commitArtifact( .flatMap(isBranchProtected -> { if (!TRUE.equals(isBranchProtected)) { return gitRedisUtils.acquireGitLock( + artifactType, baseGitMetadata.getDefaultArtifactId(), GitConstants.GitCommandConstants.COMMIT, isFileLock); @@ -1309,7 +1320,7 @@ private Mono commitArtifact( .commitArtifact(updatedBranchedArtifact, commitDTO, jsonTransformationDTO) .onErrorResume(error -> { return gitRedisUtils - .releaseFileLock(baseArtifact.getId(), TRUE) + .releaseFileLock(artifactType, baseArtifact.getId(), TRUE) .then(gitAnalyticsUtils.addAnalyticsForGitOperation( AnalyticsEvents.GIT_COMMIT, updatedBranchedArtifact, @@ -1328,7 +1339,9 @@ private Mono commitArtifact( String status = tuple.getT1(); Artifact artifactFromBranch = tuple.getT2(); Mono releaseFileLockMono = gitRedisUtils.releaseFileLock( - artifactFromBranch.getGitArtifactMetadata().getDefaultArtifactId(), isFileLock); + artifactType, + artifactFromBranch.getGitArtifactMetadata().getDefaultArtifactId(), + isFileLock); Mono updatedArtifactMono = gitArtifactHelper.updateArtifactWithSchemaVersions(artifactFromBranch); @@ -1352,7 +1365,7 @@ private Mono commitArtifact( branchedGitMetadata.getBranchName()); return gitRedisUtils - .releaseFileLock(branchedGitMetadata.getDefaultArtifactId(), TRUE) + .releaseFileLock(artifactType, branchedGitMetadata.getDefaultArtifactId(), TRUE) .then(Mono.error(error)); }); @@ -1514,7 +1527,8 @@ protected Mono getStatus( Mono statusMono = exportedArtifactJsonMono .flatMap(artifactExchangeJson -> { return gitRedisUtils - .acquireGitLock(baseArtifactId, GitConstants.GitCommandConstants.STATUS, isFileLock) + .acquireGitLock( + artifactType, baseArtifactId, GitConstants.GitCommandConstants.STATUS, isFileLock) .thenReturn(artifactExchangeJson); }) .flatMap(artifactExchangeJson -> { @@ -1545,7 +1559,7 @@ protected Mono getStatus( .getStatus(jsonTransformationDTO) .flatMap(gitStatusDTO -> { return gitRedisUtils - .releaseFileLock(baseArtifactId, isFileLock) + .releaseFileLock(artifactType, baseArtifactId, isFileLock) .thenReturn(gitStatusDTO); }); }); @@ -1621,15 +1635,20 @@ protected Mono pullArtifact(Artifact baseArtifact, Artifact branched GitArtifactMetadata branchedGitMetadata = branchedArtifact.getGitArtifactMetadata(); Mono statusMono = getStatus(baseArtifact, branchedArtifact, false, true, gitType); + ArtifactType artifactType = baseArtifact.getArtifactType(); Mono pullDTOMono = gitRedisUtils - .acquireGitLock(branchedGitMetadata.getDefaultArtifactId(), GitConstants.GitCommandConstants.PULL, TRUE) + .acquireGitLock( + artifactType, + branchedGitMetadata.getDefaultArtifactId(), + GitConstants.GitCommandConstants.PULL, + TRUE) .then(Mono.defer(() -> statusMono)) .flatMap(status -> { // Check if the repo is clean if (!CollectionUtils.isEmpty(status.getModified())) { return gitRedisUtils - .releaseFileLock(branchedGitMetadata.getDefaultArtifactId(), TRUE) + .releaseFileLock(artifactType, branchedGitMetadata.getDefaultArtifactId(), TRUE) .then( Mono.error( new AppsmithException( @@ -1641,7 +1660,7 @@ protected Mono pullArtifact(Artifact baseArtifact, Artifact branched return pullAndRehydrateArtifact(baseArtifact, branchedArtifact, gitType) // Release file lock after the pull operation .flatMap(gitPullDTO -> gitRedisUtils - .releaseFileLock(branchedGitMetadata.getDefaultArtifactId(), TRUE) + .releaseFileLock(artifactType, branchedGitMetadata.getDefaultArtifactId(), TRUE) .then(Mono.just(gitPullDTO))); }) .onErrorResume(error -> { @@ -1651,7 +1670,7 @@ protected Mono pullArtifact(Artifact baseArtifact, Artifact branched branchedGitMetadata.getBranchName()); return gitRedisUtils - .releaseFileLock(branchedGitMetadata.getDefaultArtifactId(), TRUE) + .releaseFileLock(artifactType, branchedGitMetadata.getDefaultArtifactId(), TRUE) .then(Mono.error(error)); }) .name(GitSpan.OPS_PULL) @@ -1764,6 +1783,7 @@ public Mono fetchRemoteChanges( GitArtifactMetadata baseArtifactGitData = baseArtifact.getGitArtifactMetadata(); GitArtifactMetadata refArtifactGitData = refArtifact.getGitArtifactMetadata(); + ArtifactType artifactType = baseArtifact.getArtifactType(); String baseArtifactId = baseArtifactGitData.getDefaultArtifactId(); @@ -1773,8 +1793,8 @@ public Mono fetchRemoteChanges( } Mono currUserMono = sessionUserService.getCurrentUser().cache(); // will be used to send analytics event - Mono acquireGitLockMono = - gitRedisUtils.acquireGitLock(baseArtifactId, GitConstants.GitCommandConstants.FETCH_REMOTE, isFileLock); + Mono acquireGitLockMono = gitRedisUtils.acquireGitLock( + artifactType, baseArtifactId, GitConstants.GitCommandConstants.FETCH_REMOTE, isFileLock); ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO(); jsonTransformationDTO.setWorkspaceId(baseArtifact.getWorkspaceId()); @@ -1792,7 +1812,7 @@ public Mono fetchRemoteChanges( jsonTransformationDTO, baseArtifactGitData.getGitAuth(), FALSE))) .flatMap(fetchedRemoteStatusString -> { return gitRedisUtils - .releaseFileLock(baseArtifactId, isFileLock) + .releaseFileLock(artifactType, baseArtifactId, isFileLock) .thenReturn(fetchedRemoteStatusString); }) .onErrorResume(throwable -> { @@ -1964,8 +1984,8 @@ public Mono discardChanges( protected Mono discardChanges(Artifact branchedArtifact, GitType gitType) { - GitArtifactHelper gitArtifactHelper = - gitArtifactHelperResolver.getArtifactHelper(branchedArtifact.getArtifactType()); + ArtifactType artifactType = branchedArtifact.getArtifactType(); + GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); GitHandlingService gitHandlingService = gitHandlingServiceResolver.getGitHandlingService(gitType); GitArtifactMetadata branchedGitData = branchedArtifact.getGitArtifactMetadata(); @@ -1983,13 +2003,17 @@ protected Mono discardChanges(Artifact branchedArtifact, Git jsonTransformationDTO.setRepoName(branchedGitData.getRepoName()); Mono recreatedArtifactFromLastCommit = gitRedisUtils - .acquireGitLock(branchedGitData.getDefaultArtifactId(), GitConstants.GitCommandConstants.DISCARD, TRUE) + .acquireGitLock( + artifactType, + branchedGitData.getDefaultArtifactId(), + GitConstants.GitCommandConstants.DISCARD, + TRUE) .then(gitHandlingService .recreateArtifactJsonFromLastCommit(jsonTransformationDTO) .onErrorResume(throwable -> { log.error("Git recreate ArtifactJsonFailed : {}", throwable.getMessage()); return gitRedisUtils - .releaseFileLock(branchedGitData.getDefaultArtifactId(), TRUE) + .releaseFileLock(artifactType, branchedGitData.getDefaultArtifactId(), TRUE) .then( Mono.error( new AppsmithException( @@ -2007,7 +2031,9 @@ protected Mono discardChanges(Artifact branchedArtifact, Git .flatMap(publishedArtifact -> { return gitRedisUtils .releaseFileLock( - publishedArtifact.getGitArtifactMetadata().getDefaultArtifactId(), TRUE) + artifactType, + publishedArtifact.getGitArtifactMetadata().getDefaultArtifactId(), + TRUE) .then(gitAnalyticsUtils.addAnalyticsForGitOperation( AnalyticsEvents.GIT_DISCARD_CHANGES, publishedArtifact, null)); }) @@ -2017,7 +2043,7 @@ protected Mono discardChanges(Artifact branchedArtifact, Git branchedGitData.getDefaultArtifactId(), error.getMessage()); return gitRedisUtils - .releaseFileLock(branchedGitData.getDefaultArtifactId(), TRUE) + .releaseFileLock(artifactType, branchedGitData.getDefaultArtifactId(), TRUE) .then(Mono.error(new AppsmithException(AppsmithError.GIT_ACTION_FAILED, "checkout"))); }) .name(GitSpan.OPS_DISCARD_CHANGES) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java index 84f772930dc6..d08152c44eb1 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java @@ -10,7 +10,6 @@ import com.appsmith.external.git.constants.ce.RefType; import com.appsmith.external.git.handler.FSGitHandler; import com.appsmith.git.dto.CommitDTO; -import com.appsmith.server.acl.AclPermission; import com.appsmith.server.configurations.EmailConfig; import com.appsmith.server.constants.ArtifactType; import com.appsmith.server.datasources.base.DatasourceService; @@ -106,33 +105,6 @@ public class GitFSServiceCEImpl implements GitHandlingServiceCE { private static final String ORIGIN = "origin/"; private static final String REMOTE_NAME_REPLACEMENT = ""; - private Mono addFileLock(String baseArtifactId, String commandName, boolean isLockRequired) { - if (!Boolean.TRUE.equals(isLockRequired)) { - return Mono.just(Boolean.TRUE); - } - - return Mono.defer(() -> addFileLock(baseArtifactId, commandName)); - } - - private Mono addFileLock(String baseArtifactId, String commandName) { - return gitRedisUtils.addFileLock(baseArtifactId, commandName); - } - - private Mono releaseFileLock(String baseArtifactId, boolean isLockRequired) { - if (!Boolean.TRUE.equals(isLockRequired)) { - return Mono.just(Boolean.TRUE); - } - - return releaseFileLock(baseArtifactId); - } - - private Mono releaseFileLock(String baseArtifactId) { - return gitRedisUtils - .releaseFileLock(baseArtifactId) - .name(GitSpan.RELEASE_FILE_LOCK) - .tap(Micrometer.observation(observationRegistry)); - } - @Override public Set validateGitConnectDTO(GitConnectDTO gitConnectDTO) { Set errors = new HashSet<>(); @@ -263,9 +235,6 @@ public Mono removeRepository(ArtifactJsonTransformationDTO artifactJson */ @Override public Mono> listBranches(ArtifactJsonTransformationDTO artifactJsonTransformationDTO) { - GitArtifactHelper gitArtifactHelper = - gitArtifactHelperResolver.getArtifactHelper(artifactJsonTransformationDTO.getArtifactType()); - return listBranches(artifactJsonTransformationDTO, Boolean.FALSE); } @@ -433,21 +402,6 @@ public Mono> commitArtifact( }); } - /** - * Used for pushing commits present in the given branched artifact. - * @param branchedArtifactId : id of the branched artifact. - * @param artifactType : type of the artifact - * @return : returns a string which has details of operations - */ - public Mono pushArtifact(String branchedArtifactId, ArtifactType artifactType) { - GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); - AclPermission artifactEditPermission = gitArtifactHelper.getArtifactEditPermission(); - - return gitArtifactHelper - .getArtifactById(branchedArtifactId, artifactEditPermission) - .flatMap(branchedArtifact -> pushArtifact(branchedArtifact, true)); - } - /** * Push flow for dehydrated apps * @@ -455,8 +409,8 @@ public Mono pushArtifact(String branchedArtifactId, ArtifactType artifac * @return Success message */ protected Mono pushArtifact(Artifact branchedArtifact, boolean isFileLock) { - GitArtifactHelper gitArtifactHelper = - gitArtifactHelperResolver.getArtifactHelper(branchedArtifact.getArtifactType()); + ArtifactType artifactType = branchedArtifact.getArtifactType(); + GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); Mono gitArtifactMetadataMono = Mono.just(branchedArtifact.getGitArtifactMetadata()); if (!branchedArtifact @@ -478,6 +432,7 @@ protected Mono pushArtifact(Artifact branchedArtifact, boolean isFileLoc .flatMap(gitMetadata -> { return gitRedisUtils .acquireGitLock( + artifactType, gitMetadata.getDefaultArtifactId(), GitConstants.GitCommandConstants.PUSH, isFileLock) @@ -540,8 +495,8 @@ protected Mono pushArtifact(Artifact branchedArtifact, boolean isFileLoc if (!Boolean.TRUE.equals(isFileLock)) { return Mono.just(flag); } - return Mono.defer(() -> releaseFileLock( - artifact.getGitArtifactMetadata().getDefaultArtifactId())); + return Mono.defer(() -> gitRedisUtils.releaseFileLock( + artifactType, artifact.getGitArtifactMetadata().getDefaultArtifactId(), true)); }); return pushArtifactErrorRecovery(pushStatus, artifact) @@ -676,8 +631,8 @@ public Mono createGitReference(ArtifactJsonTransformationDTO jsonTransfo @Override public Mono deleteGitReference(ArtifactJsonTransformationDTO jsonTransformationDTO) { - GitArtifactHelper gitArtifactHelper = - gitArtifactHelperResolver.getArtifactHelper(jsonTransformationDTO.getArtifactType()); + ArtifactType artifactType = jsonTransformationDTO.getArtifactType(); + GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); Path repoSuffix = gitArtifactHelper.getRepoSuffixPath( jsonTransformationDTO.getWorkspaceId(), @@ -689,8 +644,8 @@ public Mono deleteGitReference(ArtifactJsonTransformationDTO jsonTransf .onErrorResume(throwable -> { log.error("Delete branch failed {}", throwable.getMessage()); - Mono releaseLockMono = - gitRedisUtils.releaseFileLock(jsonTransformationDTO.getBaseArtifactId(), Boolean.TRUE); + Mono releaseLockMono = gitRedisUtils.releaseFileLock( + artifactType, jsonTransformationDTO.getBaseArtifactId(), Boolean.TRUE); if (throwable instanceof CannotDeleteCurrentBranchException) { return releaseLockMono.then(Mono.error(new AppsmithException(