UFAL/Zip download missing content length header - #1028
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related issues
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java (1)
120-121: Remove duplicate header setting.The Content-Disposition and Content-Type headers are set twice - once before ZIP creation (lines 120-121) and again after (lines 149-151). The first setting is unnecessary and should be removed.
- response.setHeader(HttpHeaders.CONTENT_DISPOSITION, String.format("attachment;filename=\"%s\"", name)); - response.setContentType("application/zip"); List<Bundle> bundles = item.getBundles("ORIGINAL");Also applies to: 149-151
🧹 Nitpick comments (1)
dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java (1)
86-101: Comprehensive header validation addresses PR requirements.The test effectively validates:
- Content-Length header presence and correctness
- Content-Type header starts with "application/zip"
- Content-Disposition header starts with "attachment"
This directly tests the PR objective of ensuring Content-Length header is included.
However, consider adding a basic validation that the response contains actual ZIP content to maintain some content integrity testing.
+ // Basic validation that response contains ZIP content + assertTrue("Response should contain ZIP content", responseBytes.length > 0); + // Verify ZIP magic number (first 4 bytes should be ZIP signature) + assertTrue("Response should start with ZIP signature", + responseBytes.length >= 4 && + responseBytes[0] == 0x50 && responseBytes[1] == 0x4B);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java(3 hunks)dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: dspace-dependencies / docker-build (linux/amd64, ubuntu-latest, true)
- GitHub Check: Run Integration Tests
- GitHub Check: Run Unit Tests
🔇 Additional comments (9)
dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java (5)
12-12: LGTM! New imports are appropriate.The added imports
ByteArrayOutputStreamandOutputStreamare necessary for the new buffering approach to set the Content-Length header correctly.Also applies to: 15-15
153-156: Proper resource management with try-with-resources.Good use of try-with-resources to ensure the output stream is properly closed. The explicit flush is also appropriate to ensure all data is sent to the client.
151-151: Content-Length header successfully added.The addition of
response.setContentLength(zipBytes.length)successfully addresses the PR objective by providing the missing Content-Length header that browsers need for download progress indication.
12-12: LGTM: Appropriate imports added.The new imports
ByteArrayOutputStreamandOutputStreamare correctly added to support the buffering approach for ZIP generation.Also applies to: 15-15
124-125: Good approach to enable Content-Length header setting.Using
ByteArrayOutputStreamto buffer the ZIP content allows determining the exact size before writing to the response, which is necessary for setting the Content-Length header.dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java (4)
10-10: LGTM! Appropriate test imports added.The new imports support the refactored test structure -
assertNotNullandassertTruefor header validation, andMvcResultfor capturing response details.Also applies to: 30-30, 32-32
93-94: Excellent validation of Content-Length header.The test properly validates that the Content-Length header is present and matches the actual response byte array length, which directly addresses the PR objective.
10-10: LGTM: Appropriate test imports added.The new imports support the updated test approach using
MvcResultfor header validation.Also applies to: 30-30, 32-32
81-84: Good refactor to capture response for header validation.Using
MvcResultallows access to response headers and content, which is essential for validating the Content-Length header fix.
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| ZipArchiveOutputStream zip = new ZipArchiveOutputStream(baos); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Memory usage consideration for large ZIP files.
The change from streaming directly to response to buffering in ByteArrayOutputStream will consume memory proportional to the ZIP file size. For items with many or large bitstreams, this could lead to memory issues.
Consider implementing a size limit or streaming approach with chunked encoding as an alternative for large archives.
Add a configuration property to limit ZIP size or implement chunked transfer encoding:
+// Add size validation before creating ZIP
+long estimatedSize = bundles.stream()
+ .flatMap(b -> b.getBitstreams().stream())
+ .mapToLong(Bitstream::getSizeBytes)
+ .sum();
+
+if (estimatedSize > configurationService.getLongProperty("zip.max.size", 100 * 1024 * 1024)) {
+ throw new UnprocessableEntityException("ZIP archive too large");
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | |
| ZipArchiveOutputStream zip = new ZipArchiveOutputStream(baos); | |
| // Add size validation before creating ZIP | |
| long estimatedSize = bundles.stream() | |
| .flatMap(b -> b.getBitstreams().stream()) | |
| .mapToLong(Bitstream::getSizeBytes) | |
| .sum(); | |
| if (estimatedSize > configurationService.getLongProperty("zip.max.size", 100 * 1024 * 1024)) { | |
| throw new UnprocessableEntityException("ZIP archive too large"); | |
| } | |
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | |
| ZipArchiveOutputStream zip = new ZipArchiveOutputStream(baos); |
🤖 Prompt for AI Agents
In
dspace-server-webapp/src/main/java/org/dspace/app/rest/MetadataBitstreamController.java
around lines 124 to 125, the current code buffers the entire ZIP output in a
ByteArrayOutputStream before sending it, which can cause high memory usage for
large ZIP files. To fix this, refactor the code to stream the ZIP output
directly to the HTTP response output stream using chunked transfer encoding,
avoiding full buffering in memory. Additionally, add a configuration property to
set a maximum allowed ZIP size and enforce this limit during streaming to
prevent excessive memory consumption.
There was a problem hiding this comment.
@milanmajchrak May I be sure that this situation will never happen? Because the button is not generating when there is too much files....
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
This is actually really interesting review comment, because indeed the Download ZIP button is showed up only when the files are between allowed files size, BUT we can download all files as a single zip using the CURL command, so we should restrict it, just check the download.all.limit.max.file.size property
There was a problem hiding this comment.
I asked OK in the Slack https://dspace-org.slack.com/archives/C03JD6V3UUA/p1754059084284639
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a missing Content-Length header issue when downloading ZIP files. The problem was that ZIP files were being streamed directly to the response without setting the Content-Length header, preventing browsers from showing download progress and file size information.
- Refactored ZIP generation to use ByteArrayOutputStream for content length calculation
- Added Content-Length header to ZIP download responses
- Updated tests to validate HTTP headers and ZIP content structure
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| MetadataBitstreamController.java | Modified ZIP generation to buffer content in memory and set Content-Length header |
| MetadataBitstreamControllerIT.java | Enhanced test to verify HTTP headers and ZIP structure instead of byte comparison |
Comments suppressed due to low confidence (1)
dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java:109
- The test hardcodes the expected ZIP entry name as "Bitstream" but doesn't verify this matches the actual bitstream name from the test setup. This could lead to false positives if the ZIP generation logic changes.
assertEquals("Bitstream", entry.getName());
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| ZipArchiveOutputStream zip = new ZipArchiveOutputStream(baos); |
There was a problem hiding this comment.
This is actually really interesting review comment, because indeed the Download ZIP button is showed up only when the files are between allowed files size, BUT we can download all files as a single zip using the CURL command, so we should restrict it, just check the download.all.limit.max.file.size property
Problem description
When downloading ZIP files, the missing Content-Length header prevents the browser from knowing the file size and download time.
Summary by CodeRabbit
Bug Fixes
Tests