solr icon indicating copy to clipboard operation
solr copied to clipboard

SOLR-17044: Consolidate SolrJ URL-building logic

Open gerlowskija opened this issue 1 year ago • 4 comments

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 main branch.
  • [x] I have run ./gradlew check.
  • [x] I have added tests for my changes.

gerlowskija avatar May 10 '24 17:05 gerlowskija

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.

gerlowskija avatar May 10 '24 18:05 gerlowskija

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.

gerlowskija avatar May 13 '24 11:05 gerlowskija

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.

jdyer1 avatar May 17 '24 15:05 jdyer1

Does this overlap with #2473 ? WOuld be nice to get this in.

epugh avatar May 22 '24 21:05 epugh