Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 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
@@ -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
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

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
Expand Up @@ -8,6 +8,7 @@
/**
* DTO to convey the status local git repo
*/
// TODO: @Manish modify git status DTO accordingly
@Data
public class GitStatusCE_DTO {

Expand Down
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
}
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ public class FieldNameCE {
public static final String IS_REQUIRED = "isRequired";
public static final String UNUSED_DATASOURCE = "UNUSED_DATASOURCE";
public static final String BRANCH_NAME = "branchName";
public static final String REF_NAME = "refName";
public static final String SOURCE_BRANCH = "sourceBranch";
public static final String DESTINATION_BRANCH = "destinationBranch";
public static final String DEFAULT = "default";
Expand Down

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,8 +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 Expand Up @@ -33,4 +34,15 @@ Mono<String> fetchRemoteChanges(
RefType refType);

Mono<? extends Artifact> discardChanges(String branchedArtifactId, ArtifactType artifactType, GitType gitType);

Mono<GitStatusDTO> getStatus(
String branchedArtifactId, boolean compareRemote, ArtifactType artifactType, GitType gitType);

Mono<? extends Artifact> checkoutReference(

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.

Nit, should we not be able to make a DTO out of these params?

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.

Yes, we can!

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.

Will you be replacing usage in a separate PR?

String referenceArtifactId,
String referenceToBeCheckedOut,
boolean addFileLock,
ArtifactType artifactType,
GitType gitType,
RefType refType);
}

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
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.domains.Artifact;
import com.appsmith.server.domains.GitArtifactMetadata;
Expand Down Expand Up @@ -39,8 +41,16 @@ 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,
Boolean checkRemoteReferences,
RefType refType);

Mono<Boolean> validateEmptyRepository(ArtifactJsonTransformationDTO artifactJsonTransformationDTO);

Mono<Boolean> initialiseReadMe(
Expand All @@ -51,14 +61,20 @@ Mono<Boolean> initialiseReadMe(

Mono<String> createFirstCommit(ArtifactJsonTransformationDTO jsonTransformationDTO, CommitDTO commitDTO);

// TODO: provide a proper name
Mono<Boolean> prepareChangesToBeCommitted(
ArtifactJsonTransformationDTO jsonTransformationDTO, ArtifactExchangeJson artifactExchangeJson);

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 @@ -2,8 +2,10 @@

import com.appsmith.external.constants.AnalyticsEvents;
import com.appsmith.external.dtos.GitBranchDTO;
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;
Expand Down Expand Up @@ -261,22 +263,46 @@ 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,
Boolean checkRemoteReferences,
RefType refType) {
if (RefType.BRANCH.equals(refType)) {
listBranches(artifactJsonTransformationDTO, checkRemoteReferences);
}

// TODO: include ref type for tags in fsGit Handler
return Mono.just(List.of());
}
Comment on lines +282 to +304

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.

⚠️ Potential issue

Fix the branch listing implementation

The method has two critical issues:

  1. The listBranches result is not being returned (missing return)
  2. The method always returns an empty list regardless of the branch listing result

Apply this fix:

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

    // TODO: include ref type for tags in fsGit Handler
    return Mono.just(List.of());
}
📝 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
@Override
public Mono<List<String>> listReferences(
ArtifactJsonTransformationDTO artifactJsonTransformationDTO, RefType refType) {
if (RefType.BRANCH.equals(refType)) {
listBranches(artifactJsonTransformationDTO);
}
// TODO: include ref type for tags in fsGit Handler
return Mono.just(List.of());
}
@Override
public Mono<List<String>> listReferences(
ArtifactJsonTransformationDTO artifactJsonTransformationDTO, RefType refType) {
if (RefType.BRANCH.equals(refType)) {
return listBranches(artifactJsonTransformationDTO);
}
// TODO: include ref type for tags in fsGit Handler
return Mono.just(List.of());
}


@Override
public Mono<Boolean> validateEmptyRepository(ArtifactJsonTransformationDTO artifactJsonTransformationDTO) {
GitArtifactHelper<?> gitArtifactHelper =
Expand Down Expand Up @@ -579,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 @@ -594,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 All @@ -617,4 +644,32 @@ public Mono<? extends ArtifactExchangeJson> recreateArtifactJsonFromLastCommit(
workspaceId, baseArtifactId, repoName, refName, artifactType);
});
}

@Override
public Mono<GitStatusDTO> getStatus(ArtifactJsonTransformationDTO jsonTransformationDTO) {
String workspaceId = jsonTransformationDTO.getWorkspaceId();
String baseArtifactId = jsonTransformationDTO.getBaseArtifactId();
String repoName = jsonTransformationDTO.getRepoName();
String refName = jsonTransformationDTO.getRefName();

ArtifactType artifactType = jsonTransformationDTO.getArtifactType();
GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(workspaceId, baseArtifactId, repoName);

Path repoPath = fsGitHandler.createRepoPath(repoSuffix);
return fsGitHandler.getStatus(repoPath, refName);
}
Comment on lines +634 to +661

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

Add error handling for Git operations

The method should handle potential Git exceptions that could occur during status retrieval.

Apply this improvement:

@Override
public Mono<GitStatusDTO> getStatus(ArtifactJsonTransformationDTO jsonTransformationDTO) {
    String workspaceId = jsonTransformationDTO.getWorkspaceId();
    String baseArtifactId = jsonTransformationDTO.getBaseArtifactId();
    String repoName = jsonTransformationDTO.getRepoName();
    String refName = jsonTransformationDTO.getRefName();

    ArtifactType artifactType = jsonTransformationDTO.getArtifactType();
    GitArtifactHelper<?> gitArtifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
    Path repoSuffix = gitArtifactHelper.getRepoSuffixPath(workspaceId, baseArtifactId, repoName);

    Path repoPath = fsGitHandler.createRepoPath(repoSuffix);
-    return fsGitHandler.getStatus(repoPath, refName);
+    return fsGitHandler.getStatus(repoPath, refName)
+            .onErrorResume(error -> {
+                log.error("Error getting Git status: {}", error.getMessage());
+                return Mono.error(new AppsmithException(
+                        AppsmithError.GIT_ACTION_FAILED,
+                        "status",
+                        error.getMessage()));
+            });
}

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


@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());
}
}