solr
solr copied to clipboard
SOLR-17044: Consolidate SolrJ URL-building logic
https://issues.apache.org/jira/browse/SOLR-17044
Description
URL building logic in SolrJ is complex and is duplicated in several places. This leads to bugs and added maintenance effort.
Solution
This PR consolidates much of the URL-building logic used by various SolrClient implementations into a single place (ClientUtils.buildRequestUrl(...)) that can be consumed by all "http" implementations. It builds on some nicely organized logic in HttpSolrClientBase, making only a few small tweaks (e.g. converting to a 'public static' method in order to make it more broader accessible).
One substantive change worth pointing out is that with this change HttpSolrClient now obeys SolrRequest.getBasePath(..), bringing it into line with Http2SolrClient and HttpJdkSolrClient.
I'd rather getBasePath/setBasePath not exist entirely - I mostly agree with some thoughts that David Smiley summed up on SOLR-17256. But while it exists, it probably makes sense to make similar clients (e.g. all the "http" clients) as uniform as possible in obeying or ignoring it.
Tests
Existing tests continue to pass. New unit tests in ClientUtilsTest.
Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the
mainbranch. - [x] I have run
./gradlew check. - [x] I have added tests for my changes.
Note, this PR renames "HttpSolrClientBase.getRequestPath" (added in 9.6) to the more accurate "getRequestUrl". When backporting to 9x we'll need to add getRequestUrl as a separate and keep a (deprecated) "getRequestPath" around for backcompat reasons.
Thanks for the review @jdyer1 !
but do you see end-users calling this directly? Are you sure we want it to be public?
I made it public in an effort to support the few SolrJ users who might have some reason to extend a SolrClient or create their own from scratch. But I don't have strong opinions on supporting that use case (or not), if you'd prefer it be package scoped or something else?
Could you see moving this back into HttpSolrClientBase at that time?
There's a slight cost to doing that, in that it might make the method unavailable to someone who wants to create their own SolrClient, but doesn't want to take advantage of HSCBase for whatever reason. But that's such a small sliver of usecases that it's probably not worth optimizing for. So I've got no problem seeing this move back later on if we go that route.
I do not have a strong opinion here whether buildRquestUrl should be public or not. I see the reasoning it gives users more flexibility. On the other hand, it adds to the public api surface.
Does this overlap with #2473 ? WOuld be nice to get this in.