Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -130,5 +130,17 @@ public void setDefaultApplicationId(String defaultApplicationId) {
this.defaultArtifactId = defaultApplicationId;
}

/**
* this returns the branchName instead of reference name
* @return returns the ref name.
*/
public String getRefName() {
return this.getBranchName();
}
Comment on lines +133 to +139

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Implementing proper reference name handling
  2. Adding validation for reference types
  3. 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.


public void setRefName(String refName) {
this.branchName = refName;
}
Comment on lines +141 to +143

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
public void setRefName(String refName) {
this.branchName = refName;
}
public void setRefName(String refName) {
this.refName = refName;
this.branchName = refName; // Temporary until migration is complete
}


public static class Fields {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,7 @@ Mono<? extends Artifact> checkoutReference(

Mono<? extends Artifact> createReference(
String referencedArtifactId, GitRefDTO refDTO, ArtifactType artifactType, GitType gitType);

Mono<? extends Artifact> deleteGitReference(
String baseArtifactId, GitRefDTO gitRefDTO, ArtifactType artifactType, GitType gitType);
}
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,7 @@ public Mono<? extends Artifact> createReference(
Artifact newRefArtifact = tuple.getT1();

Mono<String> refCreationMono = gitHandlingService
.prepareForNewRefCreation(jsonTransformationDTO)
.createGitReference(jsonTransformationDTO)
// TODO: ths error could be shipped to handling layer as well?
.onErrorResume(error -> Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED,
Expand Down Expand Up @@ -572,6 +572,127 @@ public Mono<? extends Artifact> createReference(
return Mono.create(sink -> createBranchMono.subscribe(sink::success, sink::error, null, sink.currentContext()));
}

@Override
public Mono<? extends Artifact> deleteGitReference(
String baseArtifactId, GitRefDTO gitRefDTO, ArtifactType artifactType, GitType gitType) {

String refName = gitRefDTO.getRefName();
RefType refType = gitRefDTO.getRefType();

if (refType == null) {
return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, REF_TYPE));
}

if (!hasText(refName)) {
return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, REF_NAME));
}

if (!hasText(baseArtifactId)) {
return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ID));
}

GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
AclPermission artifactEditPermission = gitArtifactHelper.getArtifactEditPermission();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean a separate service?


Mono<? extends Artifact> baseArtifactMono =
gitArtifactHelper.getArtifactById(baseArtifactId, artifactEditPermission);

Mono<? extends Artifact> branchedArtifactMono =
gitArtifactHelper.getArtifactByBaseIdAndBranchName(baseArtifactId, refName, artifactEditPermission);

return Mono.zip(baseArtifactMono, branchedArtifactMono).flatMap(tuple2 -> {
Artifact baseArtifact = tuple2.getT1();
Artifact referenceArtifact = tuple2.getT2();
return deleteGitReference(baseArtifact, referenceArtifact, gitType, refType);
});
}

protected Mono<? extends Artifact> deleteGitReference(
Artifact baseArtifact, Artifact referenceArtifact, GitType gitType, RefType refType) {

GitArtifactMetadata baseGitMetadata = baseArtifact.getGitArtifactMetadata();
GitArtifactMetadata referenceArtifactMetadata = referenceArtifact.getGitArtifactMetadata();

GitHandlingService gitHandlingService = gitHandlingServiceResolver.getGitHandlingService(gitType);
GitArtifactHelper<?> gitArtifactHelper =
gitArtifactHelperResolver.getArtifactHelper(baseArtifact.getArtifactType());

// TODO: write a migration to shift everything to refName in gitMetadata
final String finalRefName = referenceArtifactMetadata.getRefName();
final String baseArtifactId = referenceArtifact.getGitArtifactMetadata().getDefaultArtifactId();

if (finalRefName.equals(baseGitMetadata.getDefaultBranchName())) {
return Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED, "delete ref", " Cannot delete default branch"));
}

