SOLR-16395 JAX-RS conversion for zkversion and field related endpoints. #1682

bszabo97 · 2023-06-02T15:29:18Z

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

Description

This PR contains the JAX-RS conversion of endpoints:

/zkversion
/fields
/fields/{fieldname}
/copyfields
/dynamicfields
/dynamicfields/{fieldname}
/fieldtypes
/fieldtypes/{fieldtypename}

I also moved SchemaNameAPI to GetSchemaAPI so that everything is in one place.

There should be no difference in usage so there is no need for documentation modifications, I believe.

Solution

Please provide a short description of the approach taken to implement your solution.

Tests

I added unit tests similar to what can be seen in PR #1649

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

solr/core/src/java/org/apache/solr/handler/admin/api/GetSchemaZkVersionAPI.java

gerlowskija · 2023-06-04T14:48:11Z

I also moved SchemaNameAPI to GetSchemaAPI so that everything is in one place

🎉 Perfect, thanks! That'd been bothering me as well and was on my todo list.

Will review this over the next few days - thanks again for the PR @bszabo97 !

bszabo97 · 2023-06-05T07:17:18Z

Thank you @gerlowskija !

Also I had an issue I forgot to mention when I uploaded the PR. I tried to create unit test for endpoint /zkversion I was not able to mock ManagedIndexSchema class since it is a final class. I have tried several solutions found online but I was not happy with any of those.
Do you have any suggestions how I could test the ManagedIndexSchema branch in the code?

gerlowskija

Overall this looks great. There are a few small things that will need changed I think (missing copyright headers, etc). But otherwise we should be able to get this in soon; thanks for the PR @bszabo97 !

solr/core/src/java/org/apache/solr/handler/admin/api/GetSchemaFieldAPI.java

gerlowskija · 2023-06-07T11:21:15Z

solr/core/src/java/org/apache/solr/handler/admin/api/GetSchemaFieldAPI.java

