fix: Amazon S3 large file uploads issue fixed #36584
Conversation
WalkthroughThe changes in this pull request involve enhancements to the Changes
Assessment against linked issues
Suggested reviewers
Poem
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
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11066862934. |
github-actions
bot
added
File upload issues
|
Deploy-Preview-URL: https://ce-36584.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11100778973. |
|
Deploy-Preview-URL: https://ce-36584.dp.appsmith.com |
![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
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-interfaces/src/main/java/com/appsmith/util/SerializationUtils.java (1)
78-90: Excellent work on this new method, students!This new addition to our
SerializationUtilsclass is like upgrading our school library to accommodate larger books. It's a fantastic solution to our file size limitation problem.A few points to note:
- The method name clearly describes its purpose. Good job!
- The comments explain the reasoning behind allowing maximum length. Remember, clear documentation is key to good coding practices.
- Setting
maxStringLengthtoInteger.MAX_VALUEis a bold move. It's like removing the size limit on book reports. While it solves our immediate problem, we need to be cautious.Consider adding a configurable parameter for
maxStringLengthinstead of usingInteger.MAX_VALUEdirectly. This would be like having an adjustable bookshelf that we can resize as needed. Here's an example:- public static ObjectMapper getObjectMapperWithSourceInLocationAndMaxStringLengthEnabled() { + public static ObjectMapper getObjectMapperWithSourceInLocationAndMaxStringLengthEnabled(int maxStringLength) { ObjectMapper objectMapper = new ObjectMapper().enable(StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION.mappedFeature()); // Multipart data would be parsed using object mapper, these files may be large in the size. // Hence, the length should not be truncated, therefore allowing maximum length. objectMapper .getFactory() .setStreamReadConstraints(StreamReadConstraints.builder() - .maxStringLength(Integer.MAX_VALUE) + .maxStringLength(maxStringLength) .build()); return objectMapper; }This way, we can adjust the maximum string length as needed, providing more flexibility and control over resource usage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/BasePlugin.java (1 hunks)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/util/SerializationUtils.java (2 hunks)
🔇 Additional comments (3)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/plugins/BasePlugin.java (1)
10-11: Class, pay attention to this important change!The modification in the
objectMapperinitialization is a significant improvement. Let's break it down:
The new method
getObjectMapperWithSourceInLocationAndMaxStringLengthEnabled()retains the previous functionality of source location while adding support for maximum string length.This change directly addresses the issue reported in [Bug]: File upload to S3 fails for certain files #35601, where users encountered errors when uploading files larger than 14MB due to string value length limitations.
By enabling max string length handling, we're allowing our students (users) to upload larger files to Amazon S3 without running into those pesky errors. This is a valuable enhancement to our system's capabilities.
However, class, we must consider a few things:
Performance: Handling very large strings might have some impact on performance. We should keep an eye on this and perhaps conduct some benchmarking tests.
Testing: While the PR mentions that Cypress tests have passed, we should ensure thorough testing with various file sizes to confirm the fix works as expected.
Documentation: It would be beneficial to update our user documentation to reflect this new capability.
Let's run a quick check to see if this change is consistent across our codebase:
Class, these tests will help us ensure consistency and verify the implementation. Any questions?
app/server/appsmith-interfaces/src/main/java/com/appsmith/util/SerializationUtils.java (2)
9-9: Class, pay attention to this new import!The addition of
com.fasterxml.jackson.core.StreamReadConstraintsis a crucial step in our lesson plan. It's like bringing a new textbook to class that will help us handle larger amounts of information. Well done!
Line range hint
1-90: Class, let's summarize today's lesson!The changes made to our
SerializationUtilsclass are like upgrading our school's ability to handle larger textbooks. We've successfully addressed the issue of file size limitations, allowing our students (users) to upload larger files to Amazon S3.Key points to remember:
- We've added a new import to bring in the tools we need.
- Our new method,
getObjectMapperWithSourceInLocationAndMaxStringLengthEnabled, is the star of the show. It's like a new librarian who can handle books of any size.- We've documented our work well, which is crucial for future students who might need to understand this code.
Overall, this is a significant improvement that will benefit many of our users. Great job, class!
To ensure we haven't missed anything, let's do a quick pop quiz:
This script will help us ensure that our new method is being used correctly and that we haven't left any old size limits or TODOs lying around. Remember, thorough testing is key to a successful project!
✅ Verification successful
Let's ensure everything is in order!
To double-check our work, let's perform one more quick search to confirm there are no hidden references to the old file size limits.
All verifications passed successfully!
Great job! All checks confirm that the
SerializationUtilsclass has been updated correctly, with no remaining references to the old file size limits. The new method is properly utilized, and there are no lingering TODOs or outdated size constraints in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the new method is used correctly in the codebase # Test 1: Check if the new method is used in the codebase echo "Test 1: Searching for usage of getObjectMapperWithSourceInLocationAndMaxStringLengthEnabled" rg --type java "getObjectMapperWithSourceInLocationAndMaxStringLengthEnabled" # Test 2: Check for any remaining occurrences of the old 14MB limit echo "Test 2: Searching for any remaining 14MB limits" rg --type java "14.*MB" # Test 3: Check for any TODOs related to file size limits echo "Test 3: Searching for TODOs related to file size limits" rg --type java "TODO.*file.*size"Length of output: 927
Script:
#!/bin/bash # Description: Verify no indirect references to the old 14MB file size limit # Test 4: Search for any string mentions of '14MB' in comments or documentation echo "Test 4: Searching for '14MB' in comments or documentation" rg --type java "//.*14MB" rg --type md "14MB"Length of output: 178
|
Tested by uploading a 45 MB file in current prod env, it failed with the following error. Uploading the same file worked on the DP |
|
This is kept on hold as I am verifying whether this change will result in huge eval timing on the frontend, as with this change we are allowing file uploads more than 15mb and we need to ensure that this does not cause any breaking on the frontend evaluations. |
Description
This PR fixes the file upload issue for S3. With S3 file upload, earlier users were able to upload files only upto 14mb but now with this PR fix they can upload larger files.
Fixes #35601
or
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.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11100760284
Commit: 38e0810
Cypress dashboard.
Tags:
@tag.SanitySpec:
Mon, 30 Sep 2024 07:11:48 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit