chore: checkout remote #38390
chore: checkout remote #38390
Conversation
WalkthroughThe pull request introduces significant enhancements to the Git service layer in the Appsmith server, focusing on branch protection and auto-commit features. The changes primarily modify the Changes
Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(11 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java(4 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(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java(2 hunks)
🔇 Additional comments (34)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (1)
50-50: Check for backward compatibility due to parameter removal.
Removing the RefType refType parameter may break callers who rely on it. Confirm that all usages of listReferences have been updated accordingly and that none depend on the missing parameter's functionality. If you intend to maintain backward compatibility, consider overloading this method or communicating a clear migration path to other teams.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
621-626: Confirm whether checkout is truly required.
The TODO hints you may not always need to checkout the branch after fetching. If this is conditional or optional, consider branching logic or clarifying the intent to avoid unnecessary checkouts.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (8)
19-19: No issues with the added import.
The newly introduced AutoCommitConfig class is referenced properly.
29-29: Import convention is fine.
Nothing suspicious about adding AutoCommitResponseDTO.
35-35: Import looks correct.
The GitAutoCommitHelper dependency is needed for auto-commit operations.
56-56: Transactional support import acknowledged.
TransactionalOperator provides robust transactional behavior.
67-67: No issues with the added import.
This ArrayList import is consistent with standard usage.
93-93: Field addition is consistent.
Keeping gitRedisUtils as final aligns with your dependency injection pattern.
113-114: Properly introduced final fields.
gitAutoCommitHelper and transactionalOperator are standard dependencies for auto-commit & transactions.
1857-1979: Enhance user awareness in error handling.
When rejecting protection of multiple or non-default branches, consider making the error message more descriptive for better clarity.
Validate concurrency safety.
While transactionalOperator helps, verify that parallel updates to protected branches cannot result in inconsistent states or race conditions.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (7)
10-10: LGTM on import.
AutoCommitResponseDTO is properly introduced.
14-14: List import is fine.
No conflicts spotted with existing classes.
47-50: Signature update noted.
Switching to GitRefDTO parameter clarifies reference details and is consistent with the new approach.
58-60: New method: updateProtectedBranches.
Provides needed branch protection updates. Looks well-defined.
61-62: New method: getProtectedBranches.
Retrieves branch protection info. Straightforward addition.
63-64: New method: toggleAutoCommitEnabled.
Good to see a direct toggle approach for auto-commit.
65-66: New method: getAutoCommitProgress.
Offers insight into auto-commit status. Method naming is clear.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java (6)
6-6: Auto-commit helper import looks correct.
This import ensures usage of the newly introduced helper.
21-21: Transactional handling import.
Essential for ensuring atomic operations.
28-28: Constructor injection changes observed.
Reorganizing constructor parameters to meet new dependencies is standard practice.
42-43: Additional params for dependencies.
gitAutoCommitHelper & transactionalOperator are now part of the constructor, consistent with the updated logic.
46-46: Super constructor call.
Ensures inherited functionality remains intact after refactor.
60-61: Extended dependencies.
All references well-initialized in the superclass call.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.java (6)
6-6: Import for auto-commit.
This addition aligns with the new features introduced in CentralGitServiceCEImpl.
21-21: Transaction import is standard.
Indicates use of reactive transactions across methods.
29-29: Constructor change seen.
The shift in dependencies from GitRedisUtils to additional helpers is acceptable.
43-44: Parameters for helper & operator.
Appropriately injects GitAutoCommitHelper and transactionalOperator.
47-47: Super invocation.
Properly forwarding gitRedisUtils to the parent.
61-62: Dependency passing to superclass.
Maintains consistency with CentralGitServiceCEImpl.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java (5)
16-16: Added Flux & Mono.
Enables asynchronous event operations for analytics.
19-21: Additions for collection handling.
ArrayList and List are standard classes for storing new/old protected branches.
24-25: New analytics event constants.
GIT_ADD_PROTECTED_BRANCH & GIT_REMOVE_PROTECTED_BRANCH are aligned with your naming convention.
154-189: Comprehensive analytics method added.
sendBranchProtectionAnalytics cleanly logs newly protected vs removed branches. Implementation is succinct and uses flux merging for parallel event emission.
168-189: Fine-grained branch analytics.
Sending separate events for added or removed protected branches fosters detailed tracking.
| ArtifactJsonTransformationDTO artifactJsonTransformationDTO, Boolean checkRemoteReferences) { | ||
| if (RefType.BRANCH.equals(artifactJsonTransformationDTO.getRefType())) { |
There was a problem hiding this comment.
Complete the reactive chain for listReferences().
The current code calls listBranches(...) but does not return its result, instead returning an empty list. This likely breaks the intended functionality for listing references.
Here is a potential fix that returns the list of branch names when RefType.BRANCH is detected:
- if (RefType.BRANCH.equals(artifactJsonTransformationDTO.getRefType())) {
- listBranches(artifactJsonTransformationDTO, checkRemoteReferences);
- }
-
- return Mono.just(List.of());
+ if (RefType.BRANCH.equals(artifactJsonTransformationDTO.getRefType())) {
+ return listBranches(artifactJsonTransformationDTO, checkRemoteReferences);
+ } else {
+ // TODO: Add logic for other reference types (e.g., tags)
+ return Mono.just(List.of());
+ }Committable suggestion skipped: line range outside the PR's diff.
Updated the client version from 1 to 2 in the JsonSchemaVersionsFallback class to align with the latest schema changes. With introduction fo HTML column type in appsmith table, we don't want, apps exported from a newer version to be importable in the older version.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
298-299:⚠️ Potential issueFix missing return value in listReferences method.
The method calls
listBranchesbut doesn't return its result, which breaks the functionality for listing references.Apply this fix:
-if (RefType.BRANCH.equals(artifactJsonTransformationDTO.getRefType())) { - listBranches(artifactJsonTransformationDTO, checkRemoteReferences); -} +if (RefType.BRANCH.equals(artifactJsonTransformationDTO.getRefType())) { + return listBranches(artifactJsonTransformationDTO, checkRemoteReferences); +}
🧹 Nitpick comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (1)
61-69: Consider using a structured DTO for protected branch rulesWhile the current implementation using
List<String>works, consider creating aProtectedBranchDTOto support more complex branch protection rules in the future (e.g., required reviewers, build checks).Example structure:
public class ProtectedBranchDTO { private String branchName; private boolean requirePullRequest; private int requiredReviewers; // Add more rules as needed }app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (4)
Line range hint
58-70: UnusedPathvariable.
Path repoSuffixat line 1695 is declared but not used. Consider removing it if not needed.
378-387: Duplicate input validation.
Both this method and the overloaded one above validategitRefDTO. Consider consolidating to reduce duplication.
461-542: Multiple checkoutReference methods.
The approach is correct, but there's potential repetition across the codebase. A combined utility might be clearer.
2019-2052: Branch protection constraints.
Imposing only one protected branch (the default) may be a business requirement. If not, confirm it aligns with future plans.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(13 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java
🔇 Additional comments (13)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
622-627: Verify necessity of unconditional branch checkout.
The TODO comment suggests reviewing if branch checkout is always required after fetch. Consider making the checkout conditional based on the use case.
Run this script to analyze the usage patterns:
✅ Verification successful
Let me analyze the implementation and test cases to understand if the checkout is always required after fetch.
Let me analyze the implementation to understand if the checkout is always required and in what contexts it's being used.
Let me analyze the full context of the GitFSService implementation to understand the checkout requirement.
Branch checkout is required after fetch
Based on the test cases and implementation context, the checkout operation appears necessary after fetch to ensure the working directory reflects the fetched changes. The TODO comment can be removed since:
- The method is used in auto-commit and merge operations that require the working directory to be in sync
- Test cases verify branch tracking status after fetch, implying the need for checkout
- The method is part of GitFSService which handles file system operations
Note: If you want to optimize this in the future, consider adding a parameter to make checkout optional, but for current use cases, it's required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find callers of fetchRemoteChanges to understand checkout requirements
# Search for method calls
ast-grep --pattern 'fetchRemoteChanges($$$)'
# Search for tests to understand expected behavior
rg -l "fetchRemoteChanges.*test" -g "*.java"
Length of output: 66818
Script:
#!/bin/bash
# Search for fetchRemoteChanges method implementation and its callers
ast-grep --pattern 'fetchRemoteChanges($$$) {
$$$
}'
# Search for tests related to fetchRemoteChanges
rg -A 5 'fetchRemoteChanges' -g '*Test.java'
Length of output: 5212
Script:
#!/bin/bash
# Search for the implementation of fetchRemoteChanges to understand its usage context
rg -A 10 'fetchRemoteChanges.*\{' -g '*.java'
# Search for the implementation of checkoutToBranch to understand its purpose
ast-grep --pattern 'checkoutToBranch($$$) {
$$$
}'
Length of output: 1844
Script:
#!/bin/bash
# Search for the GitFSServiceCEImpl class implementation to understand the context
rg -A 20 'class GitFSServiceCEImpl' -g '*.java'
# Search for the specific method implementation
rg -B 5 -A 20 'fetchRemoteChanges.*\{.*repoSuffix' -g '*.java'
Length of output: 2986
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (2)
10-15: LGTM: Required imports added for new functionality
46-53: LGTM: Enhanced type safety in Git operations
The changes improve type safety by:
- Using GitPullDTO for structured pull responses
- Replacing String reference with GitRefDTO for checkout operations
Let's verify the GitRefDTO usage across the codebase:
✅ Verification successful
Proceed with the type-safe changes
The changes to use GitRefDTO and structured GitPullDTO responses are safe to implement. No legacy String-based reference checkouts were found in the codebase that would need updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining String-based reference checkouts that might need updating
ast-grep --pattern 'checkout$_($_, String $_)'
Length of output: 48
Script:
#!/bin/bash
# Let's try a different pattern to find checkout method implementations and usages
ast-grep --pattern 'checkoutReference($$$)'
# Also search for GitRefDTO usage
rg "GitRefDTO" -A 2
Length of output: 66313
Script:
#!/bin/bash
# Let's search for GitRefDTO class definition and usages
ast-grep --pattern 'class GitRefDTO'
# Also search for checkoutReference method implementations
ast-grep --pattern 'checkoutReference($_, $_, $_, $_, $_)'
Length of output: 802
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (10)
Line range hint 6-37: No issues with the newly added imports.
They look properly utilized in subsequent code references.
Line range hint 96-117: Initialization looks good.
All newly introduced dependencies appear valid and properly referenced.
355-367: Method signature is consistent.
Input validation is handled correctly. Consider adding JavaDoc for clarity if needed.
373-373: Direct delegation is clean.
This single-line return helps keep logic centralized in the protected method. Nicely done.
396-396: Validate preconditions on refName replacement.
Ensure the user-supplied ref indeed starts with "origin/" before replacing.
415-431: Logic for remote vs local references looks sound.
Ensure coverage in unit tests for edge cases (e.g., ref already local).
2054-2072: Straightforward update.
The transactional flow is appropriate. The analytics step is a nice touch.
2073-2098: Single default branch protection.
Consistently returning only the default branch if protected. This implementation matches the current business logic.
2099-2136: Concurrency consideration for auto-commit toggle.
If multiple toggles occur in parallel, the final state might be uncertain. Evaluate synchronization if this is a concern.
2137-2141: Pass-through method for auto-commit progress.
Implementation is minimal yet effective.
|
|
||
| @Override | ||
| public Mono<MergeStatusDTO> pullArtifact( | ||
| ArtifactJsonTransformationDTO jsonTransformationDTO, GitArtifactMetadata baseMetadata) { | ||
| GitArtifactHelper<?> gitArtifactHelper = | ||
| gitArtifactHelperResolver.getArtifactHelper(jsonTransformationDTO.getArtifactType()); | ||
|
|
||
| Path repoSuffix = gitArtifactHelper.getRepoSuffixPath( | ||
| jsonTransformationDTO.getWorkspaceId(), | ||
| jsonTransformationDTO.getBaseArtifactId(), | ||
| jsonTransformationDTO.getRepoName()); | ||
|
|
||
| String branchName = jsonTransformationDTO.getRefName(); | ||
|
|
||
| // git checkout and pull origin branchName | ||
| try { | ||
| return fsGitHandler | ||
| .checkoutToBranch(repoSuffix, jsonTransformationDTO.getRefName()) | ||
| .then(fsGitHandler.pullApplication( | ||
| repoSuffix, | ||
| baseMetadata.getRemoteUrl(), | ||
| branchName, | ||
| baseMetadata.getGitAuth().getPrivateKey(), | ||
| baseMetadata.getGitAuth().getPublicKey())) | ||
| .onErrorResume(error -> { | ||
| if (error.getMessage().contains("conflict")) { | ||
| return Mono.error( | ||
| new AppsmithException(AppsmithError.GIT_PULL_CONFLICTS, error.getMessage())); | ||
| } else if (error.getMessage().contains("Nothing to fetch")) { | ||
| MergeStatusDTO mergeStatus = new MergeStatusDTO(); | ||
| mergeStatus.setStatus("Nothing to fetch from remote. All changes are up to date."); | ||
| mergeStatus.setMergeAble(true); | ||
| return Mono.just(mergeStatus); | ||
| } | ||
| return Mono.error( | ||
| new AppsmithException(AppsmithError.GIT_ACTION_FAILED, "pull", error.getMessage())); | ||
| }); | ||
| } catch (IOException e) { | ||
| return Mono.error(new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, e.getMessage())); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in pullArtifact method.
The error handling can be enhanced in several ways:
- Remove the redundant IOException try-catch as fsGitHandler methods already handle these exceptions
- Use more robust error detection than string contains
- Provide more specific error messages for different types of conflicts
Apply these improvements:
- try {
return fsGitHandler
.checkoutToBranch(repoSuffix, jsonTransformationDTO.getRefName())
.then(fsGitHandler.pullApplication(
repoSuffix,
baseMetadata.getRemoteUrl(),
branchName,
baseMetadata.getGitAuth().getPrivateKey(),
baseMetadata.getGitAuth().getPublicKey()))
.onErrorResume(error -> {
- if (error.getMessage().contains("conflict")) {
+ if (error instanceof org.eclipse.jgit.api.errors.CheckoutConflictException) {
return Mono.error(
- new AppsmithException(AppsmithError.GIT_PULL_CONFLICTS, error.getMessage()));
+ new AppsmithException(AppsmithError.GIT_PULL_CONFLICTS,
+ "Conflicts detected in: " + error.getMessage()));
- } else if (error.getMessage().contains("Nothing to fetch")) {
+ } else if (error instanceof org.eclipse.jgit.api.errors.NoHeadException) {
MergeStatusDTO mergeStatus = new MergeStatusDTO();
mergeStatus.setStatus("Nothing to fetch from remote. All changes are up to date.");
mergeStatus.setMergeAble(true);
return Mono.just(mergeStatus);
}
return Mono.error(
new AppsmithException(AppsmithError.GIT_ACTION_FAILED, "pull", error.getMessage()));
});
- } catch (IOException e) {
- return Mono.error(new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, e.getMessage()));
- }Committable suggestion skipped: line range outside the PR's diff.
| .releaseFileLock(branchedGitMetadata.getDefaultArtifactId(), TRUE) | ||
| .then(Mono.just(gitPullDTO))); | ||
| }) | ||
| .name(GitSpan.OPS_PULL) |
There was a problem hiding this comment.
Release file lock in case of error
| String branchName = jsonTransformationDTO.getRefName(); | ||
|
|
||
| // git checkout and pull origin branchName | ||
| try { |
There was a problem hiding this comment.
Why is there a try-catch here?
There was a problem hiding this comment.
Since the handler layer throws the IO Exception, we are using try catch, do you mean that we should switch to reactive error handling?
| RefType refType) { | ||
| if (RefType.BRANCH.equals(refType)) { | ||
| ArtifactJsonTransformationDTO artifactJsonTransformationDTO, Boolean checkRemoteReferences) { | ||
| if (RefType.BRANCH.equals(artifactJsonTransformationDTO.getRefType())) { | ||
| listBranches(artifactJsonTransformationDTO, checkRemoteReferences); |
There was a problem hiding this comment.
I think we're missing return here?
| .publishArtifact(importedBranchedArtifact, false) | ||
| // TODO: Verify if we need to commit after pulling? (Gonna be a product | ||
| // decision, hence got | ||
| .then(commitArtifact( |
There was a problem hiding this comment.
Add a defer here to not tigger this code pre-emptively
|
|
||
| Mono<GitPullDTO> pullDTOMono = gitRedisUtils | ||
| .acquireGitLock(branchedGitMetadata.getDefaultArtifactId(), GitConstants.GitCommandConstants.PULL, TRUE) | ||
| .then(statusMono) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
2042-2044: Add null check before list operations.Consider adding explicit null check before creating new ArrayList to prevent potential NPE.
- final List<String> incomingProtectedBranches = - CollectionUtils.isEmpty(branchNames) ? new ArrayList<>() : branchNames; + final List<String> incomingProtectedBranches = new ArrayList<>(); + if (branchNames != null) { + incomingProtectedBranches.addAll(branchNames); + }
2106-2108: Add Javadoc for the auto-commit toggle method.Consider adding comprehensive Javadoc to document the behavior, parameters, and return value of this public method.
+ /** + * Toggles the auto-commit feature for a given artifact. + * + * @param baseArtifactId The ID of the base artifact + * @param artifactType The type of the artifact + * @return A Mono<Boolean> indicating the new state of auto-commit (true if enabled, false if disabled) + */ @Override public Mono<Boolean> toggleAutoCommitEnabled(String baseArtifactId, ArtifactType artifactType) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(13 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(4 hunks)
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
6-6: LGTM! New dependencies are properly integrated.
The new imports and dependency injections for MergeStatusDTO, AutoCommitConfig, GitAutoCommitHelper, and TransactionalOperator are correctly implemented.
Also applies to: 20-20, 37-37, 58-58, 116-117
1627-1627: 🛠️ Refactor suggestion
Consider moving the defer to avoid premature execution.
The Mono.defer() should wrap the entire operation to prevent premature execution of the status check.
- .then(Mono.defer(() ->statusMono))
+ .then(Mono.defer(() -> {
+ return statusMono;
+ }))Likely invalid or redundant comment.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (3)
298-303: LGTM! Note the pending TODO for other reference types.
The reactive chain is now properly completed by returning the result of listBranches(). However, support for other reference types (e.g., tags) is still pending.
736-760: 🛠️ Refactor suggestion
Improve error handling in pullArtifact method.
Several improvements can be made to the error handling:
- Replace string-based error detection with specific exception types
- Remove redundant IOException try-catch block
- Provide more specific error messages
Apply this improvement:
-try {
return fsGitHandler
.checkoutToBranch(repoSuffix, jsonTransformationDTO.getRefName())
.then(fsGitHandler.pullApplication(
repoSuffix,
baseMetadata.getRemoteUrl(),
branchName,
baseMetadata.getGitAuth().getPrivateKey(),
baseMetadata.getGitAuth().getPublicKey()))
.onErrorResume(error -> {
- if (error.getMessage().contains("conflict")) {
+ if (error instanceof org.eclipse.jgit.api.errors.CheckoutConflictException) {
return Mono.error(
- new AppsmithException(AppsmithError.GIT_PULL_CONFLICTS, error.getMessage()));
+ new AppsmithException(AppsmithError.GIT_PULL_CONFLICTS,
+ "Conflicts detected in: " + error.getMessage()));
- } else if (error.getMessage().contains("Nothing to fetch")) {
+ } else if (error instanceof org.eclipse.jgit.api.errors.NoHeadException) {
MergeStatusDTO mergeStatus = new MergeStatusDTO();
mergeStatus.setStatus("Nothing to fetch from remote. All changes are up to date.");
mergeStatus.setMergeAble(true);
return Mono.just(mergeStatus);
}
return Mono.error(
new AppsmithException(AppsmithError.GIT_ACTION_FAILED, "pull", error.getMessage()));
});
-} catch (IOException e) {
- return Mono.error(new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, e.getMessage()));
-}Likely invalid or redundant comment.
622-627: Verify if checkout is required after fetch.
The separation of fetch and checkout operations is good. However, as noted in the TODO, please verify if the checkout step is necessary in all cases. Some Git workflows might not require immediate checkout after fetch.
Run this script to check if there are any direct callers that depend on the checkout behavior:
✅ Verification successful
Let me search for more context about the usage and requirements of these methods to determine if the checkout step is necessary.
Checkout is required after fetch for this implementation
Based on the code analysis:
- The checkout is required because:
- The
checkoutToBranchoperation is used in multiple critical operations like pull, merge, and reset - Tests verify the checkout behavior after fetch
- The
GitExecutorinterface defines this as part of the core Git operations workflow
- The
The TODO comment can be removed as the checkout step is a necessary part of the Git workflow in this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct callers of fetchRemoteChanges
ast-grep --pattern 'fetchRemoteChanges($$$)'
Length of output: 66437
Script:
#!/bin/bash
# Search for direct callers of fetchRemoteChanges to understand the checkout requirement
rg -A 5 "fetchRemoteChanges" --type java
# Search for checkoutToBranch usage to understand its purpose
rg -A 5 "checkoutToBranch" --type java
# Search for any tests related to fetchRemoteChanges
rg -A 5 "test.*fetchRemoteChanges" --type java
Length of output: 70705
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
461-473: Consider adding retry mechanism for remote operationsThe checkout remote implementation could benefit from a retry mechanism for transient network issues.
Consider wrapping the remote operations with a retry mechanism:
+ @Retryable( + value = { TransportException.class }, + maxAttempts = 3, + backoff = @Backoff(delay = 1000)) protected Mono<? extends Artifact> checkoutRemoteReference( String baseArtifactId, GitRefDTO gitRefDTO, ArtifactType artifactType, GitType gitType) {
2109-2151: Add Javadoc for auto-commit methodsThe auto-commit implementation would benefit from detailed Javadoc explaining the feature's behavior and limitations.
Add Javadoc:
+/** + * Toggles the auto-commit feature for the specified artifact. + * When enabled, changes will be automatically committed based on the configured rules. + * + * @param baseArtifactId The ID of the base artifact + * @param artifactType The type of the artifact + * @return A Mono<Boolean> indicating whether auto-commit is enabled + */ @Override public Mono<Boolean> toggleAutoCommitEnabled(String baseArtifactId, ArtifactType artifactType) {
📜 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(13 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
Line range hint 96-117: Ensure proper file lock release in error scenarios
The GitRedisUtils dependency is being replaced with GitAutoCommitHelper and TransactionalOperator. While the changes look good, ensure that file locks are properly released in all error scenarios to prevent deadlocks.
✅ Verification successful
The previous search was too specific. Let's broaden our search to understand the file locking mechanisms and error handling patterns in the codebase. This will help us verify if there are proper error handling mechanisms in place for file locks.
Let's search for error handling patterns and file lock release mechanisms in transactional contexts to verify if proper error handling is in place.
File locks are properly handled in error scenarios
The codebase shows comprehensive error handling with proper file lock release mechanisms:
- Global error handlers manage file lock release, as evidenced by comments in
CommonGitServiceCEImpl.java:
/*
in case of any error, the global exception handler will release the lock
hence we don't need to do that manually
*/- Explicit error handling with lock release in critical paths:
Mono<Boolean> releaseLockMono = gitRedisUtils.releaseFileLock(...);
return releaseLockMono.then(Mono.error(new AppsmithException(...)));- Error propagation with proper cleanup in reactive chains using
onErrorResume.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for file lock release patterns in error handlers
rg -A 5 "releaseFileLock.*\.onErrorResume"
Length of output: 44
Script:
#!/bin/bash
# Let's search for file lock patterns more broadly
rg -A 5 "fileLock|lockFile"
# Also search for error handling patterns in the Git service
ast-grep --pattern 'catch ($err) { $$$ }'
# Search for onError patterns in reactive code
rg -A 5 "onError|doOnError"
Length of output: 74204
Script:
#!/bin/bash
# Let's search for error handling patterns in the Git service implementation
rg -A 5 "onErrorResume|doOnError|Mono\.error" app/server/appsmith-server/src/main/java/com/appsmith/server/git/
# Search for file lock release patterns in transactional contexts
rg -A 5 "transactionalOperator.*fileLock" app/server/appsmith-server/src/main/java/com/appsmith/server/git/
# Look for file lock handling in error scenarios
ast-grep --pattern 'Mono.defer(() => releaseFileLock($$$)).onErrorResume($$$)'
Length of output: 65840
| @Override | ||
| public Mono<List<String>> updateProtectedBranches( | ||
| String baseArtifactId, List<String> branchNames, ArtifactType artifactType) { | ||
|
|
||
| GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); | ||
| AclPermission artifactManageProtectedBranchPermission = | ||
| gitArtifactHelper.getArtifactManageProtectedBranchPermission(); | ||
|
|
||
| Mono<? extends Artifact> baseArtifactMono = | ||
| gitArtifactHelper.getArtifactById(baseArtifactId, artifactManageProtectedBranchPermission); | ||
|
|
||
| return baseArtifactMono | ||
| .flatMap(baseArtifact -> { | ||
| GitArtifactMetadata baseGitData = baseArtifact.getGitArtifactMetadata(); | ||
| final String defaultBranchName = baseGitData.getDefaultBranchName(); | ||
| final List<String> incomingProtectedBranches = | ||
| CollectionUtils.isEmpty(branchNames) ? new ArrayList<>() : branchNames; | ||
|
|
||
| // user cannot protect multiple branches | ||
| if (incomingProtectedBranches.size() > 1) { | ||
| return Mono.error(new AppsmithException(AppsmithError.UNSUPPORTED_OPERATION)); | ||
| } | ||
|
|
||
| // user cannot protect a branch which is not default | ||
| if (incomingProtectedBranches.size() == 1 | ||
| && !defaultBranchName.equals(incomingProtectedBranches.get(0))) { | ||
| return Mono.error(new AppsmithException(AppsmithError.UNSUPPORTED_OPERATION)); | ||
| } | ||
|
|
||
| return updateProtectedBranchesInArtifactAfterVerification(baseArtifact, incomingProtectedBranches); | ||
| }) | ||
| .as(transactionalOperator::transactional); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for branch name format
The branch protection implementation should validate the branch name format before processing.
Add branch name validation:
public Mono<List<String>> updateProtectedBranches(
String baseArtifactId, List<String> branchNames, ArtifactType artifactType) {
+ if (!branchNames.stream().allMatch(name -> name.matches("^[\\w.-]+$"))) {
+ return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "Invalid branch name format"));
+ }📝 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.
| @Override | |
| public Mono<List<String>> updateProtectedBranches( | |
| String baseArtifactId, List<String> branchNames, ArtifactType artifactType) { | |
| GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); | |
| AclPermission artifactManageProtectedBranchPermission = | |
| gitArtifactHelper.getArtifactManageProtectedBranchPermission(); | |
| Mono<? extends Artifact> baseArtifactMono = | |
| gitArtifactHelper.getArtifactById(baseArtifactId, artifactManageProtectedBranchPermission); | |
| return baseArtifactMono | |
| .flatMap(baseArtifact -> { | |
| GitArtifactMetadata baseGitData = baseArtifact.getGitArtifactMetadata(); | |
| final String defaultBranchName = baseGitData.getDefaultBranchName(); | |
| final List<String> incomingProtectedBranches = | |
| CollectionUtils.isEmpty(branchNames) ? new ArrayList<>() : branchNames; | |
| // user cannot protect multiple branches | |
| if (incomingProtectedBranches.size() > 1) { | |
| return Mono.error(new AppsmithException(AppsmithError.UNSUPPORTED_OPERATION)); | |
| } | |
| // user cannot protect a branch which is not default | |
| if (incomingProtectedBranches.size() == 1 | |
| && !defaultBranchName.equals(incomingProtectedBranches.get(0))) { | |
| return Mono.error(new AppsmithException(AppsmithError.UNSUPPORTED_OPERATION)); | |
| } | |
| return updateProtectedBranchesInArtifactAfterVerification(baseArtifact, incomingProtectedBranches); | |
| }) | |
| .as(transactionalOperator::transactional); | |
| } | |
| @Override | |
| public Mono<List<String>> updateProtectedBranches( | |
| String baseArtifactId, List<String> branchNames, ArtifactType artifactType) { | |
| if (!branchNames.stream().allMatch(name -> name.matches("^[\\w.-]+$"))) { | |
| return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "Invalid branch name format")); | |
| } | |
| GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); | |
| AclPermission artifactManageProtectedBranchPermission = | |
| gitArtifactHelper.getArtifactManageProtectedBranchPermission(); | |
| Mono<? extends Artifact> baseArtifactMono = | |
| gitArtifactHelper.getArtifactById(baseArtifactId, artifactManageProtectedBranchPermission); | |
| return baseArtifactMono | |
| .flatMap(baseArtifact -> { | |
| GitArtifactMetadata baseGitData = baseArtifact.getGitArtifactMetadata(); | |
| final String defaultBranchName = baseGitData.getDefaultBranchName(); | |
| final List<String> incomingProtectedBranches = | |
| CollectionUtils.isEmpty(branchNames) ? new ArrayList<>() : branchNames; | |
| // user cannot protect multiple branches | |
| if (incomingProtectedBranches.size() > 1) { | |
| return Mono.error(new AppsmithException(AppsmithError.UNSUPPORTED_OPERATION)); | |
| } | |
| // user cannot protect a branch which is not default | |
| if (incomingProtectedBranches.size() == 1 | |
| && !defaultBranchName.equals(incomingProtectedBranches.get(0))) { | |
| return Mono.error(new AppsmithException(AppsmithError.UNSUPPORTED_OPERATION)); | |
| } | |
| return updateProtectedBranchesInArtifactAfterVerification(baseArtifact, incomingProtectedBranches); | |
| }) | |
| .as(transactionalOperator::transactional); | |
| } |
…8391) ## Description Updated the client version from 1 to 2 in the JsonSchemaVersionsFallback class to align with the latest schema changes. With introduction fo HTML column type in appsmith table, we don't want, apps exported from a newer version to be importable in the older version. Fixes #38311 _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12581380732> > Commit: 51523c9 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12581380732&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Thu, 02 Jan 2025 11:33:56 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Updated client version to reflect changes in the application. - Incremented client schema version to align with application updates. - **Bug Fixes** - Corrected the static client version to ensure accurate version reporting. - **Tests** - Enhanced test setup to initialize the application with the current client schema version. - Updated expected values in tests to reflect the new client schema version. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
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/12546096629
Commit: 34136a4
Cypress dashboard.
Tags:
@tag.GitSpec:
Mon, 30 Dec 2024 12:34:51 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores