SOLR-17390: EmbeddedSolrServer now considers the ResponseParser #2774
Conversation
* Moved HttpSolrCall.getResponseWriter to SolrQueryRequest * Subtle improvements to make ContentStream work when they might not have
| @@ -934,13 +934,7 @@ protected void logAndFlushAdminRequest(SolrQueryResponse solrResp) throws IOExce | |||
| * returns the default query response writer Note: This method must not return null | |||
| */ | |||
| protected QueryResponseWriter getResponseWriter() { | |||
| String wt = solrReq.getParams().get(CommonParams.WT); | |||
There was a problem hiding this comment.
I love this werid logic being removed!
There was a problem hiding this comment.
it has moved:
Moved HttpSolrCall.getResponseWriter to SolrQueryRequest
It wasn't specific to HTTP; EmbeddedSolrServer needs to call the same logic.
| _parser.buildRequestFrom( | ||
| core, params, getContentStreams(request), request.getUserPrincipal()); | ||
| core.getSolrConfig() | ||
| .getRequestParsers() |
There was a problem hiding this comment.
Using a SolrCore's SolrRequestParsers instance instead of a default one on this class means we work with parsers and configuration specific to the core. This was previously simply overlooked.
| req.getContext().put(PATH, path); | ||
| req.getContext().put("httpMethod", request.getMethod().name()); | ||
| SolrQueryResponse rsp = new SolrQueryResponse(); | ||
| SolrRequestInfo.setRequestInfo(new SolrRequestInfo(req, rsp)); | ||
|
|
||
| core.execute(handler, req, rsp); | ||
| checkForExceptions(rsp); | ||
|
|
||
| // Check if this should stream results | ||
| if (request.getStreamingResponseCallback() != null) { |
There was a problem hiding this comment.
moved to writeResponse and simplified a little
| if (request instanceof ContentStreamUpdateRequest) { | ||
| final ContentStreamUpdateRequest csur = (ContentStreamUpdateRequest) request; | ||
| final Collection<ContentStream> cs = csur.getContentStreams(); | ||
| if (cs != null) return new HashSet<>(cs); |
There was a problem hiding this comment.
Set didn't make sense
| @@ -250,7 +250,8 @@ public final T process(SolrClient client, String collection) | |||
| throws SolrServerException, IOException { | |||
| long startNanos = System.nanoTime(); | |||
| T res = createResponse(client); | |||
| res.setResponse(client.request(this, collection)); | |||
| var namedList = client.request(this, collection); | |||
There was a problem hiding this comment.
put on its own line to help debugger step-in. I've meant to do this for years.
| if (random().nextBoolean()) { | ||
| CLIENT = JSR.newClient(); | ||
| } else { | ||
| CLIENT = new EmbeddedSolrServer(JSR.getCoreContainer(), null); |
There was a problem hiding this comment.
it works; I re-rean the test enough times to see it does, with code coverage. Not in this PR, I also played with flipping SolrTestCaseHS and some other ZK endpoint test, which helped improve this PR by uncovering things, but those changes seemed like too much scope right now.
|
Will merge in a couple days if no review. |
And * Moved HttpSolrCall.getResponseWriter to SolrQueryRequest * Subtle improvements to make ContentStream work when they might not have (cherry picked from commit c5c538a)
https://issues.apache.org/jira/browse/SOLR-17390