Skip to content

Optionally include Content-Disposition header in statement results API response #17827

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jgoz wants to merge 2 commits into from
Closed

Optionally include Content-Disposition header in statement results API response #17827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
536f037
Support Content-Distribution in statement result API
jgoz Mar 21, 2025
25a023a
Use StringUtils.format
jgoz Mar 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/api-reference/sql-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1286,6 +1286,9 @@ Getting the query results for an ingestion query returns an empty response.
* `resultFormat` (optional)
* Type: 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}`.
Copy link
Copy Markdown
Contributor

@gianm gianm Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once a max length and allowed characters are implemented, document them here.


#### Responses

Expand Down
30 changes: 26 additions & 4 deletions ...ti-stage-query/src/main/java/org/apache/druid/msq/sql/resources/SqlStatementResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ public class SqlStatementResource
{

public static final String RESULT_FORMAT = "__resultFormat";
public static final String CONTENT_DISPOSITION_RESPONSE_HEADER = "Content-Disposition";
private static final Logger log = new Logger(SqlStatementResource.class);
private final SqlStatementFactory msqSqlStatementFactory;
private final ObjectMapper jsonMapper;
Expand Down Expand Up @@ -277,6 +278,7 @@ public Response doGetResults(
@PathParam("id") final String queryId,
@QueryParam("page") Long page,
@QueryParam("resultFormat") String resultFormat,
@QueryParam("filename") String filename,
@Context final HttpServletRequest req
)
{
Expand Down Expand Up @@ -309,10 +311,18 @@ public Response doGetResults(
);
throwIfQueryIsNotSuccessful(queryId, statusPlus);

final String contentDispositionHeaderValue = filename != null ? StringUtils.format("attachment; filename=%s", filename) : null;
Copy link
Copy Markdown
Contributor

@gianm gianm Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


Optional<List<ColumnNameAndTypes>> signature = SqlStatementResourceHelper.getSignature(msqControllerTask);
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();
Copy link
Copy Markdown
Contributor

@gianm gianm Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


if (contentDispositionHeaderValue != null) {
responseBuilder.header(CONTENT_DISPOSITION_RESPONSE_HEADER, contentDispositionHeaderValue);
}

return responseBuilder.build();
}

// returning results
Expand All @@ -321,18 +331,30 @@ public Response doGetResults(
results = getResultYielder(queryId, page, msqControllerTask, closer);
if (!results.isPresent()) {
// no results, return empty
return Response.ok().build();
final Response.ResponseBuilder responseBuilder = Response.ok();

if (contentDispositionHeaderValue != null) {
responseBuilder.header(CONTENT_DISPOSITION_RESPONSE_HEADER, contentDispositionHeaderValue);
}

return responseBuilder.build();
}

ResultFormat preferredFormat = getPreferredResultFormat(resultFormat, msqControllerTask.getQuerySpec());
return Response.ok((StreamingOutput) outputStream -> resultPusher(
final Response.ResponseBuilder responseBuilder = Response.ok((StreamingOutput) outputStream -> resultPusher(
queryId,
signature,
closer,
results,
new CountingOutputStream(outputStream),
preferredFormat
)).build();
));

if (contentDispositionHeaderValue != null) {
responseBuilder.header(CONTENT_DISPOSITION_RESPONSE_HEADER, contentDispositionHeaderValue);
}

return responseBuilder.build();
}


Expand Down
15 changes: 15 additions & 0 deletions ...ery/src/test/java/org/apache/druid/msq/sql/resources/SqlMSQStatementResourcePostTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,7 @@ public void testWithDurableStorage() throws IOException
sqlStatementResult.getQueryId(),
null,
ResultFormat.OBJECTLINES.name(),
null,
SqlStatementResourceTest.makeOkRequest()
),
objectMapper
Expand All @@ -406,6 +407,7 @@ public void testWithDurableStorage() throws IOException
sqlStatementResult.getQueryId(),
0L,
ResultFormat.OBJECTLINES.name(),
null,
SqlStatementResourceTest.makeOkRequest()
),
objectMapper
Expand All @@ -419,6 +421,7 @@ public void testWithDurableStorage() throws IOException
sqlStatementResult.getQueryId(),
2L,
ResultFormat.OBJECTLINES.name(),
null,
SqlStatementResourceTest.makeOkRequest()
),
objectMapper
Expand Down Expand Up @@ -485,41 +488,47 @@ public void testMultipleWorkersWithPageSizeLimiting() throws IOException
sqlStatementResult.getQueryId(),
null,
ResultFormat.ARRAY.name(),
null,
SqlStatementResourceTest.makeOkRequest()
)));

Assert.assertEquals(rows.subList(0, 2), SqlStatementResourceTest.getResultRowsFromResponse(resource.doGetResults(
sqlStatementResult.getQueryId(),
0L,
ResultFormat.ARRAY.name(),
null,
SqlStatementResourceTest.makeOkRequest()
)));

Assert.assertEquals(rows.subList(2, 4), SqlStatementResourceTest.getResultRowsFromResponse(resource.doGetResults(
sqlStatementResult.getQueryId(),
1L,
ResultFormat.ARRAY.name(),
null,
SqlStatementResourceTest.makeOkRequest()
)));

Assert.assertEquals(rows.subList(4, 6), SqlStatementResourceTest.getResultRowsFromResponse(resource.doGetResults(
sqlStatementResult.getQueryId(),
2L,
ResultFormat.ARRAY.name(),
null,
SqlStatementResourceTest.makeOkRequest()
)));

Assert.assertEquals(rows.subList(6, 8), SqlStatementResourceTest.getResultRowsFromResponse(resource.doGetResults(
sqlStatementResult.getQueryId(),
3L,
ResultFormat.ARRAY.name(),
null,
SqlStatementResourceTest.makeOkRequest()
)));

Assert.assertEquals(rows.subList(8, 10), SqlStatementResourceTest.getResultRowsFromResponse(resource.doGetResults(
sqlStatementResult.getQueryId(),
4L,
ResultFormat.ARRAY.name(),
null,
SqlStatementResourceTest.makeOkRequest()
)));
}
Expand Down Expand Up @@ -565,6 +574,7 @@ public void testResultFormat() throws Exception
sqlStatementResult.getQueryId(),
null,
resultFormat.name(),
null,
SqlStatementResourceTest.makeOkRequest()
), objectMapper
)
Expand All @@ -577,6 +587,7 @@ public void testResultFormat() throws Exception
sqlStatementResult.getQueryId(),
0L,
resultFormat.name(),
null,
SqlStatementResourceTest.makeOkRequest()
), objectMapper
)
Expand Down Expand Up @@ -616,13 +627,15 @@ public void testResultFormatWithParamInSelect() throws IOException
sqlStatementResult.getQueryId(),
null,
ResultFormat.ARRAY.name(),
null,
SqlStatementResourceTest.makeOkRequest()
)));

Assert.assertEquals(rows, SqlStatementResourceTest.getResultRowsFromResponse(resource.doGetResults(
sqlStatementResult.getQueryId(),
0L,
ResultFormat.ARRAY.name(),
null,
SqlStatementResourceTest.makeOkRequest()
)));
}
Expand Down Expand Up @@ -695,6 +708,7 @@ public void testInsert()
actual.getQueryId(),
0L,
null,
null,
SqlStatementResourceTest.makeOkRequest()
);
Assert.assertEquals(Response.Status.OK.getStatusCode(), resultsResponse.getStatus());
Expand Down Expand Up @@ -738,6 +752,7 @@ public void testReplaceAll()
actual.getQueryId(),
0L,
null,
null,
SqlStatementResourceTest.makeOkRequest()
);
Assert.assertEquals(Response.Status.OK.getStatusCode(), resultsResponse.getStatus());
Expand Down
Loading
Loading