chore: fetch remote reference in local #38009
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe changes in this pull request introduce new fields and methods across several classes related to Git operations. The Changes
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
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 (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (1)
28-33: LGTM! Consider adding method documentation.The new method signature follows reactive patterns and includes all necessary parameters for fetch operations.
Consider adding Javadoc to describe the purpose of each parameter and the expected return value.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/GitArtifactMetadataCE.java(2 hunks)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(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java(3 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)app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java(2 hunks)
🔇 Additional comments (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/GitArtifactMetadataCE.java (1)
28-37: LGTM! Note the TODOs for future visibility changes.
The new fields are properly annotated for internal use. The TODO comments clearly indicate the plan to make these fields public once implementation is complete.
Let's verify if these fields are used consistently across the codebase:
✅ Verification successful
LGTM! The new fields are properly used across the codebase.
The fields refName and refType are used consistently in the Git-related functionality:
- Used in
ArtifactJsonTransformationDTOfor JSON transformations - Utilized in
CentralGitServiceCEfor remote changes fetching - Properly handled in
GitFSServiceCEImplfor Git filesystem operations
The transient and internal view annotations are appropriate as these fields are only used internally within the Git-related services.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check usage of refName and refType
ast-grep --pattern 'class $_ {
$$$
$_(refName|refType)
$$$
}'
Length of output: 69
Script:
#!/bin/bash
# Search for usage of refName and refType fields
rg "refName|refType" -A 2 -B 2
# Search for classes that might use these fields
ast-grep --pattern 'class $_ extends GitArtifactMetadata'
# Check for any references to GitArtifactMetadata
rg "GitArtifactMetadata" -A 2 -B 2
Length of output: 101465
app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java (1)
127-148: LGTM! Analytics implementation is clean and comprehensive.
The method properly constructs the analytics data map with all relevant Git metadata and follows existing patterns.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (1)
35-36: LGTM! Method overloading is appropriate.
The overloaded version with direct Artifact parameters provides a convenient alternative for cases where artifacts are already loaded.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java (1)
29-29: LGTM! Clean dependency injection.
The addition of SessionUserService follows proper dependency injection patterns and is correctly propagated to the superclass.
Also applies to: 45-45
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (1)
60-60: LGTM! Verify error handling in implementations.
The method signature is well-designed for Git operations.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.java (1)
13-13: Clean implementation of SessionUserService integration
The addition of SessionUserService follows proper dependency injection patterns and maintains clean code structure.
Also applies to: 30-30, 46-46
Comment on lines
972
to
973
| jsonTransformationDTO.setRefType(RefType.BRANCH); | ||
| jsonTransformationDTO.setWorkspaceId(baseArtifact.getWorkspaceId()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant assignment of refType in jsonTransformationDTO
The refType is set twice; you can remove the initial assignment.
ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO();
- jsonTransformationDTO.setRefType(RefType.BRANCH);
jsonTransformationDTO.setWorkspaceId(baseArtifact.getWorkspaceId());
jsonTransformationDTO.setBaseArtifactId(baseArtifactGitData.getDefaultArtifactId());
jsonTransformationDTO.setRepoName(baseArtifactGitData.getRepoName());
jsonTransformationDTO.setArtifactType(baseArtifact.getArtifactType());
jsonTransformationDTO.setRefName(refArtifactGitData.getRefName());
jsonTransformationDTO.setRefType(refType);Also applies to: 978-978
Comment on lines
+572
to
+591
| @Override | ||
| public Mono<String> fetchRemoteChanges(ArtifactJsonTransformationDTO jsonTransformationDTO, GitAuth gitAuth) { | ||
|
|
||
| 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); | ||
|
|
||
| Path repoPath = fsGitHandler.createRepoPath(repoSuffix); | ||
| Mono<Boolean> checkoutBranchMono = fsGitHandler.checkoutToBranch(repoSuffix, refName); | ||
|
|
||
| Mono<String> fetchRemoteMono = fsGitHandler.fetchRemote( | ||
| repoPath, gitAuth.getPublicKey(), gitAuth.getPrivateKey(), true, refName, false); | ||
|
|
||
| return checkoutBranchMono.then(Mono.defer(() -> fetchRemoteMono)); | ||
| } |
There was a problem hiding this comment.
Add error handling for checkout and fetch operations.
While the implementation sequence is correct, the method should handle potential failures in checkout and fetch operations, similar to error handling patterns used in other methods of this class (e.g., pushArtifact).
Consider adding error handling:
@Override
public Mono<String> fetchRemoteChanges(ArtifactJsonTransformationDTO jsonTransformationDTO, GitAuth gitAuth) {
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);
Path repoPath = fsGitHandler.createRepoPath(repoSuffix);
- Mono<Boolean> checkoutBranchMono = fsGitHandler.checkoutToBranch(repoSuffix, refName);
+ Mono<Boolean> checkoutBranchMono = fsGitHandler.checkoutToBranch(repoSuffix, refName)
+ .onErrorResume(error -> {
+ log.error("Error while checking out branch: {}", error.getMessage());
+ return gitAnalyticsUtils
+ .addAnalyticsForGitOperation(
+ AnalyticsEvents.GIT_CHECKOUT,
+ jsonTransformationDTO,
+ error.getClass().getName(),
+ error.getMessage(),
+ false)
+ .then(Mono.error(new AppsmithException(
+ AppsmithError.GIT_ACTION_FAILED,
+ "checkout",
+ error.getMessage())));
+ });
Mono<String> fetchRemoteMono = fsGitHandler.fetchRemote(
repoPath, gitAuth.getPublicKey(), gitAuth.getPrivateKey(), true, refName, false);
- return checkoutBranchMono.then(Mono.defer(() -> fetchRemoteMono));
+ return checkoutBranchMono.then(Mono.defer(() -> fetchRemoteMono))
+ .onErrorResume(error -> {
+ log.error("Error while fetching remote changes: {}", error.getMessage());
+ return gitAnalyticsUtils
+ .addAnalyticsForGitOperation(
+ AnalyticsEvents.GIT_FETCH,
+ jsonTransformationDTO,
+ error.getClass().getName(),
+ error.getMessage(),
+ false)
+ .then(Mono.error(new AppsmithException(
+ AppsmithError.GIT_ACTION_FAILED,
+ "fetch",
+ error.getMessage())));
+ });
}📝 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
| @Override | |
| public Mono<String> fetchRemoteChanges(ArtifactJsonTransformationDTO jsonTransformationDTO, GitAuth gitAuth) { | |
| 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); | |
| Path repoPath = fsGitHandler.createRepoPath(repoSuffix); | |
| Mono<Boolean> checkoutBranchMono = fsGitHandler.checkoutToBranch(repoSuffix, refName); | |
| Mono<String> fetchRemoteMono = fsGitHandler.fetchRemote( | |
| repoPath, gitAuth.getPublicKey(), gitAuth.getPrivateKey(), true, refName, false); | |
| return checkoutBranchMono.then(Mono.defer(() -> fetchRemoteMono)); | |
| } | |
| @Override | |
| public Mono<String> fetchRemoteChanges(ArtifactJsonTransformationDTO jsonTransformationDTO, GitAuth gitAuth) { | |
| 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); | |
| Path repoPath = fsGitHandler.createRepoPath(repoSuffix); | |
| Mono<Boolean> checkoutBranchMono = fsGitHandler.checkoutToBranch(repoSuffix, refName) | |
| .onErrorResume(error -> { | |
| log.error("Error while checking out branch: {}", error.getMessage()); | |
| return gitAnalyticsUtils | |
| .addAnalyticsForGitOperation( | |
| AnalyticsEvents.GIT_CHECKOUT, | |
| jsonTransformationDTO, | |
| error.getClass().getName(), | |
| error.getMessage(), | |
| false) | |
| .then(Mono.error(new AppsmithException( | |
| AppsmithError.GIT_ACTION_FAILED, | |
| "checkout", | |
| error.getMessage()))); | |
| }); | |
| Mono<String> fetchRemoteMono = fsGitHandler.fetchRemote( | |
| repoPath, gitAuth.getPublicKey(), gitAuth.getPrivateKey(), true, refName, false); | |
| return checkoutBranchMono.then(Mono.defer(() -> fetchRemoteMono)) | |
| .onErrorResume(error -> { | |
| log.error("Error while fetching remote changes: {}", error.getMessage()); | |
| return gitAnalyticsUtils | |
| .addAnalyticsForGitOperation( | |
| AnalyticsEvents.GIT_FETCH, | |
| jsonTransformationDTO, | |
| error.getClass().getName(), | |
| error.getMessage(), | |
| false) | |
| .then(Mono.error(new AppsmithException( | |
| AppsmithError.GIT_ACTION_FAILED, | |
| "fetch", | |
| error.getMessage()))); | |
| }); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(1 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
579-597:
Add error handling and analytics tracking.
The implementation needs error handling for both checkout and fetch operations, similar to other methods in this class.
Apply this diff to add error handling:
@Override
public Mono<String> fetchRemoteChanges(ArtifactJsonTransformationDTO jsonTransformationDTO, GitAuth gitAuth) {
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);
Path repoPath = fsGitHandler.createRepoPath(repoSuffix);
- Mono<Boolean> checkoutBranchMono = fsGitHandler.checkoutToBranch(repoSuffix, refName);
+ Mono<Boolean> checkoutBranchMono = fsGitHandler.checkoutToBranch(repoSuffix, refName)
+ .onErrorResume(error -> {
+ log.error("Error while checking out branch: {}", error.getMessage());
+ return gitAnalyticsUtils
+ .addAnalyticsForGitOperation(
+ AnalyticsEvents.GIT_CHECKOUT,
+ jsonTransformationDTO,
+ error.getClass().getName(),
+ error.getMessage(),
+ false)
+ .then(Mono.error(new AppsmithException(
+ AppsmithError.GIT_ACTION_FAILED,
+ "checkout",
+ error.getMessage())));
+ });
Mono<String> fetchRemoteMono = fsGitHandler.fetchRemote(
repoPath, gitAuth.getPublicKey(), gitAuth.getPrivateKey(), true, refName, false);
- return checkoutBranchMono.then(Mono.defer(() -> fetchRemoteMono));
+ return checkoutBranchMono
+ .then(Mono.defer(() -> fetchRemoteMono))
+ .onErrorResume(error -> {
+ log.error("Error while fetching remote changes: {}", error.getMessage());
+ return gitAnalyticsUtils
+ .addAnalyticsForGitOperation(
+ AnalyticsEvents.GIT_FETCH,
+ jsonTransformationDTO,
+ error.getClass().getName(),
+ error.getMessage(),
+ false)
+ .then(Mono.error(new AppsmithException(
+ AppsmithError.GIT_ACTION_FAILED,
+ "fetch",
+ error.getMessage())));
+ });
Comment on lines
+579
to
+597
| public Mono<String> fetchRemoteChanges(ArtifactJsonTransformationDTO jsonTransformationDTO, GitAuth gitAuth) { | ||
|
|
||
| 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); | ||
|
|
||
| Path repoPath = fsGitHandler.createRepoPath(repoSuffix); | ||
| Mono<Boolean> checkoutBranchMono = fsGitHandler.checkoutToBranch(repoSuffix, refName); | ||
|
|
||
| Mono<String> fetchRemoteMono = fsGitHandler.fetchRemote( | ||
| repoPath, gitAuth.getPublicKey(), gitAuth.getPrivateKey(), true, refName, false); | ||
|
|
||
| return checkoutBranchMono.then(Mono.defer(() -> fetchRemoteMono)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add file locking mechanism for concurrent operations.
The method should implement file locking similar to other Git operations in this class to prevent concurrent modifications.
Apply this diff to add file locking:
@Override
public Mono<String> fetchRemoteChanges(ArtifactJsonTransformationDTO jsonTransformationDTO, GitAuth gitAuth) {
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);
Path repoPath = fsGitHandler.createRepoPath(repoSuffix);
- Mono<Boolean> checkoutBranchMono = fsGitHandler.checkoutToBranch(repoSuffix, refName);
+ return addFileLock(baseArtifactId, GitConstants.GitCommandConstants.FETCH, true)
+ .flatMap(isLocked -> {
+ Mono<Boolean> checkoutBranchMono = fsGitHandler.checkoutToBranch(repoSuffix, refName);
+ Mono<String> fetchRemoteMono = fsGitHandler.fetchRemote(
+ repoPath, gitAuth.getPublicKey(), gitAuth.getPrivateKey(), true, refName, false);
- Mono<String> fetchRemoteMono = fsGitHandler.fetchRemote(
- repoPath, gitAuth.getPublicKey(), gitAuth.getPrivateKey(), true, refName, false);
-
- return checkoutBranchMono.then(Mono.defer(() -> fetchRemoteMono));
+ return checkoutBranchMono
+ .then(Mono.defer(() -> fetchRemoteMono))
+ .flatMap(result -> releaseFileLock(baseArtifactId, true)
+ .thenReturn(result));
+ });📝 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
| public Mono<String> fetchRemoteChanges(ArtifactJsonTransformationDTO jsonTransformationDTO, GitAuth gitAuth) { | |
| 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); | |
| Path repoPath = fsGitHandler.createRepoPath(repoSuffix); | |
| Mono<Boolean> checkoutBranchMono = fsGitHandler.checkoutToBranch(repoSuffix, refName); | |
| Mono<String> fetchRemoteMono = fsGitHandler.fetchRemote( | |
| repoPath, gitAuth.getPublicKey(), gitAuth.getPrivateKey(), true, refName, false); | |
| return checkoutBranchMono.then(Mono.defer(() -> fetchRemoteMono)); | |
| } | |
| @Override | |
| public Mono<String> fetchRemoteChanges(ArtifactJsonTransformationDTO jsonTransformationDTO, GitAuth gitAuth) { | |
| 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); | |
| Path repoPath = fsGitHandler.createRepoPath(repoSuffix); | |
| return addFileLock(baseArtifactId, GitConstants.GitCommandConstants.FETCH, true) | |
| .flatMap(isLocked -> { | |
| Mono<Boolean> checkoutBranchMono = fsGitHandler.checkoutToBranch(repoSuffix, refName); | |
| Mono<String> fetchRemoteMono = fsGitHandler.fetchRemote( | |
| repoPath, gitAuth.getPublicKey(), gitAuth.getPrivateKey(), true, refName, false); | |
| return checkoutBranchMono | |
| .then(Mono.defer(() -> fetchRemoteMono)) | |
| .flatMap(result -> releaseFileLock(baseArtifactId, true) | |
| .thenReturn(result)); | |
| }); | |
| } |
nidhi-nair
reviewed
Dec 6, 2024
| GitType gitType, | ||
| RefType refType); | ||
|
|
||
| Mono<String> fetchRemoteChanges( |
There was a problem hiding this comment.
Why are these methods getting exposed on the interface twice?
There was a problem hiding this comment.
Shouldn't be, revoking the interface access.
nidhi-nair
reviewed
Dec 6, 2024
| @@ -67,6 +70,7 @@ | |||
| import static com.appsmith.external.helpers.AppsmithBeanUtils.copyNestedNonNullProperties; | |||
| import static com.appsmith.server.constants.FieldName.DEFAULT; | |||
| import static com.appsmith.server.constants.SerialiseArtifactObjective.VERSION_CONTROL; | |||
| import static com.appsmith.server.constants.ce.FieldNameCE.BRANCH_NAME; | |||
There was a problem hiding this comment.
Gotta create a lint rule for this I think 😶🌫️
nidhi-nair
reviewed
Dec 6, 2024
| String baseArtifactId = baseArtifactGitData.getDefaultArtifactId(); | ||
|
|
||
| if (refArtifactGitData == null || !hasText(refArtifactGitData.getRefName())) { | ||
| return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, BRANCH_NAME)); |
There was a problem hiding this comment.
TODO for error message accepting git type
nidhi-nair
reviewed
Dec 6, 2024
| gitRedisUtils.acquireGitLock(baseArtifactId, GitConstants.GitCommandConstants.FETCH_REMOTE, isFileLock); | ||
|
|
||
| ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO(); | ||
| jsonTransformationDTO.setRefType(RefType.BRANCH); |
nidhi-nair
reviewed
Dec 6, 2024
| in case of any error, the global exception handler will release the lock | ||
| hence we don't need to do that manually | ||
| */ | ||
| log.error( |
There was a problem hiding this comment.
Error can already include git type
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
964-964: Include gitType in error message as per TODO comment.As noted in the TODO at line 962, the error message should include the Git type for better error tracking and debugging.
Apply this diff:
-return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, BRANCH_NAME)); +return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, BRANCH_NAME + " for " + gitType));
988-1001: Enhance error handling with specific error types.The error handling could be more specific by checking for common Git exceptions like
TransportException,InvalidRemoteException, etc., similar to how it's done in theconnectArtifactToGitmethod.Consider applying this pattern:
.onErrorResume(throwable -> { log.error( "Error to fetch from remote for application: {}, branch: {}, git type {}", baseArtifactId, refArtifactGitData.getRefName(), gitType, throwable); + if (throwable instanceof TransportException) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_GIT_SSH_CONFIGURATION)); + } else if (throwable instanceof InvalidRemoteException) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_GIT_CONFIGURATION, throwable.getMessage())); + } else if (throwable instanceof TimeoutException) { + return Mono.error(new AppsmithException(AppsmithError.GIT_EXECUTION_TIMEOUT)); + } return Mono.error(new AppsmithException(AppsmithError.GIT_ACTION_FAILED, "fetch", throwable.getMessage())); })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
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(6 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/CentralGitServiceCE.java
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
21-21: LGTM!
The new imports are necessary for user session management and analytics tracking.
Also applies to: 40-40
1033-1048: LGTM!
The overloaded method follows good practices by:
- Properly checking permissions
- Reusing the base implementation
- Maintaining clear separation of concerns
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (3)
951-955: Provide more specific error messages for null checksInstead of returning a generic error when
refArtifactorbaseArtifactis null, consider returning specific error messages to aid debugging.Apply this diff to improve error handling:
if (refArtifact == null) { return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "refArtifact")); } else if (baseArtifact == null) { return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "baseArtifact")); } else if (isBaseGitMetadataInvalid(baseArtifact.getGitArtifactMetadata(), gitType)) { return Mono.error(new AppsmithException(AppsmithError.INVALID_GIT_CONFIGURATION)); }
963-963: Address the TODO comment about includinggitTypein error messagesThere's a TODO comment indicating that
gitTypeshould be added to all error messages. Would you like assistance implementing this?
993-994: Improve log message wording for clarityConsider rephrasing the log message to "Error fetching from remote for application" for better readability.
Apply this diff to update the log message:
-log.error( - "Error to fetch from remote for application: {}, branch: {}, git type {}", +log.error( + "Error fetching from remote for application: {}, branch: {}, git type {}",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(6 hunks)
nidhi-nair
approved these changes
Dec 9, 2024
2 tasks
github-actions bot
pushed a commit
to Zeral-Zhang/appsmith
that referenced
this pull request
Dec 12, 2024
## Description - Added fetch remote CGS Impl Fixes # > [!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.Git" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!WARNING] > Tests have not run on the HEAD d964d59 yet > <hr>Mon, 09 Dec 2024 07:03:35 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced new methods for fetching remote changes in various Git services, enhancing synchronization capabilities. - Added new fields for internal metadata management. - Implemented user session management improvements for better handling of user-related data. - Added analytics tracking for unit execution time to improve performance insights. - **Bug Fixes** - Enhanced error handling in new fetch methods to ensure robustness. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Dec 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #
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.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12233198064
Commit: d964d59
Cypress dashboard.
Tags:
@tag.GitSpec:
Mon, 09 Dec 2024 20:08:50 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes