use POST for file download to avoid url length limits#6957
use POST for file download to avoid url length limits#6957kcondon merged 2 commits intoIQSS:developfrom
Conversation
|
Thanks @qqmyers we'll take a look. As you mentioned, those failing tests are failing on develop, so makes sense they are failing here as well. |
pdurbin
left a comment
pdurbin
left a comment
There was a problem hiding this comment.
I left a few comments about docs and error handling. I deployed the code and it seemed to work fine on two files.
| @Produces({ "application/zip" }) | ||
| public Response postDownloadDatafiles(String fileIds, @QueryParam("gbrecs") boolean gbrecs, @QueryParam("key") String apiTokenParam, @Context UriInfo uriInfo, @Context HttpHeaders headers, @Context HttpServletResponse response) throws WebApplicationException { | ||
|
|
||
| fileIds = fileIds.substring(8); // String "fileIds=" from the front |
There was a problem hiding this comment.
I was playing with this using curl and got a NullPointerException when fileIds was null (and a 500 error). Some error checking would be nice.
There was a problem hiding this comment.
There is error checking. I just defeated it by taking a substring of null before the error check is done in downloadDataFiles() :-) Will fix.
| // TODO: Rather than only supporting looking up files by their database IDs, | ||
| // consider supporting persistent identifiers. | ||
| @Path("datafiles") | ||
| @POST |
There was a problem hiding this comment.
I think this API should be documented along side the other ones in the Data Access API page of the API Guide. I can imagine the same error that was reported in #6943 could easily happen to API users.
There was a problem hiding this comment.
Added - feel free to change.
| } catch (IOException ex) { | ||
| logger.info("Failed to issue a redirect to file download url."); | ||
| } | ||
| PrimeFaces.current().executeScript("downloadFiles('"+fileDownloadUrl + "','"+ multiFileString+"');"); |
There was a problem hiding this comment.
This is just a comment but I assume that PrimeFaces is being used here because there's no JSF standard way (FacesContext way) to execute a script.
There was a problem hiding this comment.
I'm not certain but this is done elsewhere and I assume PF needed this method for some reason. I didn't find anything else with a quick web check.
5 participants
What this PR does / why we need it: uses POST for file download requests, avoiding problems with limits on the url length for requests for large numbers of files
Which issue(s) this PR closes:
Closes #6943
Special notes for your reviewer: We've used this on QDR since last May, but I just tried to compare with that fork to pull things over - haven't tested this commit.
Suggestions on how to test this: Nominally, if any download works, this is working. To verify that it's better than the prior code, you'll need to have enough files to have caused trouble. That was 1061 in the issue but I'd suggest a larger number for a test or make sure your test DB uses ids with the same number of digits because the issue is line length and ids in production will be longer than on a new test machine starting with id 1.
Does this PR introduce a user interface change? If mockups are available, please link/include them here: No
Is there a release notes update needed for this change?: No
Additional documentation: