chore: Added changes for discard #38152
Conversation
WalkthroughThe changes in this pull request involve modifications to method signatures in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Git Product
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java (3)
Line range hint
72-77: LGTM! Consider using Optional for better null handling.The change from
booleantoBooleanis acceptable, but consider usingOptional<Boolean>for more explicit null handling.-public Mono<Boolean> acquireGitLock(String baseArtifactId, String commandName, Boolean isLockRequired) { +public Mono<Boolean> acquireGitLock(String baseArtifactId, String commandName, Optional<Boolean> isLockRequired) { - if (!Boolean.TRUE.equals(isLockRequired)) { + if (!isLockRequired.orElse(false)) { return Mono.just(Boolean.TRUE); }
Line range hint
89-95: Fix parameter type inconsistency.The parameter type for
isLockRequiredshould beBooleanto maintain consistency withacquireGitLock.-public Mono<Boolean> releaseFileLock(String baseArtifactId, boolean isLockRequired) { +public Mono<Boolean> releaseFileLock(String baseArtifactId, Boolean isLockRequired) {
71-71: Address TODO comments in follow-up PR.The TODO comments indicate a need to add artifactType reference. Consider creating a follow-up task.
Would you like me to create a GitHub issue to track the addition of artifactType reference?
Also applies to: 88-88
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
1171-1172: Improve error logging.The error log is missing the actual error message from the throwable.
-log.error("Git recreate ArtifactJsonFailed", throwable.getMessage()); +log.error("Git recreate ArtifactJsonFailed: {}", throwable.getMessage(), throwable);
1139-1196: Consider breaking down the reactive chain.The reactive chain is complex and could be simplified for better maintainability.
Extract the artifact recreation logic into a separate method:
private Mono<? extends Artifact> recreateArtifactFromLastCommit( Artifact branchedArtifact, GitHandlingService gitHandlingService) { GitArtifactMetadata branchedGitData = branchedArtifact.getGitArtifactMetadata(); ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO(); jsonTransformationDTO.setArtifactType(artifactType); // ... set other fields ... return gitHandlingService .recreateArtifactJsonFromLastCommit(jsonTransformationDTO) .flatMap(artifactExchangeJson -> importService.importArtifactInWorkspaceFromGit(/*...*/)) .flatMap(importedArtifact -> gitArtifactHelper.publishArtifact(importedArtifact, true)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(1 hunks)
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (1)
34-35: LGTM! Verify method implementation coverage.
The new discardChanges method signature aligns well with the interface's existing patterns.
Let's verify the implementation coverage:
✅ Verification successful
Implementation coverage verified
The discardChanges method is properly implemented in CentralGitServiceCEImpl with:
- Complete error handling for invalid parameters
- Integration with git operations
- Proper permission checks
- Consistent with interface contract
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for implementation of discardChanges method
ast-grep --pattern 'class $_ implements CentralGitServiceCE {
$$$
public Mono<? extends Artifact> discardChanges($_, $_, $_) {
$$$
}
$$$
}'
Length of output: 153
Script:
#!/bin/bash
# Let's try a different approach to find implementations
# First, find files that might contain the implementation
rg -l "CentralGitService" --type java
# Then look for the actual implementation with context
rg "discardChanges.*\(.*\)" --type java -A 5
Length of output: 7730
Script:
#!/bin/bash
# Let's check the implementation in CentralGitServiceCEImpl
cat app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java
Length of output: 65644
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (1)
61-63: LGTM! Verify method implementation coverage.
The new recreateArtifactJsonFromLastCommit method is well-defined with appropriate parameter and return types.
Let's verify the implementation coverage:
✅ Verification successful
Method is properly implemented and used in the codebase
The method is implemented in GitFSServiceCEImpl and correctly used in CentralGitServiceCEImpl, maintaining the contract defined in the interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for implementation of recreateArtifactJsonFromLastCommit method
ast-grep --pattern 'class $_ implements GitHandlingServiceCE {
$$$
public Mono<? extends ArtifactExchangeJson> recreateArtifactJsonFromLastCommit(ArtifactJsonTransformationDTO $_) {
$$$
}
$$$
}'
Length of output: 208
Script:
#!/bin/bash
# Let's try a different approach to find implementations
# First, find all Java files that might contain the implementation
fd -e java | xargs rg -l "class.*implements.*GitHandlingServiceCE"
# Then search for the method name to ensure it exists
rg "recreateArtifactJsonFromLastCommit" -A 2
Length of output: 1736
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
1127-1129: LGTM: Input validation is properly handled.
The method correctly validates the input parameter before proceeding.
| @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 commonGitFileUtils.reconstructArtifactExchangeJsonFromGitRepoWithAnalytics( | ||
| workspaceId, baseArtifactId, repoName, refName, artifactType); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider transaction boundary.
The method performs critical Git operations but:
- Lacks explicit error handling for rebase conflicts
- 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.
| return gitRedisUtils | ||
| .acquireGitLock( | ||
| branchedGitData.getDefaultArtifactId(), | ||
| GitConstants.GitCommandConstants.DISCARD, | ||
| TRUE) | ||
| .thenReturn(branchedArtifact); | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using doFinally for lock release.
The lock release should be guaranteed even if subsequent operations fail.
Move the lock release to a doFinally operator:
-return gitRedisUtils
- .acquireGitLock(
- branchedGitData.getDefaultArtifactId(),
- GitConstants.GitCommandConstants.DISCARD,
- TRUE)
- .thenReturn(branchedArtifact);
+return gitRedisUtils
+ .acquireGitLock(
+ branchedGitData.getDefaultArtifactId(),
+ GitConstants.GitCommandConstants.DISCARD,
+ TRUE)
+ .thenReturn(branchedArtifact)
+ .doFinally(signalType ->
+ gitRedisUtils.releaseFileLock(
+ branchedGitData.getDefaultArtifactId(),
+ TRUE
+ ).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.
| return gitRedisUtils | |
| .acquireGitLock( | |
| branchedGitData.getDefaultArtifactId(), | |
| GitConstants.GitCommandConstants.DISCARD, | |
| TRUE) | |
| .thenReturn(branchedArtifact); | |
| }) | |
| return gitRedisUtils | |
| .acquireGitLock( | |
| branchedGitData.getDefaultArtifactId(), | |
| GitConstants.GitCommandConstants.DISCARD, | |
| TRUE) | |
| .thenReturn(branchedArtifact) | |
| .doFinally(signalType -> | |
| gitRedisUtils.releaseFileLock( | |
| branchedGitData.getDefaultArtifactId(), | |
| TRUE | |
| ).subscribe() | |
| ); | |
| }) |
There was a problem hiding this comment.
@sondermanish It makes sense to move this logic related to releasing the lock in a doFinally operator. Do you see any problem with this?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Here it wouldn't make any issue, however if we are preprocessing then doFinally doesn't make sense as we may run into error signal and then we wouldn't want to acquire lock
| return gitRedisUtils | ||
| .acquireGitLock( | ||
| branchedGitData.getDefaultArtifactId(), | ||
| GitConstants.GitCommandConstants.DISCARD, | ||
| TRUE) | ||
| .thenReturn(branchedArtifact); | ||
| }) |
There was a problem hiding this comment.
@sondermanish It makes sense to move this logic related to releasing the lock in a doFinally operator. Do you see any problem with this?
| .flatMap(importedArtifact -> gitArtifactHelper.publishArtifact(importedArtifact, true)); | ||
| }) | ||
| .flatMap(branchedArtifact -> gitRedisUtils | ||
| .releaseFileLock( |
There was a problem hiding this comment.
Can we also move this in a doFinally operator?
| new AppsmithException( | ||
| AppsmithError.GIT_ACTION_FAILED, | ||
| "discard changes", | ||
| "Please create a new branch and resolve the conflicts on remote repository before proceeding ahead.")); |
There was a problem hiding this comment.
Nit: "ahead" seems redundant here. How about "Please create a new branch and resolve conflicts in the remote repository before proceeding." ?
| GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); | ||
| Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(workspaceId, baseArtifactId, repoName); | ||
|
|
||
| return fsGitHandler.rebaseBranch(repoSuffix, refName).flatMap(rebaseStatus -> { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Right, however the idea is that the local and remote are always in sync.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
1116-1122: Consider using a specific type instead of wildcard generic.The return type
Mono<? extends Artifact>could be more specific to improve type safety and readability.
1175-1175: Simplify error message.The error message is redundant with "ahead" in the previous review comment.
Apply this change:
- "Please create a new branch and resolve conflicts in the remote repository before proceeding.")); + "Please create a new branch and resolve conflicts in the remote repository."));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(1 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
1127-1129: LGTM! Parameter validation is correct.
The null/empty check for branchedArtifactId is properly implemented with appropriate error handling.
| .addAnalyticsForGitOperation(AnalyticsEvents.GIT_DISCARD_CHANGES, branchedArtifact, null) | ||
| .doFinally(signalType -> gitRedisUtils.releaseFileLock( | ||
| branchedArtifact.getGitArtifactMetadata().getDefaultArtifactId(), TRUE))) | ||
| .name(GitSpan.OPS_DISCARD_CHANGES) |
There was a problem hiding this comment.
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.
| .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) |
| return Mono.just(branchedArtifact) | ||
| .doFinally(signalType -> gitRedisUtils.acquireGitLock( | ||
| branchedGitData.getDefaultArtifactId(), | ||
| GitConstants.GitCommandConstants.DISCARD, | ||
| TRUE)); | ||
| }) |
There was a problem hiding this comment.
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.
| 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)); | |
| }) |
Description
Fixes #37438
Automation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12314835405
Commit: 0db248a
Cypress dashboard.
Tags:
@tag.GitSpec:
Fri, 13 Dec 2024 12:18:45 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation