[#73, #96] Nested schema references w/configuration; default object type #97

MikeEdgar · 2019-05-04T18:27:09Z

Addresses issues #73 and #96. Criticism/feedback is welcome.

The approach to enable schema references uses a configuration extension property, mp.openapi.extensions.schema-references.enable. This is handled using the SchemaRegistry class which has been extracted to a top-level class outside of the scanner. I implemented this using ThreadLocal to avoid changing a larger number of method signatures. This could certainly be re-factored to be cleaner at some point, but I think any negative impact is low because the scanning process is not currently multi-threaded.

The changes include replicating a good number of the test cases to verify those scenarios still function when schema references are enabled.

Default type attribute on generated schemas to "object"
Extract SchemaRegistry to top-level class
Update existing test cases for defaulted "object" schema type
Add and clone test cases for schema reference support
Make several public methods used only for unit tests package private

* Default type attribute on generated schemas to "object" * Extract SchemaRegistry to top-level class * Update existing test cases for defaulted "object" schema type * Add and clone test cases for schema reference support

kenfinnigan · 2019-05-05T17:44:15Z

Unfortunately this is beyond me in terms of understanding around OpenAPI.

@EricWittmann or @msavy can you comment/approve?

EricWittmann

This looks pretty good to me. We just need to wait on Ken's response to the config property name question I think.

What do you think about using short names for the generated types regardless of whether refs are enabled or disabled?

EricWittmann · 2019-05-06T11:20:19Z

implementation/src/main/java/io/smallrye/openapi/api/OpenApiConstants.java

@@ -70,6 +70,7 @@

    public static final String SCAN_DEPENDENCIES_DISABLE = "mp.openapi.extensions.scan-dependencies.disable";
    public static final String SCAN_DEPENDENCIES_JARS = "mp.openapi.extensions.scan-dependencies.jars";
+    public static final String SCHEMA_REFERENCES_ENABLE = "mp.openapi.extensions.schema-references.enable";


I'm not sure we can name this property mp.openapi.extensions.schema-references.enable when it's not an official Microprofile OpenAPI config setting. @kenfinnigan - what do you think? Since this is a vendor-only configuration property, do we need to use a vendor namespace for property name?

@EricWittmann - I may have misread/misunderstood, but I think the prefix mp.openapi.extensions is intended for that purpose. I was thinking this should stay disabled by default until a major/minor release when users expect things to change.

https://github.com/eclipse/microprofile-open-api/blob/master/spec/src/main/asciidoc/microprofile-openapi-spec.adoc#312-vendor-extensions

Yep you're right. That's my bad. I forgot that SCAN_DEPENDENCIES was our own vendor specific feature so I didn't think/look hard enough at the prefix of the property - I just saw "mp.openapi" and thought I knew enough. :)

(which is even dumber when you consider I wrote the damn thing)

EricWittmann · 2019-05-06T11:27:34Z

implementation/src/main/java/io/smallrye/openapi/runtime/scanner/SchemaRegistry.java

+        return current.get();
+    }
+
+    public static Schema checkRegistration(IndexView index, Type entityType, TypeResolver typeResolver, Schema schema) {


This method probably warrants a bit of javadoc. :)

I agree - I'll get some additional documentation around this.

MikeEdgar · 2019-05-06T12:25:18Z

I just want to note that I do have some additional changes on this branch that I have not yet pushed up to GitHub to properly support using the @Schema's name property when saving and referencing schemas. This is related to my question (microprofile/microprofile-open-api#347) on the MP Open API issue list regarding access to the name via the Schema interface. Currently, the name is only used when scanning the @OpenAPIDefinition annotation. Since the official API for Schema does expose the name, the change relies on the implementation class, SchemaImpl.

That change will also support naming of simple schemas with @Schema(name = "...") similar to how #91 supports using JSON-B annotations.

If you're in agreement with that functionality, it might be worth waiting on this PR for those additional changes, plus the documentation for the SchemaRegistry, are complete (perhaps by tomorrow if I find some time).

EricWittmann

@kenfinnigan I was wrong about the config property prefix name - what's being used is exactly right. So I'm good with merging this. Nice work here. My other question around using short names in all cases can be handled in a different PR if we decide it's the right thing to do.

EricWittmann · 2019-05-06T12:27:36Z

Honestly I think a separate PR for the improved name support might make more sense.

MikeEdgar · 2019-05-06T12:28:52Z

Honestly I think a separate PR for the improved name support might make more sense.

Sounds good. Do you think the same for the documentation or should that be included here?

EDIT
Is your reference to using short names the same as using @Schema(name = "...") ?

EricWittmann · 2019-05-06T12:47:40Z

I think for the additional javadoc - whatever is most convenient for you. :) I guess it would better as part of this PR since it goes with this change, but it's not a huge deal, IMO.

As for the short name. What I'm referring to is that in the existing unit tests, the names of the generated schema definitions are the fully qualified Java classnames. I believe @msavy did this to avoid any name collisions (I'm hoping he can swoop in here and confirm the reasoning - I know we discussed it but I am hazy on the details). But now when enabling refs, the generated names are short names, with an incrementing counter in the (unlikely?) event of a collision. I'd prefer the short names I think, unless there is a technical reason why we need the fully qualified ones. Again, hopefully @msavy remembers the reasons... :)

MikeEdgar · 2019-05-06T12:56:19Z

Ok, I see now. Those long names in the unit tests are due to the fact that the DotName objects are created using the createSimple static method. That behavior should be limited to just the unit tests since the Schemas are scanned in isolation and then added to an OpenAPI from the test just for serialization.

