chore: package refactors and left over changes for git autocommit pr #33768
Conversation
WalkthroughWalkthroughThe changes primarily involve refactoring and restructuring the package hierarchy, particularly for auto-commit related classes in the Appsmith server codebase. Key updates include moving several classes to new packages, updating import statements accordingly, and modifying method signatures to introduce new parameters. These changes aim to improve code organization and maintainability. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (23)
Files skipped from review due to trivial changes (3)
Additional comments not posted (34)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -54,7 +54,7 @@ public Mono<Boolean> isServerAutoCommitRequired(String workspaceId, GitArtifactM | |||
| .defaultIfEmpty(FALSE) | |||
| .cache(); | |||
|
|
|||
| return Mono.defer(() -> gitRedisUtils.addFileLockWithoutRetry(defaultApplicationId)) | |||
| return Mono.defer(() -> gitRedisUtils.addFileLock(defaultApplicationId, FALSE)) | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The search results indicate that several instances of addFileLock across different files do not include the isRetryAllowed parameter. These instances need to be updated to use the new method signature.
Files and Instances to Update:
-
AutoCommitEventHandlerCEImpl.java
private Mono<Boolean> addFileLock(String defaultApplicationId).addFileLock(defaultApplicationId)return addFileLock(autoCommitEvent.getApplicationId())return addFileLock(defaultApplicationId)
-
CommonGitServiceCEImpl.java
return Mono.defer(() -> addFileLock(defaultArtifactId))private Mono<Boolean> addFileLock(String defaultArtifactId).addFileLock(defaultArtifactId)Mono<Boolean> fileLockMono = Mono.defer(() -> addFileLock(defaultArtifactId))addFileLockMono = addFileLock(defaultArtifactId)return addFileLock(tuple3.getT1().getDefaultArtifactId()).then(Mono.just(tuple3))return addFileLock(gitData.getDefaultArtifactId()).then(Mono.just(artifact))return addFileLock(artifact.getGitArtifactMetadata().getDefaultArtifactId())sourceAritfactMono = addFileLock(defaultArtifactId)Mono<? extends Artifact> checkoutRemoteBranchMono = addFileLock(defaultArtifactId)return addFileLock(srcBranchGitData.getDefaultArtifactId())return addFileLock(gitMetadata.getDefaultArtifactId()).flatMap(defaultArtifact -> addFileLock(defaultArtifactId).map(status -> defaultArtifact)).flatMap(branchedArtifact -> addFileLock(defaultArtifactId).thenReturn(branchedArtifact))return addFileLock(gitData.getDefaultArtifactId()).then(Mono.just(defaultArtifact))return addFileLock(defaultArtifactId)return addFileLock(metadata.getDefaultArtifactId())return addFileLock(defaultArtifact.getId())
-
RedisUtils.java
public Mono<Boolean> addFileLock(String key)
-
AutoCommitEligibilityHelperTest.java
Mockito.when(redisUtils.addFileLock(DEFAULT_APPLICATION_ID)).thenReturn(Mono.just(Boolean.TRUE))
Action Required:
Update the above instances to include the isRetryAllowed parameter in the method calls to addFileLock.
Analysis chain
Update to use the new addFileLock method signature with retry control.
Verify all instances of addFileLock are updated to use the new method signature with isRetryAllowed.
Verify specific instances of addFileLock without the isRetryAllowed parameter.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify integration of the new `addFileLock` method in the system.
# Test: Search for other usages of `addFileLock` to ensure they are updated.
rg --type java $'addFileLock'
Length of output: 10755
Script:
#!/bin/bash
# Description: Verify all instances of `addFileLock` are updated to use the new method signature with `isRetryAllowed`.
# Test: Search for method definitions and usages of `addFileLock` to ensure they include the `isRetryAllowed` parameter.
rg --type java 'addFileLock\('
Length of output: 8737
Script:
#!/bin/bash
# Description: Extract lines from specific files to verify if `addFileLock` needs updating.
# Files to check
files=(
"app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/AutoCommitEventHandlerCEImpl.java"
"app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CommonGitServiceCEImpl.java"
"app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedisUtils.java"
"app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/autocommit/AutoCommitEligibilityHelperTest.java"
)
# Extract lines containing `addFileLock` without `isRetryAllowed`
for file in "${files[@]}"; do
echo "Checking $file"
rg 'addFileLock\([^,]*\)' "$file"
done
Length of output: 3826
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (22)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CommonGitServiceCEImpl.java (20)
Line range hint
104-112: Consider simplifying the conditional logic for readability.private Mono<Boolean> addFileLock(String defaultArtifactId, boolean isLockRequired) { return isLockRequired ? addFileLock(defaultArtifactId) : Mono.just(Boolean.TRUE); }
Line range hint
118-126: Consider refactoring to enhance readability, similar toaddFileLock.private Mono<Boolean> releaseFileLock(String defaultArtifactId, boolean isLockRequired) { return isLockRequired ? releaseFileLock(defaultArtifactId) : Mono.just(Boolean.TRUE); }
Line range hint
158-163: Consider handling otherArtifactTypecases or throw an exception if unsupported types are provided.public GitArtifactHelper<?> getArtifactGitService(@NonNull ArtifactType artifactType) { if (artifactType != ArtifactType.APPLICATION) { throw new UnsupportedOperationException("Unsupported artifact type: " + artifactType); } return gitApplicationHelper; }
Line range hint
195-215: ValidategitArtifactMetadatabefore proceeding to avoid potential null pointer exceptions.if (gitArtifactMetadata == null) { return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "Git metadata cannot be null")); }
Line range hint
217-249: Refactor to simplify and improve error handling in the commit history retrieval logic.public Mono<List<GitLogDTO>> getCommitHistory(String branchName, String defaultArtifactId, ArtifactType artifactType) { // Simplified logic here }
Line range hint
251-423: Optimize thegetStatusmethod to reduce complexity and improve performance, especially in handling file locks and remote comparisons.protected Mono<GitStatusDTO> getStatus(Artifact defaultArtifact, Artifact branchedArtifact, String branchName, boolean isFileLock, boolean compareRemote) { // Optimized logic here }
Line range hint
425-511: RefactorfetchRemoteChangesto handle errors more gracefully and improve readability.public Mono<BranchTrackingStatus> fetchRemoteChanges(Artifact defaultArtifact, Artifact branchedArtifact, String branchName, boolean isFileLock) { // Refactored logic here }
Line range hint
513-713: Ensure robust validation ofgitConnectDTOto prevent issues with empty remote URLs or origin headers.if (StringUtils.isEmptyOrNull(gitConnectDTO.getRemoteUrl())) { return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "Remote Url")); } if (StringUtils.isEmptyOrNull(originHeader)) { return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ORIGIN)); }
Line range hint
715-1033: RefactorcommitArtifactto simplify the logic and improve error handling, especially in scenarios involving file locks and protected branches.private Mono<String> commitArtifact(GitCommitDTO commitDTO, String defaultArtifactId, String branchName, boolean doAmend, boolean isFileLock, ArtifactType artifactType) { // Simplified and improved logic here }
Line range hint
1035-1235: Enhance error recovery inpushArtifactto handle specific Git errors more effectively.protected Mono<String> pushArtifact(Artifact branchedArtifact, boolean doPublish, boolean isFileLock) { // Enhanced error recovery logic here }
Line range hint
1237-1249: EnsurepublishArtifacthandles thepublishflag correctly to avoid unnecessary operations.private Mono<? extends Artifact> publishArtifact(Artifact artifact, boolean publish) { if (!publish) { return Mono.just(artifact); } // Continue with publishing logic }
Line range hint
1251-1303: Refine the error messages inpushArtifactErrorRecoveryto provide more detailed information to the user.private Mono<String> pushArtifactErrorRecovery(String pushResult, Artifact artifact) { // Refined error messages here }
Line range hint
1305-1423: OptimizecheckoutBranchto handle remote branches more efficiently and improve error handling.public Mono<? extends Artifact> checkoutBranch(String defaultArtifactId, String branchName, boolean addFileLock, ArtifactType artifactType) { // Optimized logic for handling remote branches }
Line range hint
1535-1663: RefactorcreateBranchto simplify the branching logic and enhance error handling.public Mono<? extends Artifact> createBranch(String defaultArtifactId, GitBranchDTO branchDTO, String srcBranch, ArtifactType artifactType) { // Simplified branching logic here }
Line range hint
1665-1743: OptimizepullArtifactto handle merge conflicts more gracefully and improve the rehydration process.public Mono<GitPullDTO> pullArtifact(String defaultArtifactId, String branchName, ArtifactType artifactType) { // Optimized pull logic here }
Line range hint
1799-1877: Refine the validation logic inupdateOrCreateGitProfileForCurrentUserto handle edge cases more effectively.public Mono<Map<String, GitProfile>> updateOrCreateGitProfileForCurrentUser(GitProfile gitProfile, String defaultArtifactId) { // Refined validation logic here }
Line range hint
1897-1915: OptimizegetGitProfileForUserto efficiently retrieve and handle the Git profile for a specified user.public Mono<GitProfile> getGitProfileForUser(String defaultArtifactId) { // Optimized retrieval and handling logic here }
Line range hint
1955-2033: RefactoraddAnalyticsForGitOperationto simplify the construction and sending of analytics events.private Mono<? extends Artifact> addAnalyticsForGitOperation(AnalyticsEvents eventName, Artifact artifact, Boolean isRepoPrivate) { // Simplified construction and sending of analytics events }
Line range hint
2045-2073: RefactordeleteBranchto handle protected branches more effectively and improve error handling.public Mono<? extends Artifact> deleteBranch(String defaultArtifactId, String branchName, ArtifactType artifactType) { // Improved handling of protected branches and error handling }
Line range hint
2075-2113: OptimizediscardChangesto handle discarding changes more efficiently and improve the rehydration process.public Mono<? extends Artifact> discardChanges(String defaultArtifactId, String branchName, ArtifactType artifactType) { // Optimized discarding and rehydration logic }app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitServiceCEImpl.java (2)
46-46: Consider adding more detailed comments in the methodupdateOrCreateGitProfileForCurrentUserto explain the conditions under which errors are thrown. This will improve the maintainability of the code.
46-46: Consider adding more detailed logging in the methodgetCommitHistoryto aid in debugging and tracking the flow of operations.
| @@ -43,14 +43,14 @@ | |||
| import com.appsmith.server.exceptions.AppsmithError; | |||
| import com.appsmith.server.exceptions.AppsmithException; | |||
| import com.appsmith.server.exports.internal.ExportService; | |||
| import com.appsmith.server.git.autocommit.helpers.GitAutoCommitHelper; | |||
There was a problem hiding this comment.
The method commitApplication is complex and handles multiple scenarios. Consider refactoring to simplify the logic and improve readability, perhaps by breaking it down into smaller helper methods.
| @@ -54,7 +55,7 @@ public Mono<Boolean> isServerAutoCommitRequired(String workspaceId, GitArtifactM | |||
| .defaultIfEmpty(FALSE) | |||
| .cache(); | |||
|
|
|||
| return Mono.defer(() -> gitRedisUtils.addFileLockWithoutRetry(defaultApplicationId)) | |||
| return Mono.defer(() -> gitRedisUtils.addFileLock(defaultApplicationId, FALSE)) | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process has revealed that the addFileLock method has been updated to include the isRetryAllowed parameter in GitRedisUtils.java. However, there are multiple instances across the codebase where the old method signature is still being used without the new parameter.
- GitServiceCEImpl.java: Multiple instances of
addFileLockwithout theisRetryAllowedparameter. - CommonGitServiceCEImpl.java: Multiple instances of
addFileLockwithout theisRetryAllowedparameter. - AutoCommitEventHandlerCEImpl.java: Instances of
addFileLockwithout theisRetryAllowedparameter. - RedisUtils.java: Instances of
addFileLockwithout theisRetryAllowedparameter.
These instances need to be updated to ensure consistency with the new method signature.
Analysis chain
The updated usage of addFileLock with the isRetryAllowed parameter reflects the method signature change. Ensure that this change is handled correctly in all relevant parts of the application.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify usage of the updated addFileLock method across the project.
# Test: Search for the usage of addFileLock with the new parameter. Expect: Proper usage across the project.
rg --type java $'addFileLock'
Length of output: 10721
Script:
#!/bin/bash
# Description: Verify the method signature and usage of addFileLock in the identified files.
# Check the method signature in GitRedisUtils.java
echo "Checking method signature in GitRedisUtils.java"
rg --type java --context 5 'public Mono<Boolean> addFileLock' app/server/appsmith-server/src/main/java/com/appsmith/server/git/GitRedisUtils.java
# Check the usage in AutoCommitEligibilityHelperImpl.java
echo "Checking usage in AutoCommitEligibilityHelperImpl.java"
rg --type java --context 5 'addFileLock' app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/helpers/AutoCommitEligibilityHelperImpl.java
# Check the usage in GitServiceCEImpl.java
echo "Checking usage in GitServiceCEImpl.java"
rg --type java --context 5 'addFileLock' app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/GitServiceCEImpl.java
# Check the usage in CommonGitServiceCEImpl.java
echo "Checking usage in CommonGitServiceCEImpl.java"
rg --type java --context 5 'addFileLock' app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/CommonGitServiceCEImpl.java
# Check the usage in AutoCommitEventHandlerCEImpl.java
echo "Checking usage in AutoCommitEventHandlerCEImpl.java"
rg --type java --context 5 'addFileLock' app/server/appsmith-server/src/main/java/com/appsmith/server/git/AutoCommitEventHandlerCEImpl.java
# Check the usage in RedisUtils.java
echo "Checking usage in RedisUtils.java"
rg --type java --context 5 'addFileLock' app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/RedisUtils.java
Length of output: 29273
sondermanish
added
ok-to-test
Description
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/9284429088
Commit: 5130215
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?