SOLR-16825: Migrate v2 definitions to 'api' module, pt 4 #1960
Conversation
| @JsonProperty(NUM_SLICES) | ||
| @Schema(name = "numShards") | ||
| public Integer numShards; |
There was a problem hiding this comment.
Can you please elaborate on this; educate me, I suppose? I'm new to this. What is the distinction being made between JsonProperty and Schema annotations? I feel the "slices" nomenclature is an unfortunate one that we should get away from, at least on the API surface.
There was a problem hiding this comment.
Thanks for highlighting this section - I think having @Schema here is a mistake on my part, which I'll fix.
To answer your underlying question though:
JsonProperty
I suspect your question was mostly about @Schema, but I'll risk being pedantic and offer review...
@JsonProperty is all about how this request body is serialized/deserialized by Jackson. If you give the annotation a value, Jackson uses that as the property name when serializing/deserializing. If no value is given, the name of the instance variable is used by default. (Since the constant NUM_SLICES has the value "numShards" and matches the variable name, I probably shouldn't have specified any value here - I think this was a copy/paste oversight.)
Schema
The @Schema annotation otoh is all about metadata. It lets you provide descriptions, additional references, alternate names, etc. into the generated OpenAPI Spec ("OAS"), and therefore into the client code that's generated from that OAS.
The name property that we're using here lets you associate an identifier with the field in question. So mostly you'll see @Schema(name=... when the name of a property in the OAS should be different than the field name Jackson uses to serialize it. (This is important from a good-docs perspective sometimes, but it's also something our Java-code-generation template relies on periodically)
There's no reason to use @Schema here, since the variable name and both annotations all work out to the literal "numShards" - so I'll remove it. Not sure how this got here: maybe I started to add a description before getting distracted? In any case, I appreciate you flagging it.
I feel the "slices" nomenclature is an unfortunate one that we should get away from, at least on the API surface.
Idk whether I like "slices" or not, but I'll remove it here.
| import com.fasterxml.jackson.annotation.JsonProperty; | ||
|
|
||
| public class InstallShardDataRequestBody { | ||
| @JsonProperty(value = "location", required = true) |
There was a problem hiding this comment.
Ah; required. Surely you could have set this to true on a number of other fields in this PR? Like the name of a collection that you create.
There was a problem hiding this comment.
It wouldn't surprise me if there was some inconsistency there, for sure.
That said: I think the specific case you mentioned was intentional and not an oversight. Some of these model classes are used for multiple APIs, and have different requirements (pardon the pun) based on which API is being run.
Case in point: "name" should be required in the create-collection API. But it shouldn't be required when nested in the body of a create-alias API call, where CreateCollectionRequestBody acts as a template allowing routed-aliases to create additional collections.
Long story short, code reuse often requires us to enforce required-props the old-fashioned way instead of relying on the less-flexible Jackson.
(And even if we decided not to reuse model classes across APIs - we'd still have to rely on old-fashioned enforcement, as Jackson doesn't handle all of the content types that Solr accepts e.g. javabin)
This commit covers the create APIs for collections, shards and replicas. It also covers the collection and core level "install-shard-data" APIs. Extracting annotated interfaces for these APIs includes them in the SolrRequest- generation we now do in SolrJ
https://issues.apache.org/jira/browse/SOLR-16825
Description
SOLR-16825 added a new gradle module, 'api', which holds v2 API definitions as interfaces. This allows us to generate an OAS (and SolrRequest implementations from that) as a part of the solrj build.
But these artifacts (the OAS and generated Java code), only cover the v2 APIs that have interfaces in the 'api' module. We need to extract interfaces to live in 'api' for each v2 API in 'core' that doesn't already have one.
Solution
This PR creates 'api' interfaces for a number of v2 APIs, allowing SolrRequest implementations to be generated for them. The following APIs are covered in this PR:
Tests
PR is a refactor, so doesn't add any additional tests. But manual testing has been done to make sure the affected v2 APIs continue to work, and existing tests continue to pass.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.