chore: speeding up and verifying git apis #38952
Conversation
github-actions
bot
added
the
skip-changelog
WalkthroughThis PR introduces multiple Git enhancements across several modules. A new method, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GitHandler
participant RemoteRepo
Client->>GitHandler: pullArtifactWithoutCheckout(repoPath, remoteURL, branch, keys)
GitHandler->>RemoteRepo: Initiate pull operation using JGit
RemoteRepo-->>GitHandler: Return merge result (success or conflicts)
GitHandler->>Client: Mono<MergeStatusDTO> result
sequenceDiagram
participant User
participant GitArtifactController
participant GitProfileService
User->>GitArtifactController: GET /profile/default
GitArtifactController->>GitProfileService: Retrieve default Git profile
GitProfileService-->>GitArtifactController: Git profile data
GitArtifactController->>User: ResponseDTO with GitProfile
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (4)
✨ Finishing Touches
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
|
![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
🧹 Nitpick comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java (1)
73-74: Consider making feature flags configurable.Hard-coded feature flags could be moved to a configuration file or environment variables for easier management.
- combinedFlags.put("release_query_module_enabled", true); - combinedFlags.put("release_anvil_enabled", false); + combinedFlags.put("release_query_module_enabled", + environment.getProperty("RELEASE_QUERY_MODULE_ENABLED", Boolean.class, true)); + combinedFlags.put("release_anvil_enabled", + environment.getProperty("RELEASE_ANVIL_ENABLED", Boolean.class, false));app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
623-626: Consider making the error message more specific.The error message could better indicate that either reference names or fetch all flag must be provided.
- return Mono.error(new AppsmithException(AppsmithError.INVALID_GIT_CONFIGURATION)); + return Mono.error(new AppsmithException(AppsmithError.INVALID_GIT_CONFIGURATION, + "Either reference names or fetch all flag must be provided"));app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
641-643: Check potential performance overhead
UsingisFetchAll(TRUE)can be expensive for large repos; consider a narrower fetch if possible.
2387-2392: Unused GitBranchDTO list
The loop populatesgitBranchDTOswithout any usage. Consider removing or refactoring for clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java(3 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/ce/GitConstantsCE.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java(1 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/CentralGitServiceCEImpl.java(18 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/controllers/GitApplicationControllerCE.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(12 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/FeatureFlagServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesITWithCentralService.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
🔇 Additional comments (55)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (2)
89-93: LGTM! Method signature change is well-defined.The addition of
baseRefJsonTransformationDTOparameter tocreateGitReferencemethod provides better control over the base reference during Git operations.
89-93: LGTM! Method signature change is well-structured.The addition of
baseRefJsonTransformationDTOparameter enhances the Git reference creation by explicitly providing the base reference context.app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (7)
74-74: LGTM! Consistent Boolean handling.The use of static import
TRUEimproves code readability and maintains consistency across the file.Also applies to: 257-259, 518-519, 623-624
696-704: LGTM! Git reference handling is robust.The implementation properly handles base reference checkout before creating and pushing the new reference.
Also applies to: 715-718
811-812: LGTM! Improved error message clarity.Error messages now use
GitCommandConstantsfor consistency and provide more specific context.
74-74: LGTM! Simplified boolean checks.Using static import of
TRUEimproves code readability while maintaining the same functionality.Also applies to: 257-259
695-719: Implementation aligns with interface changes.The method now properly:
- Checks out the base reference first
- Creates and checks out the new reference
- Pushes the changes to remote
714-714: Address the TODO comment about checkout.The comment suggests that checkout functionality needs to be added. Please clarify if this is still needed given the new
pullArtifactWithoutCheckoutimplementation.
811-812: LGTM! Improved error message consistency.Using
GitCommandConstants.PULLmakes the error messages more maintainable and consistent.app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/ce/GitConstantsCE.java (1)
14-14: LGTM!The constant's name and value are clear and descriptive.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.java (4)
43-56: LGTM! Well-documented default profile endpoint.The endpoint follows RESTful conventions and includes proper logging.
58-64: LGTM! Clear GET endpoint for default profile.Good use of ResponseDTO for consistent error handling.
66-74: LGTM! Repository-specific profile endpoint.Proper use of PUT for update operation with clear logging.
76-82: LGTM! Profile retrieval endpoint.Clean implementation following the established pattern.
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/handler/FSGitHandler.java (1)
105-116: LGTM! Well-documented method signature.The method signature is consistent with the interface's style and includes clear documentation of all parameters.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java (3)
182-189: Improved parameter handling in deleteBranch endpoint.The method now accepts individual parameters instead of a DTO, making the API more flexible and RESTful.
202-203: Improved RESTful endpoint naming.Changed from
/branch/protectedto/protected-branchesfor better REST compliance.Also applies to: 212-213
246-250: Enhanced reference type support in getReferences endpoint.The endpoint now supports different reference types through the
refTypeparameter, making it more versatile.app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesITWithCentralService.java (1)
439-440: LGTM! Consistent use of GitRefDTO.Test updates correctly reflect the transition from GitBranchDTO to GitRefDTO.
Also applies to: 467-468, 485-486
app/server/appsmith-git/src/main/java/com/appsmith/git/handler/ce/FSGitHandlerCEImpl.java (2)
1019-1025: Improved fetchRemote implementation.The refactored code is more concise and handles fetch operations more efficiently.
Also applies to: 1047-1054
469-530: Verify error handling in pullArtifactWithoutCheckout.The implementation looks good but has a few areas that need attention:
- The error handling could be improved by adding specific catch blocks for different GitAPIException types.
- The merge conflict handling could benefit from more detailed error messages.
Consider adding specific exception handling:
- } catch (GitAPIException e) { - throw e; + } catch (CheckoutConflictException e) { + log.error("Checkout conflict during pull: {}", e.getMessage()); + throw e; + } catch (GitAPIException e) { + log.error("Git operation failed: {}", e.getMessage()); + throw e;app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (2)
39-40: LGTM! Return type change improves reference handling.The change from
GitBranchDTOtoGitRefDTOaligns with the broader refactoring for better Git reference handling.
72-73: LGTM! Parameter changes improve flexibility.Breaking down
GitRefDTOinto individual parameters (refName,refType) provides better control and clarity over reference handling.app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (32)
10-10: Replacing import with GitCommandConstants
No concerns with the new import statement; usage looks consistent.
431-431: Assigning artifactType
Explicitly setting the artifact type is appropriate for consistency.
449-449: Adding GitCommandConstants in error message
Makes the failure context clearer.
634-634: Introducing FetchRemoteDTO
Creating a DTO for fetching remote references looks fine.
645-645: Remote references fetch call
Invoking remote fetch is consistent with the flow.
648-650: Error handling with onErrorResume
Gracefully handles exceptions with a clear error message.
693-696: Ref creation onErrorResume
Error message clearly indicates a ref creation issue.
726-728: Explicit error for CREATE_BRANCH
Keeps error handling pattern uniform.
850-850: Analytics event update
UsingAnalyticsEvents.GIT_DELETE_BRANCHfor clarity is good.
1082-1082: Utilizing README_FILE_NAME
ReferencingGitConstants.README_FILE_NAMEis straightforward.
1088-1088: First commit message
ApplyingGitConstants.FIRST_COMMITaligns with the commit flow.
1548-1548: Extracting refType
Local variable usage is fine.
1554-1556: Gathering artifact details
Assigning artifactType, baseArtifactId, workspaceId is consistent.
1559-1562: Creating ArtifactJsonTransformationDTO
Passing the parameters ensures correct rehydration.
1564-1568: Setting up FetchRemoteDTO
Specifying branch name, ref type, and fetch scope.
1570-1570: Export by artifact
Export step appears logically placed.
1572-1573: Acquiring lock for STATUS
Using zipper to combine export with redis locking is clear.
1575-1577: Comment on redis lock acquisition
Good clarification about the lifecycle of the lock.
1582-1587: Conditionally compare remote
Verifies metadata and fetches remote if needed.
1590-1592: Fetching remote references
onErrorResume ensures controlled fallback.
1610-1613: Throwing GIT_ACTION_FAILED
Consistent error reporting with relevant command reference.
1620-1620: Releasing file lock
Properly cleans up resources within usingWhen.
1682-1685: Lock acquisition for GIT PULL
Implementation matches the existing lock pattern.
1715-1716: Mono bridging
Mono.create usage is valid for bridging the subscriber context.
1753-1754: Building ArtifactJsonTransformationDTO
Constructor parameters appear correct.
1800-1804: Commit artifact arguments
Method call includes the correct parameters.
1852-1852: Measuring elapsed time
Capturing performance metrics is useful for analysis.
1865-1865: Fetch error message
Returning a clear reason for fetch failures.
1878-1878: Naming the operation
Tagging it asOPS_FETCH_REMOTEfosters uniform instrumentation.
2146-2148: Initialize ArtifactJsonTransformationDTO
Constructed with workspace ID and local branch reference.
2150-2150: Apply the RefType
Setting the branchedGitData's refType is coherent.
2398-2405: Building GitRefDTO objects
Legitimately constructs and returns the list of references.
| .pullArtifactWithoutCheckout( | ||
| repoSuffix, | ||
| baseMetadata.getRemoteUrl(), | ||
| branchName, | ||
| baseMetadata.getGitAuth().getPrivateKey(), | ||
| baseMetadata.getGitAuth().getPublicKey())) | ||
| baseMetadata.getGitAuth().getPublicKey()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add pullArtifactWithoutCheckout to the interface.
The method pullArtifactWithoutCheckout is used in the implementation but not defined in the GitHandlingServiceCE interface.
Add the method to the interface to maintain consistency:
Mono<MergeStatusDTO> pullArtifactWithoutCheckout(
Path repoPath,
String remoteUrl,
String branchName,
String privateKey,
String publicKey) throws IOException;
| @@ -70,6 +70,8 @@ public Mono<Map<String, Boolean>> getAllFeatureFlagsForUser() { | |||
| .map(remoteAndTenantFlags -> { | |||
| Map<String, Boolean> combinedFlags = new HashMap<>(); | |||
| combinedFlags.putAll(remoteAndTenantFlags.getT1()); | |||
| combinedFlags.put("release_query_module_enabled", true); | |||
| combinedFlags.put("release_anvil_enabled", false); | |||
There was a problem hiding this comment.
Remove these changes please.
| @@ -446,7 +446,7 @@ protected Mono<? extends Artifact> checkoutReference( | |||
|
|
|||
| return Mono.error(new AppsmithException( | |||
| AppsmithError.GIT_ACTION_FAILED, | |||
| "checkout", | |||
| GitCommandConstants.CHECKOUT_BRANCH, | |||
There was a problem hiding this comment.
Nit: Constant naming *CHECKOUT_REF
| .releaseFileLock(artifactType, branchedGitData.getDefaultArtifactId(), TRUE) | ||
| .then(Mono.error(new AppsmithException(AppsmithError.GIT_ACTION_FAILED, "checkout"))); | ||
| }) | ||
| Mono<? extends Artifact> lockHandledRecreateArtifactFromLastCommit = Mono.usingWhen( |
There was a problem hiding this comment.
Nit: The naming could be better.
| String workspaceId = baseArtifact.getWorkspaceId(); | ||
| String baseArtifactId = baseGitData.getDefaultArtifactId(); | ||
| String repoName = baseGitData.getRepoName(); | ||
| RefType refType = RefType.branch; // baseGitData.getRefType(); |
There was a problem hiding this comment.
QQ: Why refType is explicitly set as branch?
There was a problem hiding this comment.
this method is explicitly present for branches, that is why we are setting it manually
| try { | ||
| return fsGitHandler | ||
| .checkoutToBranch(repoSuffix, jsonTransformationDTO.getRefName()) | ||
| .then(fsGitHandler.pullApplication( | ||
| .pullArtifactWithoutCheckout( |
There was a problem hiding this comment.
Can you please explain this change? Why don't we need to checkout to the branch before pulling? Does it impose any risk? Is it helping us with better performance?
There was a problem hiding this comment.
This is a performance optimization, this wouldn't cause any potential problems as this flow is only called via one flow which is pull, for pull we do status, which checkout the branch
| * Note : The master branch here refers to the artifact that was created even before connecting to git | ||
| */ | ||
| @JsonView(Views.Public.class) | ||
| @PostMapping("/profile/default") |
There was a problem hiding this comment.
QQ: Where is the endpoint for ssh keypair? IIRC, that should be part of GitArtifactControllerCE.
There was a problem hiding this comment.
it's handled in the api controller
## 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 Number` _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.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/13112635630> > Commit: f85c2bb > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13112635630&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Mon, 03 Feb 2025 12:27:57 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** - Introduced a new way to pull remote changes without switching branches, simplifying Git operations. - Added endpoints for managing user Git configurations, including default and repository-specific profiles. - **Refactor** - Streamlined Git branch and reference management with improved parameter handling and updated URL structures. - Enhanced error reporting and consistency in remote fetch and merge operations. <!-- 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/13112635630
Commit: f85c2bb
Cypress dashboard.
Tags:
@tag.GitSpec:
Mon, 03 Feb 2025 12:27:57 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor