fix: file upload fixed when using Base64 type in FilePicker and multipart/form-data in REST API #39332
Conversation
…sue-39227/file-upload-base64-multipart
WalkthroughThis PR enhances multipart form data handling in the Changes
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java (1)
352-362: Switch to UTF-8 for broader compatibility.
Using ISO-8859-1 could limit support for extended characters. Adopting UTF-8 helps accommodate a wider range of text inputs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java(2 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 (8)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java (8)
44-44: The import is used as intended.
This import statement forCollectionsis valid and utilized inparseMultipartData.
303-321: Robust fallback handling for base64 vs. JSON.
This logic correctly branches between Base64 processing and JSON-based multipart parsing, ensuring comprehensive coverage of file inputs.
323-327: Convenient default overload.
Providing a simplified overload with default filename and MIME type is a neat approach to reduce code duplication.
329-350: Consider catching invalid Base64 data gracefully.
Although the current approach fails fast, gracefully handling potential decoding errors (e.g., malformed strings) might offer clearer error messages and debugging hints.
364-375: Straightforward metadata parsing.
Extracting the MIME type from the substring is concise and readable.
377-388: Good streaming approach for large files.
Reading bytes into aFlux<DataBuffer>is a sensible pattern for handling potentially large payloads.
390-401: Clear JSON parsing logic.
This approach neatly differentiates single objects from arrays while providing an informative exception if content is invalid.
403-415: Sufficient initial MIME-to-extension map.
Fallback to an empty extension is a useful default; consider expanding mappings for broader file types as needed.
github-actions
bot
added
Integrations Pod General
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13391123950. |
|
Deploy-Preview-URL: https://ce-39332.dp.appsmith.com |
|
@sneha122 @appsmithorg/query-widgets-pod I have tested the file upload with the following types of files and it works fine. |
...h-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java
Outdated
Show resolved
Hide resolved
...h-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java
Outdated
Show resolved
Hide resolved
...h-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java (7)
325-351: LGTM! Well-structured base64 processing with proper error handling.The base64 processing methods are well-organized with clear separation of concerns. The overloaded method provides good flexibility for custom filenames and MIME types.
Consider adding a brief comment explaining the format of
fileValueparameter in the base method, similar to how you documented themetadataPartformat.
384-395: Consider making the buffer size configurable.The buffer size of 4096 bytes is hardcoded in the
addPartToBodymethod. Consider making this configurable to allow tuning for different file sizes and memory constraints.+ private static final int DEFAULT_BUFFER_SIZE = 4096; + private void addPartToBody( MultipartBodyBuilder bodyBuilder, String key, byte[] bytes, ClientHttpRequest outputMessage, String filename, String mimeType) { Flux<DataBuffer> data = DataBufferUtils.readInputStream( - () -> new ByteArrayInputStream(bytes), outputMessage.bufferFactory(), 4096); + () -> new ByteArrayInputStream(bytes), outputMessage.bufferFactory(), DEFAULT_BUFFER_SIZE); bodyBuilder.asyncPart(key, data, DataBuffer.class).filename(filename).contentType(MediaType.valueOf(mimeType)); }
372-382: Add input validation for MIME type extraction.While the MIME type extraction works well, consider adding validation to ensure the extracted MIME type follows the correct format (type/subtype).
private String extractMimeType(String metadataPart, String defaultMimeType) { + if (metadataPart == null || metadataPart.isEmpty()) { + return defaultMimeType; + } + if (metadataPart.startsWith("data:")) { int endIndex = metadataPart.indexOf(';'); if (endIndex > 5) { - return metadataPart.substring(5, endIndex); + String mimeType = metadataPart.substring(5, endIndex); + return mimeType.matches("^\\w+/[-+.\\w]+$") ? mimeType : defaultMimeType; } else { - return metadataPart.substring(5); + String mimeType = metadataPart.substring(5); + return mimeType.matches("^\\w+/[-+.\\w]+$") ? mimeType : defaultMimeType; } } return defaultMimeType; }
326-351: Add validation for base64 content.Consider adding validation for the base64 content to handle malformed input gracefully.
private void processBase64Data( String fileValue, String key, MultipartBodyBuilder bodyBuilder, ClientHttpRequest outputMessage, String filename, String defaultMimeType) { String[] parts = fileValue.split(BASE64_DELIMITER, 2); if (parts.length != 2) { throw new AppsmithPluginException( AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, ERROR_INVALID_BASE64_FORMAT); } String metadataPart = parts[0]; String base64Content = parts[1]; + // Validate base64 content + if (!base64Content.matches("^[A-Za-z0-9+/]*={0,2}$")) { + throw new AppsmithPluginException( + AppsmithPluginError.PLUGIN_DATASOURCE_ARGUMENT_ERROR, + "Invalid base64 content format"); + } String mimeType = extractMimeType(metadataPart, defaultMimeType); byte[] decodedBytes = Base64.getMimeDecoder().decode(base64Content); addPartToBody(bodyBuilder, key, decodedBytes, outputMessage, filename, mimeType); }
354-363: Consider using configurable character encoding.The method currently uses ISO_8859_1 encoding. Consider making the encoding configurable to support different character sets.
private void processRegularData( String data, String key, MultipartBodyBuilder bodyBuilder, ClientHttpRequest outputMessage, String filename, - String mimeType) { - byte[] bytes = data.getBytes(StandardCharsets.ISO_8859_1); + String mimeType, + Charset charset) { + byte[] bytes = data.getBytes(charset != null ? charset : StandardCharsets.ISO_8859_1); addPartToBody(bodyBuilder, key, bytes, outputMessage, filename, mimeType); }
303-323: Add method documentation for clarity.The method handles both base64 and regular multipart data processing. Consider adding Javadoc to explain:
- The expected format of input data
- The difference between base64 and regular data handling
- The purpose of each processing path
325-351: Add unit tests for base64 processing.The base64 processing methods handle various scenarios but lack test coverage. Consider adding tests for:
- Valid base64 data with different MIME types
- Invalid base64 format
- Edge cases with missing or malformed metadata
Would you like me to help generate unit test cases for these scenarios?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/exceptions/pluginExceptions/BasePluginErrorMessages.java(1 hunks)app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java(3 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/exceptions/pluginExceptions/BasePluginErrorMessages.java
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / ci-test / ci-test (19)
- GitHub Check: perform-test / ci-test / ci-test (15)
- GitHub Check: perform-test / ci-test / ci-test (13)
- GitHub Check: perform-test / ci-test / ci-test (12)
- GitHub Check: perform-test / ci-test / ci-test (10)
- GitHub Check: perform-test / ci-test / ci-test (7)
- GitHub Check: perform-test / ci-test / ci-test (6)
- GitHub Check: perform-test / ci-test / ci-test (5)
- GitHub Check: perform-test / ci-test / ci-test (3)
- GitHub Check: perform-test / ci-test / ci-test (0)
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (9)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/exceptions/pluginExceptions/BasePluginErrorMessages.java (3)
24-27: LGTM! Clear and descriptive error messages.The new error message constants follow the existing pattern and provide clear feedback for validation failures.
24-27: LGTM! Clear and descriptive error messages.The new error message constants provide clear guidance about the expected formats for multipart data and base64 content.
24-27: LGTM! Clear and descriptive error messages.The new error message constants follow the existing pattern and provide clear guidance for error scenarios.
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/DataUtils.java (6)
51-52: LGTM! Proper error message imports.The error messages are imported statically, following Java best practices.
51-52: LGTM! Consistent error handling with descriptive messages.The error handling is well-implemented using the new constants, providing clear feedback for invalid formats.
Also applies to: 238-239, 341-343, 404-406
365-395: LGTM! Well-documented and reusable helper methods.The helper methods are well-structured with clear documentation and examples. The buffer size of 4096 bytes in
addPartToBodyis a good choice for streaming data.
44-44: LGTM! Necessary imports added.The new imports support the enhanced multipart form data handling.
Also applies to: 51-52
365-382: LGTM! Well-documented MIME type extraction.The method is well-documented with examples and handles edge cases gracefully by falling back to the default MIME type.
397-407: LGTM! Improved error handling and return type.The method now:
- Returns a strongly-typed List
- Provides specific error messages for invalid data
- Handles both single object and array cases efficiently
NilanshBansal
deleted the
fix/issue-39227/file-upload-base64-multipart
branch
|
@NilanshBansal Let's create a separate task and add test cases for this functionality as discussed |
github-actions
bot
added
QA
Description
From the table here, this issue points to the 3rd one in the list.
Fixes #39227
Fixes #38177
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13392351852
Commit: c731c65
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 18 Feb 2025 14:36:12 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor