-
Notifications
You must be signed in to change notification settings - Fork 736
SOLR-10998: Obey 'Accept' header in v2 APIs #3262
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.
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
SOLR-10998: Obey 'Accept' header in v2 APIs #3262
Conversation
'wt' takes precedence if specified for now, if not provided Solr (through our use of Jersey) will pick a response format in keeping with the specified 'Accept' header. JSON responses remains the "default" if neither 'wt' nor an "Accept" header are specified. Still needed: - tests - CHANGES.txt entry
This is some nice alignment with normal web expectations.. |
I love what you wrote -- what this PR accomplishes. It's a mystery to me how this PR accomplishes that goal. A test would be nice. |
Tests added in
Really it doesn't. Jersey will parse the "Accept" header out of the box, and choose a "MessageBodyWriter" (akin to our ResponseWriter") based on that. But I'd hidden this support in the past by creating a "response filter" ( So what this PR is really doing is tweaking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A forward-looking question: In SolrJ, assuming "V2", how might we compose an Accept header for multiple formats, and then on the receiving end, choose the right ResponseParser for the final format chosen by the server?
Even assuming Jackson, I see us supporting application/json
, application/cbor
, and could add "smile" to the list, as all 3 are supported with small changes thanks to Jackson's well-designed API.
Our current abstractions are a bit rigid, with the ResponseParser that must supply a "wt" and that which consumes an inputStream without knowledge of the actual mime type in the response header, as it's not passed to the ResponseParser. Maybe we basically just need to change ResponseParser, and make the SolrClients get content types from the RP (to put in "Accept") instead of the "wt" param? Sounds pretty straight-forward actually.
private HttpClient getRawClient() { | ||
return ((CloudLegacySolrClient) cluster.getSolrClient()).getHttpClient(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're trying to get away from Apache HttpClient. Can you please try Jetty HttpClient instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the best way to do that? Is there one?
Per the Slack discussion here, there's not yet a general utility for creating a Jetty client. And adding a well-thought-out version of that is conceptually unrelated and beyond the scope of this PR.
I can throw together a client-creation snippet just for this test, but that feels a little hacky and the sort of thing that'd be likely to run afoul of our test randomization and cause flaky failures down the line whenever SSL or whatever variable is randomly toggled.
Hmm... The one thing that's missing I think is some sort of "CompoundResponseParser" - something that supports a number of content types via delegation and then chooses one at response-parsing time based on the "Content-type" header in the response. |
'wt' takes precedence if specified for now, if not provided Solr (through our use of Jersey) will pick a response format in keeping with the specified 'Accept' header. JSON responses remains the "default" if neither 'wt' nor an "Accept" header are specified.
'wt' takes precedence if specified for now, if not provided Solr (through our use of Jersey) will pick a response format in keeping with the specified 'Accept' header.
JSON responses remains the "default" if neither 'wt' nor an "Accept" header are specified.
https://issues.apache.org/jira/browse/SOLR-10998
Description
The HTTP spec defines an "Accept" header that users can provide to specify the response format (or formats, plural) that they're willing to "accept" in a response. This is part of the HTTP specs support for "content negotiation" more generally.
Solr today doesn't support this, and opts to have users specify their response format in a "wt" (i.e. "writer type") query-parameter. We should obey "Accept" headers, when they are provided.
Solution
This PR modifies Solr to obey "Accept" headers on v2 API requests, unless an overriding "wt" parameter is also provided. Jersey (which is used for v2 requests) actually obeys the "Accept" header out of the box, but a bug in our
MediaTypeOverridingFilter
(which implements the 'wt' override) was causing the value of "Accept" to be ignored altogether.With this change, the order of precedence for v2 APIs is:
wt
parameter, thenAccept
header, and lastly "JSON" is used as a fallback/default.Tests
See test cases in V2ApiIntegrationTest
Checklist
Please review the following and check all that apply:
main
branch../gradlew check
.