chore: Git resource map conversions #37920
Conversation
WalkthroughThe pull request introduces significant enhancements to the handling of Git resources across multiple classes in the Appsmith project. Key modifications include the addition of new methods for saving artifacts and retrieving existing files in Git repositories, updates to constructors to incorporate Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 7
🧹 Outside diff range and nitpick comments (14)
app/server/appsmith-git/src/main/java/com/appsmith/git/helpers/DSLTransformerHelper.java (3)
51-51: Consider improving readability of the conditional expression.The line combines multiple operations in a single statement. Consider breaking it down for better maintainability.
-String childPrefix = isCanvasWidget(childObject) ? prefix + widgetName + DELIMITER_POINT : prefix; +String childPrefix; +if (isCanvasWidget(childObject)) { + childPrefix = prefix + widgetName + DELIMITER_POINT; +} else { + childPrefix = prefix; +}
109-119: Add logging for invalid paths.The method silently ignores invalid paths. Consider adding debug logging to help with troubleshooting.
if (lastDirectoryIndex <= 0) { + log.debug("Ignoring invalid path: {}", path); // This is not a valid path anymore, ignore continue; }
239-248: Extract string manipulation logic into separate methods.The multiple string replacements and regex operations make the code harder to maintain. Consider extracting these operations into well-named helper methods.
+private static String cleanMainContainerPath(String path) { + return path.replace(MAIN_CONTAINER, EMPTY_STRING) + .replace(DELIMITER_POINT, DELIMITER_PATH); +} + +private static String removeCanvasWidget(String path) { + return path.replaceAll(CANVAS_WIDGET, EMPTY_STRING); +} + public static String getPathToWidgetFile(String key, JSONObject jsonObject, String widgetName) { - String childPath = key.replace(MAIN_CONTAINER, EMPTY_STRING).replace(DELIMITER_POINT, DELIMITER_PATH); - childPath = childPath.replaceAll(CANVAS_WIDGET, EMPTY_STRING); + String childPath = cleanMainContainerPath(key); + childPath = removeCanvasWidget(childPath);app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/templates/providers/ce/ExchangeJsonTestTemplateProviderCE.java (1)
136-173: Reduce Code Duplication inassertResourceComparisonsThere is repetitive code when asserting resource counts. Consider refactoring to eliminate duplication by iterating over resource types.
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (1)
Line range hint
161-173: Avoid Using 'New' in Method NamesRename
saveArtifactToLocalRepoNewto reflect its purpose more clearly, such assaveArtifactToLocalRepoWithGitResourceMap.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (1)
161-173: Rename Method for ClarityThe method name
saveArtifactToLocalRepoNewis unclear. Consider renaming it tosaveArtifactToLocalRepoUsingGitResourceMapfor better clarity.app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/models/GitResourceIdentity.java (1)
26-27: LGTM! Consider documenting constructor behavior.The addition of @nonnull filePath is appropriate for Git operations. Since this class uses @requiredargsconstructor, this field will be automatically added to the constructor parameters.
Consider adding a brief comment explaining the expected format/structure of filePath (e.g., relative to repo root, forward slashes, etc.).
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ArtifactGitFileUtilsCE.java (2)
16-17: Consider reordering methods for better cohesionGroup related methods together:
- Move
createArtifactExchangeJsonObject()next tocreateArtifactReferenceObject()- Place
setArtifactDependentPropertiesInJson()next tosetArtifactDependentResources()Also applies to: 30-30
29-29: Remove unnecessary empty lineThe empty line at line 29 creates unnecessary spacing between interface methods.
app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java (2)
104-106: Add error handling for JSON deserializationThe JSON parsing could fail silently. Consider wrapping it in a try-catch block with appropriate test failure messages.
- ArtifactExchangeJson artifactExchangeJson = - objectMapper.copy().disable(MapperFeature.USE_ANNOTATIONS).readValue(artifactJson, exchangeJsonType); + try { + return objectMapper.copy() + .disable(MapperFeature.USE_ANNOTATIONS) + .readValue(artifactJson, exchangeJsonType); + } catch (IOException e) { + throw new RuntimeException("Failed to parse test artifact JSON: " + e.getMessage(), e); + }
132-145: Use try-with-resources for directory handlingThe directory cleanup might be skipped if an exception occurs. Consider using try-with-resources or @tempdir.
- Files.createDirectories(Path.of("./container-volumes/git-storage/test123")); - - Mono<Path> responseMono = - commonGitFileUtils.saveArtifactToLocalRepoNew(Path.of("test123"), originalArtifactJson, "irrelevant"); - - StepVerifier.create(responseMono) - .assertNext(response -> { - Assertions.assertThat(response).isNotNull(); - }) - .verifyComplete(); - - FileUtils.deleteDirectory( - Path.of("./container-volumes/git-storage/test123").toFile()); + Path testPath = Path.of("./container-volumes/git-storage/test123"); + try { + Files.createDirectories(testPath); + Mono<Path> responseMono = + commonGitFileUtils.saveArtifactToLocalRepoNew(Path.of("test123"), originalArtifactJson, "irrelevant"); + StepVerifier.create(responseMono) + .assertNext(response -> { + Assertions.assertThat(response).isNotNull(); + }) + .verifyComplete(); + } finally { + FileUtils.deleteDirectory(testPath.toFile()); + }app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java (1)
46-47: Consider configuring ObjectMapper for test scenariosThe default ObjectMapper configuration might not cover all test cases. Consider customizing it for testing edge cases.
+ ObjectMapper objectMapper = new ObjectMapper() + .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false) + .configure(SerializationFeature.FAIL_ON_EMPTY_BEANS, false); fileUtils = new FileUtilsImpl( - gitServiceConfig, gitExecutor, fileOperations, ObservationHelper.NOOP, new ObjectMapper()); + gitServiceConfig, gitExecutor, fileOperations, ObservationHelper.NOOP, objectMapper);app/server/appsmith-interfaces/src/main/java/com/appsmith/external/dtos/ce/ModifiedResourcesCE.java (1)
48-60: Consider consolidating similar methods.
isResourceUpdatedandisResourceUpdatedNewhave nearly identical logic. Consider consolidating them into a single generic method to reduce code duplication.- public boolean isResourceUpdated(String resourceType, String resourceName) { - return StringUtils.isNotEmpty(resourceType) - && (isAllModified - || (!CollectionUtils.isEmpty(modifiedResourceMap.get(resourceType)) - && modifiedResourceMap.get(resourceType).contains(resourceName))); - } - - public boolean isResourceUpdatedNew(GitResourceType resourceType, String resourceIdentifier) { - return StringUtils.isNotEmpty(resourceIdentifier) - && (isAllModified - || (!CollectionUtils.isEmpty(modifiedResourceIdentifiers.get(resourceType)) - && modifiedResourceIdentifiers.get(resourceType).contains(resourceIdentifier))); - } + private <T> boolean isResourceUpdatedGeneric(T resourceType, String resourceIdentifier, Map<T, Set<String>> resourceMap) { + return StringUtils.isNotEmpty(resourceIdentifier) + && (isAllModified + || (!CollectionUtils.isEmpty(resourceMap.get(resourceType)) + && resourceMap.get(resourceType).contains(resourceIdentifier))); + } + + public boolean isResourceUpdated(String resourceType, String resourceName) { + return isResourceUpdatedGeneric(resourceType, resourceName, modifiedResourceMap); + } + + public boolean isResourceUpdatedNew(GitResourceType resourceType, String resourceIdentifier) { + return isResourceUpdatedGeneric(resourceType, resourceIdentifier, modifiedResourceIdentifiers); + }app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java (1)
38-39: Add JavaDoc for saveArtifactToGitRepo method.Other methods in the interface have detailed documentation. Please add JavaDoc explaining the purpose, parameters, return value, and when exceptions are thrown.
+ /** + * Saves artifact resources to the Git repository using the provided resource map + * @param baseRepoSuffix path suffix used to create a repo path + * @param gitResourceMap map containing Git resources to be saved + * @param branchName target branch name + * @return Path where the artifact is stored + * @throws GitAPIException when Git operations fail + * @throws IOException when file operations fail + */ Mono<Path> saveArtifactToGitRepo(Path baseRepoSuffix, GitResourceMap gitResourceMap, String branchName) throws GitAPIException, IOException;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (22)
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java(10 hunks)app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsImpl.java(2 hunks)app/server/appsmith-git/src/main/java/com/appsmith/git/helpers/DSLTransformerHelper.java(7 hunks)app/server/appsmith-git/src/test/java/com/appsmith/git/helpers/FileUtilsImplTest.java(2 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/dtos/ModifiedResources.java(2 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/dtos/ce/ModifiedResourcesCE.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/FileInterface.java(2 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/models/GitResourceIdentity.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/exportable/ActionCollectionExportableServiceCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java(9 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ApplicationJsonCE.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ArtifactExchangeJsonCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedExportableResourcesCE_DTO.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ArtifactGitFileUtilsCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java(12 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/exportable/CustomJSLibExportableServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/exportable/NewActionExportableServiceCEImpl.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/exportable/NewPageExportableServiceCEImpl.java(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java(3 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/templates/providers/ce/ExchangeJsonTestTemplateProviderCE.java(5 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/actioncollections/exportable/ActionCollectionExportableServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/exportable/NewActionExportableServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/newpages/exportable/NewPageExportableServiceCEImpl.java
🔇 Additional comments (10)
app/server/appsmith-git/src/main/java/com/appsmith/git/helpers/DSLTransformerHelper.java (2)
19-22: LGTM! Clean static imports for commonly used constants.
The static imports improve code readability by reducing verbosity.
31-31: LGTM! Consistent usage of static import.
app/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/exportable/CustomJSLibExportableServiceCEImpl.java (2)
3-3: LGTM: Import addition is appropriate
The GitResourceType import is correctly added and utilized in the resource identifier updates.
Line range hint 108-114: Verify intended behavior of isContextAgnostic parameter
The overloaded method ignores the isContextAgnostic parameter. Is this intentional or should this parameter influence the export behavior?
✅ Verification successful
Parameter isContextAgnostic is intentionally ignored
The overloaded method is part of a delegation pattern where the new method with isContextAgnostic parameter simply delegates to the original method. This is a valid implementation as the comment indicates that only unpublished JS libraries are considered for export, making the context agnostic parameter irrelevant for this specific service.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other implementations using isContextAgnostic
ast-grep --pattern 'getExportableEntities($_, $_, $_, $_, Boolean isContextAgnostic)'
Length of output: 87
Script:
#!/bin/bash
# Let's try a broader search for the method name and check its implementations
ast-grep --pattern 'getExportableEntities($$$)'
# Also search for the interface definition
rg "getExportableEntities.*isContextAgnostic" -A 3
Length of output: 67494
Script:
#!/bin/bash
# Let's search for the method implementation in the file under review
rg -A 10 "getExportableEntities.*isContextAgnostic" app/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/exportable/CustomJSLibExportableServiceCEImpl.java
# Also search for any interface or abstract class that might define this method
rg -A 10 "getExportableEntities.*isContextAgnostic" app/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/exportable/
Length of output: 306
Script:
#!/bin/bash
# Let's try a broader search for the method name in the file
rg -A 10 "getExportableEntities" app/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/exportable/CustomJSLibExportableServiceCEImpl.java
# Also search for the interface definition
rg -A 10 "interface.*ExportableService" app/server/appsmith-server/src/main/java/com/appsmith/server/jslibs/exportable/
Length of output: 1372
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/dtos/ModifiedResources.java (1)
3-3: LGTM! Clean CE implementation.
Good implementation of CE separation pattern. The class documentation accurately describes the purpose and usage.
Also applies to: 14-14
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedExportableResourcesCE_DTO.java (1)
18-18: LGTM! Verify map usage in export flow.
The new contextNameToGitSyncIdMap follows the established pattern.
Let's verify the map usage in the export flow:
✅ Verification successful
Let me check the usage context of this map to ensure it's properly integrated into the export flow.
Let me check how this DTO is used in the export functionality.
Let me check one final thing to see how this DTO is populated and used.
LGTM - Map addition follows established pattern in export DTO
The new contextNameToGitSyncIdMap is properly integrated into the export flow:
- Follows same initialization pattern as other maps in the DTO
- Used in both full and partial export services
- Consistent with the DTO's purpose of mapping resources during export
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of contextNameToGitSyncIdMap in export-related files
rg -l "contextNameToGitSyncIdMap" | grep -i "export"
Length of output: 156
Script:
#!/bin/bash
# Check the implementation details around the map usage
rg "contextNameToGitSyncIdMap" -A 5 -B 5
Length of output: 1445
Script:
#!/bin/bash
# Search for references to MappedExportableResourcesCE_DTO in export-related code
rg -l "MappedExportableResourcesCE_DTO" | grep -i "export"
# Also check for any direct usage patterns
ast-grep --pattern 'contextNameToGitSyncIdMap.put($$$)'
Length of output: 320
Script:
#!/bin/bash
# Look for implementations that use this DTO in export context
rg -A 5 -B 5 "new MappedExportableResources.*DTO\(\)"
Length of output: 3782
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsImpl.java (1)
8-8: LGTM: Clean constructor modification
The addition of ObjectMapper follows good dependency injection practices and maintains consistency with the superclass.
Also applies to: 26-28
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsImpl.java (1)
9-9: LGTM: Consistent constructor modification
The ObjectMapper integration follows the same pattern as other implementations in the codebase.
Also applies to: 19-24
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ApplicationJsonCE.java (1)
127-130: 🛠️ Refactor suggestion
Add type safety checks in setArtifact method
The unchecked cast to Application could fail at runtime. Consider adding type validation before the cast.
@Override
public <T extends Artifact> void setArtifact(T application) {
+ if (!(application instanceof Application)) {
+ throw new IllegalArgumentException("Artifact must be an Application instance");
+ }
this.exportedApplication = (Application) application;
}app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ArtifactExchangeJsonCE.java (1)
33-34: LGTM! Well-designed generic methods.
The generic methods with bounded type parameters provide good flexibility while maintaining type safety.
Also applies to: 74-74
.../src/main/java/com/appsmith/server/jslibs/exportable/CustomJSLibExportableServiceCEImpl.java
Show resolved
Hide resolved
| .filter(action -> !PluginType.JS.equals(action.getPluginType()) | ||
| && action.getUnpublishedAction().getActionConfiguration() != null | ||
| && !(action.getUnpublishedAction() | ||
| .getActionConfiguration() | ||
| .getBody() | ||
| == null | ||
| || (action.getPluginType().equals(PluginType.REMOTE) | ||
| && action.getUnpublishedAction() | ||
| .getActionConfiguration() | ||
| .getFormData() | ||
| == null))) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Complex Condition for Readability
The filter condition is complex and hard to read. Consider extracting parts into well-named variables or helper methods to enhance readability.
...psmith/server/git/resourcemap/templates/providers/ce/ExchangeJsonTestTemplateProviderCE.java
Show resolved
Hide resolved
| modifiedResources | ||
| .getModifiedResourceIdentifiers() | ||
| .get(GitResourceType.CONTEXT_CONFIG) | ||
| .addAll(pageIdentifiersSet); |
There was a problem hiding this comment.
Prevent Potential NullPointerException
Ensure modifiedResources.getModifiedResourceIdentifiers().get(GitResourceType.CONTEXT_CONFIG) is not null before calling addAll(). Initialize it if necessary.
Apply this diff:
modifiedResources
.getModifiedResourceIdentifiers()
- .get(GitResourceType.CONTEXT_CONFIG)
+ .computeIfAbsent(GitResourceType.CONTEXT_CONFIG, k -> new HashSet<>())
.addAll(pageIdentifiersSet);📝 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.
| modifiedResources | |
| .getModifiedResourceIdentifiers() | |
| .get(GitResourceType.CONTEXT_CONFIG) | |
| .addAll(pageIdentifiersSet); | |
| modifiedResources | |
| .getModifiedResourceIdentifiers() | |
| .computeIfAbsent(GitResourceType.CONTEXT_CONFIG, k -> new HashSet<>()) | |
| .addAll(pageIdentifiersSet); |
| filesInRepo.parallelStream() | ||
| .filter(path -> !Files.isDirectory(baseRepoPath.resolve(path))) | ||
| .forEach(filePath -> { | ||
| Tuple2<GitResourceIdentity, Object> identity = getGitResourceIdentity(baseRepoPath, filePath); | ||
|
|
||
| resourceMap.put(identity.getT1(), identity.getT2()); | ||
| }); |
There was a problem hiding this comment.
Handle Potential Null Values
getGitResourceIdentity may return null. Add a null check before using the returned value to prevent a NullPointerException.
Apply this diff:
.stream()
.filter(path -> !Files.isDirectory(baseRepoPath.resolve(path)))
.forEach(filePath -> {
Tuple2<GitResourceIdentity, Object> identity = getGitResourceIdentity(baseRepoPath, filePath);
+ if (identity != null) {
resourceMap.put(identity.getT1(), identity.getT2());
+ }
});📝 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.
| filesInRepo.parallelStream() | |
| .filter(path -> !Files.isDirectory(baseRepoPath.resolve(path))) | |
| .forEach(filePath -> { | |
| Tuple2<GitResourceIdentity, Object> identity = getGitResourceIdentity(baseRepoPath, filePath); | |
| resourceMap.put(identity.getT1(), identity.getT2()); | |
| }); | |
| filesInRepo.stream() | |
| .filter(path -> !Files.isDirectory(baseRepoPath.resolve(path))) | |
| .forEach(filePath -> { | |
| Tuple2<GitResourceIdentity, Object> identity = getGitResourceIdentity(baseRepoPath, filePath); | |
| if (identity != null) { | |
| resourceMap.put(identity.getT1(), identity.getT2()); | |
| } | |
| }); |
| @Override | ||
| public <T extends Context> void setContextList(List<T> contextList) { | ||
| this.pageList = (List<NewPage>) contextList; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety checks in setContextList method
The unchecked cast of the entire list could fail at runtime. Consider validating list elements.
@Override
public <T extends Context> void setContextList(List<T> contextList) {
+ if (contextList != null && !contextList.stream().allMatch(context -> context instanceof NewPage)) {
+ throw new IllegalArgumentException("All contexts must be NewPage instances");
+ }
this.pageList = (List<NewPage>) contextList;
}📝 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.
| @Override | |
| public <T extends Context> void setContextList(List<T> contextList) { | |
| this.pageList = (List<NewPage>) contextList; | |
| } | |
| @Override | |
| public <T extends Context> void setContextList(List<T> contextList) { | |
| if (contextList != null && !contextList.stream().allMatch(context -> context instanceof NewPage)) { | |
| throw new IllegalArgumentException("All contexts must be NewPage instances"); | |
| } | |
| this.pageList = (List<NewPage>) contextList; | |
| } |
| public void putResource(String resourceType, String resourceName) { | ||
| if (!this.modifiedResourceMap.containsKey(resourceType)) { | ||
| this.modifiedResourceMap.put(resourceType, new HashSet<>()); | ||
| } | ||
| this.modifiedResourceMap.get(resourceType).add(resourceName); | ||
| } | ||
|
|
||
| /** | ||
| * Adds a set of resources to the map. Will create a new set if no set found for the provided resource type. | ||
| * It'll append the resources to the set. | ||
| * @param resourceType String, type of the resource e.g. PAGE_LST | ||
| * @param resourceNames Set of String, names of the resource e.g. Home Page, About page | ||
| */ | ||
| public void putResource(String resourceType, Set<String> resourceNames) { | ||
| if (!this.modifiedResourceMap.containsKey(resourceType)) { | ||
| this.modifiedResourceMap.put(resourceType, new HashSet<>()); | ||
| } | ||
| this.modifiedResourceMap.get(resourceType).addAll(resourceNames); | ||
| } |
There was a problem hiding this comment.
Add null checks in putResource methods.
Both putResource methods should validate input parameters to prevent NullPointerException.
public void putResource(String resourceType, String resourceName) {
+ if (resourceType == null || resourceName == null) {
+ return;
+ }
if (!this.modifiedResourceMap.containsKey(resourceType)) {
this.modifiedResourceMap.put(resourceType, new HashSet<>());
}
this.modifiedResourceMap.get(resourceType).add(resourceName);
}
public void putResource(String resourceType, Set<String> resourceNames) {
+ if (resourceType == null || resourceNames == null) {
+ return;
+ }
if (!this.modifiedResourceMap.containsKey(resourceType)) {
this.modifiedResourceMap.put(resourceType, new HashSet<>());
}
this.modifiedResourceMap.get(resourceType).addAll(resourceNames);
}📝 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 void putResource(String resourceType, String resourceName) { | |
| if (!this.modifiedResourceMap.containsKey(resourceType)) { | |
| this.modifiedResourceMap.put(resourceType, new HashSet<>()); | |
| } | |
| this.modifiedResourceMap.get(resourceType).add(resourceName); | |
| } | |
| /** | |
| * Adds a set of resources to the map. Will create a new set if no set found for the provided resource type. | |
| * It'll append the resources to the set. | |
| * @param resourceType String, type of the resource e.g. PAGE_LST | |
| * @param resourceNames Set of String, names of the resource e.g. Home Page, About page | |
| */ | |
| public void putResource(String resourceType, Set<String> resourceNames) { | |
| if (!this.modifiedResourceMap.containsKey(resourceType)) { | |
| this.modifiedResourceMap.put(resourceType, new HashSet<>()); | |
| } | |
| this.modifiedResourceMap.get(resourceType).addAll(resourceNames); | |
| } | |
| public void putResource(String resourceType, String resourceName) { | |
| if (resourceType == null || resourceName == null) { | |
| return; | |
| } | |
| if (!this.modifiedResourceMap.containsKey(resourceType)) { | |
| this.modifiedResourceMap.put(resourceType, new HashSet<>()); | |
| } | |
| this.modifiedResourceMap.get(resourceType).add(resourceName); | |
| } | |
| /** | |
| * Adds a set of resources to the map. Will create a new set if no set found for the provided resource type. | |
| * It'll append the resources to the set. | |
| * @param resourceType String, type of the resource e.g. PAGE_LST | |
| * @param resourceNames Set of String, names of the resource e.g. Home Page, About page | |
| */ | |
| public void putResource(String resourceType, Set<String> resourceNames) { | |
| if (resourceType == null || resourceNames == null) { | |
| return; | |
| } | |
| if (!this.modifiedResourceMap.containsKey(resourceType)) { | |
| this.modifiedResourceMap.put(resourceType, new HashSet<>()); | |
| } | |
| this.modifiedResourceMap.get(resourceType).addAll(resourceNames); | |
| } |
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImplTest.java (1)
164-164: Consider using dynamic UUID for gitSyncId in testsWhile setting gitSyncId is correct, using a dynamic UUID instead of hardcoded "p1" would ensure better test isolation.
- newPage.setGitSyncId("p1"); + newPage.setGitSyncId(UUID.randomUUID().toString());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImplTest.java(1 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImplTest.java (1)
164-164: Verify test coverage for gitSyncId scenarios
Let's verify that the test coverage remains comprehensive with the addition of gitSyncId.
✅ Verification successful
Let me check the test coverage more thoroughly by examining the actual test implementations.
Test coverage for gitSyncId is adequate
The codebase already contains comprehensive test coverage for gitSyncId in ImportServiceTests.java, including:
- Verification of gitSyncId preservation during import/export
- Explicit assertions checking gitSyncId equality
- Tests covering scenarios with page addition and removal
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any test cases specifically validating gitSyncId behavior
rg -l "gitSyncId" --type java app/server/appsmith-server/src/test/java/
# Check for any existing assertions on gitSyncId
ast-grep --pattern 'assertThat($x.getGitSyncId())'
Length of output: 226
Script:
#!/bin/bash
# Check the content of test files containing gitSyncId
rg "gitSyncId" -B 2 -A 2 app/server/appsmith-server/src/test/java/com/appsmith/server/git/autocommit/AutoCommitEventHandlerImplTest.java
rg "gitSyncId" -B 2 -A 2 app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java
# Look for any test methods that might be testing gitSyncId functionality
ast-grep --pattern 'void $testMethod() {
$$$
gitSyncId
$$$
}'
Length of output: 1585
Failed server tests
|
|
Force merging since changes since approval are for compile errors and test changes |
## Description > [!TIP] > _Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team)._ > > _Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR._ Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Git" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: <https://github.com/appsmithorg/appsmith/actions/runs/12145100933> > Commit: c78857b > Workflow: `PR Automation test suite` > Tags: `@tag.Git` > Spec: `` > <hr>Tue, 03 Dec 2024 17:36:22 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Enhanced Git resource handling with the ability to save artifacts to Git repositories. - Introduced new methods for managing modified resources and artifact exchange JSON. - Improved tracking of updated entities during export operations, including custom JavaScript libraries and new pages. - New functionality for handling application artifacts and contexts in Git. - **Bug Fixes** - Refined error handling and control flow in auto-commit and migration processes. - **Documentation** - Updated comments and method signatures for clarity and improved understanding. - **Tests** - Added new test cases to validate the conversion processes and resource comparisons. - Enhanced existing tests to utilize new data structures and improve clarity. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12145100933
Commit: c78857b
Cypress dashboard.
Tags:
@tag.GitSpec:
Tue, 03 Dec 2024 18:37:08 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests