Iqss/8726 better HTTP range request support #8727
Conversation
Version 5.10
| if (headers.getRequestHeaders().containsKey("Range")) { | ||
| return Response.status(Response.Status.PARTIAL_CONTENT).entity(downloadInstance).build(); | ||
| } | ||
| return Response.ok(downloadInstance).build(); |
There was a problem hiding this comment.
Could you please clarify what this change does?
Specifically, changing the return value from DownloadInstance to Response - are you positive that it's still going to stream the output? (As opposed to trying to read everything from the DownloadInstance into memory at once, and then build the Response?)
There was a problem hiding this comment.
Let's start with the requirement:
If the request contains a "Range" header and the requested range of bytes is returned, a HTTP status of 206 (Partial Content) instead of 200 must be returned. See also
https://datatracker.ietf.org/doc/html/rfc2616#section-14.35.2
So, these lines change the status code of the HTTP response dependent on the existence of the Range header in the HTTP request. The only way to change it is by using the Response object. (Setting it directly in the HttpServletResponse would be overwritten by Jersey later).
However, there is no real difference between returning the entity (DownloadInstance) directly, or wrapped in a Response object. Except that Response allows more control over settings like the status code. The custom MessageBodyWriter for DownloadInstance is still called, and that is where the streaming happens.
There was a problem hiding this comment.
Thanks. For the record, this is very old code. Written for a very early version of Jersey.
We have an open issue for refactoring that download subsystem. But it's going to be a project, with years worth of special case handling added there.
|
Sorry, I rushed to move this into QA. |
|
Uh, sorry, didn't have the integration tests on my mind. I'll have a look at them. |
|
I'm not aware of any external tools that this would break, no. This native range support is a relatively new addition; and even if anyone is already relying on it, the change would be trivial on their end. But this makes me realize - it would be proper style to announce this change, even if it's not likely to affect many people. Our normal procedure for having something included in the next release notes is to add your PR-specific note as a file under doc/release-notes (please see the notes already there for an example). Just one sentence - the return code is changing, the extra headers are being added, per the http spec... And, uh, better late than never: thank you for contributing to the project! 😄 |
|
Thanks. I've added notes for this PR under doc/release-notes. FWIW: We just went productive with the ZIP Previewer, which relies on these changes, see e.g. here: https://edmond.mpdl.mpg.de/file.xhtml?fileId=182288 |
Very nice, I like it a lot! I'll be looking forward to having it available @ GDCC. |
|
Hi @haarli would you mind refreshing this branch from develop? We've since released a new version and the version numbers have incremented. Regards, Kevin |
This PR addresses #8726 with the following changes:
[from L.A.:]
A quick note on how/what to test/QA:
A call to file download api with the Range request should now return 206 instead of 200; and the extra headers as described above.
May make sense to confirm that regular downloads (without the range header) are not affected and everything is working as before.
This would be best to test on locally-stored files; or, if testing with s3 storage, make sure to disable the direct download option. (otherwise the return codes and the headers you're seeing are not ours, but Jeff Bezos's).
(sorry if the above is all stating the obvious)