I added a componentize helper to the unit test parent class (implementation/src/test/java/io/smallrye/openapi/runtime/scanner/OpenApiDataObjectScannerTestBase.java) that should assist in generating DotNames that behave the same as those used within the scanner.

At this point, it would definitely be more convenient to push the JavaDoc with the next PR.

msavy · 2019-05-06T13:15:05Z

Travelling at the moment, so sorry for slow responses.

@MikeEdgar Yes, that's right. I did it that way for convenience in testing. Apologies if it was confusing.

@EricWittmann Not sure if we're talking about the same issue, but I seem to recall one of the complications we encountered was that when merging schemas (using the multi-step merge in OAI spec), it is not always clear whether schema entities of the same short name actually refer to the same thing. Hence we tended not to attempt to normalise schema entities.

Overall these changes look good, and are minimally invasive. More docs would be appreciated, but as a separate PR seems reasonable to me :-). Or you could do a squash with force push, whatever works for you.

msavy · 2019-05-06T13:51:53Z

I'm still not sure the method of referencing here is guaranteed to be safe given how OAI merging is done. Thoughts?

MikeEdgar · 2019-05-06T14:57:58Z

I'm still not sure the method of referencing here is guaranteed to be safe given how OAI merging is done. Thoughts?

@msavy - I think you are right. This implementation currently only consults the schemas found during the annotation scanning phase. Even within that, it is limited to only field-level references. The further enhancements I'm working on to use the @Schema name can be extended to consult the OAI model for schemas already added with the same key.

The collision detection would need to be more extensive than just comparing names, however. A previously-added Schema could only be considered a match if both the name AND the internal structure match the one found in the annotation scan (agree?). The situation gets complicated with scenarios like those in the "kitchen sink" test cases. Otherwise, the "name + 1" approach would be used in the SchemaRegistry.

EricWittmann · 2019-05-06T15:13:18Z

Separate PR for the docs is fine, for sure.

And yeah, @msavy I think you're right - it was merging that was the issue. So @MikeEdgar - what is supposed to happen is that the OAI docs from each phase (annotation scan, static file inclusion, model reader, etc) are merged together into a single spec. When merging from these different phases, schemas with the same name have their content merged. An example use-case is that you could provide e.g. a static file that includes just Summaries and Descriptions of things and then let annotation scanning provide the technical details. So in that case we want the schema name from the static file to match the one generated during annotation scanning (so that the information found in the static doc is successfully merged into the same schema definition as the one generated). This makes collisions really nasty.

So the question is what do we make the schema name to avoid collision but ALSO support easy merging with other phases?

I think I'd rather have simple names if possible, but am unsure what is the best approach to support that goal.

EricWittmann · 2019-05-06T15:13:43Z

Either way, I'm going to merge this PR - but we can continue the discussion here if we want. :)

EricWittmann reviewed May 6, 2019

View reviewed changes

EricWittmann mentioned this pull request May 6, 2019

Ensure JAX-RS Request Body is properly handled when non JAX-RS annotations present #98

Merged

EricWittmann approved these changes May 6, 2019

View reviewed changes

EricWittmann merged commit 1cc317d into smallrye:master May 6, 2019

MikeEdgar deleted the 73_support_nested_schema_refs branch May 7, 2019 00:36

This was referenced Jun 4, 2019

Schemas generated from scanned objects missing type="object" #96

Closed

Add schema reference of nested objects #73

Closed

Include type parameters in generated schema names #121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#73, #96] Nested schema references w/configuration; default object type #97

[#73, #96] Nested schema references w/configuration; default object type #97

MikeEdgar commented May 4, 2019

kenfinnigan commented May 5, 2019

EricWittmann left a comment

EricWittmann May 6, 2019

MikeEdgar May 6, 2019

EricWittmann May 6, 2019

EricWittmann May 6, 2019

MikeEdgar May 6, 2019

MikeEdgar commented May 6, 2019

EricWittmann left a comment

EricWittmann commented May 6, 2019

MikeEdgar commented May 6, 2019 •

edited

Loading

EricWittmann commented May 6, 2019

MikeEdgar commented May 6, 2019

msavy commented May 6, 2019

msavy commented May 6, 2019

MikeEdgar commented May 6, 2019

EricWittmann commented May 6, 2019

EricWittmann commented May 6, 2019

[#73, #96] Nested schema references w/configuration; default object type #97

[#73, #96] Nested schema references w/configuration; default object type #97

Conversation

MikeEdgar commented May 4, 2019

kenfinnigan commented May 5, 2019

EricWittmann left a comment

Choose a reason for hiding this comment

EricWittmann May 6, 2019

Choose a reason for hiding this comment

MikeEdgar May 6, 2019

Choose a reason for hiding this comment

EricWittmann May 6, 2019

Choose a reason for hiding this comment

EricWittmann May 6, 2019

Choose a reason for hiding this comment

MikeEdgar May 6, 2019

Choose a reason for hiding this comment

MikeEdgar commented May 6, 2019

EricWittmann left a comment

Choose a reason for hiding this comment

EricWittmann commented May 6, 2019

MikeEdgar commented May 6, 2019 • edited Loading

EricWittmann commented May 6, 2019

MikeEdgar commented May 6, 2019

msavy commented May 6, 2019

msavy commented May 6, 2019

MikeEdgar commented May 6, 2019

EricWittmann commented May 6, 2019

EricWittmann commented May 6, 2019

MikeEdgar commented May 6, 2019 •

edited

Loading