SOLR-17705 for V2 API #3282

dsmiley · 2025-03-22T19:19:58Z

https://issues.apache.org/jira/browse/SOLR-17705

Moves JSON parsing into the ResponseParser where it belongs.
Removes generated intermediate SolrResponse layer classes.

This PR is branched off of another PR, #3270, and so it includes all the changes there. Once that merges, this one will show what's new. I recommend looking at only the recent commit. And since it updates generated code, a real review requires checking out the code to generate and look at it.

Is not necessarily a SolrResponse anymore. Helps V2 API and reduces presence of NamedList.

# Conflicts: # solr/solrj/src/java/org/apache/solr/client/solrj/request/QueryRequest.java

We no longer enforce this; doesn't appear possible.

Moves JSON parsing into the ResponseParser where it belongs. Removes generated intermediate SolrResponse layer classes.

dsmiley · 2025-03-22T19:21:54Z

solr/core/src/java/org/apache/solr/cli/CLIUtils.java

@@ -322,7 +322,7 @@ public static boolean safeCheckCoreExists(String solrUrl, String coreName, Strin
          Thread.sleep(clamPeriodForStatusPollMs);
        }
        final var coreStatusReq = new CoresApi.GetCoreStatus(coreName);
-        final var coreStatusRsp = coreStatusReq.process(solrClient).getParsed();


I admit the result of this work as seen in consuming/calling code right here is merely removing getParsed() since there is now no intermediate class holding the parsed value. But anyone browsing the generated code / class hierarchy will now see something simpler.

dsmiley · 2025-03-22T19:22:40Z

solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java

@@ -96,7 +97,7 @@ public static void postFile(SolrClient client, ByteBuffer buffer, String name, S
        uploadReq.setSig(List.of(sig));
      }

-      final var uploadRsp = uploadReq.process(client).getParsed();
+      final UploadToFileStoreResponse uploadRsp = uploadReq.process(client);


IMO we've been using var too much.

Haha, you turned me on to it a year or two back and I've loved it since. I get that the type is less visible, but it makes the code scan better in places where the type name is a bit of a mouthful and "tidy" would otherwise split it up into multiple lines

But, to each their own.

dsmiley · 2025-03-22T19:24:01Z

solr/solrj/src/resources/java-template/api.mustache

@@ -91,17 +90,13 @@ public class {{classname}} {
    {{#operation}}
    {{^vendorExtensions.x-omitFromCodegen}}
        {{#vendorExtensions.x-rawOutput}}
-        public static class {{operationIdCamelCase}}Response extends InputStreamResponse {}


dsmiley · 2025-03-22T19:24:08Z

solr/solrj/src/resources/java-template/api.mustache

        {{/vendorExtensions.x-rawOutput}}
        {{^vendorExtensions.x-rawOutput}}
-        public static class {{operationIdCamelCase}}Response extends JacksonParsingResponse<{{modelPackage}}.{{returnType}}> {


dsmiley · 2025-03-22T19:25:27Z

solr/solrj/src/test/org/apache/solr/client/solrj/ApiMustacheTemplateTests.java

seems obsolete now

[0] It's obsolete in its current form, but in theory it still makes sense to have some sort of test validating that the v2 deserialization plumbing actually works as intended?

We use the generated v2 code in a few places (e.g. CLIUtils, TestDistribFileStore), so I guess there's some implicit testing of this already? Maybe that's enough? idk - just thinking aloud.

dsmiley · 2025-03-28T05:28:39Z

WDYT @gerlowskija ?

gerlowskija

I left a few comments inline. No huge issues, but a few ideas and questions. Curious what you think!

What's the relationship between this PR and the one that underlies it? Are you looking for consensus on both before you merge the underlying one? (Mostly asking to make sure I'm directing my review effort in the right place to keep you unblocked...)

gerlowskija · 2025-04-02T18:21:57Z

solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java

@@ -96,7 +97,7 @@ public static void postFile(SolrClient client, ByteBuffer buffer, String name, S
        uploadReq.setSig(List.of(sig));
      }

-      final var uploadRsp = uploadReq.process(client).getParsed();
+      final UploadToFileStoreResponse uploadRsp = uploadReq.process(client);


Haha, you turned me on to it a year or two back and I've loved it since. I get that the type is less visible, but it makes the code scan better in places where the type name is a bit of a mouthful and "tidy" would otherwise split it up into multiple lines

But, to each their own.

gerlowskija · 2025-04-03T12:05:48Z

solr/solrj/src/java/org/apache/solr/client/solrj/JacksonDataBindResponseParser.java

+import org.apache.solr.common.util.SimpleOrderedMap;
+
+/** Parses JSON, deserializing to a domain type (class) via Jackson data-bind. */
+public class JacksonDataBindResponseParser<T> extends ResponseParser {


[0] Maybe remove "DataBind" from the name, as it's implied/redundant once Jackson is mentioned?

[-0] Wdyt about marking this class "lucene.experimental" so we can rename or make changes more freely later if needed. I guess we're still held to the ResponseParser interface, so things couldn't change too drastically, but it still might be prudent since this is something that we're still trying to get right in SolrJ.

Jackson does not imply Jackson-DataBind -- the latter depends on the former, of course. Jackson is the parsing, DataBind is the, well, data binding -- mapping to POJOs via annotations etc.

Happy to add lucene.experimental wherever.

Jackson is the parsing, DataBind is the, well, data binding -- mapping to POJOs via annotations etc.

Oh, OK, fair enough.

I've only ever used Jackson with those annotations - I know in theory that the dependencies come separately, but it's hard for me to imagine how you'd even use Jackson if you're not converting to+from some POJO. Is it just all Map and List instances without databind? (That's rhetorical I guess - I can google it.)

gerlowskija · 2025-04-03T12:08:28Z

solr/solrj/src/java/org/apache/solr/client/solrj/JacksonDataBindResponseParser.java

+
+  @Override
+  public String getWriterType() {
+    return "json";


[0] I think this would need to be changed at the ResponseParser level, but it's a bummer that this method can't return a list, or indicate handling for multiple write-types. There's no reason our Jackson support should be specific to JSON, as you indicate in some comments below.

Much larger issue than this PR though...

That's a separate issue for sure, and was on my mind a bit as I worked on this

gerlowskija · 2025-04-03T13:02:42Z

solr/solrj/src/test/org/apache/solr/client/solrj/ApiMustacheTemplateTests.java

[0] It's obsolete in its current form, but in theory it still makes sense to have some sort of test validating that the v2 deserialization plumbing actually works as intended?

We use the generated v2 code in a few places (e.g. CLIUtils, TestDistribFileStore), so I guess there's some implicit testing of this already? Maybe that's enough? idk - just thinking aloud.

gerlowskija · 2025-04-03T13:03:56Z

solr/test-framework/src/java/org/apache/solr/cloud/ConfigRequest.java


 /**
 * A class for making a request to the config handler. Tests can use this e.g. to add custom
 * components, handlers, parsers, etc. to an otherwise generic configset.
 */
-@SuppressWarnings({"rawtypes"})
-public class ConfigRequest extends CollectionRequiringSolrRequest {
+public class ConfigRequest extends CollectionRequiringSolrRequest<SolrResponse> {


[0] Unrelated to your PR, but I wonder why this lives in test-framework...

gerlowskija · 2025-04-03T13:38:25Z

solr/solrj/src/java/org/apache/solr/client/solrj/JacksonDataBindResponseParser.java

+      parsedVal = mapper.readValue(new InputStreamReader(stream, encoding), typeParam);
+    }
+
+    return SimpleOrderedMap.of("response", parsedVal);


[0] processResponse's callers (HttpSolrClientBase, HttpSolrClient) do some error-checking on this method's retval, looking for an "error" key in the NamedList as an indicator of error. This implementation will never return a NL with that key, so it loses the benefits of that error-checking.

IMO we needn't handle that in this PR. It pre-exists your change in InputStreamResponseParser, NoOpResponseParser, etc. And there's a JIRA ticket for thinking through how we want error-handling to work in v2, so it won't get lost or fall through the cracks.

But I'm curious if you had any thoughts.

I had a plan in the back of my mind of uniting JacksonParsingResponse and InputStreamResponse, which would allow us to hook into some of the validation the ISR currently has. But it was never a great plan, and it doesn't make any sense if JPR goes away entirely. It'd be hacky, but we could have callers look for instanceof SolrJerseyResponse and inspect that POJO for errors? Or maybe the client only looks at the HTTP status code for v2 APIs?

Ah; good catch! Hmm. It'd be good to induce an error in a test using this API so we can verify that whatever we want to happen, actually happens. I would personally rather prefer that become a new evolutionary issue/PR rather than increasing the scope of this one. I could see us making bigger changes to accommodate that concern. I'll expand on that in the other comment...

I would personally rather prefer that become a new evolutionary issue/PR rather than increasing the scope of this one

Sounds good - we've already got this tracked in SOLR-17549, so we're good on that front.

gerlowskija · 2025-04-03T13:48:14Z

solr/solrj/src/java/org/apache/solr/client/solrj/JacksonDataBindResponseParser.java

+  //  accordingly, maybe json, cbor, smile
+
+  @Override
+  public NamedList<Object> processResponse(InputStream stream, String encoding) throws IOException {


[0] I love that process can now return arbitrary types; it's a shame that can't extend to the retval of processResponse, and that we have to repackage our arbitrary type as a NL for all of the various SolrClient-internal NL-inspection code.

I think I brought this up in the related PR, but it strikes me again whether we shouldn't be slightly stricter on the allowed return types. Allow process to return types other than NL, but require retvals to implement some interface that exposes the information that most of our SolrClient impls look at internally: wasSuccessful(), getErrorMessage(), etc. That'd give our clients a uniform way to inspect responses and throw SolrServerExceptions, etc, regardless of whether we parse the response into a NamedList, or a SolrJerseyResponse, or an InputStreamResponse, or...

(This would break the error-handling logjam I've been wrestling with for awhile now too - see comment on L62 for more details.)

I would honestly hate to bring back a root type merely for error handling. Java has exceptions for this; let's use that instead. We could make processResponse return whatever and for successful responses and throw something for the error case. That's a bigger change than the scope of this PR, so WDYT about merging this without additional error handling?

I propose a new JIRA issue (or your existing SOLR-17549 maybe) enhance the ResponseParser abstraction to take on the responsibility of error detection and throwing, and thus reducing some of the scope in the HTTP SolrClient side. I could imagine the processResponse method having more parameters including the HTTP status code, and using that to determine are we going to use data-bind or are we going to look for the error. We'd need to do this for XML & JavaBin; finding a way to reuse some logic where it's similar. Don't need to spec this out this second.

I like that overall plan - it makes sense conceptually to have ResponseParser do error detection, since that's part of the response. (And it'd be a really nice simplification for our SolrClient implementations themselves.)

And as mentioned elsewhere - it's fine with me to not put that in this PR. It's definitely a bigger change.

dsmiley · 2025-04-03T18:39:44Z

What's the relationship between this PR and the one that underlies it? Are you looking for consensus on both before you merge the underlying one?

Yes; these are important changes that require peer review. I don't want to be a cowboy here, doing whatever to important APIs.

dsmiley · 2025-04-09T14:27:00Z

Good. After merging the parent issue tonight, I'll brush up this PR, adding lucene.experimental but mostly unchanged, update CHANGES.txt. Should be in mergeable shape tomorrow.

# Conflicts: # solr/CHANGES.txt # solr/core/src/test/org/apache/solr/handler/admin/ZookeeperReadAPITest.java # solr/solrj/src/resources/java-template/api.mustache

dsmiley

This PR should be ready to merge. Okay @gerlowskija ?

dsmiley · 2025-04-15T03:07:01Z

Well I'll merge this in another day if I don't hear back.

dsmiley added 7 commits March 16, 2025 19:22

SOLR-17705 SolrRequest's type is now unbounded

81ef1aa

Is not necessarily a SolrResponse anymore. Helps V2 API and reduces presence of NamedList.

clarify javadocs

40b2595

Fix generics issue

27b6afb

Merge branch 'main' into SOLR-17705-SolrRequestUnbounded

cd5fbb2

# Conflicts: # solr/solrj/src/java/org/apache/solr/client/solrj/request/QueryRequest.java

Remove HealthCheckHandlerTest.testHealthCheckHandlerWithCloudClient

ed6da8c

We no longer enforce this; doesn't appear possible.

CHANGES.txt

a2fe26c

SOLR-17705: SolrJ V2 API

4f0d3b2

Moves JSON parsing into the ResponseParser where it belongs. Removes generated intermediate SolrResponse layer classes.

dsmiley requested a review from gerlowskija March 22, 2025 19:19

github-actions bot added test-framework client:solrj tests cat:cloud cat:cli cat:schema cat:packagemanager labels Mar 22, 2025

dsmiley commented Mar 22, 2025

View reviewed changes

gerlowskija reviewed Apr 3, 2025

View reviewed changes

dsmiley added 2 commits April 11, 2025 01:02

Merge branch 'main' into fork/dsmiley/SOLR-17705-V2API

bfc1f0c

# Conflicts: # solr/CHANGES.txt # solr/core/src/test/org/apache/solr/handler/admin/ZookeeperReadAPITest.java # solr/solrj/src/resources/java-template/api.mustache

finishing touches...

b6057e8

github-actions bot removed the cat:schema label Apr 11, 2025

Fix CHANGES.TXT

cdb16d3

dsmiley commented Apr 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-17705 for V2 API #3282

SOLR-17705 for V2 API #3282

dsmiley commented Mar 22, 2025

dsmiley Mar 22, 2025

dsmiley Mar 22, 2025

gerlowskija Apr 2, 2025

dsmiley Mar 22, 2025

dsmiley Mar 22, 2025

dsmiley Mar 22, 2025

gerlowskija Apr 3, 2025

dsmiley commented Mar 28, 2025

gerlowskija left a comment

gerlowskija Apr 2, 2025

gerlowskija Apr 3, 2025

dsmiley Apr 3, 2025

gerlowskija Apr 9, 2025

gerlowskija Apr 3, 2025

dsmiley Apr 3, 2025

gerlowskija Apr 3, 2025

gerlowskija Apr 3, 2025

gerlowskija Apr 3, 2025

dsmiley Apr 3, 2025

gerlowskija Apr 9, 2025

gerlowskija Apr 3, 2025

dsmiley Apr 3, 2025

gerlowskija Apr 9, 2025

dsmiley commented Apr 3, 2025

dsmiley commented Apr 9, 2025

dsmiley left a comment

dsmiley commented Apr 15, 2025

SOLR-17705 for V2 API #3282

Are you sure you want to change the base?

SOLR-17705 for V2 API #3282

Conversation

dsmiley commented Mar 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmiley commented Mar 28, 2025

gerlowskija left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmiley commented Apr 3, 2025

dsmiley commented Apr 9, 2025

dsmiley left a comment

Choose a reason for hiding this comment

dsmiley commented Apr 15, 2025