-
Notifications
You must be signed in to change notification settings - Fork 679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SOLR-17044: Expose api "version" on SolrRequest objects #2456
SOLR-17044: Expose api "version" on SolrRequest objects #2456
Conversation
Solr uses different API roots for its v1 and v2 APIs: "/solr" for v1, and "/api" for v2. Adding a method for each SolrRequest to indicate which API version it belongs to will allow SolrJ to build the full request path more reliably without relying on fragile 'instanceof' checks.
PackageToolTest currently fails with this PR, due to some quirky GenericSolrRequest usage in the PackageTool and PackageUtils code. It'd take a bit of untangling, but is do-able to fix. That said - rather than tweaking but retaining the GenericSolrRequest usage, I wonder if it isn't worth moving this part of the code away from GSR entirely in favor of specific SolrRequest implementations. The package APIs aren't currently covered in SolrJ, which is why this code needed to use GSR historically. But this might be a nice chance to add these APIs to SolrJ and kill "two birds with one stone". It'd be more work, but might be worth biting off. Will think it through over the weekend. If I decide to add the APIs to SolrJ, I'll do it in a separate PR that blocks this one. |
From my perspective, GSR is the wrong way to think about a generic interaction.. It has all the overhead and complexity of our SolrJ apis, but without any of the nice typing, helping you craft a query. And in MANY places I see us instead work around GSR by doing our own JSON get/post etc with a straight up HTTP client. I'd love to see 1) Anywhere GSR is used, ask ourselves why we don't have a proper Java request object? 2) GenericSolrRequest deprecated, and then maybe make it a bit easier to just "Send a darn json query to this arbitrary endpoint".. |
Yeah - there's a lot, a lot of GSR usage that's only there because folks want to us an API that isn't covered in SolrJ. IMO there's still some valid usecases for keeping GSR around, even if only to give users a quick-and-dirty way to hit whatever custom endpoint they add to Solr using a plugin or whatever. But 95% of the uses in Solr today are not that haha. |
I think that is why I wish we didn't have GSR, and just had a better JSON request approach. Let's just accept that JSON string is okay, and use that... |
Should we create a new JIRA to track moving away from GSR? Something to help unstuck this PR? |
This PR has had no activity for 60 days and is now labeled as stale. Any new activity or converting it to draft will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. Thank you for your contribution! |
Fixes involved making GenericV2SolrRequest a little more flexible in whether the provided path includes the v2 api-root (i.e. "/api") or not. Additionally, switches PackageUtils over to using the generated v2 SolrRequest classes where possible (instead of GenericV2SolrRequest).
Circling back to this now that many of the file-store APIs have been converted to JAX-RS and the nicer fix for PackageToolTest is now do-able. I've switched a bit of GSR usage over to the appropriate generated SolrRequest implementations, and things now pass. Planning to merge this shortly! (There's still much more to do in terms of eliminating GSR usage, but we'll be able to handle that as more and more v2 APIs have SolrJ coverage.) |
These commits were lovely to see! |
Solr uses different API roots for its v1 and v2 APIs: "/solr" for v1, and "/api" for v2. Adding a method for each SolrRequest to indicate which API version it belongs to will allow SolrJ to build the full request path more reliably without relying on fragile 'instanceof' checks.
https://issues.apache.org/jira/browse/SOLR-17044
Description
SolrJ often needs to know whether a given request is for a v1 or v2 API, but doesn't have an easy way to do so. The best it can do is one of two approximations: either check the path string for "____v2", or do an
instanceof V2Request
check. Both of these are exceedingly brittle.Solution
This PR introduces a new request on the SolrRequest class:
getApiVersion()
. It returns an ApiVersion enum value, which can either be "V1" or "V2". The method has a default implementation on SolrRequest which returns the enum value "V1", overriding this on a number of subclasses that represent v2 requests (V2Request, various generated SolrJ classes, etc.)As usual with these sort of changes,
GenericSolrRequest
introduces some complications. There's no great way to determine whether a given GSR represents a v1 or v2 API without resorting to the same brittle string inspection we're trying to avoid. This PR solves this by assuming that 'GenericSolrRequest' instances represent v1 API requests, and adds a trivial 'GenericV2SolrRequest' class to represent v2 requests. (Open to other ways of doing this if folks don't like this approach)Tests
Existing tests continue to pass.
Checklist
Please review the following and check all that apply:
main
branch../gradlew check
.