Skip to content

chore: implementation of artifact type agnostic import #39237

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.

Sign up for GitHub

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

Merged
sondermanish merged 3 commits into from
Feb 13, 2025
Merged

chore: implementation of artifact type agnostic import #39237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51f200f
added changes for common git commit
sondermanish Feb 13, 2025
3972081
added changes
sondermanish Feb 13, 2025
bdb5b65
added more logs
sondermanish Feb 13, 2025
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
10 changes: 10 additions & 0 deletions app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1200,6 +1200,16 @@ public Mono<Object> reconstructMetadataFromGitRepo(
return metadataMono.subscribeOn(scheduler);
}

@Override
public Mono<Object> reconstructMetadataFromGitRepository(Path repoSuffix) {
Mono<Object> metadataMono = Mono.fromCallable(() -> {
Path baseRepoPath = Paths.get(gitServiceConfig.getGitRootPath()).resolve(repoSuffix);
return fileOperations.readFile(baseRepoPath.resolve(CommonConstants.METADATA + JSON_EXTENSION));
});

return metadataMono.subscribeOn(scheduler);
}
Comment on lines +1204 to +1211
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 13, 2025

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 file operations.

The method should handle file system errors explicitly.

Consider adding error handling:

 Mono<Object> metadataMono = Mono.fromCallable(() -> {
     Path baseRepoPath = Paths.get(gitServiceConfig.getGitRootPath()).resolve(repoSuffix);
-    return fileOperations.readFile(baseRepoPath.resolve(CommonConstants.METADATA + JSON_EXTENSION));
+    try {
+        return fileOperations.readFile(baseRepoPath.resolve(CommonConstants.METADATA + JSON_EXTENSION));
+    } catch (Exception e) {
+        log.error("Error reading metadata from repository: {}", e.getMessage());
+        throw new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, e.getMessage());
+    }
 });
📝 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 Mono<Object> reconstructMetadataFromGitRepository(Path repoSuffix) {
Mono<Object> metadataMono = Mono.fromCallable(() -> {
Path baseRepoPath = Paths.get(gitServiceConfig.getGitRootPath()).resolve(repoSuffix);
return fileOperations.readFile(baseRepoPath.resolve(CommonConstants.METADATA + JSON_EXTENSION));
});
return metadataMono.subscribeOn(scheduler);
}
public Mono<Object> reconstructMetadataFromGitRepository(Path repoSuffix) {
Mono<Object> metadataMono = Mono.fromCallable(() -> {
Path baseRepoPath = Paths.get(gitServiceConfig.getGitRootPath()).resolve(repoSuffix);
try {
return fileOperations.readFile(baseRepoPath.resolve(CommonConstants.METADATA + JSON_EXTENSION));
} catch (Exception e) {
log.error("Error reading metadata from repository: {}", e.getMessage());
throw new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, e.getMessage());
}
});
return metadataMono.subscribeOn(scheduler);
}