Mono<? extends Artifact> deleteReferenceMono = gitPrivateRepoHelper
.isBranchProtected(baseGitMetadata, finalRefName)
.flatMap(isBranchProtected -> {
if (!TRUE.equals(isBranchProtected)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to be an instance of where we can flip the conditional like you've been suggesting?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same stuff, even if we flip we are returning the error.

return gitRedisUtils.acquireGitLock(
baseArtifactId, GitConstants.GitCommandConstants.DELETE, TRUE);
}

return Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED,
"delete",
"Cannot delete protected branch " + finalRefName));
})
.flatMap(ignoreLockAcquisition -> {
ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO();
jsonTransformationDTO.setWorkspaceId(baseArtifact.getWorkspaceId());
jsonTransformationDTO.setArtifactType(baseArtifact.getArtifactType());
jsonTransformationDTO.setRefType(refType);
jsonTransformationDTO.setRefName(finalRefName);
jsonTransformationDTO.setBaseArtifactId(baseArtifactId);
jsonTransformationDTO.setRepoName(referenceArtifactMetadata.getRepoName());

return gitHandlingService
.deleteGitReference(jsonTransformationDTO)
.doFinally(signalType -> gitRedisUtils.releaseFileLock(baseArtifactId, TRUE))
.flatMap(isReferenceDeleted -> {
if (FALSE.equals(isReferenceDeleted)) {
return Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED,
" delete branch. Branch does not exists in the repo"));
Comment on lines +655 to +658

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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 (referenceArtifact.getId().equals(baseArtifactId)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we delete in this case, do you know?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't delete baseArtifact, where would we keep the gitMetadata then?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Mono.just(referenceArtifact);
}

return gitArtifactHelper
.deleteArtifactByResource(referenceArtifact)
.onErrorResume(throwable -> {
log.error(
"An error occurred while deleting db artifact and resources for reference {}",
throwable.getMessage());

return gitAnalyticsUtils

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should log error before transofrming

.addAnalyticsForGitOperation(
AnalyticsEvents.GIT_DELETE_BRANCH,
referenceArtifact,
throwable.getClass().getName(),
throwable.getMessage(),
baseGitMetadata.getIsRepoPrivate())
.then(Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED,
"Cannot delete branch from database")));
});
});
})
.flatMap(deletedArtifact -> gitAnalyticsUtils.addAnalyticsForGitOperation(
AnalyticsEvents.GIT_DELETE_BRANCH,
deletedArtifact,
deletedArtifact.getGitArtifactMetadata().getIsRepoPrivate()))
.name(GitSpan.OPS_DELETE_BRANCH)
.tap(Micrometer.observation(observationRegistry));

return Mono.create(
sink -> deleteReferenceMono.subscribe(sink::success, sink::error, null, sink.currentContext()));
}

/**
* Connect the artifact from Appsmith to a git repo
* This is the prerequisite step needed to perform all the git operation for an artifact
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,5 +76,7 @@ Mono<? extends ArtifactExchangeJson> recreateArtifactJsonFromLastCommit(

Mono<GitStatusDTO> getStatus(ArtifactJsonTransformationDTO jsonTransformationDTO);

Mono<String> prepareForNewRefCreation(ArtifactJsonTransformationDTO artifactJsonTransformationDTO);
Mono<String> createGitReference(ArtifactJsonTransformationDTO artifactJsonTransformationDTO);

Mono<Boolean> deleteGitReference(ArtifactJsonTransformationDTO jsonTransformationDTO);
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import io.micrometer.observation.ObservationRegistry;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.eclipse.jgit.api.errors.CannotDeleteCurrentBranchException;
import org.eclipse.jgit.api.errors.EmptyCommitException;
import org.eclipse.jgit.api.errors.InvalidRemoteException;
import org.eclipse.jgit.api.errors.TransportException;
Expand Down Expand Up @@ -661,7 +662,7 @@ public Mono<GitStatusDTO> getStatus(ArtifactJsonTransformationDTO jsonTransforma
}

@Override
public Mono<String> prepareForNewRefCreation(ArtifactJsonTransformationDTO jsonTransformationDTO) {
public Mono<String> createGitReference(ArtifactJsonTransformationDTO jsonTransformationDTO) {
GitArtifactHelper<?> gitArtifactHelper =
gitArtifactHelperResolver.getArtifactHelper(jsonTransformationDTO.getArtifactType());

Expand All @@ -672,4 +673,29 @@ public Mono<String> prepareForNewRefCreation(ArtifactJsonTransformationDTO jsonT

return fsGitHandler.createAndCheckoutToBranch(repoSuffix, jsonTransformationDTO.getRefName());
}

@Override
public Mono<Boolean> deleteGitReference(ArtifactJsonTransformationDTO jsonTransformationDTO) {
GitArtifactHelper<?> gitArtifactHelper =
gitArtifactHelperResolver.getArtifactHelper(jsonTransformationDTO.getArtifactType());

Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(
jsonTransformationDTO.getWorkspaceId(),
jsonTransformationDTO.getBaseArtifactId(),
jsonTransformationDTO.getRepoName());

return fsGitHandler
.deleteBranch(repoSuffix, jsonTransformationDTO.getRefName())
.onErrorResume(throwable -> {
log.error("Delete branch failed {}", throwable.getMessage());
if (throwable instanceof CannotDeleteCurrentBranchException) {
return Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED,
"delete branch",
"Cannot delete current checked out branch"));
}
return Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED, "delete branch", throwable.getMessage()));
});
}
}