chore: json to map conversion with test template #37788
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on modifications to Git resource management within the Appsmith application. Key alterations include the renaming of an enum constant, the introduction of new methods for handling artifact-dependent resources, and enhancements to the management of application data in a Git-compatible format. Additionally, new testing classes and methods have been added to ensure the integrity of the changes, particularly related to JSON conversion and resource mapping. 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: 10
🧹 Outside diff range and nitpick comments (15)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/models/GitResourceType.java (2)
9-9: Document the semantic changeThe shift from PAGE_CONFIG to CONTEXT_CONFIG suggests a broader semantic change in how resources are categorized. Please add a comment explaining the rationale and implications of this change.
+ /** + * Represents configuration for context-based resources, replacing the previous page-based approach. + * Used in Git resource mapping for JSON to map conversion. + */ CONTEXT_CONFIG,
9-9: Consider adding migration strategyFor breaking changes like this enum rename, we should ensure there's a migration path for existing data.
Consider:
- Adding a temporary backwards compatibility layer
- Including database migration scripts if this enum is persisted
- Documenting the migration steps in the PR description
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ArtifactGitFileUtilsCE.java (1)
16-16: Consider adding @nonnull annotations for consistency.Other methods in this interface use @nonnull annotations. Consider adding them to the parameters if null values are not acceptable.
- void setArtifactDependentResources(ArtifactExchangeJson artifactExchangeJson, GitResourceMap gitResourceMap); + void setArtifactDependentResources(@NonNull ArtifactExchangeJson artifactExchangeJson, @NonNull GitResourceMap gitResourceMap);app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ArtifactExchangeJsonCE.java (1)
67-69: Add Javadoc for the new method.Document the purpose of getContextList() and what type of objects it's expected to return.
Add documentation like:
+ /** + * Returns a list of context objects for internal processing. + * @return List of context objects + */ @JsonView(Views.Internal.class) List getContextList();app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/templates/contexts/ExchangeJsonContext.java (3)
13-20: Add class-level documentationConsider adding Javadoc to describe the purpose of this test context class and its role in the test framework.
+/** + * Test context for JSON to resource map conversion tests. + * Provides parameter resolution and test template invocation context for parameterized testing. + */ public class ExchangeJsonContext implements TestTemplateInvocationContext, ParameterResolver {
21-26: Add parameter validationConsider adding null checks for fileName and artifactExchangeJsonType parameters.
public ExchangeJsonContext( String fileName, Class<? extends ArtifactExchangeJson> artifactExchangeJsonType, int resourceMapKeyCount) { + if (fileName == null || artifactExchangeJsonType == null) { + throw new IllegalArgumentException("fileName and artifactExchangeJsonType must not be null"); + } this.fileName = fileName; this.artifactExchangeJsonType = artifactExchangeJsonType; this.resourceMapKeyCount = resourceMapKeyCount; }
52-56: Add documentation for parameter resolutionConsider adding Javadoc to explain the parameter resolution behavior.
+/** + * Resolves parameters by returning this context instance. + * Only called if {@link #supportsParameter} returns true. + */ @Override public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext) throws ParameterResolutionException { return this; }app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java (2)
27-43: Add class-level documentationConsider adding Javadoc to describe the test class's purpose and the role of each injected dependency.
+/** + * Tests the conversion of artifact JSON to Git resource map format. + * Validates the mapping logic and resource handling using various test contexts. + */ @SpringBootTest @TestInstance(TestInstance.Lifecycle.PER_CLASS) public class ExchangeJsonConversionTests {
55-77: Extract assertion logic to improve readabilityConsider extracting the assertion block into a separate method for better maintainability.
+ private void assertGitResourceMap( + GitResourceMap gitResourceMap, + ArtifactExchangeJson exchangeJson, + ExchangeJsonContext context) { + assertThat(gitResourceMap).isNotNull(); + + if (exchangeJson.getModifiedResources() == null) { + assertThat(gitResourceMap.getModifiedResources()).isNull(); + } else { + assertThat(exchangeJson.getModifiedResources()) + .isEqualTo(gitResourceMap.getModifiedResources()); + } + Map<GitResourceIdentity, Object> resourceMap = gitResourceMap.getGitResourceMap(); + assertThat(resourceMap).isNotNull(); + + assertThat(resourceMap).hasSize(context.resourceMapKeyCount()); + + long count = templateProvider.assertResourceComparisons(exchangeJson, resourceMap); + + assertThat(count).isEqualTo(context.resourceMapKeyCount()); + }app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ApplicationJsonCE.java (1)
137-140: Add Javadoc for the new methodSince this is implementing an interface method, adding documentation would help explain its purpose in this context.
+ /** + * Returns the list of pages associated with this application. + * Implementation of ArtifactExchangeJsonCE interface. + * @return List of NewPage objects + */ @Override public List getContextList() { return this.pageList; }app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/templates/providers/ce/ExchangeJsonTestTemplateProviderCE.java (2)
20-32: Consider improving test template provider implementation.
- The
supportsTestTemplatemethod unconditionally returns true, which might be too permissive.- The context creation uses hard-coded values which could be externalized for better maintainability.
Consider this improvement:
+ /** Test template provider for JSON to map conversion tests */ public class ExchangeJsonTestTemplateProviderCE implements TestTemplateInvocationContextProvider { + private static final String TEST_FILE = "valid-application.json"; + private static final int EXPECTED_RESOURCE_COUNT = 23; @Override public boolean supportsTestTemplate(ExtensionContext extensionContext) { - return true; + return extensionContext.getTestMethod() + .map(method -> method.isAnnotationPresent(ExchangeJsonTest.class)) + .orElse(false); } @Override public Stream<TestTemplateInvocationContext> provideTestTemplateInvocationContexts( ExtensionContext extensionContext) { - ExchangeJsonContext context = new ExchangeJsonContext("valid-application.json", ApplicationJson.class, 23); + ExchangeJsonContext context = new ExchangeJsonContext(TEST_FILE, ApplicationJson.class, EXPECTED_RESOURCE_COUNT); return Stream.of(context); }
103-113: Add input validation and documentation.The helper method could benefit from parameter validation and JavaDoc documentation.
Consider this improvement:
+ /** + * Filters resources from the resource map by the specified resource type. + * + * @param resourceMap the map containing all resources + * @param resourceType the type of resources to filter + * @return list of resources matching the specified type + * @throws IllegalArgumentException if resourceMap or resourceType is null + */ protected List<Object> getResourceListByType( Map<GitResourceIdentity, Object> resourceMap, GitResourceType resourceType) { + if (resourceMap == null || resourceType == null) { + throw new IllegalArgumentException("Resource map and type must not be null"); + } return resourceMap.entrySet().stream() .filter(entry -> resourceType.equals(entry.getKey().getResourceType())) .map(Map.Entry::getValue) .collect(Collectors.toList()); }app/server/appsmith-server/src/test/resources/test_assets/ImportExportServiceTest/valid-application.json (1)
Line range hint
667-676: Security Warning: Sensitive information exposed in decryptedFieldsThe
decryptedFieldssection contains database credentials in plain text. This is a security risk if this file is committed to version control.Consider:
- Moving sensitive data to environment variables
- Using a secrets management service
- Adding this file to
.gitignoreif it's not meant for version controlapp/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (1)
245-312: Refactor to eliminate code duplicationMethods
setNewActionsInResourceMapandsetActionCollectionsInResourceMaphave similar logic structures. Consider refactoring to a common method to improve maintainability.Also applies to: 314-341
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (1)
152-152: Avoid modifying input parameter directlySetting
applicationJson.setModifiedResources(null);modifies the input parameter directly, which might lead to unintended side effects. Consider creating a copy ofapplicationJsonbefore making modifications.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/models/GitResourceType.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ApplicationJsonCE.java(1 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/helpers/CommonGitFileUtils.java(2 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(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/templates/contexts/ExchangeJsonContext.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/templates/providers/ExchangeJsonTestTemplateProvider.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/templates/providers/ce/ExchangeJsonTestTemplateProviderCE.java(1 hunks)app/server/appsmith-server/src/test/resources/test_assets/ImportExportServiceTest/valid-application.json(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/templates/providers/ExchangeJsonTestTemplateProvider.java
🔇 Additional comments (7)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/git/models/GitResourceType.java (1)
9-9: Verify all usages of the renamed enum constant
The change from PAGE_CONFIG to CONTEXT_CONFIG appears to be a breaking change that could affect existing code and persisted data.
Let's verify the impact:
✅ Verification successful
Enum constant rename appears safe to proceed
Based on the codebase analysis:
- No references to the old
PAGE_CONFIGvalue were found - Only two active usages of the new
CONTEXT_CONFIGvalue exist - No serialized forms of the old enum value were detected in configs
The change appears to be part of a clean implementation rather than a breaking change to existing code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to PAGE_CONFIG
rg "PAGE_CONFIG" --type java
# Search for existing uses of the new CONTEXT_CONFIG
rg "CONTEXT_CONFIG" --type java
# Look for potential serialized forms in JSON/YAML configs
rg -i "page_config" --type json --type yaml
Length of output: 693
Script:
#!/bin/bash
# Search for git-related files that might reference the old enum value
fd -e java -e json -e yaml -e properties git --exec rg -l "PAGE_CONFIG|page_config" {} \;
# Look for any serialized forms in test resources
fd -e json -e yaml -p "test.*resources" --exec rg -l "PAGE_CONFIG|page_config" {} \;
# Check git-related classes for enum usage patterns
ast-grep --pattern 'class $_ {
$$$
GitResourceType $_ $$$
$$$
}'
Length of output: 257
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/ArtifactGitFileUtilsCE.java (1)
16-16: LGTM! Method signature is clear and well-placed.
The new method aligns well with the interface's responsibility of managing Git artifact resources.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java (2)
7-7: LGTM: Clean import additions
The new imports for ActionCollectionService and NewActionService are properly organized and align with the class's enhanced responsibilities.
Also applies to: 10-10
28-29: LGTM: Constructor dependency injection follows best practices
The addition of NewActionService and ActionCollectionService as constructor dependencies is properly implemented and matches the superclass requirements. The constructor maintains clean dependency injection principles.
Let's verify the superclass constructor signature matches:
Also applies to: 37-38
✅ Verification successful
Constructor dependency injection matches superclass requirements
The constructor in CommonGitFileUtils correctly passes all required dependencies to its superclass CommonGitFileUtilsCE in the exact order:
- applicationGitFileUtils
- fileUtils
- fileOperations
- analyticsService
- sessionUserService
- newActionService
- actionCollectionService
- jsonSchemaVersions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the superclass constructor signature
ast-grep --pattern 'class CommonGitFileUtilsCE {
$$$
CommonGitFileUtilsCE(
$_,
$_,
$_,
$_,
$_,
$_,
$_,
$_
) {
$$$
}
}'
Length of output: 164
Script:
#!/bin/bash
# Let's try a different approach to find the superclass constructor
rg -U "class CommonGitFileUtilsCE.*?\{.*?CommonGitFileUtilsCE\(.*?\)" --multiline -A 10
Length of output: 89
Script:
#!/bin/bash
# Also search for the child class constructor to compare
rg -U "class CommonGitFileUtils.*?\{.*?CommonGitFileUtils\(.*?\)" --multiline -A 10
Length of output: 85
Script:
#!/bin/bash
# Let's also check if these classes exist in different locations
fd "CommonGitFileUtils(CE)?\.java$"
Length of output: 228
Script:
#!/bin/bash
# Let's directly read both files to compare constructors
cat "app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java" | grep -A 10 "CommonGitFileUtilsCE("
cat "app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java" | grep -A 10 "CommonGitFileUtils("
Length of output: 783
Script:
#!/bin/bash
# Let's get the full file content to see the complete constructor chain
cat "app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/CommonGitFileUtils.java"
Length of output: 1723
Script:
#!/bin/bash
# Let's check the superclass to verify the constructor parameters match
cat "app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java"
Length of output: 31015
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ArtifactExchangeJsonCE.java (1)
6-6: LGTM on JsonView usage.
The @JSONVIEW annotation with Views.Internal.class is correctly applied to restrict serialization access.
Also applies to: 13-13, 68-69
app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java (1)
82-82: Verify test asset files existence
Let's verify that all required test asset files are present in the repository.
✅ Verification successful
Test assets are present and correctly located
All required test asset files are present in the correct location at app/server/appsmith-server/src/test/resources/test_assets/ImportExportServiceTest/. The file path used in the test class is relative to the resources directory, which is the standard Java test resources convention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all test asset files exist
fd -t f . "test_assets/ImportExportServiceTest" -x echo "Found test asset: {}"
Length of output: 201
Script:
#!/bin/bash
# Let's first check the actual test file to understand the test data structure
rg -A 5 -B 5 "test_assets/ImportExportServiceTest" .
# Also search for similar test asset patterns
fd -t f . "test" -x echo "Found test file: {}" | grep -i "asset"
Length of output: 83926
Script:
#!/bin/bash
# Let's check if the test assets directory exists in the correct location relative to the test file
fd -t f . app/server/appsmith-server/src/test/resources/test_assets/ImportExportServiceTest -x echo "Found test asset: {}"
Length of output: 2604
app/server/appsmith-server/src/test/resources/test_assets/ImportExportServiceTest/valid-application.json (1)
633-634: LGTM: Git sync IDs are properly configured
The gitSyncId properties are correctly set for both JavaScript objects, and the new flag is appropriately set to false indicating these are existing objects.
Let's verify the uniqueness of the Git sync IDs:
Also applies to: 654-655
✅ Verification successful
Let me gather more context about these gitSyncId duplicates.
The gitSyncId "jso1" appears to be a test fixture value and is distinct from production IDs
The gitSyncId "jso1" is found in a test resource file and follows a different format from the production IDs seen in the application export file (which use UUID-like format). This is expected behavior for test fixtures.
- The duplicates found are in a Cypress test fixture file and not in the file under review
- The test file's "jso1" ID is intentionally simplified for testing purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for duplicate gitSyncId values
rg -o 'gitSyncId": "[^"]*' . | sort | uniq -d
Length of output: 295
Script:
#!/bin/bash
# Check the content around these duplicate gitSyncId values to understand their context
rg -B2 -A2 '6209d7096b4b1e154a3c01ea_6209e8776b4b1e154a3c02ad|6209d7096b4b1e154a3c01ea_6209ef2f6b4b1e154a3c02d0' ./app/client/cypress/fixtures/TableMigrationAppExported.json
# Also check if these IDs appear in the file under review
rg 'jso1|6209d7096b4b1e154a3c01ea_6209e8776b4b1e154a3c02ad|6209d7096b4b1e154a3c01ea_6209ef2f6b4b1e154a3c02d0' ./app/server/appsmith-server/src/test/resources/test_assets/ImportExportServiceTest/valid-application.json
Length of output: 1043
...server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ArtifactExchangeJsonCE.java
Outdated
Show resolved
Hide resolved
...rc/test/java/com/appsmith/server/git/resourcemap/templates/contexts/ExchangeJsonContext.java
Show resolved
Hide resolved
...th-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ApplicationJsonCE.java
Outdated
Show resolved
Hide resolved
...psmith/server/git/resourcemap/templates/providers/ce/ExchangeJsonTestTemplateProviderCE.java
Show resolved
Hide resolved
...erver/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java
Outdated
Show resolved
Hide resolved
...erver/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java
Outdated
Show resolved
Hide resolved
...server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java
Show resolved
Hide resolved
...server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java
Outdated
Show resolved
Hide resolved
...server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java
Outdated
Show resolved
Hide resolved
...server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ApplicationJsonCE.java
Outdated
Show resolved
Hide resolved
...erver/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (2)
278-299: Consider extracting REMOTE plugin handling to a separate method.The special case handling for REMOTE type plugins makes the code harder to read and maintain.
Consider extracting this logic:
+ private void handleRemotePluginAction(NewAction newAction) { + if (PluginType.REMOTE.equals(newAction.getPluginType()) + && newAction.getUnpublishedAction().getActionConfiguration() != null + && newAction.getUnpublishedAction().getActionConfiguration().getFormData() != null) { + String body = new Gson().toJson( + newAction.getUnpublishedAction().getActionConfiguration().getFormData(), + Map.class); + newAction.getUnpublishedAction().getActionConfiguration().setFormData(null); + return body; + } + return null; + }
351-365: Add Javadoc for helper methods.The helper methods lack documentation explaining their purpose and behavior.
Add Javadoc comments:
+ /** + * Removes unnecessary fields from an action before Git commit. + * This ensures only the unpublished version is committed and the action is properly sanitized. + * + * @param action The action to be sanitized + */ private void removeUnwantedFieldFromAction(NewAction action) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/git/resourcemap/ExchangeJsonConversionTests.java
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (2)
5-7: LGTM! Dependencies are properly injected.
The new service dependencies are correctly added using constructor injection via @RequiredArgsConstructor.
Also applies to: 76-77
195-206: LGTM! Well-structured resource map creation.
The method follows good separation of concerns by delegating artifact-specific operations to the appropriate helper methods.
...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
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (2)
255-322: Extract complex processing logic into separate methodsThe method is complex with multiple responsibilities. Consider extracting the processing logic for REMOTE and JS type plugins into separate methods for better maintainability.
Consider this structure:
protected void setNewActionsInResourceMap( ArtifactExchangeJson artifactExchangeJson, Map<GitResourceIdentity, Object> resourceMap) { if (artifactExchangeJson.getActionList() == null) { return; } artifactExchangeJson.getActionList().stream() .filter(this::isValidAction) .peek(newAction -> newActionService.generateActionByViewMode(newAction, false)) .forEach(newAction -> { removeUnwantedFieldFromAction(newAction); - String body = // ... existing body extraction logic + String body = extractActionBody(newAction); processActionByType(newAction, body, resourceMap); }); } +private boolean isValidAction(NewAction newAction) { + return newAction.getUnpublishedAction() != null + && newAction.getUnpublishedAction().getDeletedAt() == null; +} +private void processActionByType(NewAction newAction, String body, Map<GitResourceIdentity, Object> resourceMap) { + if (PluginType.REMOTE.equals(newAction.getPluginType())) { + processRemoteTypeAction(newAction, resourceMap); + } else if (PluginType.JS.equals(newAction.getPluginType())) { + processJSTypeAction(newAction, resourceMap); + } else { + processRegularAction(newAction, body, resourceMap); + } +}
324-351: Apply similar refactoring pattern as setNewActionsInResourceMapFor consistency and maintainability, consider applying the same refactoring pattern used in setNewActionsInResourceMap.
Consider extracting the processing logic:
+private boolean isValidActionCollection(ActionCollection collection) { + return collection.getUnpublishedCollection() != null + && collection.getUnpublishedCollection().getDeletedAt() == null; +} +private void processActionCollection(ActionCollection collection, Map<GitResourceIdentity, Object> resourceMap) { + String body = collection.getUnpublishedCollection().getBody() != null + ? collection.getUnpublishedCollection().getBody() + : ""; + collection.getUnpublishedCollection().setBody(null); + + addToResourceMap(collection, body, resourceMap); +}app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (1)
180-188: Consider extracting widget processing logicThe widget flattening and processing logic is complex and could be moved to a separate method for better maintainability.
+private void processWidgets(String pageId, JSONObject dsl, Map<GitResourceIdentity, Object> resourceMap) { + Map<String, org.json.JSONObject> result = + DSLTransformerHelper.flatten(new org.json.JSONObject(dsl.toString())); + result.forEach((key, jsonObject) -> { + String widgetId = pageId + "-" + jsonObject.getString(WIDGET_ID); + GitResourceIdentity widgetIdentity = + new GitResourceIdentity(GitResourceType.WIDGET_CONFIG, widgetId); + resourceMap.put(widgetIdentity, jsonObject); + }); +}Then use it in the main method:
-Map<String, org.json.JSONObject> result = - DSLTransformerHelper.flatten(new org.json.JSONObject(dsl.toString())); -result.forEach((key, jsonObject) -> { - String widgetId = newPage.getGitSyncId() + "-" + jsonObject.getString(WIDGET_ID); - GitResourceIdentity widgetIdentity = - new GitResourceIdentity(GitResourceType.WIDGET_CONFIG, widgetId); - resourceMap.put(widgetIdentity, jsonObject); -}); +processWidgets(newPage.getGitSyncId(), dsl, resourceMap);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ApplicationJsonCE.java(1 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/helpers/ce/CommonGitFileUtilsCE.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ApplicationJsonCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/ArtifactExchangeJsonCE.java
🔇 Additional comments (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java (3)
5-7: LGTM: New service fields and imports are well-structured
The addition of Git resource management imports and service fields follows good practices and maintains consistency with the existing codebase.
Also applies to: 78-79
353-368: LGTM: Well-documented cleanup methods
The helper methods for removing unwanted fields are well-documented and follow good practices for data sanitization.
210-253:
Add null checks for nested object access
Several potential NullPointerException risks exist when accessing nested properties:
datasource.getGitSyncId()access without null checktheme.getUnpublishedCollection()access without null checkjsLib.getUidString()access without null check
Apply these defensive checks:
if (datasourceList != null) {
datasourceList.forEach(datasource -> {
+ if (datasource != null && datasource.getGitSyncId() != null) {
removeUnwantedFieldsFromDatasource(datasource);
GitResourceIdentity identity =
new GitResourceIdentity(GitResourceType.DATASOURCE_CONFIG, datasource.getGitSyncId());
resourceMap.put(identity, datasource);
+ }
});
}Likely invalid or redundant comment.
app/server/appsmith-server/src/main/java/com/appsmith/server/applications/git/ApplicationGitFileUtilsCEImpl.java (3)
Line range hint 559-567: LGTM: Good handling of REMOTE plugin type
The special handling for REMOTE plugin type with proper type checking and form data deserialization is well implemented.
183-183:
Add null check for widget ID
Ensure the widget ID exists before accessing it to prevent potential NullPointerException.
-String widgetId = newPage.getGitSyncId() + "-" + jsonObject.getString(WIDGET_ID);
+if (!jsonObject.has(WIDGET_ID)) {
+ throw new AppsmithException(AppsmithError.INVALID_PARAMETER, "Widget ID is missing");
+}
+String widgetId = newPage.getGitSyncId() + "-" + jsonObject.getString(WIDGET_ID);Likely invalid or redundant comment.
169-170:
Add null check for layouts list
The code assumes that layouts list exists and has at least one element. Add validation to prevent potential IndexOutOfBoundsException.
-JSONObject dsl =
- newPage.getUnpublishedPage().getLayouts().get(0).getDsl();
+List<Layout> layouts = newPage.getUnpublishedPage().getLayouts();
+if (layouts == null || layouts.isEmpty()) {
+ throw new AppsmithException(AppsmithError.INVALID_PARAMETER, "Page layouts");
+}
+JSONObject dsl = layouts.get(0).getDsl();Likely invalid or redundant comment.
...erver/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/CommonGitFileUtilsCE.java
Show resolved
Hide resolved
## 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 --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12064617641> > Commit: bf8b3bf > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12064617641&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Thu, 28 Nov 2024 08:38:34 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** - Updated Git resource type to include `CONTEXT_CONFIG`. - Introduced methods to manage artifact-dependent resources and context lists. - Enhanced Git resource management with new actions and collections. - Added new properties and components in the exported application structure, including new data sources, pages, and actions. - **Bug Fixes** - Improved handling of metadata fields in application exports. - **Documentation** - Added unit tests for artifact JSON to Git resource map conversion. - **Chores** - Updated JSON structure for exported applications with new components and properties. <!-- 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/12064617641
Commit: bf8b3bf
Cypress dashboard.
Tags:
@tag.GitSpec:
Thu, 28 Nov 2024 08:38:34 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
CONTEXT_CONFIG.Bug Fixes
Documentation
Chores