Skip to content
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

chore: Added changes for discard #38152

Merged
merged 3 commits into from
Dec 13, 2024
Merged

chore: Added changes for discard #38152

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
90d6fb8
added changes for discard
sondermanish Dec 13, 2024
826eb18
added interface method
sondermanish Dec 13, 2024
0db248a
added review changes
sondermanish Dec 13, 2024
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
2 changes: 1 addition & 1 deletion app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public Mono<Boolean> releaseFileLock(String defaultApplicationId) {
* @return : Boolean for whether the lock is acquired
*/
// TODO @Manish add artifactType reference in incoming prs.
public Mono<Boolean> acquireGitLock(String baseArtifactId, String commandName, boolean isLockRequired) {
public Mono<Boolean> acquireGitLock(String baseArtifactId, String commandName, Boolean isLockRequired) {
if (!Boolean.TRUE.equals(isLockRequired)) {
return Mono.just(Boolean.TRUE);
}
Expand Down
2 changes: 2 additions & 0 deletions ...er/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,6 @@ Mono<String> fetchRemoteChanges(
ArtifactType artifactType,
GitType gitType,
RefType refType);

Mono<? extends Artifact> discardChanges(String branchedArtifactId, ArtifactType artifactType, GitType gitType);
}
80 changes: 80 additions & 0 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 @@ -1112,4 +1112,84 @@ private Mono<? extends Artifact> updateArtifactWithGitMetadataGivenPermission(
.getArtifactHelper(artifact.getArtifactType())
.saveArtifact(artifact);
}

/**
* Resets the artifact to last commit, all uncommitted changes are lost in the process.
* @param branchedArtifactId : id of the branchedArtifact
* @param artifactType type of the artifact
* @param gitType what is the intended implementation type
* @return : a publisher of an artifact.
*/
@Override
public Mono<? extends Artifact> discardChanges(
String branchedArtifactId, ArtifactType artifactType, GitType gitType) {

if (!hasText(branchedArtifactId)) {
return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ARTIFACT_ID));
}

GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
AclPermission artifactEditPermission = gitArtifactHelper.getArtifactEditPermission();

Mono<? extends Artifact> branchedArtifactMonoCached =
gitArtifactHelper.getArtifactById(branchedArtifactId, artifactEditPermission);

Mono<? extends Artifact> recreatedArtifactFromLastCommit;

// Rehydrate the artifact from local file system
recreatedArtifactFromLastCommit = branchedArtifactMonoCached
.flatMap(branchedArtifact -> {
GitArtifactMetadata branchedGitData = branchedArtifact.getGitArtifactMetadata();
if (branchedGitData == null || !hasText(branchedGitData.getDefaultArtifactId())) {
return Mono.error(
new AppsmithException(AppsmithError.INVALID_GIT_CONFIGURATION, GIT_CONFIG_ERROR));
}

return Mono.just(branchedArtifact)
.doFinally(signalType -> gitRedisUtils.acquireGitLock(
branchedGitData.getDefaultArtifactId(),
GitConstants.GitCommandConstants.DISCARD,
TRUE));
})
Comment on lines +1148 to +1153
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Lock acquisition should be moved to a reactive chain.

The lock acquisition in doFinally is not properly integrated into the reactive chain. This could lead to timing issues.

Apply this change:

-            .doFinally(signalType -> gitRedisUtils.acquireGitLock(
-                    branchedGitData.getDefaultArtifactId(),
-                    GitConstants.GitCommandConstants.DISCARD,
-                    TRUE));
+            .flatMap(artifact -> gitRedisUtils
+                    .acquireGitLock(
+                            branchedGitData.getDefaultArtifactId(),
+                            GitConstants.GitCommandConstants.DISCARD,
+                            TRUE)
+                    .thenReturn(artifact));
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return Mono.just(branchedArtifact)
.doFinally(signalType -> gitRedisUtils.acquireGitLock(
branchedGitData.getDefaultArtifactId(),
GitConstants.GitCommandConstants.DISCARD,
TRUE));
})
return Mono.just(branchedArtifact)
.flatMap(artifact -> gitRedisUtils
.acquireGitLock(
branchedGitData.getDefaultArtifactId(),
GitConstants.GitCommandConstants.DISCARD,
TRUE)
.thenReturn(artifact));
})

.flatMap(branchedArtifact -> {
GitArtifactMetadata branchedGitData = branchedArtifact.getGitArtifactMetadata();
ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO();
// Because this operation is only valid for branches
jsonTransformationDTO.setArtifactType(artifactType);
jsonTransformationDTO.setRefType(RefType.BRANCH);
jsonTransformationDTO.setWorkspaceId(branchedArtifact.getWorkspaceId());
jsonTransformationDTO.setBaseArtifactId(branchedGitData.getDefaultArtifactId());
jsonTransformationDTO.setRefName(branchedGitData.getRefName());
jsonTransformationDTO.setRepoName(branchedGitData.getRepoName());

GitHandlingService gitHandlingService = gitHandlingServiceResolver.getGitHandlingService(gitType);

return gitHandlingService
.recreateArtifactJsonFromLastCommit(jsonTransformationDTO)
.onErrorResume(throwable -> {
log.error("Git recreate ArtifactJsonFailed : {}", throwable.getMessage());
return Mono.error(
new AppsmithException(
AppsmithError.GIT_ACTION_FAILED,
"discard changes",
"Please create a new branch and resolve conflicts in the remote repository before proceeding."));
})
.flatMap(artifactExchangeJson -> importService.importArtifactInWorkspaceFromGit(
branchedArtifact.getWorkspaceId(),
branchedArtifact.getId(),
artifactExchangeJson,
branchedGitData.getBranchName()))
// Update the last deployed status after the rebase
.flatMap(importedArtifact -> gitArtifactHelper.publishArtifact(importedArtifact, true));
})
.flatMap(branchedArtifact -> gitAnalyticsUtils
.addAnalyticsForGitOperation(AnalyticsEvents.GIT_DISCARD_CHANGES, branchedArtifact, null)
.doFinally(signalType -> gitRedisUtils.releaseFileLock(
branchedArtifact.getGitArtifactMetadata().getDefaultArtifactId(), TRUE)))
.name(GitSpan.OPS_DISCARD_CHANGES)
Comment on lines +1186 to +1189
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Lock release should handle errors gracefully.

The lock release in doFinally should handle potential errors to ensure the lock is always released, even in error cases.

Apply this change:

-                        .doFinally(signalType -> gitRedisUtils.releaseFileLock(
-                                branchedArtifact.getGitArtifactMetadata().getDefaultArtifactId(), TRUE)))
+                        .doFinally(signalType -> gitRedisUtils
+                                .releaseFileLock(
+                                        branchedArtifact.getGitArtifactMetadata().getDefaultArtifactId(),
+                                        TRUE)
+                                .onErrorResume(error -> {
+                                    log.error("Error releasing git lock: {}", error.getMessage());
+                                    return Mono.empty();
+                                })
+                                .subscribe()))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
.addAnalyticsForGitOperation(AnalyticsEvents.GIT_DISCARD_CHANGES, branchedArtifact, null)
.doFinally(signalType -> gitRedisUtils.releaseFileLock(
branchedArtifact.getGitArtifactMetadata().getDefaultArtifactId(), TRUE)))
.name(GitSpan.OPS_DISCARD_CHANGES)
.addAnalyticsForGitOperation(AnalyticsEvents.GIT_DISCARD_CHANGES, branchedArtifact, null)
.doFinally(signalType -> gitRedisUtils
.releaseFileLock(
branchedArtifact.getGitArtifactMetadata().getDefaultArtifactId(),
TRUE)
.onErrorResume(error -> {
log.error("Error releasing git lock: {}", error.getMessage());
return Mono.empty();
})
.subscribe())
.name(GitSpan.OPS_DISCARD_CHANGES)

.tap(Micrometer.observation(observationRegistry));

return Mono.create(sink ->
recreatedArtifactFromLastCommit.subscribe(sink::success, sink::error, null, sink.currentContext()));
}
}
3 changes: 3 additions & 0 deletions ...r/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,7 @@ Mono<Tuple2<? extends Artifact, String>> commitArtifact(
Artifact branchedArtifact, CommitDTO commitDTO, ArtifactJsonTransformationDTO jsonTransformationDTO);

Mono<String> fetchRemoteChanges(ArtifactJsonTransformationDTO jsonTransformationDTO, GitAuth gitAuth);

Mono<? extends ArtifactExchangeJson> recreateArtifactJsonFromLastCommit(
ArtifactJsonTransformationDTO jsonTransformationDTO);
}
19 changes: 19 additions & 0 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 @@ -598,4 +598,23 @@ public Mono<String> fetchRemoteChanges(ArtifactJsonTransformationDTO jsonTransfo

return checkoutBranchMono.then(Mono.defer(() -> fetchRemoteMono));
}

@Override
public Mono<? extends ArtifactExchangeJson> recreateArtifactJsonFromLastCommit(
ArtifactJsonTransformationDTO jsonTransformationDTO) {

String workspaceId = jsonTransformationDTO.getWorkspaceId();
String baseArtifactId = jsonTransformationDTO.getBaseArtifactId();
String repoName = jsonTransformationDTO.getRepoName();
String refName = jsonTransformationDTO.getRefName();

ArtifactType artifactType = jsonTransformationDTO.getArtifactType();
GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(workspaceId, baseArtifactId, repoName);

return fsGitHandler.rebaseBranch(repoSuffix, refName).flatMap(rebaseStatus -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

QQ: We don't do a complete discard, do we? Given that there are commits exist in the local branch but not in the remote, we just rebase changes on top of those commits, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, however the idea is that the local and remote are always in sync.

return commonGitFileUtils.reconstructArtifactExchangeJsonFromGitRepoWithAnalytics(
workspaceId, baseArtifactId, repoName, refName, artifactType);
});
}
Comment on lines +602 to +619
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling and consider transaction boundary.

The method performs critical Git operations but:

  1. Lacks explicit error handling for rebase conflicts
  2. Missing transaction boundary for atomic operations

Consider wrapping the operations in a transaction and add specific error handling:

@Override
public Mono<? extends ArtifactExchangeJson> recreateArtifactJsonFromLastCommit(
        ArtifactJsonTransformationDTO jsonTransformationDTO) {
    String workspaceId = jsonTransformationDTO.getWorkspaceId();
    String baseArtifactId = jsonTransformationDTO.getBaseArtifactId();
    String repoName = jsonTransformationDTO.getRepoName();
    String refName = jsonTransformationDTO.getRefName();

    ArtifactType artifactType = jsonTransformationDTO.getArtifactType();
    GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
    Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(workspaceId, baseArtifactId, repoName);

-   return fsGitHandler.rebaseBranch(repoSuffix, refName).flatMap(rebaseStatus -> {
+   return transactionalOperator.transactional(
+       fsGitHandler.rebaseBranch(repoSuffix, refName)
+           .onErrorResume(error -> {
+               if (error instanceof org.eclipse.jgit.api.errors.RebaseConflictException) {
+                   return Mono.error(new AppsmithException(
+                       AppsmithError.GIT_ACTION_FAILED,
+                       "rebase",
+                       "Conflicts detected during rebase. Please resolve conflicts and try again."
+                   ));
+               }
+               return Mono.error(error);
+           })
+   ).flatMap(rebaseStatus -> {
        return commonGitFileUtils.reconstructArtifactExchangeJsonFromGitRepoWithAnalytics(
                workspaceId, baseArtifactId, repoName, refName, artifactType);
    });
}

Committable suggestion skipped: line range outside the PR's diff.

}
Loading