andywebb1975 commented on code in PR #2435:
URL: https://github.com/apache/solr/pull/2435#discussion_r1590381848
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java:
##########
@@ -238,7 +238,7 @@ private PreparedRequest prepareGet(
validateGetRequest(solrRequest);
reqb.GET();
decorateRequest(reqb, solrRequest);
- reqb.uri(new URI(url + "?" + queryParams));
+ reqb.uri(new URI(url + queryParams.toQueryString()));
Review Comment:
hi James - I've explored this some more. I'm even more convinced we should
use `toQueryString()` now.
https://github.com/apache/solr/pull/2433 now has a series of commits where
I've added test cases and other checks to demonstrate the value of the changes.
The current tests don't exercise the URI encoding enough to show the
difference in behaviour - but adding in another `U+1234` and some curly quotes
triggers failures for the PUT/POST URL generation when using `toString()` - I
saw the same exception as we had previously for GETs.
I've also seen that the request body content length becomes inconsistent
with that of the provided query string unless it's pre-encoded by
`toQueryString()` - when we use `toString()` it's relying on
`HttpRequest.BodyPublisher` re-encoding it. As I've noted in the code comment I
wouldn't suggest doing that check for real, but I'll see if I can convert it to
a test.
Hope this helps!
Andy
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
