Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: reference lifecycle #38174

Merged
merged 15 commits into from
Dec 17, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package com.appsmith.external.dtos;

import com.appsmith.external.git.constants.ce.RefType;
import lombok.AllArgsConstructor;
import lombok.Data;
import lombok.NoArgsConstructor;

@Data
@AllArgsConstructor
@NoArgsConstructor
public class GitRefDTO {

String refName;

RefType refType;

boolean isDefault;

boolean createdFromLocal;
Comment on lines +13 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add field validation and documentation.

The fields would benefit from:

  1. Validation annotations for refName
  2. JavaDoc documentation explaining each field's purpose
  3. Making fields final for immutability
+import javax.validation.constraints.NotNull;
+
 @Data
 @AllArgsConstructor
 @NoArgsConstructor
 public class GitRefDTO {
 
+    /** The name of the Git reference (e.g., branch name or tag name) */
+    @NotNull
-    String refName;
+    private final String refName;
 
+    /** The type of the Git reference (BRANCH or TAG) */
+    @NotNull
-    RefType refType;
+    private final RefType refType;
 
+    /** Indicates if this is the default reference (e.g., default branch) */
-    boolean isDefault;
+    private final boolean isDefault;
 
+    /** Indicates if the reference was created in the local repository */
-    boolean createdFromLocal;
+    private final boolean createdFromLocal;

Committable suggestion skipped: line range outside the PR's diff.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package com.appsmith.external.git.constants.ce;

public enum RefType {
BRANCH,
TAG
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package com.appsmith.server.domains.ce;

import com.appsmith.external.git.constants.ce.RefType;
import com.appsmith.external.models.AppsmithDomain;
import com.appsmith.external.views.Views;
import com.appsmith.server.constants.ce.RefType;
import com.appsmith.server.domains.AutoCommitConfig;
import com.appsmith.server.domains.GitAuth;
import com.appsmith.server.domains.GitProfile;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package com.appsmith.server.git.central;

import com.appsmith.external.dtos.GitStatusDTO;
import com.appsmith.external.git.constants.ce.RefType;
import com.appsmith.git.dto.CommitDTO;
import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.constants.ce.RefType;
import com.appsmith.server.domains.Artifact;
import com.appsmith.server.dtos.ArtifactImportDTO;
import com.appsmith.server.dtos.GitConnectDTO;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package com.appsmith.server.git.central;

import com.appsmith.external.constants.AnalyticsEvents;
import com.appsmith.external.dtos.GitRefDTO;
import com.appsmith.external.dtos.GitStatusDTO;
import com.appsmith.external.git.constants.GitConstants;
import com.appsmith.external.git.constants.GitSpan;
import com.appsmith.external.git.constants.ce.RefType;
import com.appsmith.external.models.Datasource;
import com.appsmith.external.models.DatasourceStorage;
import com.appsmith.git.dto.CommitDTO;
Expand All @@ -12,7 +14,6 @@
import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.constants.FieldName;
import com.appsmith.server.constants.GitDefaultCommitMessage;
import com.appsmith.server.constants.ce.RefType;
import com.appsmith.server.datasources.base.DatasourceService;
import com.appsmith.server.domains.Artifact;
import com.appsmith.server.domains.GitArtifactMetadata;
Expand Down Expand Up @@ -397,8 +398,9 @@ protected Mono<? extends Artifact> checkoutReference(

if (referenceToBeCheckedOut.startsWith(ORIGIN)) {

// checking for local present references first
checkedOutArtifactMono = gitHandlingService
.listReferences(jsonTransformationDTO, refType)
.listReferences(jsonTransformationDTO, FALSE, refType)
.flatMap(gitRefs -> {
long branchMatchCount = gitRefs.stream()
.filter(gitRef -> gitRef.equals(finalRefName))
Expand Down Expand Up @@ -437,6 +439,134 @@ protected Mono<? extends Artifact> checkoutRemoteBranch(Artifact baseArtifact, S
return null;
}

public Mono<? extends Artifact> checkoutReference(
String referencedArtifactId,
GitRefDTO refDTO,
ArtifactType artifactType,
GitType gitType,
RefType refType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we sending ref type separately here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can put it in the DTO itself, right.


/*
1. Check if the src artifact is available and user have sufficient permissions
2. Create and checkout to requested branch
3. Rehydrate the artifact from source artifact reference
*/

if (!hasText(refDTO.getRefName()) || refDTO.getRefName().startsWith(ORIGIN)) {
return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, BRANCH_NAME));
}

GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
GitHandlingService gitHandlingService = gitHandlingServiceResolver.getGitHandlingService(gitType);
AclPermission artifactEditPermission = gitArtifactHelper.getArtifactEditPermission();

Mono<Tuple2<? extends Artifact, ? extends Artifact>> baseAndBranchedArtifactMono =
getBaseAndBranchedArtifacts(referencedArtifactId, artifactType, artifactEditPermission);

Mono<? extends Artifact> createBranchMono = baseAndBranchedArtifactMono
.flatMap(artifactTuples -> {
Artifact baseArtifact = artifactTuples.getT1();
Artifact parentArtifact = artifactTuples.getT2();

GitArtifactMetadata baseGitMetadata = baseArtifact.getGitArtifactMetadata();
GitAuth baseGitAuth = baseGitMetadata.getGitAuth();
GitArtifactMetadata parentGitMetadata = parentArtifact.getGitArtifactMetadata();

ArtifactJsonTransformationDTO jsonTransformationDTO = new ArtifactJsonTransformationDTO();
jsonTransformationDTO.setWorkspaceId(baseArtifact.getWorkspaceId());
jsonTransformationDTO.setBaseArtifactId(baseGitMetadata.getDefaultArtifactId());
jsonTransformationDTO.setRepoName(baseGitMetadata.getRepoName());
jsonTransformationDTO.setArtifactType(baseArtifact.getArtifactType());
jsonTransformationDTO.setRefType(refType);
jsonTransformationDTO.setRefName(refDTO.getRefName());

if (parentGitMetadata == null
|| !hasText(parentGitMetadata.getDefaultArtifactId())
|| !hasText(parentGitMetadata.getRepoName())) {
// TODO: add refType in error
return Mono.error(
new AppsmithException(
AppsmithError.INVALID_GIT_CONFIGURATION,
"Unable to find the parent reference. Please create a reference from other available references"));
}

Mono<Boolean> acquireGitLockMono = gitRedisUtils.acquireGitLock(
baseGitMetadata.getDefaultArtifactId(),
GitConstants.GitCommandConstants.CREATE_BRANCH,
FALSE);
Mono<String> fetchRemoteMono =
gitHandlingService.fetchRemoteChanges(jsonTransformationDTO, baseGitAuth, TRUE);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update fetchRemoteChanges calls to match new method signature

The fetchRemoteChanges method signature has changed. Update the calls at lines 498 and 1334 to match the new parameters, preventing compilation errors.

Example correction:

- gitHandlingService.fetchRemoteChanges(jsonTransformationDTO, baseGitAuth, TRUE);
+ gitHandlingService.fetchRemoteChanges(baseArtifact, refArtifact, TRUE, gitType, refType);

Also applies to: 1334-1335


return acquireGitLockMono
.flatMap(ignoreLockAcquisition -> fetchRemoteMono.onErrorResume(error ->
Mono.error(new AppsmithException(AppsmithError.GIT_ACTION_FAILED, "fetch", error))))
.flatMap(ignoreFetchString -> gitHandlingService
.listReferences(jsonTransformationDTO, TRUE, refType)
.flatMap(refList -> {
boolean isDuplicateName = refList.stream()
// We are only supporting origin as the remote name so this is safe
// but needs to be altered if we start supporting user defined remote
// names
.anyMatch(ref -> ref.replaceFirst(ORIGIN, REMOTE_NAME_REPLACEMENT)
.equals(refDTO.getRefName()));

if (isDuplicateName) {
return Mono.error(new AppsmithException(
AppsmithError.DUPLICATE_KEY_USER_ERROR,
"remotes/origin/" + refDTO.getRefName(),
FieldName.BRANCH_NAME));
}

Mono<? extends ArtifactExchangeJson> artifactExchangeJsonMono =
exportService.exportByArtifactId(
parentArtifact.getId(), VERSION_CONTROL, artifactType);

Mono<? extends Artifact> newArtifactFromSourceMono =
// TODO: add refType support over here
gitArtifactHelper.createNewArtifactForCheckout(
parentArtifact, refDTO.getRefName());

return Mono.zip(newArtifactFromSourceMono, artifactExchangeJsonMono);
}))
.flatMap(tuple -> {
ArtifactExchangeJson exportedJson = tuple.getT2();
Artifact newRefArtifact = tuple.getT1();

Mono<String> refCreationMono = gitHandlingService
.prepareForNewRefCreation(jsonTransformationDTO)
// TODO: ths error could be shipped to handling layer as well?
.onErrorResume(error -> Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED,
"ref creation preparation",
error.getMessage())));

return refCreationMono
.flatMap(ignoredString -> {
return importService.importArtifactInWorkspaceFromGit(
newRefArtifact.getWorkspaceId(),
newRefArtifact.getId(),
exportedJson,
refDTO.getRefName());
})
// after the branch is created, we need to reset the older branch to initial
// commit
.doOnSuccess(newImportedArtifact ->
discardChanges(parentArtifact.getId(), artifactType, gitType));
});
})
.flatMap(newImportedArtifact -> gitAnalyticsUtils
.addAnalyticsForGitOperation(
AnalyticsEvents.GIT_CREATE_BRANCH,
newImportedArtifact,
newImportedArtifact.getGitArtifactMetadata().getIsRepoPrivate())
.doFinally(signalType -> gitRedisUtils.releaseFileLock(
newImportedArtifact.getGitArtifactMetadata().getDefaultArtifactId(), TRUE)))
.name(GitSpan.OPS_CREATE_BRANCH)
.tap(Micrometer.observation(observationRegistry));

return Mono.create(sink -> createBranchMono.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 Expand Up @@ -1201,8 +1331,8 @@ public Mono<String> fetchRemoteChanges(

// current user mono has been zipped just to run in parallel.
Mono<String> fetchRemoteMono = acquireGitLockMono
.then(Mono.defer(() ->
gitHandlingService.fetchRemoteChanges(jsonTransformationDTO, baseArtifactGitData.getGitAuth())))
.then(Mono.defer(() -> gitHandlingService.fetchRemoteChanges(
jsonTransformationDTO, baseArtifactGitData.getGitAuth(), FALSE)))
.flatMap(fetchedRemoteStatusString -> {
return gitRedisUtils
.releaseFileLock(baseArtifactId, isFileLock)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package com.appsmith.server.git.central;

import com.appsmith.external.dtos.GitStatusDTO;
import com.appsmith.external.git.constants.ce.RefType;
import com.appsmith.git.dto.CommitDTO;
import com.appsmith.server.constants.ce.RefType;
import com.appsmith.server.domains.Artifact;
import com.appsmith.server.domains.GitArtifactMetadata;
import com.appsmith.server.domains.GitAuth;
Expand Down Expand Up @@ -41,9 +41,15 @@ void setRepositoryDetailsInGitArtifactMetadata(

Mono<Boolean> removeRepository(ArtifactJsonTransformationDTO artifactJsonTransformationDTO);

Mono<List<String>> listBranches(
ArtifactJsonTransformationDTO artifactJsonTransformationDTO, Boolean checkRemoteBranches);

Mono<List<String>> listBranches(ArtifactJsonTransformationDTO artifactJsonTransformationDTO);

Mono<List<String>> listReferences(ArtifactJsonTransformationDTO artifactJsonTransformationDTO, RefType refType);
Mono<List<String>> listReferences(
ArtifactJsonTransformationDTO artifactJsonTransformationDTO,
Boolean checkRemoteReferences,
RefType refType);

Mono<Boolean> validateEmptyRepository(ArtifactJsonTransformationDTO artifactJsonTransformationDTO);

Expand All @@ -62,10 +68,13 @@ Mono<Boolean> prepareChangesToBeCommitted(
Mono<Tuple2<? extends Artifact, String>> commitArtifact(
Artifact branchedArtifact, CommitDTO commitDTO, ArtifactJsonTransformationDTO jsonTransformationDTO);

Mono<String> fetchRemoteChanges(ArtifactJsonTransformationDTO jsonTransformationDTO, GitAuth gitAuth);
Mono<String> fetchRemoteChanges(
ArtifactJsonTransformationDTO jsonTransformationDTO, GitAuth gitAuth, Boolean isFetchAll);

Mono<? extends ArtifactExchangeJson> recreateArtifactJsonFromLastCommit(
ArtifactJsonTransformationDTO jsonTransformationDTO);

Mono<GitStatusDTO> getStatus(ArtifactJsonTransformationDTO jsonTransformationDTO);

Mono<String> prepareForNewRefCreation(ArtifactJsonTransformationDTO artifactJsonTransformationDTO);
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package com.appsmith.server.git.dtos;

import com.appsmith.external.git.constants.ce.RefType;
import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.constants.ce.RefType;
import lombok.Data;

// TODO: Find a better name for this DTO
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@
import com.appsmith.external.dtos.GitStatusDTO;
import com.appsmith.external.git.constants.GitConstants;
import com.appsmith.external.git.constants.GitSpan;
import com.appsmith.external.git.constants.ce.RefType;
import com.appsmith.external.git.handler.FSGitHandler;
import com.appsmith.git.dto.CommitDTO;
import com.appsmith.server.acl.AclPermission;
import com.appsmith.server.configurations.EmailConfig;
import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.constants.ce.RefType;
import com.appsmith.server.datasources.base.DatasourceService;
import com.appsmith.server.domains.Artifact;
import com.appsmith.server.domains.GitArtifactMetadata;
Expand Down Expand Up @@ -263,27 +263,40 @@ public Mono<List<String>> listBranches(ArtifactJsonTransformationDTO artifactJso
GitArtifactHelper<?> gitArtifactHelper =
gitArtifactHelperResolver.getArtifactHelper(artifactJsonTransformationDTO.getArtifactType());

return listBranches(artifactJsonTransformationDTO, Boolean.FALSE);
}

@Override
public Mono<List<String>> listBranches(
ArtifactJsonTransformationDTO jsonTransformationDTO, Boolean listRemoteBranches) {
GitArtifactHelper<?> gitArtifactHelper =
gitArtifactHelperResolver.getArtifactHelper(jsonTransformationDTO.getArtifactType());

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

return fsGitHandler
.listBranches(repoSuffix)
.flatMapMany(Flux::fromIterable)
.filter(gitBranchDTO -> {
return StringUtils.hasText(gitBranchDTO.getBranchName())
&& !gitBranchDTO.getBranchName().startsWith("origin");
boolean branchToBeListed = !gitBranchDTO.getBranchName().startsWith("origin")
|| Boolean.TRUE.equals(listRemoteBranches);

return StringUtils.hasText(gitBranchDTO.getBranchName()) && branchToBeListed;
})
.map(GitBranchDTO::getBranchName)
.collectList();
}

@Override
public Mono<List<String>> listReferences(
ArtifactJsonTransformationDTO artifactJsonTransformationDTO, RefType refType) {
ArtifactJsonTransformationDTO artifactJsonTransformationDTO,
Boolean checkRemoteReferences,
RefType refType) {
if (RefType.BRANCH.equals(refType)) {
listBranches(artifactJsonTransformationDTO);
listBranches(artifactJsonTransformationDTO, checkRemoteReferences);
}

// TODO: include ref type for tags in fsGit Handler
Expand Down Expand Up @@ -592,7 +605,8 @@ private Mono<String> pushArtifactErrorRecovery(String pushResult, Artifact artif
* @return : returns string for remote fetch
*/
@Override
public Mono<String> fetchRemoteChanges(ArtifactJsonTransformationDTO jsonTransformationDTO, GitAuth gitAuth) {
public Mono<String> fetchRemoteChanges(
ArtifactJsonTransformationDTO jsonTransformationDTO, GitAuth gitAuth, Boolean isFetchAll) {

String workspaceId = jsonTransformationDTO.getWorkspaceId();
String baseArtifactId = jsonTransformationDTO.getBaseArtifactId();
Expand All @@ -607,7 +621,7 @@ public Mono<String> fetchRemoteChanges(ArtifactJsonTransformationDTO jsonTransfo
Mono<Boolean> checkoutBranchMono = fsGitHandler.checkoutToBranch(repoSuffix, refName);

Mono<String> fetchRemoteMono = fsGitHandler.fetchRemote(
repoPath, gitAuth.getPublicKey(), gitAuth.getPrivateKey(), true, refName, false);
repoPath, gitAuth.getPublicKey(), gitAuth.getPrivateKey(), true, refName, isFetchAll);

return checkoutBranchMono.then(Mono.defer(() -> fetchRemoteMono));
}
Expand Down Expand Up @@ -645,4 +659,17 @@ public Mono<GitStatusDTO> getStatus(ArtifactJsonTransformationDTO jsonTransforma
Path repoPath = fsGitHandler.createRepoPath(repoSuffix);
return fsGitHandler.getStatus(repoPath, refName);
}

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

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

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