+import org.apache.solr.schema.IndexSchema;
+import org.apache.solr.security.PermissionNameProvider;
+
+public class GetSchemaFieldAPI extends GetSchemaAPI {


[0] Might be worth a small javadoc comment at the class level here to list out what APIs are in this class.

[+1] Oh, neat - so this class inherits the class-level @Path annotation from GetSchemaAPI? Cool.

FYI @bszabo97

It looks like inheriting from GetSchemaAPI causes a minor warning when you enable "strict validation" of Jersey resources. ("Strict validation" is governed by a setting in JerseyApplications.java. It's disabled by default for speed reasons, but it's useful to enable it from time to time in dev as a way to lint the JAX-RS code a bit. This bug in particular was caught by @stillalex - kudos to him!)

[[FATAL] A resource model has ambiguous (sub-)resource method for HTTP method GET and input mime-types as defined by"@Consumes" and "@Produces" annotations at Java methods public org.apache.solr.handler.admin.api.GetSchemaAPI$SchemaInfoResponse org.apache.solr.handler.admin.api.GetSchemaAPI.getSchemaInfo() and public org.apache.solr.handler.admin.api.GetSchemaAPI$SchemaInfoResponse org.apache.solr.handler.admin.api.GetSchemaAPI.getSchemaInfo() at matching regular expression /(cores|collections)/([^/]+)/schema. These two methods produces and consumes exactly the same mime-types and therefore their invocation as a resource methods will always fail.; source='org.glassfish.jersey.server.model.RuntimeResource@4272ba2e', [FATAL] A resource model has ambiguous (sub-)resource method for HTTP method GET and input mime-types as defined by"@Consumes" and "@Produces" annotations at Java methods public org.apache.solr.handler.admin.api.GetSchemaAPI$SchemaSimilarityResponse org.apache.solr.handler.admin.api.GetSchemaAPI.getSchemaSimilarity() and public org.apache.solr.handler.admin.api.GetSchemaAPI$SchemaSimilarityResponse org.apache.solr.handler.admin.api.GetSchemaAPI.getSchemaSimilarity() at matching regular expression /similarity. These two methods produces and consumes exactly the same mime-types and therefore their invocation as a resource methods will always fail.; source='org.glassfish.jersey.server.model.RuntimeResource@4651ba9d', [FATAL] A resource model has ambiguous (sub-)resource method for HTTP method GET and input mime-types as defined by"@Consumes" and "@Produces" annotations at Java methods public org.apache.solr.handler.admin.api.GetSchemaAPI$SchemaUniqueKeyResponse org.apache.solr.handler.admin.api.GetSchemaAPI.getSchemaUniqueKey() and public org.apache.solr.handler.admin.api.GetSchemaAPI$SchemaUniqueKeyResponse org.apache.solr.handler.admin.api.GetSchemaAPI.getSchemaUniqueKey() at matching regular expression /uniquekey. These two methods produces and consumes exactly the same mime-types and therefore their invocation as a resource methods will always fail.; source='org.glassfish.jersey.server.model.RuntimeResource@10595d07', [FATAL] A resource model has ambiguous (sub-)resource method for HTTP method GET and input mime-types as defined by"@Consumes" and "@Produces" annotations at Java methods public org.apache.solr.handler.admin.api.GetSchemaAPI$SchemaVersionResponse org.apache.solr.handler.admin.api.GetSchemaAPI.getSchemaVersion() and public org.apache.solr.handler.admin.api.GetSchemaAPI$SchemaVersionResponse org.apache.solr.handler.admin.api.GetSchemaAPI.getSchemaVersion() at matching regular expression /version. These two methods produces and consumes exactly the same mime-types and therefore their invocation as a resource methods will always fail.; source='org.glassfish.jersey.server.model.RuntimeResource@733752b1', [FATAL] A resource model has ambiguous (sub-)resource method for HTTP method GET and input mime-types as defined by"@Consumes" and "@Produces" annotations at Java methods public org.apache.solr.handler.admin.api.GetSchemaAPI$SchemaNameResponse org.apache.solr.handler.admin.api.GetSchemaAPI.getSchemaName() throws java.lang.Exception and public org.apache.solr.handler.admin.api.GetSchemaAPI$SchemaNameResponse org.apache.solr.handler.admin.api.GetSchemaAPI.getSchemaName() throws java.lang.Exception at matching regular expression /name. These two methods produces and consumes exactly the same mime-types and therefore their invocation as a resource methods will always fail.; source='org.glassfish.jersey.server.model.RuntimeResource@24433533'] at org.glassfish.jersey.server.ApplicationHandler.initialize(ApplicationHandler.java:375) ~[?:?] at org.glassfish.jersey.server.ApplicationHandler.lambda$initialize$1(ApplicationHandler.java:297) ~[?:?] at org.glassfish.jersey.internal.Errors.process(Errors.java:292) ~[?:?] at org.glassfish.jersey.internal.Errors.process(Errors.java:274) ~[?:?] at org.glassfish.jersey.internal.Errors.processWithException(Errors.java:232) ~[?:?] at org.glassfish.jersey.server.ApplicationHandler.initialize(ApplicationHandler.java:296) ~[?:?] at org.glassfish.jersey.server.ApplicationHandler.<init>(ApplicationHandler.java:261) ~[?:?] at org.glassfish.jersey.server.ApplicationHandler.<init>(ApplicationHandler.java:236) ~[?:?] at org.apache.solr.core.SolrCore.lambda$new$2(SolrCore.java:1155) ~[?:?]

In short - GetSchemaFieldAPI inherits all of the endpoints that GetSchemaAPI offers, and Jersey complains because under normal circumstances you don't want multiple registrations for the same endpoint (as they'd shadow one another).

I'll take care of this when I go to backport shortly, but figured I'd mention it here as a heads up to anyone that reads this.

minor suggestion. could we have the 'strict' validation based off a system property? so we can have a smoke test (just a simple Solr startup) running nightly or something.

gerlowskija · 2023-06-07T11:32:15Z

solr/core/src/java/org/apache/solr/handler/admin/api/GetSchemaFieldAPI.java

