chore: implementation of artifact type agnostic import #39237
Conversation
sondermanish
requested review from
nidhi-nair
and removed request for
a team
WalkthroughThis pull request introduces new functionality and refinements across multiple Git-related modules. It adds methods for reconstructing metadata, importing artifacts, and fetching repository details while updating method signatures and constant definitions. Additional modifications include domain property type changes, enhanced error handling in analytics, and improved query conditions. The changes span interfaces, implementations, controllers, and utility classes to support reactive programming. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as GitArtifactControllerCE
participant Service as CentralGitServiceCEImpl
participant Handler as GitHandlingServiceCE / GitFSServiceCEImpl
Client->>Controller: importArtifactFromGit(workspaceId, gitConnectDTO, gitType)
Controller->>Service: importArtifactFromGit(...)
Service->>Handler: fetchRemoteRepository(gitConnectDTO, gitAuth, dto)
Handler-->>Service: Return remote repo details
Service->>Handler: obtainArtifactTypeFromGitRepository(dto)
Handler-->>Service: Return artifact type
Service->>Handler: updateImportedRepositoryDetails(baseArtifact, dto)
Handler-->>Service: Return updated repository path
Service-->>Controller: ArtifactImportDTO
Controller-->>Client: ResponseDTO<ArtifactImportDTO>
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
168-178: Use more descriptive variable name for placeholder.
While naming a local variableplaceholderis acceptable, consider describing its usage or context for better readability.
179-238: Refactor to reduce nesting.
This chunk of logic for remote repository fetching and artifact creation can be extracted to a separate private method to keep things more readable and testable.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java (1)
25-46: Constructor injection is appropriate.
IncludingGitServiceConfigin the constructor matches the usage required in the superclass. Ensure that null checks or validations are performed if needed.app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Theme.java (1)
66-74: Ensure consistent naming for getters.
Using bothgetSystemTheme()andisSystemTheme()may cause confusion. If both are necessary, confirm they don't cause serialization naming collisions.app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (1)
27-28: Consider adding Javadoc for the new method.The new overloaded method lacks documentation explaining how it differs from the existing one, particularly regarding how the artifact type is determined.
Add documentation like:
+/** + * Imports an artifact from Git without requiring explicit artifact type specification. + * The artifact type is determined automatically from the repository metadata. + * + * @param workspaceId The workspace ID where the artifact will be imported + * @param gitConnectDTO The Git connection details + * @param gitType The type of Git service being used + * @return A Mono containing the imported artifact details + */app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java (1)
74-74: Add Javadoc for the new method.The method lacks documentation explaining its purpose and parameters.
Add documentation like:
+/** + * Reconstructs metadata from a Git repository at the specified path. + * + * @param repoSuffix The path to the Git repository + * @return A Mono containing the reconstructed metadata object + */app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (1)
44-46: Consider consolidating fetchRemoteRepository methods.There are now two methods for fetching remote repositories with slightly different parameters.
Consider:
- Making one method private and having it called by the public method
- Or documenting the specific use case for each method
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (1)
167-202: LGTM! Consider adding timeout configuration for clone operation.The error handling and cleanup are well implemented. The method appropriately maps Git errors to domain exceptions and tracks analytics.
Consider adding a configurable timeout for the clone operation to prevent long-running operations from blocking resources:
return fsGitHandler .cloneRemoteIntoArtifactRepo( - temporaryStorage, gitConnectDTO.getRemoteUrl(), gitAuth.getPrivateKey(), gitAuth.getPublicKey()) + temporaryStorage, gitConnectDTO.getRemoteUrl(), gitAuth.getPrivateKey(), gitAuth.getPublicKey()) + .timeout(Duration.ofSeconds(gitConfig.getCloneTimeoutSeconds()))app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (1)
804-829: Consider simplifying error handling using pattern matching.The error handling flow could be more concise.
Consider this more streamlined approach:
- if (metadataJsonObject == null) { - return Mono.error(new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR)); - } - - JsonElement artifactJsonType = metadataJsonObject.get(ARTIFACT_JSON_TYPE); - - if (artifactJsonType == null) { - return Mono.error(new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR)); - } - - return Mono.just(artifactJsonType.getAsString()); + return Optional.ofNullable(metadataJsonObject) + .map(obj -> obj.get(ARTIFACT_JSON_TYPE)) + .map(JsonElement::getAsString) + .map(Mono::just) + .orElseGet(() -> Mono.error(new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR)));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/ce/GitConstantsCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Theme.java(2 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)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java(8 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomApplicationRepositoryCEImpl.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (23)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (4)
88-88: Import statement organization looks fine.
No concerns with the newUUIDimport.
147-166: Validate workspace ID before processing.
Consider providing a more descriptive error message or logging the workspace ID for easier debugging.
240-256: Consider robust error handling for resource updates.
When updating imported repository details, ensure exceptions fromupdateImportedRepositoryDetails(...)are properly caught or tracked to prevent partial failures.
257-334: Verify concurrency during Mono.usingWhen flow.
Reactive sequences are chained here. Confirm that there are no race conditions on shared resources (like external services) or potential memory leaks if subscriptions are canceled abruptly.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java (1)
5-5: New GitServiceConfig import is consistent.
The addition ofGitServiceConfigappears correctly aligned with other imports.app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Theme.java (2)
52-52: Switched to wrapper Boolean.
This allows null states forisSystemTheme. Verify if all downstream usage can handle null values gracefully.
62-64: Setter method sufficiency.
This setter is straightforward. Ensure that there's no conflict when calling it from different places in the codebase (e.g., test scenarios, migrations).app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/constants/ce/GitConstantsCE.java (1)
13-13: LGTM! New constant for artifact JSON type.The constant follows the naming convention and is appropriately placed in the constants class.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/GitHandlingServiceCE.java (2)
38-41: Address TODO comment and document method parameters.The TODO comment suggests the method might be accepting more parameters than necessary.
Consider:
- Reviewing which parameters are actually needed
- Removing unused parameters
- Adding proper documentation
42-42: LGTM! Clear method signature for artifact type detection.The method name clearly indicates its purpose of determining the artifact type from a Git repository.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java (6)
75-77: Improved null safety in branch name retrieval.The null check prevents potential NullPointerException when accessing GitArtifactMetadata.
82-113: LGTM! New analytics method follows established patterns.The new method provides a focused way to track Git operations with proper error handling.
126-129: Improved null safety in Git data handling.The null check ensures safe access to GitArtifactMetadata before using it.
75-77: LGTM! Improved null safety with explicit checks.The null checks for artifact and its metadata prevent potential NullPointerException.
82-113: LGTM! New analytics method provides flexibility.The new method allows tracking Git operations without requiring an artifact context, which is useful for system-level operations.
126-129: LGTM! Improved null safety in analytics data collection.The null check before accessing GitArtifactMetadata prevents potential issues.
app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java (2)
74-74: LGTM! New constant follows naming conventions.The
ARTIFACTconstant aligns with the artifact type agnostic implementation.
74-74: LGTM! New constant follows naming convention.The ARTIFACT constant aligns with the artifact type agnostic approach.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomApplicationRepositoryCEImpl.java (2)
164-166: LGTM! Enhanced query to support both application and artifact IDs.The OR condition allows flexible querying using either ID type, improving compatibility with the artifact type agnostic approach.
164-166: LGTM! Enhanced query to support both ID types.The OR condition allows querying by either application ID or artifact ID, providing better flexibility.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/fs/GitFSServiceCEImpl.java (2)
204-211: LGTM! Clean and focused implementation.The method has a single responsibility and properly delegates to the utility class.
214-226: LGTM! Well-structured repository path handling.The method properly handles repository path construction and movement using appropriate helpers.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.java (1)
88-91: LGTM! The change aligns with making the import feature artifact type agnostic.The removal of the artifact type parameter simplifies the API and makes it more flexible.
| // 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)); |
There was a problem hiding this comment.
💡 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
importArtifactFromGitwithout the artifact type parameter.
Once these modifications are in place, the implementation will consistently reflect the type-agnostic design.
| public Mono<Path> moveArtifactFromTemporaryToBaseArtifactIdRepository( | ||
| Path currentRepositoryPath, Path newRepositoryPath) { | ||
| Path currentGitPath = Path.of(gitServiceConfig.getGitRootPath()).resolve(currentRepositoryPath); | ||
| Path targetPath = Path.of(gitServiceConfig.getGitRootPath()).resolve(newRepositoryPath); | ||
|
|
||
| return Mono.fromCallable(() -> { | ||
| if (!Files.exists(targetPath)) { | ||
| Files.createDirectories(targetPath); | ||
| } | ||
|
|
||
| return Files.move(currentGitPath, targetPath, REPLACE_EXISTING); | ||
| }) | ||
| .subscribeOn(Schedulers.boundedElastic()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file operations.
The method should handle potential file system errors explicitly.
Consider adding explicit error handling:
return Mono.fromCallable(() -> {
+ try {
if (!Files.exists(targetPath)) {
Files.createDirectories(targetPath);
}
return Files.move(currentGitPath, targetPath, REPLACE_EXISTING);
+ } catch (IOException e) {
+ log.error("Error moving artifact from {} to {}: {}", currentGitPath, targetPath, e.getMessage());
+ throw new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, e.getMessage());
+ }
})
.subscribeOn(Schedulers.boundedElastic());📝 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 Mono<Path> moveArtifactFromTemporaryToBaseArtifactIdRepository( | |
| Path currentRepositoryPath, Path newRepositoryPath) { | |
| Path currentGitPath = Path.of(gitServiceConfig.getGitRootPath()).resolve(currentRepositoryPath); | |
| Path targetPath = Path.of(gitServiceConfig.getGitRootPath()).resolve(newRepositoryPath); | |
| return Mono.fromCallable(() -> { | |
| if (!Files.exists(targetPath)) { | |
| Files.createDirectories(targetPath); | |
| } | |
| return Files.move(currentGitPath, targetPath, REPLACE_EXISTING); | |
| }) | |
| .subscribeOn(Schedulers.boundedElastic()); | |
| } | |
| public Mono<Path> moveArtifactFromTemporaryToBaseArtifactIdRepository( | |
| Path currentRepositoryPath, Path newRepositoryPath) { | |
| Path currentGitPath = Path.of(gitServiceConfig.getGitRootPath()).resolve(currentRepositoryPath); | |
| Path targetPath = Path.of(gitServiceConfig.getGitRootPath()).resolve(newRepositoryPath); | |
| return Mono.fromCallable(() -> { | |
| try { | |
| if (!Files.exists(targetPath)) { | |
| Files.createDirectories(targetPath); | |
| } | |
| return Files.move(currentGitPath, targetPath, REPLACE_EXISTING); | |
| } catch (IOException e) { | |
| log.error("Error moving artifact from {} to {}: {}", currentGitPath, targetPath, e.getMessage()); | |
| throw new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, e.getMessage()); | |
| } | |
| }) | |
| .subscribeOn(Schedulers.boundedElastic()); | |
| } |
| 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); | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| 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); | |
| } |
| this.isSystemTheme = isSystemTheme; | ||
| } | ||
|
|
||
| @JsonView({Views.Internal.class}) |
There was a problem hiding this comment.
Why do we have 2 of these getters?
There was a problem hiding this comment.
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
|
|
||
| final String repoName = gitHandlingService.getRepoName(gitConnectDTO); | ||
|
|
||
| String placeholder = "temp" + UUID.randomUUID(); |
There was a problem hiding this comment.
Can you add a comment about what this placeholder is meant for?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (4)
291-298: Consider extracting validation logic into a separate method.The validation logic for checking empty artifacts could be moved to a helper method to improve readability and reusability.
-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 (isArtifactEmpty(artifactExchangeJson, gitArtifactHelper)) { + return Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, + "import", + "Cannot import artifact from an empty repo")); +} +private boolean isArtifactEmpty(ArtifactExchangeJson artifactExchangeJson, GitArtifactHelper<?> gitArtifactHelper) { + return artifactExchangeJson.getArtifact() == null + || gitArtifactHelper.isContextInArtifactEmpty(artifactExchangeJson); +}
315-316: Consider providing more specific error messages.The error handling could be more specific about what failed during the import process rather than just passing through the message.
-.onErrorResume(throwable -> Mono.error(new AppsmithException( - AppsmithError.GIT_FILE_SYSTEM_ERROR, throwable.getMessage()))) +.onErrorResume(throwable -> { + log.error("Failed to import artifact in workspace from git", throwable); + String errorMessage = "Failed to import: " + (throwable.getMessage() != null ? throwable.getMessage() : "Unknown error"); + return Mono.error(new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, errorMessage)); +})
329-336: Consider consolidating error handling for publisher cancellation and throwable error.The error handling logic for both cases is identical. Consider consolidating them into a single method to avoid code duplication.
+private Mono<? extends Artifact> handleImportError(ArtifactJsonTransformationDTO jsonTransformationDTO, GitType gitType) { + return deleteArtifactCreatedFromGitImport(jsonTransformationDTO, gitType); +} -(baseArtifact, throwableError) -> { - // on error - return deleteArtifactCreatedFromGitImport(jsonTransformationDTO, gitType); -}, -baseArtifact -> { - // on publisher cancellation - return deleteArtifactCreatedFromGitImport(jsonTransformationDTO, gitType); -} +(baseArtifact, throwableError) -> handleImportError(jsonTransformationDTO, gitType), +baseArtifact -> handleImportError(jsonTransformationDTO, gitType)
338-340: Consider using simpler subscription pattern.The current subscription pattern could be simplified by using
asoperator instead of creating a new Mono.-return Mono.create( - sink -> importGitArtifactMono.subscribe(sink::success, sink::error, null, sink.currentContext())); +return importGitArtifactMono.as(mono -> Mono.create(sink -> + mono.subscribe(sink::success, sink::error, null, sink.currentContext())));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
170-170: Add a descriptive comment for the placeholder variable.A past review comment already requested this. The placeholder variable needs a clear comment explaining its purpose and why it's needed for the temporary storage of repository components.
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Theme.java
Show resolved
Hide resolved
...r/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java
Show resolved
Hide resolved
...erver/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java
Show resolved
Hide resolved
...erver/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java
Show resolved
Hide resolved
| FieldName.WORKSPACE_ID, | ||
| defaultIfNull(workspaceId, ""), | ||
| FieldName.ARTIFACT_ID, | ||
| defaultIfNull("", ""), |
There was a problem hiding this comment.
it's not possible in this flow as this flow is explicitly made for flows which doesn't have context of artifact type
...psmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.java
Show resolved
Hide resolved
| .importArtifactInWorkspaceFromGit( | ||
| workspaceId, baseArtifact.getId(), artifactExchangeJson, defaultBranch) | ||
| .onErrorResume(throwable -> Mono.error(new AppsmithException( | ||
| AppsmithError.GIT_FILE_SYSTEM_ERROR, throwable.getMessage()))); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (3)
789-802:⚠️ Potential issueAdd error handling for file operations.
The method should handle potential file system errors explicitly.
Apply this diff to add error handling:
return Mono.fromCallable(() -> { + try { if (!Files.exists(targetPath)) { Files.createDirectories(targetPath); } return Files.move(currentGitPath, targetPath, REPLACE_EXISTING); + } catch (IOException e) { + log.error("Error moving artifact from {} to {}: {}", currentGitPath, targetPath, e.getMessage(), e); + throw new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, e.getMessage()); + } }) .subscribeOn(Schedulers.boundedElastic());
789-802: 🛠️ Refactor suggestionAdd error handling for file operations.
The method should handle potential file system errors explicitly.
Apply this diff to add error handling:
return Mono.fromCallable(() -> { + try { if (!Files.exists(targetPath)) { Files.createDirectories(targetPath); } return Files.move(currentGitPath, targetPath, REPLACE_EXISTING); + } catch (IOException e) { + log.error("Error moving artifact from {} to {}: {}", currentGitPath, targetPath, e.getMessage(), e); + throw new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, e.getMessage()); + } }) .subscribeOn(Schedulers.boundedElastic());
789-802:⚠️ Potential issueAdd error handling for file operations.
The method should handle potential file system errors explicitly.
return Mono.fromCallable(() -> { + try { if (!Files.exists(targetPath)) { Files.createDirectories(targetPath); } return Files.move(currentGitPath, targetPath, REPLACE_EXISTING); + } catch (IOException e) { + log.error("Error moving artifact from {} to {}: {}", currentGitPath, targetPath, e.getMessage()); + throw new AppsmithException(AppsmithError.GIT_FILE_SYSTEM_ERROR, e.getMessage()); + } }) .subscribeOn(Schedulers.boundedElastic());
🧹 Nitpick comments (8)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (4)
804-834: Simplify the Mono creation pattern.The
Mono.createwrapper is unnecessary sinceartifactTypeMonois already a Mono.Apply this diff to simplify:
- return Mono.create(sink -> artifactTypeMono.subscribe(sink::success, sink::error, null, sink.currentContext())); + return artifactTypeMono;
812-815: Enhance error logging.Include more context in the error log to aid troubleshooting.
Apply this diff:
- log.error( - "Error in retrieving the metadata from the file system for repository {}", repoSuffix); + log.error( + "Error in retrieving the metadata from the file system. Repository: {}, Metadata: {}", + repoSuffix, metadataJsonObject);
804-834: Enhance error logging for better troubleshooting.The error messages could be more descriptive to aid in troubleshooting.
Apply this diff to improve error messages:
- log.error( - "Error in retrieving the metadata from the file system for repository {}", repoSuffix); + log.error( + "Failed to retrieve metadata from repository {}. Metadata JSON object is null.", + repoSuffix); - log.error( - "artifactJsonType attribute not found in the metadata file for repository {}", - repoSuffix); + log.error( + "Missing required 'artifactJsonType' attribute in metadata file for repository {}. " + + "Available attributes: {}", + repoSuffix, + metadataJsonObject.keySet());
804-834: Simplify Mono creation and error handling.The current implementation uses unnecessary
Mono.create. The code can be simplified by removing it and directly returning theartifactTypeMono.- return Mono.create(sink -> artifactTypeMono.subscribe(sink::success, sink::error, null, sink.currentContext())); + return artifactTypeMono;app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (3)
162-173: Consider adding documentation for the placeholder usage.The placeholder generation logic needs documentation to explain its purpose and lifecycle.
- String placeholder = "temp" + UUID.randomUUID(); + // Temporary placeholder ID used to hold repository components until artifact type is determined + String placeholder = "temp" + UUID.randomUUID();
183-184: Consider adding error handling for Git authentication.The Git authentication retrieval should include specific error handling for authentication failures.
- Mono<GitAuth> gitAuthMonoCached = gitHandlingService.getGitAuthForUser().cache(); + Mono<GitAuth> gitAuthMonoCached = gitHandlingService.getGitAuthForUser() + .onErrorResume(error -> Mono.error(new AppsmithException( + AppsmithError.GIT_ACTION_FAILED, + "authentication", + "Failed to retrieve Git authentication: " + error.getMessage()))) + .cache();
334-339: Consider adding retry logic for cleanup operations.The cleanup operation in case of errors should include retry logic to ensure resources are properly cleaned up.
- return deleteArtifactCreatedFromGitImport(jsonTransformationDTO, gitType); + return Mono.defer(() -> deleteArtifactCreatedFromGitImport(jsonTransformationDTO, gitType)) + .retryWhen(Retry.backoff(3, Duration.ofSeconds(1)) + .filter(throwable -> throwable instanceof TransientException));app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java (1)
82-113: Consider using null instead of empty string as default.The implementation looks good, but using an empty string as default for
artifactIdmight mask potential issues. Consider usingnullto make missing values more explicit.analyticsProps.putAll(Map.of( FieldName.WORKSPACE_ID, defaultIfNull(workspaceId, ""), FieldName.ARTIFACT_ID, - defaultIfNull("", ""), + null, "isRepoPrivate", defaultIfNull(isRepoPrivate, ""), "isSystemGenerated", defaultIfNull(isSystemGenerated, "")));
📜 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/domains/Theme.java(2 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/utils/GitAnalyticsUtils.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/domains/Theme.java
👮 Files not reviewed due to content moderation or server errors (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (11)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java (9)
75-77: LGTM! Good defensive programming.The null checks for
artifactandgitArtifactMetadataprevent potentialNullPointerException.
82-113: LGTM! Well-structured analytics method.The new method follows consistent patterns with proper error handling and safe defaults.
129-129: LGTM! Added artifact type to analytics.Addresses previous feedback about including ARTIFACT_TYPE in analytics props.
151-151: LGTM! More accurate field naming.Renamed from
branchApplicationIdtobranchedArtifactIdfor better clarity.
75-77: LGTM! Good defensive programming.The null checks for
artifactandgitArtifactMetadataprevent potentialNullPointerException.
82-113: LGTM! Well-structured analytics method.The new method follows existing patterns and properly handles error cases in analytics props.
129-129: LGTM! Added artifact type to analytics.Addresses the previous review comment about including ARTIFACT_TYPE in analytics props.
151-151: LGTM! Better field naming.Renamed from
branchApplicationIdtobranchedArtifactIdto better reflect the artifact-agnostic approach.
125-130: LGTM! Good defensive programming.The null checks and addition of artifactType to analytics props improve the robustness of the code.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
177-182: Cache repository privacy check result.Good use of caching the repository privacy check result. This helps avoid redundant API calls.
185-244: LGTM! Well-structured error handling and artifact creation.The implementation properly handles repository limit checks, fetches remote repositories, and creates artifacts with appropriate error handling.
| // TODO: change the implementation to compare datasource with gitSyncIds instead. | ||
| if (checkIsDatasourceNameConflict( |
There was a problem hiding this comment.
💡 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.
Description
Fixes #
Issue NumberAutomation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13305383592
Commit: bdb5b65
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 13 Feb 2025 11:38:58 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor