Optionally include Content-Disposition header in statement results API response#17827
Optionally include Content-Disposition header in statement results API response#17827jgoz wants to merge 2 commits intoapache:masterfrom
Conversation
github-actions
Bot
added
Area - Documentation
Area - Batch Ingestion
Area - Querying
Area - MSQ
vogievetsky
added
the
Needs web console change
| ); | ||
| throwIfQueryIsNotSuccessful(queryId, statusPlus); | ||
|
|
||
| final String contentDispositionHeaderValue = filename != null ? StringUtils.format("attachment; filename=%s", filename) : null; |
There was a problem hiding this comment.
there should be some validation of filename, such as:
- maximum length
- allowed characters
- anything else that may make sense given how it's expected to be used
as to "allowed characters", please check into what happens if ; or whitespace is provided in the filename?
| if (!signature.isPresent() || MSQControllerTask.isIngestion(msqControllerTask.getQuerySpec())) { | ||
| // Since it's not a select query, nothing to return. | ||
| return Response.ok().build(); | ||
| final Response.ResponseBuilder responseBuilder = Response.ok(); |
There was a problem hiding this comment.
please factor this into a method so it doesn't need to be repeated three times. the signature of the method could be Response.ResponseBuilder addContentDisposition(Response.ResponseBuilder, String)
| * Defines the format in which the results are presented. The following options are supported `arrayLines`,`objectLines`,`array`,`object`, and `csv`. The default is `object`. | ||
| * `filename` (optional) | ||
| * Type: String | ||
| * If set, attaches a `Content-Disposition` header to the response with the value of `attachment; filename={filename}`. |
There was a problem hiding this comment.
once a max length and allowed characters are implemented, document them here.
|
Following Gian's comments I think it makes sense to restrict the filename to 255 chars and to ensure that it does not contain |
|
inclided in #17840 |
|
I had the comments addressed in a commit, but was waiting to go over them with @vogievetsky before pushing. They were nearly identical to the changes that ended up being merged, so that is probably good :) Anyway, thank you for taking this over! |
4 participants
Description
Adds support for an optional
filenamequery parameter to the/druid/v2/sql/statements/{queryId}/resultsAPI. When provided, the response will include a headerContent-Disposition: attachment; filename={filename}, which will instruct a web browser to save the response as a file rather than displaying it inline.This save-as-attachment behavior could be achieved by adding a "download" attribute to the results link, but this only works for same-origin URLs (as in the Web Console). If the UI origin is different from the Druid API origin, browsers will ignore the attribute and serve the results inline, which is poor UX for files that are potentially very large.
For the sake of consistency, all successful responses in
SqlStatementResource.doGetResultsmay include this header, even if there are no results.Release note
Improved: The "Get query results" statements API supports an optional
filenamequery parameter. When provided, the response will instruct web browsers to save the results as a file instead of showing them inline (via theContent-Dispositionheader).Key changed/added classes in this PR
SqlStatementResourceThis PR has: