Skip to content

chore: git detach and push algorithm refinement #39298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sondermanish merged 3 commits into from
Feb 14, 2025
Merged

chore: git detach and push algorithm refinement #39298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
772f143
modified git detach flow
sondermanish Feb 14, 2025
c544815
modified logic for git push
sondermanish Feb 14, 2025
888cf6f
pr review comments
sondermanish Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 52 additions & 47 deletions ...ppsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1600,77 +1599,83 @@ private Mono<String> 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<? extends Artifact> detachRemote(
String branchedArtifactId, ArtifactType artifactType, GitType gitType) {

GitHandlingService gitHandlingService = gitHandlingServiceResolver.getGitHandlingService(gitType);
GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
AclPermission gitConnectPermission = gitArtifactHelper.getArtifactGitConnectPermission();

Mono<Tuple2<? extends Artifact, ? extends Artifact>> baseAndBranchedArtifactMono =
getBaseAndBranchedArtifacts(branchedArtifactId, artifactType, gitConnectPermission);
Mono<? extends Artifact> branchedArtifactMono =
gitArtifactHelper.getArtifactById(branchedArtifactId, gitConnectPermission);

Mono<? extends Artifact> disconnectMono = baseAndBranchedArtifactMono
.flatMap(artifactTuples -> {
Artifact baseArtifact = artifactTuples.getT1();
Mono<? extends Artifact> 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<String> localBranches = tuple.getT1();
Artifact baseArtifact = tuple.getT2();

GitArtifactMetadata baseGitMetadata = baseArtifact.getGitArtifactMetadata();
localBranches.remove(baseGitMetadata.getRefName());
String baseArtifactId = branchedGitMetadata.getDefaultArtifactId();
String repoName = branchedGitMetadata.getRepoName();
String workspaceId = branchedArtifact.getWorkspaceId();
String refName = branchedGitMetadata.getRefName();
RefType refType = RefType.branch;

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<Boolean> removeRepoMono = gitHandlingService.removeRepository(jsonTransformationDTO);
Mono<? extends Artifact> updatedArtifactMono = gitArtifactHelper.saveArtifact(baseArtifact);

Flux<? extends Artifact> deleteAllBranchesFlux =
gitArtifactHelper.deleteAllBranches(branchedArtifactId, localBranches);
AclPermission artifactEditPermission = gitArtifactHelper.getArtifactEditPermission();

Mono<? extends Artifact> deleteAllBranchesExceptBase = gitArtifactHelper
.getAllArtifactByBaseId(baseArtifactId, artifactEditPermission)
.flatMap(artifact -> {
if (artifact.getGitArtifactMetadata() == null
|| RefType.tag.equals(artifact.getGitArtifactMetadata()
.getRefType())) {
Copy link
Collaborator

@subrata71 subrata71 Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems llike this logic should precede the above logic from L#1649-L#1659

Copy link
Contributor Author

@sondermanish sondermanish Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true!

return Mono.just(artifact);
}

return Mono.zip(updatedArtifactMono, removeRepoMono, deleteAllBranchesFlux.collectList())
.map(Tuple3::getT1);
// 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);
})
.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));
Expand Down
96 changes: 40 additions & 56 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -482,18 +481,17 @@ public Mono<Tuple2<? extends Artifact, String>> 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()));
});
}

/**
* 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<String> pushArtifact(Artifact branchedArtifact, boolean isFileLock) {
protected Mono<String> pushArtifact(Artifact branchedArtifact) {
ArtifactType artifactType = branchedArtifact.getArtifactType();
GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
Mono<GitArtifactMetadata> gitArtifactMetadataMono = Mono.just(branchedArtifact.getGitArtifactMetadata());
Expand All @@ -515,16 +513,7 @@ protected Mono<String> pushArtifact(Artifact branchedArtifact, boolean isFileLoc
// Make sure that ssh Key is unEncrypted for the use.
Mono<String> gitPushResult = gitArtifactMetadataMono
.flatMap(gitMetadata -> {
return gitRedisUtils
.acquireGitLock(
Copy link
Collaborator

@subrata71 subrata71 Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please state the reason of removing Git lock placement logic here?

Copy link
Contributor Author

@sondermanish sondermanish Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is always getting called from inside the commit section

Copy link
Contributor Author

@sondermanish sondermanish Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which means that the redis lock would never be required

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())
Expand All @@ -536,61 +525,55 @@ protected Mono<String> 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(
Copy link
Collaborator

@subrata71 subrata71 Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Naming. You wanted to mean pushArtifact maybe

Copy link
Contributor Author

@sondermanish sondermanish Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an already existing method, can I take this in another refactor session?

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<Boolean> 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));
Expand Down Expand Up @@ -622,7 +605,8 @@ private Mono<String> 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(
Expand All @@ -632,7 +616,7 @@ private Mono<String> 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())));
}
Expand Down
Loading