chore(git): idempotent ref creation in CentralGitServiceCEImpl#41758
Merged
Conversation
subrata71
approved these changes
Apr 22, 2026
Contributor
WalkthroughThe Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
698-700: Prefer aFieldNameconstant over the literal"artifactType".The surrounding guards use
FieldName.ID,REF_NAME,REF_TYPEetc. Dropping a raw string here breaks the pattern and makes the error payload inconsistent with the rest of this class. If no constant exists yet, addingFieldName.ARTIFACT_TYPEwould be the cleanest landing spot.🧹 Suggested tweak
- if (artifactType == null) { - return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "artifactType")); - } + if (artifactType == null) { + return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ARTIFACT_TYPE)); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java` around lines 698 - 700, The null-check in CentralGitServiceCEImpl currently returns an AppsmithException using the raw string "artifactType"; replace this literal with the FieldName constant to match the pattern used elsewhere (e.g., FieldName.ID, REF_NAME, REF_TYPE) by changing the error to use FieldName.ARTIFACT_TYPE; if FieldName.ARTIFACT_TYPE does not yet exist, add that constant to the FieldName holder (enum/class) and then use FieldName.ARTIFACT_TYPE in the Mono.error call so the error payload remains consistent across the class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java`:
- Around line 698-700: The null-check in CentralGitServiceCEImpl currently
returns an AppsmithException using the raw string "artifactType"; replace this
literal with the FieldName constant to match the pattern used elsewhere (e.g.,
FieldName.ID, REF_NAME, REF_TYPE) by changing the error to use
FieldName.ARTIFACT_TYPE; if FieldName.ARTIFACT_TYPE does not yet exist, add that
constant to the FieldName holder (enum/class) and then use
FieldName.ARTIFACT_TYPE in the Mono.error call so the error payload remains
consistent across the class.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08ca41fd-1a6e-41c8-ad11-789b3719b673
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Tip
TL;DR — Tightens
CentralGitServiceCEImpl.createReference(...)so branch creation is idempotent and does not silently convert a successful branch into an error response. Fixes an orphaned DB artifact on partial failure, a Redis lock that was never actually acquired (acquireGitLock(..., FALSE)), a double lock release, analytics/release failures masquerading as git failures, a false-positive duplicate-name check, a wrong DTO inlistReferences, a misleading error label, and missing null guards in the public overload.Tightens the branch-creation flow in
CentralGitServiceCEImpl.createReference(...). No behavior change on the happy path; the changes fix partial-failure handling and add the guard rails needed to make the operation safely retryable.Fixes applied to
CentralGitServiceCEImpl.javaacquireGitLockwas being called withisLockRequired = FALSE, whichGitRedisUtilsshort-circuits to a no-op (the create-branch flow never actually held a lock). Flipped toTRUEand wrapped acquire/release inMono.usingWhen(...)so the Redis lock is acquired and released exactly once per invocation, regardless of success, error, or cancellation. The outeronErrorResumeno longer touches the lock, eliminating the prior double-release.addAnalyticsForGitOperationnow runs inside.onErrorResume(err -> Mono.empty()).thenReturn(newImportedArtifact)so an analytics or lock-release failure can't convert a fully-completed branch creation into aGIT_ACTION_FAILEDresponse.createGitReference → importArtifactInWorkspaceFromGit → publishArtifactPostRefCreationfails aftergenerateArtifactForRefCreationhas already persisted a new artifact, we now callgitArtifactHelper.deleteArtifactByResource(newRefArtifact)before re-raising. Rollback failures are logged and swallowed so they don't mask the original git error. Scope deliberately stops beforediscardChangeson the source branch — by that point the remote ref already exists and deleting the DB row would orphan it.listReferencesuses the right DTO — Switched fromcreateRefTransformationDTO(the not-yet-existing target ref) tobaseRefTransformationDTO(the source ref) when listing refs for the duplicate-name check.createGitReferenceare now wrapped as"ref creation"instead of the misleading"ref creation preparation".refName.replaceFirst(ORIGIN, REMOTE_NAME_REPLACEMENT)with a newstripOriginPrefix(...)helper that only stripsorigin/when it appears as a prefix. Ref names likefeat/origin/thingvsfeat/thingare no longer falsely reported as duplicates.INVALID_PARAMETERguards forreferencedArtifactIdandartifactTypeto fail fast instead of NPE'ing insidegetArtifactHelper.Followups (out of scope for this PR)
The same un-anchored
.replaceFirst(ORIGIN, REMOTE_NAME_REPLACEMENT)pattern still exists at a few other sites inCentralGitServiceCEImpl(lines 519, 622, 2642, 3206),GitFSServiceCEImpl.java, andCommonGitServiceCEImpl.java. A separate sweep can convert those to the sharedstripOriginPrefixhelper.Automation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Improvements
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/24778917502
Commit: eddfad1
Cypress dashboard.
Tags:
@tag.GitSpec:
Wed, 22 Apr 2026 13:08:14 UTC