andywebb1975 commented on code in PR #2435:
URL: https://github.com/apache/solr/pull/2435#discussion_r1588398916
##########
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:
I've had a closer look. I should say I'm never looked "under the hood" of
the clients before and also I'm not familiar with all their use-cases.
I hadn't clocked that `toQueryString()` adds its own `?` - so yes, this line
is better than my version.
I do think line 316 should change this way too, as `toQueryString()` does a
more comprehensive job of encoding strings than `toString()` and it explicitly
uses UTF-8 encoding, while the description of `toString()` indicates it's just
intended for logging purposes.
My PR changed the method used at line 305 too, which would have injected an
unwanted `?` - but I'm not sure that using `toString()` is right either for the
same reasons as above. I think it'd be worth revisiting this block in general -
I'm not clear on the intent of the code to be able to say how it should change,
but my understanding is that line 302 makes a second reference to the same
object, which doesn't seem right to me. I may be wrong!
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
