chore: Support String inputs for import service methods #39337
Conversation
This is so that these service methods can be used by other services for higher-order functionality. In such cases, passing the JSON content as a `String` is much more convenient than to build a `Part` implementation instance.
WalkthroughThe changes update artifact import functionality by introducing methods to convert a file part to a string and altering method signatures to accept JSON string inputs instead of file parts. Both core and partial import service interfaces and implementations are updated accordingly. Test cases have been revised to reflect these new signatures and conversion flows. Dependencies such as ActionCollectionService are removed, and file-based resource handling in tests is replaced by string-based processing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PartialImportService as PI
participant ImportService as IS
Client->>PI: importResourceInPage(wsId, appId, pageId, branch, file Part)
Note over PI: Check input type and convert if needed
PI->>IS: readFilePartToString(file Part)
IS-->>PI: file contents (String)
PI->>IS: extractArtifactExchangeJsonAndSaveArtifact(file contents, wsId, artifactId)
IS-->>PI: ArtifactImportDTO
PI-->>Client: Application
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
Failed server tests
|
sharat87
requested review from
mohanarpit,
nidhi-nair,
trishaanand and
AnaghHegde
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/PartialImportServiceTest.java (1)
251-255: Consider handling potential IOException explicitly.While
@SneakyThrowsworks, it's better to handle IOExceptions explicitly in test code for better error messages and debugging.- @SneakyThrows - private String readResource(String filePath) { - return StreamUtils.copyToString( - new DefaultResourceLoader().getResource(filePath).getInputStream(), StandardCharsets.UTF_8); + private String readResource(String filePath) throws IOException { + try { + return StreamUtils.copyToString( + new DefaultResourceLoader().getResource(filePath).getInputStream(), StandardCharsets.UTF_8); + } catch (IOException e) { + throw new IOException("Failed to read resource: " + filePath, e); + }app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCE.java (1)
14-15: LGTM! Consider adding Javadoc.The method overload aligns well with the objective of supporting String inputs. Consider adding Javadoc to clarify the expected JSON format of
fileContents.+ /** + * Imports a resource into a page using JSON content provided as a string. + * @param workspaceId The ID of the workspace + * @param applicationId The ID of the application + * @param pageId The ID of the page + * @param branchName The name of the branch + * @param fileContents JSON string containing the resource configuration + * @return A Mono containing the updated Application + */ Mono<Application> importResourceInPage( String workspaceId, String applicationId, String pageId, String branchName, String fileContents);app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java (1)
132-145: Consider extracting charset to a constant.The implementation looks good, but the charset could be defined as a constant for consistency and maintainability.
+ private static final Charset DEFAULT_CHARSET = StandardCharsets.UTF_8; // ... - return new String(data); + return new String(data, DEFAULT_CHARSET);app/server/appsmith-server/src/test/java/com/appsmith/server/git/ServerSchemaMigrationEnforcerTest.java (1)
280-284: Consider explicit exception handling instead of @SneakyThrows.While @SneakyThrows simplifies the code, explicit exception handling would make error cases more visible and maintainable.
- @SneakyThrows - private String readResource(String filePath) { + private String readResource(String filePath) throws IOException { return StreamUtils.copyToString( new DefaultResourceLoader().getResource(filePath).getInputStream(), StandardCharsets.UTF_8); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceImpl.java(0 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/controllers/ApplicationControllerTest.java(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/ServerSchemaMigrationEnforcerTest.java(5 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/PartialImportServiceTest.java(7 hunks)
💤 Files with no reviewable changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceImpl.java
🔇 Additional comments (11)
app/server/appsmith-server/src/test/java/com/appsmith/server/controllers/ApplicationControllerTest.java (3)
24-24: LGTM!The new imports are necessary for the updated test method stubs and follow Java conventions.
Also applies to: 33-33
74-74: LGTM!The change to use
anyString()makes the test more type-safe and aligns with the new service method signature that accepts JSON content as a String input.
105-105: LGTM!The change to use
any(Part.class)makes the test more type-safe while maintaining coverage for the legacy Part-based input path.app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/PartialImportServiceTest.java (2)
40-40: LGTM! Required imports added for string-based resource handling.The new imports support the transition from file-based to string-based resource handling.
Also applies to: 49-49, 51-51, 56-56
275-276: LGTM! Test methods updated to use string-based JSON content.The changes align with the PR objective of supporting String inputs for import service methods. The test cases now:
- Read JSON content directly from resource files
- Pass JSON strings to the import methods
- Maintain the same verification logic
Also applies to: 278-278, 327-328, 332-332, 389-390, 392-394, 444-446, 451-452
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCE.java (1)
14-15:❓ Verification inconclusive
Verify parameter validation in implementation.
Ensure that the implementation validates the
fileContentsparameter for valid JSON format and required structure.
🏁 Script executed:
#!/bin/bash # Search for the implementation class to verify parameter validation ast-grep --pattern 'class $_ implements PartialImportServiceCE { $$$ importResourceInPage($_, $_, $_, $_, String $_) { $$$ } $$$ }'Length of output: 145
Action Required: Please confirm that the implementation of
importResourceInPageproperly validates thefileContentsparameter to ensure it is valid JSON and adheres to the required structure. Our initial automated search did not yield conclusive evidence regarding parameter validation in the implementing class(es) ofPartialImportServiceCE. It is strongly recommended to manually verify the validation logic in the implementation to ensure robustness.app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCE.java (2)
42-44: LGTM! Well-structured interface additions.The new methods provide a clean abstraction for string-based imports while maintaining consistency with existing patterns.
57-67: LGTM! Well-documented method overload.The overloaded method maintains API consistency while adding string input support.
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java (1)
163-168: LGTM! Clean implementation reusing existing logic.Good use of composition to maintain DRY principle while adding string input support.
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java (1)
108-114: LGTM! Clean delegation to string-based implementation.Good separation of concerns while maintaining existing functionality.
app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (1)
862-863: LGTM! Test updated to use string input instead of FilePart.The test has been simplified by removing the FilePart mock and using a string argument directly, which aligns with the updated method signature.
.../appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java
Show resolved
Hide resolved
|
While I'm approving this PR, I'm concerned about the performance impact of this change. Generally, using FilePart is more memory efficient than converting & reading the whole file as a string into memory. Before merging, my suggestion would be to experiment with uploading a large JSON file in the import. |
|
@mohanarpit, good point. And in that sense, I'm actually not "changing" anything here. The The actual process isn't changing. I now see how this impression came up. I forgot to remove that old method doing the same thing. I've done that now. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCE.java (2)
34-34: Add documentation for readFilePartToString method.Please add Javadoc to describe the method's purpose, parameters, and return value.
+ /** + * Reads the contents of a file part and converts it to a string. + * + * @param file The file part to be read + * @return A Mono containing the file contents as a string + */ Mono<String> readFilePartToString(Part file);
36-36: Add documentation for extractArtifactExchangeJson method.Please add Javadoc to describe the method's purpose, parameters, and return value.
+ /** + * Extracts an ArtifactExchangeJson object from a JSON string. + * + * @param jsonString The JSON string to parse + * @return A Mono containing the parsed ArtifactExchangeJson + */ Mono<? extends ArtifactExchangeJson> extractArtifactExchangeJson(String jsonString);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java
🧰 Additional context used
🧠 Learnings (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCE.java (1)
Learnt from: sharat87
PR: appsmithorg/appsmith#39337
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:154-160
Timestamp: 2025-02-18T15:18:20.927Z
Learning: Specific JSON parsing error handling using try-catch blocks isn't needed in the ImportServiceCEImpl's extractArtifactExchangeJson method as Mono.fromCallable provides adequate error handling.
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java (1)
Learnt from: sharat87
PR: appsmithorg/appsmith#39337
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:154-160
Timestamp: 2025-02-18T15:18:20.927Z
Learning: Specific JSON parsing error handling using try-catch blocks isn't needed in the ImportServiceCEImpl's extractArtifactExchangeJson method as Mono.fromCallable provides adequate error handling.
⏰ 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 (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCE.java (1)
49-59: LGTM! Well-documented overloaded method.The new overloaded method is well-documented and maintains consistency with the existing interface patterns.
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java (4)
103-117: Well-implemented file content reading logic!The method correctly handles content type validation and properly manages data buffer resources.
125-132: Clean implementation following previous feedback!The method correctly uses
Mono.fromCallablefor JSON parsing, which provides adequate error handling as discussed in previous reviews.
134-140: Successfully implements String input support!The method elegantly chains the new string-based processing while maintaining compatibility with file-based inputs, fulfilling the PR's objective.
103-117:❓ Verification inconclusive
Verify performance with large JSON files.
Based on the PR discussion about performance implications, it would be prudent to verify the memory efficiency of the string-based approach with large JSON files.
Consider testing with a large JSON file (e.g., >10MB) to ensure acceptable memory usage:
Also applies to: 134-140
🏁 Script executed:
#!/bin/bash # Generate a large JSON file and test memory usage python3 -c ' import json import sys # Generate a large JSON object large_json = { "items": [{"id": i, "value": "x" * 1000} for i in range(10000)] } json.dump(large_json, sys.stdout) ' > large_test.json # Find and execute tests that use readFilePartToString rg -l "readFilePartToString" | grep "Test" | xargs -I {} echo "Found test file: {}"Length of output: 275
Verify performance with large JSON files.
The current implementation reads the full file content into memory, which might lead to memory inefficiencies with large JSON files (e.g., >10MB). The automated script did not identify any test files targeting this scenario, so please ensure this case is manually verified. Consider adding dedicated performance tests if this functionality will handle large files in production.
Description
This is so that these service methods can be used by other services for higher-order functionality. In such cases, passing the JSON content as a
Stringis much more convenient than to build aPartimplementation instance.Automation
/test sanity ImportExport
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13440118520
Commit: e382050
Cypress dashboard.
Tags:
@tag.Sanity, @tag.ImportExportSpec:
Thu, 20 Feb 2025 17:15:00 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
These updates enhance the flexibility and robustness of our import functionality, providing a smoother and more intuitive experience when working with artifacts and resources.