SAK-52189 Resources Error when uploading a file equal to the maximum size #14263
Conversation
WalkthroughUpdated upload size validation in two Java classes to account for multipart encoding overhead by adding a 64KB margin to size limits and adjusting limit comparisons and where the margin is applied. Changes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
content/content-tool/tool/src/java/org/sakaiproject/content/tool/ResourcesHelperAction.java(1 hunks)kernel/api/src/main/java/org/sakaiproject/util/RequestFilter.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
content/content-tool/tool/src/java/org/sakaiproject/content/tool/ResourcesHelperAction.javakernel/api/src/main/java/org/sakaiproject/util/RequestFilter.java
🧬 Code graph analysis (2)
content/content-tool/tool/src/java/org/sakaiproject/content/tool/ResourcesHelperAction.java (1)
library/src/webapp/js/postem-dropzone.js (1)
uploadMax(26-26)
kernel/api/src/main/java/org/sakaiproject/util/RequestFilter.java (1)
library/src/webapp/js/postem-dropzone.js (1)
uploadMax(26-26)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
🔇 Additional comments (2)
kernel/api/src/main/java/org/sakaiproject/util/RequestFilter.java (1)
971-978: Margin addition to upload limits correctly handles multipart overhead.The 64KB margin added to both
setSizeMaxandsetFileSizeMaxallows Apache Commons FileUpload to accept requests where the content-length equals the configured limit plus multipart overhead. This aligns with the PR objective of allowing files at exactly the maximum size.content/content-tool/tool/src/java/org/sakaiproject/content/tool/ResourcesHelperAction.java (1)
1942-1948: The original review comment's logic is sound, but repository access is unavailable for direct verification.The web search confirms that
FileItem.getSize()returns the file content size without multipart overhead (boundaries, headers, form fields). This validates the original review's core concern:
- In ResourcesHelperAction.java:
fileSizeincludes multipart overhead (from Content-Length header), so the comparisonfileSize > (uploadMaxBytes + margin)correctly allows files at exactly the configured limit.- In RequestFilter.java:
item.getSize()does not include overhead, so using>=would incorrectly reject files at exactly the configured limit.This represents a genuine inconsistency that contradicts the PR objective. However, I cannot directly verify the code in RequestFilter.java (line 1028) due to repository access issues.
kernel/api/src/main/java/org/sakaiproject/util/RequestFilter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
kernel/api/src/main/java/org/sakaiproject/util/RequestFilter.java (1)
971-979: Margin logic is reasonable, but guard uploadMax and align per‑file exception handlingThe 64KB buffer to compensate for multipart overhead and Commons’
>=semantics makes sense and should address the “file exactly at the limit” failures. Two follow‑ups would tighten this:
Avoid clamping “unlimited” configs to 64KB
uploadMaxis initialised to-1and only set whenm_uploadMaxSize > 0(or a valid override is present). If someone configuresm_uploadMaxSize <= 0to mean “no limit”, this block will currently enforce64KBinstead of “unlimited”:long margin = 64L * 1024L; // 64KB margin for multipart overhead upload.setSizeMax(uploadMax + margin); upload.setFileSizeMax(uploadMax + margin);Consider guarding with
if (uploadMax > 0)so you keep the previous “no limit” behavior whenuploadMaxis negative:- long margin = 64L * 1024L; // 64KB margin for multipart overhead - - upload.setSizeMax(uploadMax + margin); - upload.setFileSizeMax(uploadMax + margin); + if (uploadMax > 0) { + long margin = 64L * 1024L; // 64KB margin for multipart overhead + + upload.setSizeMax(uploadMax + margin); + upload.setFileSizeMax(uploadMax + margin); + }Now that
fileSizeMaxis set, surface per‑file limit breaches consistently
Withupload.setFileSizeMax(uploadMax + margin)active, Commons FileUpload can throwFileUploadBase.FileSizeLimitExceededExceptionfor per‑file overages. Right now that will fall into the genericFileUploadExceptioncatch block and be reported as"exception"rather than"size_limit_exceeded", unlike the existingSizeLimitExceededExceptionhandling.It would be good to add a dedicated catch for
FileUploadBase.FileSizeLimitExceededExceptionthat mirrors theSizeLimitExceededExceptionpath (set"upload.status"to"size_limit_exceeded", populate"upload.exception"and"upload.limit"), so tools see a consistent limit‑exceeded signal instead of a generic parsing error. This also matches the existing TODO comment just below the current catch blocks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
kernel/api/src/main/java/org/sakaiproject/util/RequestFilter.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
kernel/api/src/main/java/org/sakaiproject/util/RequestFilter.java
🧬 Code graph analysis (1)
kernel/api/src/main/java/org/sakaiproject/util/RequestFilter.java (1)
library/src/webapp/js/postem-dropzone.js (1)
uploadMax(26-26)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
1 participant
https://sakaiproject.atlassian.net/browse/SAK-52189
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.