+
+  public static class SchemaListFieldsResponse extends SolrJerseyResponse {
+    @JsonProperty("fields")
+    public Object fields;


[Q] Should this really be typed as "Object"?

I totally understand if the response format is too complex to represent with a more specific type. I've run into a few response fields where I punted and left them as Object or Map<String, Object> due to complexity.

But, at least from what I thought I knew of this API, it seems like we could go with List<Object>? Or are there some cases where the "fields" part of this response isn't a list/array?

To be honest I used the type Object since I have seen that the call propertyValues.get() returns an Object and did not really knew if there could be any possible scenarios where this is not a List... But the tests also suggest that this should always be an ArrayList so I will go ahead and change it everywhere

solr/core/src/java/org/apache/solr/handler/admin/api/GetSchemaFieldAPI.java

solr/core/src/java/org/apache/solr/handler/admin/api/GetSchemaZkVersionAPI.java

gerlowskija · 2023-06-07T13:17:17Z

solr/core/src/java/org/apache/solr/handler/admin/api/GetSchemaZkVersionAPI.java

+  @Produces({MediaType.APPLICATION_JSON, MediaType.APPLICATION_ATOM_XML, BINARY_CONTENT_TYPE_V2})
+  @PermissionName(PermissionNameProvider.Name.SCHEMA_READ_PERM)
+  public SchemaZkVersionResponse getSchemaZkVersion(
+      @DefaultValue("-1") @QueryParam("refreshIfBelowVersion") Integer refreshIfBelowVersion)


[+1] Good use of @DefaultValue, I need to be using that more...

gerlowskija · 2023-06-07T13:21:13Z

solr/core/src/java/org/apache/solr/jersey/InjectionFactories.java

@@ -104,6 +106,24 @@ public IndexSchema provide() {
    public void dispose(IndexSchema instance) {}
  }

+  public static class ReuseFromContextSolrParamsFactory implements Factory<SolrParams> {


[+1] Technically, v2 code that needs the SolrParams could inject the SolrQueryRequest and call getParams. So this injector isn't strictly required.

But I'm a big fan of only injecting what you actually need - it makes the code easier to understand, and really helps simplify unit tests. So 👍

gerlowskija · 2023-06-07T13:23:01Z

solr/core/src/test/org/apache/solr/handler/admin/api/GetSchemaAPITest.java

+   * response format.
+   */
+  @Test
+  public void testResponseCanBeParsedBySolrJ() {


[+1] Great idea to test the SolrJ compatibility - I should've been doing this in a lot of other places but hadn't thought of it.

It was your idea, I just copied this test from the already existing one :D I can add similar tests to more places if you think it would be useful.

Yikes - it worries me that I didn't remember haha.

I can add similar tests to more places if you think it would be useful.

Absolutely, if you're willing. I think this would be a great type of test to have for other APIs as well!

solr/core/src/test/org/apache/solr/handler/admin/api/GetSchemaFieldsAPITest.java

gerlowskija

Overall this looks great. There are a few small things that will need changed I think (missing copyright headers, etc). But otherwise we should be able to get this in soon; thanks for the PR @bszabo97 !

gerlowskija · 2023-06-07T13:37:25Z

I tried to create unit test for endpoint /zkversion I was not able to mock ManagedIndexSchema class since it is a final class. I have tried several solutions found online but I was not happy with any of those

Yeah, I've run into these sort of restrictions at different points as well. Unfortunately there's no easy workaround that I know of. I think more recent versions of Mockito have a way to mock 'final' classes. I tried to upgrade the Mockito that Solr uses at some point but ran into some dep conflicts that were tricky to resolve.

If the test would add enough value to justify the effort, we could look at removing the 'final' modifier from ManagedIndexSchema. I could be wrong, but I don't think anyone would object to that as long as there's no especially compelling reason for MIS to be "final". I can take that on and get back to you shortly

bszabo97 · 2023-06-07T14:21:50Z

If the test would add enough value to justify the effort, we could look at removing the 'final' modifier from ManagedIndexSchema.

The only thing that concerns me a little bit is that I was not able to find any other tests testing this part of the code, but still I am not really sure if this test alone would justify removing the final modifier. If you think that it is not needed anyways we can take a look at removing it.

Other then that thank you for the review, I will work on the requested changes!

gerlowskija

LGTM - still need to test, but then I'll aim to commit!

gerlowskija · 2023-06-17T15:03:39Z

solr/core/src/java/org/apache/solr/handler/admin/api/GetSchemaFieldAPI.java

+  }
+
+  private Object listAllFieldsOfType(String realName, SolrParams params) {
+    String camelCaseRealName = IndexSchema.nameMapping.get(realName);


6 is a lot, 🤔

Ultimately, I think there will be value in being as explicit as possible here. But most of those benefits rely on JIRA tickets that haven't been finished yet (e.g. using these API definitions to auto-generate client code).

So, I'm happy to go either way here. It'll set us up really nicely if we're more explicit with the API params. But if you don't want the method signature to be noisy in the meantime, that also makes sense to me.

gerlowskija · 2023-06-17T15:09:21Z

solr/core/src/test/org/apache/solr/handler/admin/api/GetSchemaAPITest.java

+   * response format.
+   */
+  @Test
+  public void testResponseCanBeParsedBySolrJ() {


Yikes - it worries me that I didn't remember haha.

I can add similar tests to more places if you think it would be useful.

Absolutely, if you're willing. I think this would be a great type of test to have for other APIs as well!

…1682) Migrates several v2 `GET /schema` APIs from the legacy annotation framework to JAX-RS, including: - `/zkversion` - lookup the version of the schema in ZK - `/schema/fields` - list the (non-dynamic) fields in the schema - `/schema/fields/fName` - information about a particular (non-dynamic) field in the schema - `/schema/dynamicfields` - list the (dynamic) fields in the schema - `/schema/dynamicfields/fName` - information about a particular (dynamic) field in the schema - `/schema/fieldtypes` - list the field types in the schema - `/schema/fieldtypes/tName` - information about a particular fieldtype in the schema - `/schema/copyfields` - list the copyfields in the schema This change doesn't modify the APIs themselves, so users should remain unaffected.

bszabo97 added 11 commits June 1, 2023 10:47

JAX-RS conversion for fields and copyfields endpoints

5ed7c97

zkVersion initial

2f0ad3e

Initial dynamic field and field type

1cfc18f

Add GetSchemaFieldAPI class

7981c1d

Fix insertPackageInfo

d2ace7a

tidy

4c87528

Move zkVersion to separate class

dace56e

Delete V2SchemaAPIMappingTest.testGetSchemaInfoApis since it is empty

b2da3ab

Move SchameNameAPI to GetSchemaAPI

d8debf1

Add unit tests for new classes

cb24820

Delete ZkVersionRequestBody

53d9f64

sonatype-lift bot reviewed Jun 2, 2023

View reviewed changes

solr/core/src/java/org/apache/solr/handler/admin/api/GetSchemaZkVersionAPI.java Show resolved Hide resolved

gerlowskija requested changes Jun 7, 2023

View reviewed changes

Changes according to PR comments

8766219

gerlowskija approved these changes Jun 17, 2023

View reviewed changes

Add TODO comment about SolrParams passthrough usage

e9269d5

gerlowskija merged commit c9430bb into apache:main Jun 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-16395 JAX-RS conversion for zkversion and field related endpoints. #1682

SOLR-16395 JAX-RS conversion for zkversion and field related endpoints. #1682

bszabo97 commented Jun 2, 2023

gerlowskija commented Jun 4, 2023

bszabo97 commented Jun 5, 2023

gerlowskija left a comment

gerlowskija Jun 7, 2023

gerlowskija Jun 27, 2023

stillalex Jun 27, 2023

gerlowskija Jun 7, 2023

bszabo97 Jun 9, 2023

gerlowskija Jun 7, 2023

gerlowskija Jun 7, 2023

gerlowskija Jun 7, 2023

bszabo97 Jun 7, 2023

gerlowskija Jun 17, 2023

gerlowskija left a comment

gerlowskija commented Jun 7, 2023

bszabo97 commented Jun 7, 2023

gerlowskija left a comment

gerlowskija Jun 17, 2023

gerlowskija Jun 17, 2023

SOLR-16395 JAX-RS conversion for zkversion and field related endpoints. #1682

SOLR-16395 JAX-RS conversion for zkversion and field related endpoints. #1682

Conversation

bszabo97 commented Jun 2, 2023

Description

Solution

Tests

Checklist

gerlowskija commented Jun 4, 2023

bszabo97 commented Jun 5, 2023

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

gerlowskija left a comment

Choose a reason for hiding this comment

gerlowskija commented Jun 7, 2023

bszabo97 commented Jun 7, 2023

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