chore: added git controller layer #38446
Conversation
WalkthroughThis pull request introduces comprehensive enhancements to the Git functionality within the Appsmith server, focusing on expanding Git-related services, controllers, and utility classes. The changes include adding new URL constants, implementing methods for branch management, SSH key generation, artifact metadata retrieval, and creating controllers to handle various Git operations across applications and artifacts. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
🧹 Nitpick comments (10)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.java (1)
38-38: Addfinalfor Constructor Parameter
Marking parameters asfinalcan help avoid accidental reassignment and clarify intent.public CentralGitServiceCECompatibleImpl( ... - GitDeployKeysRepository gitDeployKeysRepository, + final GitDeployKeysRepository gitDeployKeysRepository, ... ) {app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java (3)
76-84: Consider renaming “commit” endpoint for clarity.
The method name might cause confusion if you expand functionality in the future. A more descriptive name (e.g.commitChanges) can improve discoverability and maintainability.
141-150: Add optional concurrency checks when fetching remote changes.
Concurrent fetch operations for large repos can be resource-heavy. Evaluate if concurrency limits or in-progress checks are desired.
190-213: Ensure auto-commit service handles large commits gracefully.
Big commits might cause performance issues. Consider chunking or asynchronous batching.app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.java (2)
40-50: Revisit the usage ofArtifactTypeparameter.
There's a TODO comment about removingArtifactTypefrom methods. Consider finalizing that decision soon to avoid confusion and keep the API surface smaller if it's no longer necessary.
63-66: Ensure sensitive logs are minimized.
Frequent logging is helpful, but be mindful of data we might inadvertently log (e.g., repository URLs, tokens).app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (3)
384-385: Clarify error handling upon invalid references.
When callingcheckoutReference, it might be beneficial to add a structured error message if the reference is not found.
2042-2068: Favor concise naming inlistBranchForArtifact.
Consider renaming it to something more direct likelistBranchesfor brevity, reflecting that it’s always artifact-specific.
2479-2502: Handle multiple key types or future expansions.
ThegenerateSSHKeymethod is well-defined, but adding a validation or default fallback forkeyTypemight prevent confusion if new key types are introduced.app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
698-709: Add error handling for checkout failures.
Consider adding logging or graceful error handling if the specified remote branch doesn't exist or the checkout fails unexpectedly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/UrlCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java(3 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(10 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(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationController.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactController.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.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(1 hunks)
🔇 Additional comments (25)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.java (2)
14-14: New Repository Import
Great to seeGitDeployKeysRepositorybeing introduced. This import aligns well with the constructor changes below, so no objections here.
57-57: Proper Dependency Forwarding
PassinggitDeployKeysRepositoryto the superclass correctly wires up the new dependency. Looks good!app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java (3)
49-51: Great use of constructor injection!
Instantiating these services through required constructor arguments is clean and clear.
65-72: Validate gitConnectDTO inputs before connecting.
Performing a quick check (e.g. valid repo URL, non-blank SSH key) can prevent downstream errors.
180-187: Check for edge cases in protected branch logic.
If a branch is already protected or the request is empty, confirm that your service handles these scenarios gracefully.app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.java (2)
1-3: Package naming and placement look good.
Having a dedicated package for Git controllers maintains clarity and file organization.
70-74: Validate request parameters before generating SSH keys.
Although the code is straightforward, consider checking thekeyTypeif you plan to accept multiple key formats in the future.app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
2505-2537: Expose minimal sensitive data ingetGitArtifactMetadata.
Currently returning broad metadata, includingpublicKey. Ensure that no secrets or private keys ever leak.app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactController.java (2)
13-13: Confirm the endpoint is correct.
Verify thatUrl.GIT_ARTIFACT_URLaligns with your overall service routing plan.
15-17: Constructor dependencies look consistent.
The injection ofCentralGitServiceandGitProfileUtilsthrough the superclass looks straightforward.app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationController.java (2)
13-13: Validate route overlap with other controllers.
Double-check that no existing routes inadvertently conflict withUrl.GIT_APPLICATION_URL.
16-19: Constructor injection aligns with design.
AcceptingautoCommitServicein the same constructor is clear and fosters modular design.app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/UrlCE.java (1)
35-36: URLs align with REST patterns.
AddingGIT_APPLICATION_URLandGIT_ARTIFACT_URLis clear and consistent with your versioned API structure.app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java (3)
14-14: No issues with the new import.
It’s good to see a dedicated repository class for deploy keys. Keep it up.
37-37: Appropriate addition of dependency.
InjectingGitDeployKeysRepositoryfor handling SSH keys is consistent with the approach in other parts of the codebase. No concerns here.
56-56: Dependency passed to superclass.
Ensures that the parent implementation has access to the new repository. Looks good.app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (5)
3-15: Imports for new Git objects confirmed.
These imported classes (GitBranchDTO,GitArtifactMetadata,GitAuth,GitDocsDTO) will help structure the data for the newly added methods.
38-40: MethodlistBranchForArtifactis well-defined.
Parameters and return type look appropriate. Confirm unit tests to ensure behavior with or withoutpruneBranches.
78-79: NewgenerateSSHKeymethod.
This is a direct and concise contract. Check that keys are stored securely and that any usage logs are properly handled for compliance.
80-81:getGitArtifactMetadatamethod addition.
This provides clarity when retrieving artifact metadata. Ensure that downstream usage handles possible null returns if the artifact doesn’t exist.
82-82:getGitDocUrlsmethod.
Easy way to ensure consistent doc references. No issues found.app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (2)
53-55: NewgetDefaultBranchFromRepositorymethod.
This can be critical for certain Git operations. Double-check that it gracefully handles edge cases with missing or invalid references.
83-84:checkoutRemoteReferencemethod.
Straightforward addition focusing on remote references. Make sure any concurrency concerns are addressed if multiple checkouts happen simultaneously.app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java (1)
190-218: NewsendGitAnalyticsEventmethod.
Offers more flexibility to pass additional properties. Ensure that personal or sensitive data does not leak into analytics logs ifextraPropscan include PII.app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
307-326: Add a null or empty check for the remote URL.
Just to be safe, consider validatingbaseGitData.getRemoteUrl()to ensure it’s neithernullnor empty.
| @GetMapping("/{branchedApplicationId}/branch") | ||
| public Mono<ResponseDTO<List<GitBranchDTO>>> branch( | ||
| @PathVariable String branchedApplicationId, | ||
| @RequestParam(required = false, defaultValue = "false") Boolean pruneBranches) { | ||
| log.debug("Going to get branch list for application {}", branchedApplicationId); | ||
| return centralGitService | ||
| .listBranchForArtifact( | ||
| branchedApplicationId, BooleanUtils.isTrue(pruneBranches), ARTIFACT_TYPE, GIT_TYPE) | ||
| .map(result -> new ResponseDTO<>(HttpStatus.OK.value(), result, null)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing pagination for branch listing.
If a repository has a large number of branches, returning them all might degrade performance.
| @GetMapping("/{referencedApplicationId}/checkout-ref") | ||
| public Mono<ResponseDTO<? extends Artifact>> checkoutReference( | ||
| @PathVariable String referencedApplicationId, @RequestBody GitRefDTO gitRefDTO) { | ||
| return centralGitService | ||
| .checkoutReference(referencedApplicationId, gitRefDTO, true, ARTIFACT_TYPE, GIT_TYPE) | ||
| .map(result -> new ResponseDTO<>(HttpStatus.OK.value(), result, null)); | ||
| } |
There was a problem hiding this comment.
GET request with a RequestBody is unexpected.
Generally, GET endpoints shouldn’t contain bodies. Consider changing to a POST or adding parameters to the query string.
-@GetMapping("/{referencedApplicationId}/checkout-ref")
-public Mono<ResponseDTO<? extends Artifact>> checkoutReference(
- @PathVariable String referencedApplicationId, @RequestBody GitRefDTO gitRefDTO) {
+@PostMapping("/{referencedApplicationId}/checkout-ref")
+public Mono<ResponseDTO<? extends Artifact>> checkoutReference(
+ @PathVariable String referencedApplicationId, @RequestBody GitRefDTO gitRefDTO) {📝 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.
| @GetMapping("/{referencedApplicationId}/checkout-ref") | |
| public Mono<ResponseDTO<? extends Artifact>> checkoutReference( | |
| @PathVariable String referencedApplicationId, @RequestBody GitRefDTO gitRefDTO) { | |
| return centralGitService | |
| .checkoutReference(referencedApplicationId, gitRefDTO, true, ARTIFACT_TYPE, GIT_TYPE) | |
| .map(result -> new ResponseDTO<>(HttpStatus.OK.value(), result, null)); | |
| } | |
| @PostMapping("/{referencedApplicationId}/checkout-ref") | |
| public Mono<ResponseDTO<? extends Artifact>> checkoutReference( | |
| @PathVariable String referencedApplicationId, @RequestBody GitRefDTO gitRefDTO) { | |
| return centralGitService | |
| .checkoutReference(referencedApplicationId, gitRefDTO, true, ARTIFACT_TYPE, GIT_TYPE) | |
| .map(result -> new ResponseDTO<>(HttpStatus.OK.value(), result, null)); | |
| } |
There was a problem hiding this comment.
Thanks rabbit, really missed that!
There was a problem hiding this comment.
@sondermanish You're welcome! Let me know if there's anything else you'd like to explore or adjust.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
653-664: Consider adding error handling for checkout failures.While the implementation is clean and focused, it would benefit from explicit error handling for common checkout scenarios like:
- Branch doesn't exist
- Conflicts during checkout
- Network issues during remote operations
public Mono<String> checkoutRemoteReference(ArtifactJsonTransformationDTO jsonTransformationDTO) { GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(jsonTransformationDTO.getArtifactType()); Path repoSuffix = gitArtifactHelper.getRepoSuffixPath( jsonTransformationDTO.getWorkspaceId(), jsonTransformationDTO.getBaseArtifactId(), jsonTransformationDTO.getRepoName()); - return fsGitHandler.checkoutRemoteBranch(repoSuffix, jsonTransformationDTO.getRefName()); + return fsGitHandler + .checkoutRemoteBranch(repoSuffix, jsonTransformationDTO.getRefName()) + .onErrorResume(error -> { + if (error instanceof RefNotFoundException) { + return Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, + "checkout", + "Remote branch not found: " + jsonTransformationDTO.getRefName())); + } + return Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, + "checkout", + error.getMessage())); + });app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (3)
2096-2152: Consider breaking down the complex branch listing logic.While the implementation is functionally correct, consider extracting some of the nested logic into separate helper methods to improve readability and maintainability.
Consider refactoring like this:
protected Mono<List<GitBranchDTO>> getBranchList( Artifact baseArtifact, Artifact branchedArtifact, Boolean pruneBranches, boolean syncDefaultBranchWithRemote, GitType gitType) { - GitArtifactMetadata baseGitData = baseArtifact.getGitArtifactMetadata(); - GitArtifactMetadata branchedGitData = branchedArtifact.getGitArtifactMetadata(); - // ... rest of the current implementation + return validateGitMetadata(baseArtifact, branchedArtifact, gitType) + .flatMap(metadata -> createTransformationDTO(metadata, baseArtifact)) + .flatMap(dto -> processBranchList(dto, pruneBranches, syncDefaultBranchWithRemote)); } + private Mono<GitMetadata> validateGitMetadata(Artifact baseArtifact, Artifact branchedArtifact, GitType gitType) { + // Extract validation logic here + } + private Mono<ArtifactJsonTransformationDTO> createTransformationDTO(GitMetadata metadata, Artifact baseArtifact) { + // Extract DTO creation logic here + } + private Mono<List<GitBranchDTO>> processBranchList( + ArtifactJsonTransformationDTO dto, + Boolean pruneBranches, + boolean syncDefaultBranchWithRemote) { + // Extract branch processing logic here + }
2540-2573: Consider adding caching and sensitive data filtering.The implementation is functionally correct but consider:
- Adding caching for frequently accessed metadata
- Ensuring sensitive data is properly filtered before returning
Consider adding:
@Override public Mono<GitArtifactMetadata> getGitArtifactMetadata(String baseArtifactId, ArtifactType artifactType) { + return cacheService.get(getCacheKey(baseArtifactId), () -> { return Mono.zip(baseArtifactMono, userDataService.getForCurrentUser()) .map(tuple -> { // ... existing implementation + return filterSensitiveData(baseGitMetadata); }); + }); } +private GitArtifactMetadata filterSensitiveData(GitArtifactMetadata metadata) { + // Remove or mask sensitive information + return metadata; +}
2582-2593: Optimize documentation URL retrieval.Since this returns static data, consider caching the result or creating the list once at startup.
Consider optimizing like this:
+private static final List<GitDocsDTO> CACHED_GIT_DOCS = initializeGitDocs(); +private static List<GitDocsDTO> initializeGitDocs() { + return Arrays.stream(ErrorReferenceDocUrl.values()) + .map(docUrl -> { + GitDocsDTO dto = new GitDocsDTO(); + dto.setDocKey(docUrl); + dto.setDocUrl(docUrl.getDocUrl()); + return dto; + }) + .collect(Collectors.toList()); +} @Override public Mono<List<GitDocsDTO>> getGitDocUrls() { - ErrorReferenceDocUrl[] docSet = ErrorReferenceDocUrl.values(); - List<GitDocsDTO> gitDocsDTOList = new ArrayList<>(); - for (ErrorReferenceDocUrl docUrl : docSet) { - GitDocsDTO gitDocsDTO = new GitDocsDTO(); - gitDocsDTO.setDocKey(docUrl); - gitDocsDTO.setDocUrl(docUrl.getDocUrl()); - gitDocsDTOList.add(gitDocsDTO); - } - return Mono.just(gitDocsDTOList); + return Mono.just(CACHED_GIT_DOCS); }
📜 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(10 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(2 hunks)
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
276-295: LGTM! Well-structured implementation of default branch retrieval.The method properly validates Git authentication, constructs repository paths using helper classes, and delegates to the appropriate handler. The error handling for invalid Git configuration is comprehensive.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
2068-2094: LGTM! Well-structured implementation for branch listing.The implementation properly handles permissions and provides good separation of concerns.
| @Override | ||
| public Mono<GitAuth> generateSSHKey(String keyType) { | ||
| GitAuth gitAuth = GitDeployKeyGenerator.generateSSHKey(keyType); | ||
|
|
||
| GitDeployKeys gitDeployKeys = new GitDeployKeys(); | ||
| gitDeployKeys.setGitAuth(gitAuth); | ||
|
|
||
| return sessionUserService | ||
| .getCurrentUser() | ||
| .flatMap(user -> { | ||
| gitDeployKeys.setEmail(user.getEmail()); | ||
| return gitDeployKeysRepository | ||
| .findByEmail(user.getEmail()) | ||
| .switchIfEmpty(gitDeployKeysRepository.save(gitDeployKeys)) | ||
| .flatMap(gitDeployKeys1 -> { | ||
| if (gitDeployKeys.equals(gitDeployKeys1)) { | ||
| return Mono.just(gitDeployKeys1); | ||
| } | ||
| // Overwrite the existing keys | ||
| gitDeployKeys1.setGitAuth(gitDeployKeys.getGitAuth()); | ||
| return gitDeployKeysRepository.save(gitDeployKeys1); | ||
| }); | ||
| }) | ||
| .thenReturn(gitAuth); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for SSH key type and consider rate limiting.
While the implementation is secure, consider adding:
- Validation for the key type parameter
- Rate limiting for key generation to prevent abuse
Add validation like this:
@Override
public Mono<GitAuth> generateSSHKey(String keyType) {
+ if (!isValidKeyType(keyType)) {
+ return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "Invalid SSH key type"));
+ }
+
+ return checkRateLimit()
+ .then(Mono.defer(() -> {
GitAuth gitAuth = GitDeployKeyGenerator.generateSSHKey(keyType);
// ... rest of the implementation
+ }));
}
+private boolean isValidKeyType(String keyType) {
+ return Arrays.asList("rsa", "ed25519").contains(keyType.toLowerCase());
+}
+private Mono<Void> checkRateLimit() {
+ // Implement rate limiting logic
+ return Mono.empty();
+}Committable suggestion skipped: line range outside the PR's diff.
| return Mono.error(error); | ||
| }); | ||
| }) | ||
| .thenMany(gitArtifactHelper.getAllArtifactByBaseId(baseArtifactId, artifactEditPermission)) |
| protected static final GitType GIT_TYPE = GitType.FILE_SYSTEM; | ||
|
|
||
| @JsonView(Views.Public.class) | ||
| @PostMapping("/import/{workspaceId}") |
| } | ||
|
|
||
| @JsonView(Views.Public.class) | ||
| @PostMapping("/{sourceApplicationId}/create-ref") |
| } | ||
|
|
||
| @JsonView(Views.Public.class) | ||
| @GetMapping("/{branchedApplicationId}/branch") |
There was a problem hiding this comment.
should be plural, can handle later
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 (1)
4-4: Add unit tests for the formula function.The TODO comment indicates missing test coverage for this critical mathematical operation.
Would you like me to help generate comprehensive unit tests for this function?
app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java (1)
58-62: Consider extracting the ResponseDTO mapping into a utility method.Many endpoints follow the pattern
map(result -> new ResponseDTO<>(HttpStatus.OK.value(), result, null)). A shared helper method could improve consistency and reduce duplication.
📜 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/central/CentralGitServiceCEImpl.java(10 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.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(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java
🔇 Additional comments (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
276-295: LGTM! Well-structured implementation of default branch retrieval.The method properly validates Git authentication, handles error cases, and follows a clean separation of concerns pattern.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
1-2: LGTM! Clean and straightforward implementation.
6-6: Verify requests package version for security vulnerabilities.The fixed version 2.26.0 should be checked for known security issues.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java (4)
45-47: Good use of a dedicated base URL for Git operations.Defining a base URL (
Url.GIT_APPLICATION_URL) under one controller helps keep API paths organized and maintainable.
79-84: Validate request body for null or invalid fields.While this endpoint logs the branchedApplicationId, ensure there is proper null-checking or validation for the
commitDTOto avoid NPEs or undesired behavior.
215-224: Consider implementing pagination for branch listing.If a repository has many branches, returning them all in a single request may degrade performance.
1-225: Overall structure and integration of Git operations look solid!Your use of reactive programming across multiple endpoints is consistent, and the logging statements help with debugging. Continue monitoring for potential performance bottlenecks or large payloads, especially for meta and status operations.
| @Override | ||
| public Mono<String> checkoutRemoteReference(ArtifactJsonTransformationDTO jsonTransformationDTO) { | ||
| GitArtifactHelper<?> gitArtifactHelper = | ||
| gitArtifactHelperResolver.getArtifactHelper(jsonTransformationDTO.getArtifactType()); | ||
|
|
||
| Path repoSuffix = gitArtifactHelper.getRepoSuffixPath( | ||
| jsonTransformationDTO.getWorkspaceId(), | ||
| jsonTransformationDTO.getBaseArtifactId(), | ||
| jsonTransformationDTO.getRepoName()); | ||
|
|
||
| return fsGitHandler.checkoutRemoteBranch(repoSuffix, jsonTransformationDTO.getRefName()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for remote branch checkout.
The method should handle potential errors that could occur during remote branch checkout, such as non-existent branches or network issues.
public Mono<String> checkoutRemoteReference(ArtifactJsonTransformationDTO jsonTransformationDTO) {
GitArtifactHelper<?> gitArtifactHelper =
gitArtifactHelperResolver.getArtifactHelper(jsonTransformationDTO.getArtifactType());
Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(
jsonTransformationDTO.getWorkspaceId(),
jsonTransformationDTO.getBaseArtifactId(),
jsonTransformationDTO.getRepoName());
- return fsGitHandler.checkoutRemoteBranch(repoSuffix, jsonTransformationDTO.getRefName());
+ return fsGitHandler
+ .checkoutRemoteBranch(repoSuffix, jsonTransformationDTO.getRefName())
+ .onErrorResume(error -> Mono.error(
+ new AppsmithException(
+ AppsmithError.GIT_ACTION_FAILED,
+ "checkout",
+ 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.
| @Override | |
| public Mono<String> checkoutRemoteReference(ArtifactJsonTransformationDTO jsonTransformationDTO) { | |
| GitArtifactHelper<?> gitArtifactHelper = | |
| gitArtifactHelperResolver.getArtifactHelper(jsonTransformationDTO.getArtifactType()); | |
| Path repoSuffix = gitArtifactHelper.getRepoSuffixPath( | |
| jsonTransformationDTO.getWorkspaceId(), | |
| jsonTransformationDTO.getBaseArtifactId(), | |
| jsonTransformationDTO.getRepoName()); | |
| return fsGitHandler.checkoutRemoteBranch(repoSuffix, jsonTransformationDTO.getRefName()); | |
| } | |
| @Override | |
| public Mono<String> checkoutRemoteReference(ArtifactJsonTransformationDTO jsonTransformationDTO) { | |
| GitArtifactHelper<?> gitArtifactHelper = | |
| gitArtifactHelperResolver.getArtifactHelper(jsonTransformationDTO.getArtifactType()); | |
| Path repoSuffix = gitArtifactHelper.getRepoSuffixPath( | |
| jsonTransformationDTO.getWorkspaceId(), | |
| jsonTransformationDTO.getBaseArtifactId(), | |
| jsonTransformationDTO.getRepoName()); | |
| return fsGitHandler | |
| .checkoutRemoteBranch(repoSuffix, jsonTransformationDTO.getRefName()) | |
| .onErrorResume(error -> Mono.error( | |
| new AppsmithException( | |
| AppsmithError.GIT_ACTION_FAILED, | |
| "checkout", | |
| error.getMessage()))); | |
| } |
Failed server tests
|
## Description - Added controller layer for the new git implementation Fixes #`Issue Number` ## Automation /ok-to-test tags="@tag.Git" ### 🔍 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/12582917398> > Commit: 3fa98cd > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12582917398&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Thu, 02 Jan 2025 15:04:31 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 - **New Features** - Enhanced Git integration with new endpoints for managing Git repositories. - Added support for Git application and artifact operations. - Introduced new methods for branch management, SSH key generation, and metadata retrieval. - Expanded Git-related functionality with new controllers and services. - **Improvements** - Added more comprehensive Git analytics tracking. - Improved Git repository interaction capabilities. - Enhanced support for Git operations across different artifact types. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Fixes #
Issue NumberAutomation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12582917398
Commit: 3fa98cd
Cypress dashboard.
Tags:
@tag.GitSpec:
Thu, 02 Jan 2025 15:04:31 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Improvements