chore: Add code-split for service layer Git integration for packages #38497
Conversation
WalkthroughThe pull request introduces a significant refactoring of Git-related utility methods across multiple server-side Java classes. The primary focus is on shifting from application-level Git connectivity checks to artifact metadata-level checks. This involves renaming methods in 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: 1
📜 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/applications/base/ApplicationServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/searchentities/SearchEntitySolutionCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ApplicationPageServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (17)
app/server/appsmith-server/src/main/java/com/appsmith/server/searchentities/SearchEntitySolutionCEImpl.java (1)
92-93: Nice refactor to artifact-based checks.
No issues observed with the updated filtering logic. This ensures a more granular check for Git connectivity.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java (2)
143-151: Straightforward artifact-based branching logic.
This improves clarity by focusing on the artifact metadata rather than the entire Application object.
155-162: Good shift to metadata for Git connectivity checks.
No immediate concerns here; the function covers the key conditions well.app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/GitUtilsTest.java (12)
14-15: Updated imports for artifact-based methods
These aligned imports look good.
219-219: Accurate coverage for a connected artifact
The assertion validates the newisArtifactConnectedToGitlogic properly.
227-227: Ensuring connectivity returns false with null metadata
Test coverage is clear and correct.
238-238: Asserting not connected with missing defaultAppId
Good negative check scenario.
249-249: Testing missing remoteUrl
Appropriately ensures connection is reported as false.
261-261: Checks empty strings
Verifies zero-length strings lead to not connected status.
275-275: Verifies default branch condition
The assertion properly confirms default branching for connected artifacts.
289-289: Confirms non-default branch
As expected, the assertion ensures the artifact branch mismatch is detected.
297-297: No Git metadata
This negative test is well-structured.
306-306: Handles null metadata gracefully
No concerns.
320-320: Ref name null check
Correctly tests the branch comparison.
334-334: Null default branch name
Ensures the function behaves correctly without a default branch.app/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java (2)
212-213: Streamlined filtering for default branched artifacts
This code effectively clarifies Git-connected applications for the user.
903-904: Uses new artifact-based connectivity check
This improves consistency with the rest of the refactor.
| if (GitUtils.isArtifactConnectedToGit(application.getGitArtifactMetadata())) { | ||
| return applicationService.findAllApplicationsByBaseApplicationId( | ||
| gitData.getDefaultArtifactId(), applicationPermission.getDeletePermission()); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Migration from isApplicationConnectedToGit to isArtifactConnectedToGit appears incomplete
The old method is still present in the codebase and being actively used. In ApplicationImportServiceCEImpl.java, the new isArtifactConnectedToGit method is actually calling the old isApplicationConnectedToGit method, suggesting an incomplete migration.
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/imports/ApplicationImportServiceCEImpl.java: Review if this implementation should be updated to use the new Git utility method directlyapp/server/appsmith-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCE.java: Consider if the old method should be deprecated
🔗 Analysis chain
Confirm all references to the old method are removed.
This line correctly adopts the artifact-level check instead of the legacy application-level Git check. Ensure that any remaining usage of GitUtils.isApplicationConnectedToGit has been replaced with isArtifactConnectedToGit throughout the codebase.
Run the following script to scan for any references to the now retired isApplicationConnectedToGit:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to isApplicationConnectedToGit across the repository
ast-grep --pattern $'isApplicationConnectedToGit($_)'
Length of output: 66109
Script:
#!/bin/bash
# Search for any remaining references to isApplicationConnectedToGit in Java files
rg "isApplicationConnectedToGit" -t java -C 2
Length of output: 2401
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (2)
1229-1229: Consider adding a null check before setting gitSyncId.The method call should be guarded with a null check on the action parameter to prevent potential NullPointerException.
- setGitSyncIdInNewAction(action); + if (action != null) { + setGitSyncIdInNewAction(action); + }
1237-1242: Consider using parallel stream for better performance.Since the operations are independent, you could potentially improve performance by using parallel streams for large action lists.
- actions.stream().filter(action -> action.getGitSyncId() == null).forEach(this::setGitSyncIdInNewAction); + actions.parallelStream().filter(action -> action.getGitSyncId() == null).forEach(this::setGitSyncIdInNewAction); return Flux.fromIterable(actions) .flatMap(this::sanitizeAction) .collectList() .flatMapMany(repository::saveAll);app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java (1)
195-201: Add null check for entity parameterWhile the implementation is clean, consider adding a null check to prevent NullPointerException:
public static <T extends RefAwareDomain> T resetEntityReferences(T entity) { + if (entity == null) { + return null; + } entity.setBaseId(entity.getId()); entity.setRefType(null); entity.setRefName(null); return entity; }app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java (1)
261-279: Consider implementing circuit breaker patternGiven this is a critical path for Git disconnection operations, consider implementing a circuit breaker pattern to handle potential cascading failures in the reactive chain, especially for large applications with many pages and actions.
📜 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/actioncollections/base/ActionCollectionServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (1)
1232-1232: LGTM! Good use of method chaining.The code properly chains the sanitization and save operations.
app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/base/ActionCollectionServiceCE.java (1)
85-86: LGTM!The interface method signature is well-defined with clear parameter types.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java (3)
144-149: Well-structured method documentation!The updated JavaDoc accurately reflects the parameter changes and maintains good documentation standards.
Also applies to: 156-160
150-152: Clean implementation of metadata checks!The refactored implementation maintains the necessary validation checks while simplifying the code by operating directly on GitArtifactMetadata.
Also applies to: 161-163
149-163: Verify method rename impactLet's verify that all callers have been updated to use the new method names.
✅ Verification successful
Method rename implementation is correct
The old method
isApplicationConnectedToGitexists in the ApplicationService interface as part of the public API, while internally it uses the new generic artifact-based methods from GitUtils. This is a valid implementation pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to old method names rg "isDefaultBranchedApplication|isApplicationConnectedToGit" --type java # Search for usage of new methods to confirm proper adoption rg "isDefaultBranchedArtifact|isArtifactConnectedToGit" --type javaLength of output: 5837
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java (2)
23-23: LGTM: GitUtils import added for entity reference reset functionalityThe import aligns with the refactoring to centralize Git-related utility methods.
262-277: Verify error handling in the reactive chainThe refactoring to use
GitUtils.resetEntityReferenceslooks good, but the reactive chain could benefit from error handling to prevent silent failures.Consider adding error handling:
Flux<NewPage> newPageFlux = Flux.fromIterable(baseApplication.getPages()) .flatMap(page -> newPageService.findById(page.getId(), null)) .map(GitUtils::resetEntityReferences) .collectList() .flatMapMany(newPageService::saveAll) + .onErrorResume(error -> { + log.error("Error resetting page references", error); + return Flux.error(new AppsmithException(AppsmithError.GIT_ACTION_FAILED, error.getMessage())); + }) .cache();✅ Verification successful
Error handling suggestion is valid and aligns with codebase patterns
The suggested error handling follows the established pattern used consistently across other git operations in the codebase. The addition of
onErrorResumewithAppsmithException(AppsmithError.GIT_ACTION_FAILED)would make the code more robust and consistent with similar implementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check for similar reactive chains with error handling ast-grep --pattern 'flatMapMany($_).onErrorResume($_)' # Look for logging patterns in the class rg "log\." app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/GitApplicationHelperCEImpl.java # Check AppsmithException usage with GIT_ACTION_FAILED rg "AppsmithException.*GIT_ACTION_FAILED" -A 2Length of output: 7144
| @Override | ||
| public Flux<ActionCollection> findByArtifactIdAndArtifactType(String artifactId, ArtifactType artifactType) { | ||
| return repository.findByApplicationId(artifactId); | ||
| } |
There was a problem hiding this comment.
Implementation needs improvement
The current implementation has several issues:
- The
artifactTypeparameter is unused - Missing null checks for parameters
- No error handling for invalid inputs
Consider this implementation:
@Override
public Flux<ActionCollection> findByArtifactIdAndArtifactType(String artifactId, ArtifactType artifactType) {
+ if (artifactId == null) {
+ return Flux.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ARTIFACT_ID));
+ }
+ if (artifactType == null) {
+ return Flux.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ARTIFACT_TYPE));
+ }
+ // Handle different artifact types if needed
+ if (artifactType == ArtifactType.APPLICATION) {
return repository.findByApplicationId(artifactId);
+ }
+ return Flux.error(new AppsmithException(AppsmithError.UNSUPPORTED_OPERATION));
}📝 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 Flux<ActionCollection> findByArtifactIdAndArtifactType(String artifactId, ArtifactType artifactType) { | |
| return repository.findByApplicationId(artifactId); | |
| } | |
| @Override | |
| public Flux<ActionCollection> findByArtifactIdAndArtifactType(String artifactId, ArtifactType artifactType) { | |
| if (artifactId == null) { | |
| return Flux.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ARTIFACT_ID)); | |
| } | |
| if (artifactType == null) { | |
| return Flux.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ARTIFACT_TYPE)); | |
| } | |
| // Handle different artifact types if needed | |
| if (artifactType == ArtifactType.APPLICATION) { | |
| return repository.findByApplicationId(artifactId); | |
| } | |
| return Flux.error(new AppsmithException(AppsmithError.UNSUPPORTED_OPERATION)); | |
| } |
subrata71
deleted the
chore/code-split-service-layer-git-integration-for-packages
branch
Description
EE counterpart PR: https://github.com/appsmithorg/appsmith-ee/pull/5863
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/12646186524
Commit: f7e7566
Cypress dashboard.
Tags:
@tag.GitSpec:
Tue, 07 Jan 2025 07:00:44 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Refactor
Tests
The changes enhance the precision and maintainability of Git-related functionality across the application's backend services.