chore: pr for delete references #38210
Conversation
WalkthroughThe pull request introduces enhancements to the Git reference management system in the Appsmith server. A new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
Git Product
![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 (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
621-623: Remove leading space in error messageThere is an unnecessary leading space in the error message.
Apply this diff:
-return Mono.error(new AppsmithException( - AppsmithError.GIT_ACTION_FAILED, "delete ref", " Cannot delete default branch")); +return Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, "delete ref", "Cannot delete default branch"));app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (1)
53-54: Add Javadoc for the new methodConsider adding Javadoc comments to
deleteReferenceto explain its purpose and usage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(2 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/fs/GitFSServiceCEImpl.java(3 hunks)
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
541-544: Change approved
Refactoring to use createGitReference improves clarity and consistency.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (1)
79-81: Approve addition of new methods
The addition of createGitReference and deleteGitReference methods enhances the interface's functionality.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
677-700: Error handling in deleteGitReference is appropriate
The implementation correctly handles exceptions when deleting a branch.
| if (FALSE.equals(isReferenceDeleted)) { | ||
| return Mono.error(new AppsmithException( | ||
| AppsmithError.GIT_ACTION_FAILED, | ||
| " delete branch. Branch does not exists in the repo")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct the error message formatting
Fix the grammatical error and improve consistency in the error message.
Apply this diff:
-return Mono.error(new AppsmithException(
- AppsmithError.GIT_ACTION_FAILED,
- " delete branch. Branch does not exists in the repo"));
+return Mono.error(new AppsmithException(
+ AppsmithError.GIT_ACTION_FAILED,
+ "delete",
+ "Branch does not exist in the repository"));📝 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.
| if (FALSE.equals(isReferenceDeleted)) { | |
| return Mono.error(new AppsmithException( | |
| AppsmithError.GIT_ACTION_FAILED, | |
| " delete branch. Branch does not exists in the repo")); | |
| if (FALSE.equals(isReferenceDeleted)) { | |
| return Mono.error(new AppsmithException( | |
| AppsmithError.GIT_ACTION_FAILED, | |
| "delete", | |
| "Branch does not exist in the repository")); |
| } | ||
|
|
||
| if (!hasText(refName)) { | ||
| return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, BRANCH_NAME)); |
| @@ -572,6 +572,120 @@ public Mono<? extends Artifact> createReference( | |||
| return Mono.create(sink -> createBranchMono.subscribe(sink::success, sink::error, null, sink.currentContext())); | |||
| } | |||
|
|
|||
| @Override | |||
| public Mono<? extends Artifact> deleteReference( | |||
There was a problem hiding this comment.
Nit, deleteGitReference in method name
| @@ -572,6 +572,120 @@ public Mono<? extends Artifact> createReference( | |||
| return Mono.create(sink -> createBranchMono.subscribe(sink::success, sink::error, null, sink.currentContext())); | |||
| } | |||
|
|
|||
| @Override | |||
| public Mono<? extends Artifact> deleteReference( | |||
| String baseArtifactId, String refName, ArtifactType artifactType, GitType gitType, RefType refType) { | |||
There was a problem hiding this comment.
Nit, can we use dto here?
There was a problem hiding this comment.
same gitRefDTO?
| } | ||
|
|
||
| GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); | ||
| AclPermission artifactEditPermission = gitArtifactHelper.getArtifactEditPermission(); |
There was a problem hiding this comment.
We ended up creating a lot of permission methods in the helper. Should we instead create a single getArtifactPermision method and access its methods directly for all our use cases?
There was a problem hiding this comment.
do you mean a separate service?
| gitArtifactHelperResolver.getArtifactHelper(baseArtifact.getArtifactType()); | ||
|
|
||
| // TODO: write a migration to shift everything to refName in gitMetadata | ||
| final String finalRefName = referenceArtifactMetadata.getBranchName(); |
There was a problem hiding this comment.
Can you in the mean time create an internal annotated getRefName method that returns branch name when ref name is null?
| Mono<? extends Artifact> deleteReferenceMono = gitPrivateRepoHelper | ||
| .isBranchProtected(baseGitMetadata, finalRefName) | ||
| .flatMap(isBranchProtected -> { | ||
| if (!TRUE.equals(isBranchProtected)) { |
There was a problem hiding this comment.
Is this meant to be an instance of where we can flip the conditional like you've been suggesting?
There was a problem hiding this comment.
same stuff, even if we flip we are returning the error.
| " delete branch. Branch does not exists in the repo")); | ||
| } | ||
|
|
||
| if (referenceArtifact.getId().equals(baseArtifactId)) { |
There was a problem hiding this comment.
Why don't we delete in this case, do you know?
There was a problem hiding this comment.
we don't delete baseArtifact, where would we keep the gitMetadata then?
There was a problem hiding this comment.
That's a bug then .. It's a stray object that we can never get rid of. Let's add a TODO to clean this up when we pull metadata out of artifact
| return gitArtifactHelper | ||
| .deleteArtifactByResource(referenceArtifact) | ||
| .onErrorResume(throwable -> { | ||
| return gitAnalyticsUtils |
There was a problem hiding this comment.
Should log error before transofrming
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
620-621: Address TODO comment for reference name migrationThere's a pending migration to shift everything to refName in gitMetadata.
Would you like me to help create a GitHub issue to track this migration task?
632-641: Consider inverting the condition for better readabilityThe nested if-else structure could be simplified by inverting the condition.
-if (!TRUE.equals(isBranchProtected)) { - return gitRedisUtils.acquireGitLock( - baseArtifactId, GitConstants.GitCommandConstants.DELETE, TRUE); -} - -return Mono.error(new AppsmithException( - AppsmithError.GIT_ACTION_FAILED, - "delete", - "Cannot delete protected branch " + finalRefName)); +if (TRUE.equals(isBranchProtected)) { + return Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, + "delete", + "Cannot delete protected branch " + finalRefName)); +} + +return gitRedisUtils.acquireGitLock( + baseArtifactId, GitConstants.GitCommandConstants.DELETE, TRUE);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/GitArtifactMetadataCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(2 hunks)
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (1)
52-54: LGTM! Method signature follows interface conventions
The new deleteGitReference method signature is well-defined and consistent with other methods in the interface.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (4)
575-593: LGTM! Comprehensive parameter validation
The implementation properly validates all required parameters before proceeding.
655-659: Fix error message formatting
The error message needs improvement for clarity and consistency.
Apply this diff:
-return Mono.error(new AppsmithException(
- AppsmithError.GIT_ACTION_FAILED,
- " delete branch. Branch does not exists in the repo"));
+return Mono.error(new AppsmithException(
+ AppsmithError.GIT_ACTION_FAILED,
+ "delete",
+ "Branch does not exist in the repository"));668-672: Log error before analytics
The error should be logged before transforming for analytics to ensure proper error tracking.
Move the log statement before the analytics transformation.
541-541: Method name change from prepareForNewRefCreation to createGitReference
The method name has been updated to better reflect its purpose.
| public void setRefName(String refName) { | ||
| this.branchName = refName; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent state between refName and branchName
The setter directly updates branchName instead of refName, which could lead to inconsistent state.
-public void setRefName(String refName) {
- this.branchName = refName;
+public void setRefName(String refName) {
+ this.refName = refName;
+ this.branchName = refName; // Temporary until migration is complete
}📝 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.
| public void setRefName(String refName) { | |
| this.branchName = refName; | |
| } | |
| public void setRefName(String refName) { | |
| this.refName = refName; | |
| this.branchName = refName; // Temporary until migration is complete | |
| } |
| /** | ||
| * this returns the branchName instead of reference name | ||
| * @return returns the ref name. | ||
| */ | ||
| public String getRefName() { | ||
| return this.getBranchName(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving the reference name handling
The current implementation returns branchName instead of refName, creating tight coupling. Based on the TODO comment, this appears to be temporary, but should be addressed to properly separate branch and reference concepts.
Consider:
- Implementing proper reference name handling
- Adding validation for reference types
- Documenting the migration plan in the TODO
-public String getRefName() {
- return this.getBranchName();
-}
+public String getRefName() {
+ return StringUtils.hasText(this.refName) ? this.refName : this.getBranchName();
+}Committable suggestion skipped: line range outside the PR's diff.
Description
Fixes #37456
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
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12374977893
Commit: ccb37fc
Cypress dashboard.
Tags:
@tag.GitSpec:
Tue, 17 Dec 2024 15:09:45 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Changes