chore: git detach and push algorithm refinement #39298
Conversation
WalkthroughThis PR updates the artifact processing logic in two services. In the Central Git service, the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CentralGitService
participant Database
Client->>CentralGitService: detachRemote(artifactId, artifactType, gitType)
CentralGitService->>Database: Query branched artifact & metadata
Database-->>CentralGitService: Artifact data
CentralGitService->>Database: Remove branched artifacts except base
Database-->>CentralGitService: Confirmation
CentralGitService-->>Client: Returns base artifact
sequenceDiagram
participant Client
participant GitFSService
participant GitRepo
participant Logger
Client->>GitFSService: commitArtifact(artifact, commitDTO, transformationDTO)
GitFSService->>GitRepo: pushArtifact(artifact)
alt Push Successful
GitRepo-->>GitFSService: Push confirmation
else Push Failure (TransportException)
GitFSService->>Logger: Log error & trigger error recovery
Logger-->>GitFSService: Error acknowledged
end
GitFSService-->>Client: Returns (artifact, commit message)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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
|
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: 0
🧹 Nitpick comments (7)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (4)
1602-1602: Clarify doc comment for branched artifacts
The updated comment more precisely states that only the base artifact is excluded from removal. This is clear and succinct.
1617-1638: Ensure robust error handling in Mono chain
While the chaining logic is clear, handle potential null or unexpected states (e.g. missing metadata) to avoid hidden exceptions.
1617-1624: ValidatebranchedArtifact.getGitArtifactMetadata()before usage.You already have a check at line 1624, but ensure any subsequent usage also handles potential null references. This helps guard against unexpected NullPointerExceptions.
1677-1679: Analytics scopes might be expanded.Currently, you send an event with a
falsemarker forisRepoPrivate. Consider whether a distinction is needed if the repository was previously private. This can give more accurate insights.app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (3)
495-577: Refine pushArtifact logic
The push logic is streamlined. Watch for corner cases like missing keys or empty remote URL, which could cause push failures.
595-610: Improved push error recovery logic.The
pushArtifactErrorRecoverymethod now clearly identifies and reacts to typical push failures like branch protection rules. The logic that checks the string"pre-receive hook declined"is practical, but you may consider a more robust detection approach (e.g., checking exception types or codes) if available to avoid relying on string matching alone.
559-578: Refined push error handling.Applying extra checks for “REJECTED_NONFASTFORWARD” vs. “REJECTED_OTHERREASON” is good. Consider capturing more granular error codes from the Git library if needed. This will further improve diagnostic clarity.
📜 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(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (15)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (9)
1606-1607: Add doc detail for gitType parameter
Including the@param gitTypeannotation is good for clarity and helps future maintainers.
1609-1612: Extend method signature to accept GitType
Verifying that all existing callers and references are updated accordingly is recommended.
1602-1685: Streamlined removal of branched artifacts and local repo.The updated
detachRemotemethod effectively removes all branch artifacts while preserving the base artifact’s metadata. The use ofMono.zipto parallelize discarding DB references and removing the local repository is sensible, provided that these operations are indeed independent and do not require a particular sequence. If the sequence matters, consider chaining them withflatMapinstead. Otherwise, the overall flow for disconnecting the repository and resetting the base artifact’s Git metadata appears consistent. Nicely done!
1602-1602: Documentation enhancement is aligned with updated logic.Including in the docstring that all branched artifacts (except the base) will be removed clarifies expectations for developers.
1609-1612: Method signature now acceptsGitType.The extra parameter helps route the correct handling service. Verify that all callers of
detachRemotehave been updated accordingly.
1630-1634: Ensure consistency of naming convention.Variables
baseArtifactId,repoName,workspaceId,refNameare introduced in a concise block. This naming style is consistent but make sure it’s uniformly applied throughout the class to improve readability.
1636-1640: JSON transformation steps are logical.Retaining ref information in the
ArtifactJsonTransformationDTOclarifies the context for further operations.
1645-1660: Handle concurrency carefully.You acquire an edit permission and filter + operate on multiple artifacts. This is typically safe with short operations, but double-check that no race conditions arise when concurrently detaching multiple artifacts from the same base.
1667-1674: Deletion of branched artifacts is appropriate.You remove all non-base artifacts while resetting the base artifact’s metadata. This aligns with your docstring. Good approach for a clean detach.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (6)
484-485: Remove isFileLock argument from pushArtifact call
Check all usages to ensure they now conform to the updated signature.
609-610: Handle upstream changes
Immediately throwing an error for upstream changes is valid. Consider offering ways to auto-resolve or stash local changes if needed.
484-484: Updated commit flow invocation.The direct invocation of
pushArtifact(tuple.getT2())removes the file-lock parameter, simplifying the call site. This aligns with the new push flow design without apparent side effects. Good job!
495-583: Removal of file-lock parameter frompushArtifact.The newly introduced
pushArtifact(Artifact branchedArtifact)signature drops theisFileLockparameter, which might affect concurrency safeguards previously in place. Confirm that all locking or synchronization needs are handled elsewhere or are no longer necessary in this flow. Otherwise, the error handling for transport failures and remote rejections (e.g.,TransportException) is neatly structured here.
484-484: Commit flow updated to invokepushArtifactwithoutisFileLock.This aligns with the simplified signature. Ensure all internal calls and error paths are tested to confirm no file lock logic is unintentionally lost.
495-546: SimplifiedpushArtifactsignature and improved logging.
- Removing
isFileLockhelps streamline usage.- The logging statements provide helpful context on success/fail.
- Verify that older references to the old signature are removed or updated to prevent compile errors.
|
|
||
| return fsGitHandler | ||
| .checkoutToBranch( | ||
| .pushApplication( |
There was a problem hiding this comment.
Nit: Naming. You wanted to mean pushArtifact maybe
There was a problem hiding this comment.
This is an already existing method, can I take this in another refactor session?
| Mono<String> gitPushResult = gitArtifactMetadataMono | ||
| .flatMap(gitMetadata -> { | ||
| return gitRedisUtils | ||
| .acquireGitLock( |
There was a problem hiding this comment.
Can you please state the reason of removing Git lock placement logic here?
There was a problem hiding this comment.
Because this is always getting called from inside the commit section
There was a problem hiding this comment.
which means that the redis lock would never be required
| @@ -493,7 +492,7 @@ public Mono<Tuple2<? extends Artifact, String>> commitArtifact( | |||
| * @param branchedArtifact application which needs to be pushed to remote repo | |||
There was a problem hiding this comment.
Nit: comment has reference to application
| .map(Tuple3::getT1); | ||
| if (artifact.getGitArtifactMetadata() == null | ||
| || RefType.tag.equals(artifact.getGitArtifactMetadata() | ||
| .getRefType())) { |
There was a problem hiding this comment.
It seems llike this logic should precede the above logic from L#1649-L#1659
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
1612-1679: Consider batch operations for artifact deletion.While the logic is correct, consider using batch operations for deleting branched artifacts to improve performance when dealing with repositories having many branches.
- Mono<? extends Artifact> deleteAllBranchesExceptBase = gitArtifactHelper - .getAllArtifactByBaseId(baseArtifactId, artifactEditPermission) - .flatMap(artifact -> { - if (artifact.getGitArtifactMetadata() == null - || RefType.tag.equals(artifact.getGitArtifactMetadata() - .getRefType())) { - return Mono.just(artifact); - } - - // it's established that git artifact metadata is not null - if (!artifact.getId().equals(baseArtifactId)) { - return gitArtifactHelper.deleteArtifactByResource(artifact); - } + Mono<? extends Artifact> deleteAllBranchesExceptBase = gitArtifactHelper + .getAllArtifactByBaseId(baseArtifactId, artifectEditPermission) + .collectList() + .flatMap(artifacts -> { + List<Artifact> artifactsToDelete = artifacts.stream() + .filter(artifact -> + artifact.getGitArtifactMetadata() != null && + !RefType.tag.equals(artifact.getGitArtifactMetadata().getRefType()) && + !artifact.getId().equals(baseArtifactId)) + .collect(Collectors.toList()); + + return Flux.fromIterable(artifactsToDelete) + .flatMap(gitArtifactHelper::deleteArtifactByResource) + .then(Mono.justOrEmpty(artifacts.stream() + .filter(artifact -> artifact.getId().equals(baseArtifactId)) + .findFirst()));
📜 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(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
1599-1611: LGTM! Method signature and documentation improvements.The updated signature with
GitTypeparameter and clarified documentation about artifact removal behavior enhances the API's clarity.
## Description - logic modification for two flows - git detach - git push 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/13330609252> > Commit: 888cf6f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13330609252&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Fri, 14 Feb 2025 14:41:10 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 - **Refactor** - Streamlined version control operations for more consistent and reliable artifact management. - Simplified method signatures for better usability in artifact management processes. - **Bug Fixes** - Improved error messaging and recovery during remote interactions, providing clearer feedback when issues occur. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
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/13330609252
Commit: 888cf6f
Cypress dashboard.
Tags:
@tag.GitSpec:
Fri, 14 Feb 2025 14:41:10 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Refactor
Bug Fixes