From 772f14393ddb337cdc92526913a9e2af2ad1a7bf Mon Sep 17 00:00:00 2001 From: sondermanish Date: Fri, 14 Feb 2025 16:42:13 +0530 Subject: [PATCH 1/3] modified git detach flow --- .../git/central/CentralGitServiceCEImpl.java | 100 ++++++++++-------- 1 file changed, 53 insertions(+), 47 deletions(-) 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 f6ab0a0862c5..9d29536b3110 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 @@ -76,7 +76,6 @@ import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.util.function.Tuple2; -import reactor.util.function.Tuple3; import java.io.IOException; import java.time.Instant; @@ -1600,77 +1599,84 @@ private Mono commitArtifact( /** * Method to remove all the git metadata for the artifact and connected resources. This will remove: * - local repo - * - all the branched artifacts present in DB except for default artifact + * - all the branched artifacts present in DB except for base artifact * * @param branchedArtifactId : id of any branched artifact for the given repo * @param artifactType : type of artifact - * @return : the base artifact after removal of git flow. + * @param gitType: type of git service + * @return : the base artifact after removal of git attributes and other branches. */ + @Override public Mono detachRemote( String branchedArtifactId, ArtifactType artifactType, GitType gitType) { + GitHandlingService gitHandlingService = gitHandlingServiceResolver.getGitHandlingService(gitType); GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); AclPermission gitConnectPermission = gitArtifactHelper.getArtifactGitConnectPermission(); - Mono> baseAndBranchedArtifactMono = - getBaseAndBranchedArtifacts(branchedArtifactId, artifactType, gitConnectPermission); + Mono branchedArtifactMono = + gitArtifactHelper.getArtifactById(branchedArtifactId, gitConnectPermission); - Mono disconnectMono = baseAndBranchedArtifactMono - .flatMap(artifactTuples -> { - Artifact baseArtifact = artifactTuples.getT1(); + Mono disconnectMono = branchedArtifactMono + .flatMap(branchedArtifact -> { + GitArtifactMetadata branchedGitMetadata = branchedArtifact.getGitArtifactMetadata(); - if (isBaseGitMetadataInvalid(baseArtifact.getGitArtifactMetadata(), gitType)) { + if (branchedArtifact.getGitArtifactMetadata() == null) { return Mono.error(new AppsmithException( AppsmithError.INVALID_GIT_CONFIGURATION, "Please reconfigure the artifact to connect to git repo")); } - GitArtifactMetadata gitArtifactMetadata = baseArtifact.getGitArtifactMetadata(); - ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO(); - jsonTransformationDTO.setRefType(RefType.branch); - jsonTransformationDTO.setWorkspaceId(baseArtifact.getWorkspaceId()); - jsonTransformationDTO.setBaseArtifactId(gitArtifactMetadata.getDefaultArtifactId()); - jsonTransformationDTO.setRepoName(gitArtifactMetadata.getRepoName()); - jsonTransformationDTO.setArtifactType(baseArtifact.getArtifactType()); - jsonTransformationDTO.setRefName(gitArtifactMetadata.getRefName()); - - // Remove the git contents from file system - return Mono.zip( - gitHandlingService.listReferences(jsonTransformationDTO, false), Mono.just(baseArtifact)); - }) - .flatMap(tuple -> { - List localBranches = tuple.getT1(); - Artifact baseArtifact = tuple.getT2(); + String baseArtifactId = branchedGitMetadata.getDefaultArtifactId(); + String repoName = branchedGitMetadata.getRepoName(); + String workspaceId = branchedArtifact.getWorkspaceId(); + String refName = branchedGitMetadata.getRefName(); + RefType refType = RefType.branch; - GitArtifactMetadata baseGitMetadata = baseArtifact.getGitArtifactMetadata(); - localBranches.remove(baseGitMetadata.getRefName()); - - baseArtifact.setGitArtifactMetadata(null); - gitArtifactHelper.resetAttributeInBaseArtifact(baseArtifact); + ArtifactJsonTransformationDTO jsonTransformationDTO = + new ArtifactJsonTransformationDTO(workspaceId, baseArtifactId, repoName, artifactType); - ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO(); - jsonTransformationDTO.setRefType(RefType.branch); - jsonTransformationDTO.setWorkspaceId(baseArtifact.getWorkspaceId()); - jsonTransformationDTO.setBaseArtifactId(baseGitMetadata.getDefaultArtifactId()); - jsonTransformationDTO.setRepoName(baseGitMetadata.getRepoName()); - jsonTransformationDTO.setArtifactType(baseArtifact.getArtifactType()); - jsonTransformationDTO.setRefName(baseGitMetadata.getRefName()); + jsonTransformationDTO.setRefName(refName); + jsonTransformationDTO.setRefType(refType); // Remove the parent artifact branch name from the list Mono removeRepoMono = gitHandlingService.removeRepository(jsonTransformationDTO); - Mono updatedArtifactMono = gitArtifactHelper.saveArtifact(baseArtifact); - Flux deleteAllBranchesFlux = - gitArtifactHelper.deleteAllBranches(branchedArtifactId, localBranches); + AclPermission artifactEditPermission = gitArtifactHelper.getArtifactEditPermission(); + + Mono deleteAllBranchesExceptBase = gitArtifactHelper + .getAllArtifactByBaseId(baseArtifactId, artifactEditPermission) + .flatMap(artifact -> { + if (artifact.getGitArtifactMetadata() != null + && artifact.getId().equals(baseArtifactId)) { + // base Artifact condition fulfilled + artifact.setGitArtifactMetadata(null); + gitArtifactHelper.resetAttributeInBaseArtifact(artifact); + return gitArtifactHelper + .saveArtifact(artifact) + .flatMap(baseArtifact -> { + return gitArtifactHelper.disconnectEntitiesOfBaseArtifact(baseArtifact); + }); + } - return Mono.zip(updatedArtifactMono, removeRepoMono, deleteAllBranchesFlux.collectList()) - .map(Tuple3::getT1); + if (artifact.getGitArtifactMetadata() == null + || RefType.tag.equals(artifact.getGitArtifactMetadata() + .getRefType())) { + return Mono.just(artifact); + } + + return gitArtifactHelper.deleteArtifactByResource(artifact); + }) + .filter(artifact -> { + return artifact.getId().equals(baseArtifactId); + }) + .next(); + + return Mono.zip(deleteAllBranchesExceptBase, removeRepoMono).map(Tuple2::getT1); }) - .flatMap(updatedBaseArtifact -> { - return gitArtifactHelper - .disconnectEntitiesOfBaseArtifact(updatedBaseArtifact) - .then(gitAnalyticsUtils.addAnalyticsForGitOperation( - AnalyticsEvents.GIT_DISCONNECT, updatedBaseArtifact, false)); + .flatMap(disconnectedBaseArtifact -> { + return gitAnalyticsUtils.addAnalyticsForGitOperation( + AnalyticsEvents.GIT_DISCONNECT, disconnectedBaseArtifact, false); }) .name(GitSpan.OPS_DETACH_REMOTE) .tap(Micrometer.observation(observationRegistry)); From c5448151c925f85720f44ec45756235b8c434afe Mon Sep 17 00:00:00 2001 From: sondermanish Date: Fri, 14 Feb 2025 17:58:30 +0530 Subject: [PATCH 2/3] modified logic for git push --- .../server/git/fs/GitFSServiceCEImpl.java | 93 ++++++++----------- 1 file changed, 39 insertions(+), 54 deletions(-) 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 6b8f5eb355a5..859087c9de7c 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 @@ -5,7 +5,6 @@ import com.appsmith.external.dtos.GitRefDTO; import com.appsmith.external.dtos.GitStatusDTO; import com.appsmith.external.dtos.MergeStatusDTO; -import com.appsmith.external.git.constants.GitConstants; import com.appsmith.external.git.constants.GitConstants.GitCommandConstants; import com.appsmith.external.git.constants.GitSpan; import com.appsmith.external.git.constants.ce.RefType; @@ -482,7 +481,7 @@ public Mono> commitArtifact( result.append(".\nPush Result : "); return Mono.zip( Mono.just(tuple.getT2()), - pushArtifact(tuple.getT2(), false) + pushArtifact(tuple.getT2()) .map(pushResult -> result.append(pushResult).toString())); }); } @@ -493,7 +492,7 @@ public Mono> commitArtifact( * @param branchedArtifact application which needs to be pushed to remote repo * @return Success message */ - protected Mono pushArtifact(Artifact branchedArtifact, boolean isFileLock) { + protected Mono pushArtifact(Artifact branchedArtifact) { ArtifactType artifactType = branchedArtifact.getArtifactType(); GitArtifactHelper gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); Mono gitArtifactMetadataMono = Mono.just(branchedArtifact.getGitArtifactMetadata()); @@ -515,16 +514,7 @@ protected Mono pushArtifact(Artifact branchedArtifact, boolean isFileLoc // Make sure that ssh Key is unEncrypted for the use. Mono gitPushResult = gitArtifactMetadataMono .flatMap(gitMetadata -> { - return gitRedisUtils - .acquireGitLock( - artifactType, - gitMetadata.getDefaultArtifactId(), - GitConstants.GitCommandConstants.PUSH, - isFileLock) - .thenReturn(branchedArtifact); - }) - .flatMap(artifact -> { - GitArtifactMetadata gitData = artifact.getGitArtifactMetadata(); + GitArtifactMetadata gitData = branchedArtifact.getGitArtifactMetadata(); if (gitData == null || !StringUtils.hasText(gitData.getRefName()) @@ -536,61 +526,55 @@ protected Mono pushArtifact(Artifact branchedArtifact, boolean isFileLoc } Path baseRepoSuffix = gitArtifactHelper.getRepoSuffixPath( - artifact.getWorkspaceId(), gitData.getDefaultArtifactId(), gitData.getRepoName()); + branchedArtifact.getWorkspaceId(), gitData.getDefaultArtifactId(), gitData.getRepoName()); GitAuth gitAuth = gitData.getGitAuth(); return fsGitHandler - .checkoutToBranch( + .pushApplication( baseRepoSuffix, - artifact.getGitArtifactMetadata().getRefName()) - .then(Mono.defer(() -> fsGitHandler - .pushApplication( - baseRepoSuffix, - gitData.getRemoteUrl(), - gitAuth.getPublicKey(), - gitAuth.getPrivateKey(), - gitData.getRefName()) - .zipWith(Mono.just(artifact)))) + gitData.getRemoteUrl(), + gitAuth.getPublicKey(), + gitAuth.getPrivateKey(), + gitData.getRefName()) .onErrorResume(error -> gitAnalyticsUtils .addAnalyticsForGitOperation( AnalyticsEvents.GIT_PUSH, - artifact, + branchedArtifact, error.getClass().getName(), error.getMessage(), - artifact.getGitArtifactMetadata().getIsRepoPrivate()) - .flatMap(application1 -> { + gitData.getIsRepoPrivate()) + .flatMap(artifact -> { + log.error("Error during git push: {}", error.getMessage()); if (error instanceof TransportException) { return Mono.error( new AppsmithException(AppsmithError.INVALID_GIT_SSH_CONFIGURATION)); } + return Mono.error(new AppsmithException( - AppsmithError.GIT_ACTION_FAILED, "push", error.getMessage())); + AppsmithError.GIT_ACTION_FAILED, + GitCommandConstants.PUSH, + error.getMessage())); })); }) - .flatMap(tuple -> { - String pushResult = tuple.getT1(); - Artifact artifact = tuple.getT2(); - return pushArtifactErrorRecovery(pushResult, artifact).zipWith(Mono.just(artifact)); - }) - // Add BE analytics - .flatMap(tuple2 -> { - String pushStatus = tuple2.getT1(); - Artifact artifact = tuple2.getT2(); - Mono fileLockReleasedMono = Mono.just(TRUE).flatMap(flag -> { - if (!TRUE.equals(isFileLock)) { - return Mono.just(flag); - } - return Mono.defer(() -> gitRedisUtils.releaseFileLock( - artifactType, artifact.getGitArtifactMetadata().getDefaultArtifactId(), true)); - }); - - return pushArtifactErrorRecovery(pushStatus, artifact) - .then(fileLockReleasedMono) - .then(gitAnalyticsUtils.addAnalyticsForGitOperation( - AnalyticsEvents.GIT_PUSH, - artifact, - artifact.getGitArtifactMetadata().getIsRepoPrivate())) - .thenReturn(pushStatus); + .flatMap(pushResult -> { + log.info( + "Push result for artifact {} with id {} : {}", + branchedArtifact.getName(), + branchedArtifact.getId(), + pushResult); + + return pushArtifactErrorRecovery(pushResult, branchedArtifact) + .flatMap(pushStatus -> { + // Add analytics + return gitAnalyticsUtils + .addAnalyticsForGitOperation( + AnalyticsEvents.GIT_PUSH, + branchedArtifact, + branchedArtifact + .getGitArtifactMetadata() + .getIsRepoPrivate()) + .thenReturn(pushStatus); + }); }) .name(GitSpan.OPS_PUSH) .tap(Micrometer.observation(observationRegistry)); @@ -622,7 +606,8 @@ private Mono pushArtifactErrorRecovery(String pushResult, Artifact artif AppsmithError.GIT_UPSTREAM_CHANGES.getErrorType(), AppsmithError.GIT_UPSTREAM_CHANGES.getMessage(), gitMetadata.getIsRepoPrivate()) - .flatMap(application1 -> Mono.error(new AppsmithException(AppsmithError.GIT_UPSTREAM_CHANGES))); + .then(Mono.error(new AppsmithException(AppsmithError.GIT_UPSTREAM_CHANGES))); + } else if (pushResult.contains("REJECTED_OTHERREASON") || pushResult.contains("pre-receive hook declined")) { Path path = gitArtifactHelper.getRepoSuffixPath( @@ -632,7 +617,7 @@ private Mono pushArtifactErrorRecovery(String pushResult, Artifact artif .resetHard(path, gitMetadata.getRefName()) .then(Mono.error(new AppsmithException( AppsmithError.GIT_ACTION_FAILED, - "push", + GitCommandConstants.PUSH, "Unable to push changes as pre-receive hook declined. Please make sure that you don't have any rules enabled on the branch " + gitMetadata.getRefName()))); } From 888cf6f94de9769741a54fe55adeae89b828ce1a Mon Sep 17 00:00:00 2001 From: sondermanish Date: Fri, 14 Feb 2025 19:23:11 +0530 Subject: [PATCH 3/3] pr review comments --- .../git/central/CentralGitServiceCEImpl.java | 25 +++++++++---------- .../server/git/fs/GitFSServiceCEImpl.java | 3 +-- 2 files changed, 13 insertions(+), 15 deletions(-) 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 9d29536b3110..a4c5052f3caa 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 @@ -1647,25 +1647,24 @@ public Mono detachRemote( Mono deleteAllBranchesExceptBase = gitArtifactHelper .getAllArtifactByBaseId(baseArtifactId, artifactEditPermission) .flatMap(artifact -> { - if (artifact.getGitArtifactMetadata() != null - && artifact.getId().equals(baseArtifactId)) { - // base Artifact condition fulfilled - artifact.setGitArtifactMetadata(null); - gitArtifactHelper.resetAttributeInBaseArtifact(artifact); - return gitArtifactHelper - .saveArtifact(artifact) - .flatMap(baseArtifact -> { - return gitArtifactHelper.disconnectEntitiesOfBaseArtifact(baseArtifact); - }); - } - if (artifact.getGitArtifactMetadata() == null || RefType.tag.equals(artifact.getGitArtifactMetadata() .getRefType())) { return Mono.just(artifact); } - return gitArtifactHelper.deleteArtifactByResource(artifact); + // it's established that git artifact metadata is not null + if (!artifact.getId().equals(baseArtifactId)) { + return gitArtifactHelper.deleteArtifactByResource(artifact); + } + + // base Artifact condition fulfilled + artifact.setGitArtifactMetadata(null); + gitArtifactHelper.resetAttributeInBaseArtifact(artifact); + + return gitArtifactHelper.saveArtifact(artifact).flatMap(baseArtifact -> { + return gitArtifactHelper.disconnectEntitiesOfBaseArtifact(baseArtifact); + }); }) .filter(artifact -> { return artifact.getId().equals(baseArtifactId); 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 859087c9de7c..df6fc99f9afc 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 @@ -488,8 +488,7 @@ public Mono> commitArtifact( /** * Push flow for dehydrated apps - * - * @param branchedArtifact application which needs to be pushed to remote repo + * @param branchedArtifact artifact which needs to be pushed to remote repo * @return Success message */ protected Mono pushArtifact(Artifact branchedArtifact) {