@Override
public Mono<Object> reconstructPageFromGitRepo(
String pageName, String branchName, Path baseRepoSuffixPath, Boolean resetToLastCommitRequired) {
Expand Down
2 changes: 2 additions & 0 deletions app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ Mono<Object> reconstructMetadataFromGitRepo(
Path repoSuffixPath,
Boolean isResetToLastCommitRequired);

Mono<Object> reconstructMetadataFromGitRepository(Path repoSuffix);

Mono<Object> reconstructPageFromGitRepo(
String pageName, String branchName, Path repoSuffixPath, Boolean checkoutRequired);

Expand Down
1 change: 1 addition & 0 deletions ...smith-interfaces/src/main/java/com/appsmith/external/git/constants/ce/GitConstantsCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ public class GitConstantsCE {
public static final String ACTION_COLLECTION_LIST = "actionCollectionList";

public static final String README_FILE_NAME = "README.md";
public static final String ARTIFACT_JSON_TYPE = "artifactJsonType";

public static final String FIRST_COMMIT = "System added readme file";
public static final String DEFAULT_COMMIT_MESSAGE = "System generated commit, ";
Expand Down
1 change: 1 addition & 0 deletions app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ public class FieldNameCE {
public static final String ACTIONS = "actions";
public static final String ASSET = "asset";
public static final String APPLICATION = "application";
public static final String ARTIFACT = "artifact";
public static final String SOURCE_APPLICATION = "sourceApplication";
public static final String COMMENT = "comment";
public static final String COMMENT_THREAD = "commentThread";
Expand Down
18 changes: 17 additions & 1 deletion app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Theme.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ public class Theme extends BaseDomain {

@JsonProperty("isSystemTheme") // manually setting property name to make sure it's compatible with Gson
@JsonView({Views.Public.class, Git.class})
private boolean isSystemTheme = false; // should be false by default
// The primitive is changed to wrapper because of serialization - deserialization issues
private Boolean isSystemTheme = false; // should be false by default

@Data
@AllArgsConstructor
Expand All @@ -59,6 +60,21 @@ public static class Colors {
private String backgroundColor;
}

public void setSystemTheme(boolean isSystemTheme) {
this.isSystemTheme = isSystemTheme;
}

@JsonView({Views.Internal.class})
Copy link
Contributor

@nidhi-nair nidhi-nair Feb 13, 2025

Choose a reason for hiding this comment

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

Why do we have 2 of these getters?

Copy link
Contributor Author

@sondermanish sondermanish Feb 13, 2025

Choose a reason for hiding this comment

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

the isSystemTheme() was getting used directly from the system due to primitive type and also getSystemTheme was implied, however due to change in type had to use both, I'll remove this in a separate pr

public Boolean getSystemTheme() {
return this.isSystemTheme;
}

// To be deleted in later on refactors
@JsonView({Views.Internal.class})
public Boolean isSystemTheme() {
return this.isSystemTheme;
}

@Override
public void sanitiseToExportDBObject() {
this.setId(null);
Expand Down
3 changes: 3 additions & 0 deletions ...er/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ public interface CentralGitServiceCE {
Mono<? extends ArtifactImportDTO> importArtifactFromGit(
String workspaceId, GitConnectDTO gitConnectDTO, ArtifactType artifactType, GitType gitType);

Mono<? extends ArtifactImportDTO> importArtifactFromGit(
String workspaceId, GitConnectDTO gitConnectDTO, GitType gitType);

Mono<? extends Artifact> connectArtifactToGit(
String baseArtifactId,
ArtifactType artifactType,
Expand Down
199 changes: 199 additions & 0 deletions ...ppsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.TimeoutException;

import static com.appsmith.external.git.constants.ce.GitConstantsCE.DEFAULT_COMMIT_MESSAGE;
Expand Down Expand Up @@ -143,6 +144,204 @@ protected Mono<Boolean> isRepositoryLimitReachedForWorkspace(String workspaceId,
return gitPrivateRepoHelper.isRepoLimitReached(workspaceId, true);
}

@Override
public Mono<? extends ArtifactImportDTO> importArtifactFromGit(
String workspaceId, GitConnectDTO gitConnectDTO, GitType gitType) {
if (!StringUtils.hasText(workspaceId)) {
return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.WORKSPACE_ID));
}

GitHandlingService gitHandlingService = gitHandlingServiceResolver.getGitHandlingService(gitType);
Set<String> errors = gitHandlingService.validateGitConnectDTO(gitConnectDTO);

if (!CollectionUtils.isEmpty(errors)) {
return Mono.error(new AppsmithException(
AppsmithError.INVALID_PARAMETER, errors.stream().findAny().get()));
}

final String repoName = gitHandlingService.getRepoName(gitConnectDTO);

// since at this point in the import flow, there is no context about the artifact type
// it needs to be retrieved from the fetched repository itself. however, in order to retrieve
// the artifact type from repository, the repository needs to be saved.
// for saving the repo an identifier is required (which usually is the artifact id);
// however, the artifact could only be generated after the artifact type is known.
// hence this is a temporary placeholder to hold the repository and it's components
String placeholder = "temp" + UUID.randomUUID();
Copy link
Contributor

@nidhi-nair nidhi-nair Feb 13, 2025

Choose a reason for hiding this comment

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

Can you add a comment about what this placeholder is meant for?

ArtifactJsonTransformationDTO tempJsonTransformationDTO =
new ArtifactJsonTransformationDTO(workspaceId, placeholder, repoName);

ArtifactJsonTransformationDTO jsonDTOPostRefCreation =
new ArtifactJsonTransformationDTO(workspaceId, placeholder, repoName);

Mono<Boolean> isRepositoryPrivateMonoCached =
gitHandlingService.isRepoPrivate(gitConnectDTO).cache();

Mono<Boolean> isRepositoryLimitReachedForWorkspaceMono = isRepositoryPrivateMonoCached.flatMap(
isRepositoryPrivate -> isRepositoryLimitReachedForWorkspace(workspaceId, isRepositoryPrivate));

Mono<GitAuth> gitAuthMonoCached = gitHandlingService.getGitAuthForUser().cache();

Mono<? extends Artifact> blankArtifactForImportMono = isRepositoryLimitReachedForWorkspaceMono
.flatMap(isLimitReachedForPrivateRepositories -> {
if (!TRUE.equals(isLimitReachedForPrivateRepositories)) {
return gitAuthMonoCached;
}

return gitAnalyticsUtils
.addAnalyticsForGitOperation(
AnalyticsEvents.GIT_IMPORT,
workspaceId,
AppsmithError.GIT_APPLICATION_LIMIT_ERROR.getErrorType(),
AppsmithError.GIT_APPLICATION_LIMIT_ERROR.getMessage(),
true,
false)
.then(Mono.error(new AppsmithException(AppsmithError.GIT_APPLICATION_LIMIT_ERROR)));
})
.flatMap(gitAuth -> {
return gitHandlingService
.fetchRemoteRepository(gitConnectDTO, gitAuth, tempJsonTransformationDTO)
.flatMap(defaultBranch -> {
return Mono.zip(
Mono.just(defaultBranch),
gitHandlingService.obtainArtifactTypeFromGitRepository(
tempJsonTransformationDTO),
isRepositoryPrivateMonoCached,
Mono.just(gitAuth));
});
})
.flatMap(tuple4 -> {
String defaultBranch = tuple4.getT1();
ArtifactType artifactType = tuple4.getT2();
Boolean isRepoPrivate = tuple4.getT3();
GitAuth gitAuth = tuple4.getT4();

GitArtifactHelper<?> contextHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType);
AclPermission workspacePermission = contextHelper.getWorkspaceArtifactCreationPermission();

Mono<Workspace> workspaceMono = workspaceService
.findById(workspaceId, workspacePermission)
.switchIfEmpty(Mono.error(new AppsmithException(
AppsmithError.NO_RESOURCE_FOUND, FieldName.WORKSPACE, workspaceId)));

return workspaceMono
.flatMap(workspace -> contextHelper.createArtifactForImport(workspaceId, repoName))
.map(baseArtifact -> {
GitArtifactMetadata gitArtifactMetadata = new GitArtifactMetadata();
gitArtifactMetadata.setGitAuth(gitAuth);
gitArtifactMetadata.setDefaultArtifactId(baseArtifact.getId());
gitArtifactMetadata.setDefaultBranchName(defaultBranch);
gitArtifactMetadata.setRefName(defaultBranch);
gitArtifactMetadata.setRepoName(repoName);
gitArtifactMetadata.setIsRepoPrivate(isRepoPrivate);
gitArtifactMetadata.setLastCommittedAt(Instant.now());

gitHandlingService.setRepositoryDetailsInGitArtifactMetadata(
gitConnectDTO, gitArtifactMetadata);
baseArtifact.setGitArtifactMetadata(gitArtifactMetadata);
return baseArtifact;
});
});

Mono<? extends Artifact> containerArtifactForImport = Mono.usingWhen(
blankArtifactForImportMono,
baseArtifact -> {
GitArtifactMetadata baseGitMetadata = baseArtifact.getGitArtifactMetadata();
String defaultBranch = baseGitMetadata.getDefaultBranchName();

jsonDTOPostRefCreation.setBaseArtifactId(baseGitMetadata.getDefaultArtifactId());
jsonDTOPostRefCreation.setRefType(RefType.branch);
jsonDTOPostRefCreation.setRefName(defaultBranch);
jsonDTOPostRefCreation.setArtifactType(baseArtifact.getArtifactType());

return Mono.just(baseArtifact);
},
// in any case repository details needs to be updated with the base artifact id
baseArtifact ->
gitHandlingService.updateImportedRepositoryDetails(baseArtifact, tempJsonTransformationDTO));

Mono<? extends ArtifactImportDTO> importGitArtifactMono = Mono.usingWhen(
containerArtifactForImport,
baseArtifact -> {
GitArtifactMetadata baseGitMetadata = baseArtifact.getGitArtifactMetadata();
String defaultBranch = baseGitMetadata.getDefaultBranchName();
GitArtifactHelper<?> gitArtifactHelper =
gitArtifactHelperResolver.getArtifactHelper(baseArtifact.getArtifactType());

Mono<List<Datasource>> datasourceMono = datasourceService
.getAllByWorkspaceIdWithStorages(workspaceId, datasourcePermission.getEditPermission())
.collectList();

Mono<List<Plugin>> pluginMono =
pluginService.getDefaultPlugins().collectList();

Mono<? extends ArtifactExchangeJson> artifactExchangeJsonMono = gitHandlingService
.reconstructArtifactJsonFromGitRepository(jsonDTOPostRefCreation)
.onErrorResume(error -> {
log.error("Error while constructing artifact from git repo", error);
return Mono.error(
new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, error.getMessage()));
});

return Mono.zip(artifactExchangeJsonMono, datasourceMono, pluginMono)
.flatMap(data -> {
ArtifactExchangeJson artifactExchangeJson = data.getT1();
List<Datasource> datasourceList = data.getT2();
List<Plugin> pluginList = data.getT3();

if (artifactExchangeJson.getArtifact() == null
|| gitArtifactHelper.isContextInArtifactEmpty(artifactExchangeJson)) {
return Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED,
"import",
"Cannot import artifact from an empty repo"));
}

// If there is an existing datasource with the same name but a different type from that
// in the repo, the import api should fail
// TODO: change the implementation to compare datasource with gitSyncIds instead.
if (checkIsDatasourceNameConflict(
Comment on lines +302 to +303
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 13, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Implement TODO for datasource comparison.

The TODO comment indicates a need to compare datasources using gitSyncIds instead of names for better robustness.

Let's check for existing gitSyncId implementations:

🏁 Script executed:

#!/bin/bash
# Search for gitSyncId usage in datasource comparisons
rg -A 5 "gitSyncId.*datasource"

Length of output: 87921

Implement gitSyncId comparison for datasources

The current implementation in CentralGitServiceCEImpl.java (lines 302–303) still relies on comparing datasource names via the checkIsDatasourceNameConflict() method. In contrast, our JSON models and other parts of the codebase consistently use gitSyncId to uniquely identify datasources. For better robustness and consistency, please update the implementation to compare datasources using their gitSyncId values rather than their names.

datasourceList, artifactExchangeJson.getDatasourceList(), pluginList)) {
return Mono.error(new AppsmithException(
AppsmithError.GIT_ACTION_FAILED,
"import",
"Datasource already exists with the same name"));
}

artifactExchangeJson.getArtifact().setGitArtifactMetadata(baseGitMetadata);
return importService
.importArtifactInWorkspaceFromGit(
workspaceId, baseArtifact.getId(), artifactExchangeJson, defaultBranch)
.onErrorResume(throwable -> {
log.error("Error in importing the artifact {}", baseArtifact.getId());
return Mono.error(new AppsmithException(
AppsmithError.GIT_FILE_SYSTEM_ERROR, throwable.getMessage()));
});
})
.flatMap(artifact -> gitArtifactHelper.publishArtifact(artifact, false))
.flatMap(artifact -> importService.getArtifactImportDTO(
artifact.getWorkspaceId(), artifact.getId(), artifact, artifact.getArtifactType()));
},
baseArtifact -> {
// on success send analytics
return gitAnalyticsUtils.addAnalyticsForGitOperation(
AnalyticsEvents.GIT_IMPORT,
baseArtifact,
baseArtifact.getGitArtifactMetadata().getIsRepoPrivate());
},
(baseArtifact, throwableError) -> {
// on error
return deleteArtifactCreatedFromGitImport(jsonDTOPostRefCreation, gitType);
},
baseArtifact -> {
// on publisher cancellation
return deleteArtifactCreatedFromGitImport(jsonDTOPostRefCreation, gitType);
});

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

@Override
public Mono<? extends ArtifactImportDTO> importArtifactFromGit(
String workspaceId, GitConnectDTO gitConnectDTO, ArtifactType artifactType, GitType gitType) {
Expand Down
11 changes: 11 additions & 0 deletions ...r/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.appsmith.external.dtos.MergeStatusDTO;
import com.appsmith.external.git.dtos.FetchRemoteDTO;
import com.appsmith.git.dto.CommitDTO;
import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.domains.Artifact;
import com.appsmith.server.domains.GitArtifactMetadata;
import com.appsmith.server.domains.GitAuth;
Expand All @@ -15,6 +16,7 @@
import reactor.core.publisher.Mono;
import reactor.util.function.Tuple2;

import java.nio.file.Path;
import java.util.List;
import java.util.Set;

Expand All @@ -33,6 +35,15 @@ public interface GitHandlingServiceCE {

Boolean isGitAuthInvalid(GitAuth gitAuth);

// TODO: use only the required params
Mono<Path> updateImportedRepositoryDetails(
Artifact baseArtifact, ArtifactJsonTransformationDTO jsonTransformationDTO);

Mono<ArtifactType> obtainArtifactTypeFromGitRepository(ArtifactJsonTransformationDTO jsonTransformationDTO);

Mono<String> fetchRemoteRepository(
GitConnectDTO gitConnectDTO, GitAuth gitAuth, ArtifactJsonTransformationDTO jsonTransformationDTO);

Mono<String> fetchRemoteRepository(
GitConnectDTO gitConnectDTO, GitAuth gitAuth, Artifact artifact, String repoName);

Expand Down
3 changes: 1 addition & 2 deletions ...ith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.appsmith.server.git.controllers;

import com.appsmith.external.views.Views;
import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.constants.Url;
import com.appsmith.server.domains.GitAuth;
import com.appsmith.server.domains.GitProfile;
Expand Down Expand Up @@ -88,7 +87,7 @@ public Mono<ResponseDTO<? extends ArtifactImportDTO>> importArtifactFromGit(

// TODO: remove artifact type from methods.
return centralGitService
.importArtifactFromGit(workspaceId, gitConnectDTO, ArtifactType.APPLICATION, GIT_TYPE)
.importArtifactFromGit(workspaceId, gitConnectDTO, GIT_TYPE)
.map(result -> new ResponseDTO<>(HttpStatus.CREATED.value(), result, null));
Comment on lines 88 to 91
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 13, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

LGTM! Removal of artifact type parameter aligns with type-agnostic design.

The change simplifies the import process by removing the explicit artifact type parameter, making the implementation more flexible.

Let's verify that all callers have been updated to handle the new signature:

🏁 Script executed:

#!/bin/bash
# Search for any remaining calls with ArtifactType parameter
rg -A 2 "importArtifactFromGit.*ArtifactType"

Length of output: 5210

Action: Update Caller Invocations for Artifact Type Removal

The changes in GitArtifactControllerCE.java look good and correctly remove the artifact type parameter. However, the search results reveal that several callers still invoke importArtifactFromGit with an artifact type (e.g., ArtifactType.APPLICATION). Please update these callers to use the new signature. Specific locations requiring changes include:

  • app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/GitControllerCE.java – Update the call to remove the artifact type argument.
  • app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java – Refactor all test cases to call the updated version of importArtifactFromGit without the artifact type parameter.

Once these modifications are in place, the implementation will consistently reflect the type-agnostic design.

}

Expand Down
Loading