Use @Schema 'name' for props and schema refs; Fix #25 #99
Conversation
|
Looks like my last minute edit of a JavaDoc is causing an issue. I'll get that fixed up and update the request. |
This is actually something we discussed in the spec committee. Specifically whether the spec should require implementations to provide warnings when the annotations are used incorrectly. My position on this is that there are WAY too many cases where annotations could be used to produce invalid an OpenAPI document. It would be very difficult to cover all such cases during annotation processing. So instead of that, we are planning on adding (optional) validation capabilities to the spec. This will allow vendors to provide validation of the final generated OpenAPI doc. That said, I am by no means opposed to issuing warnings for commonly mis-used annotations. But the task of actually warning about every (or even most) incorrectly used annotations is very hard. Especially when you consider that the phases get merged together into a final document, and it's hard to know when scanning the annotations what the final result will be after all the phases are combined. |
* Consolidate duplicate Schema utility functions into SchemaFactory * Construct SchemaRegistry with the IndexView * Use custom key to store schemas in registry for added control of the equals/hashCode methods * Update JAX-RS annotation scanning to address issue #25
MikeEdgar
force-pushed
the
use_provided_schema_name
branch
from
ad096ae to
68a0295
Compare
|
Those are some good points regarding validation and warnings. I'd be interested in following along with those discussions for additional spec capabilities. It would be ideal if there are some defined scenarios that are invalid, such as the example of using ref along with other properties. |
|
Here is a link to some discussion on the topic: microprofile/microprofile-open-api#339 |
|
Ha ha ha - according to GH, my comment was made "4 hours from now". No one ask me for my flux capacitor, I'm not sharing! (also this is going to be a confusing thread to anyone who looks at it in the future) To be clear, this comment should be the first one in the thread. |
I did notice that myself as well :-) I pushed the updated JavaDoc and it looks to be OK now. |
|
|
||
| if (paramInfo.in == In.PATH) { | ||
| parameter.setRequired(true); | ||
| } |
| @@ -68,59 +111,157 @@ public static Schema readSchema(IndexView index, | |||
| schema.setExternalDocs(readExternalDocs(annotation.value(OpenApiConstants.PROP_EXTERNAL_DOCS))); | |||
| schema.setDeprecated((Boolean) overrides.getOrDefault(OpenApiConstants.PROP_DEPRECATED, JandexUtil.booleanValue(annotation, OpenApiConstants.PROP_DEPRECATED))); | |||
| schema.setType((Schema.SchemaType) overrides.getOrDefault(OpenApiConstants.PROP_TYPE, JandexUtil.enumValue(annotation, OpenApiConstants.PROP_TYPE, Schema.SchemaType.class))); | |||
| schema.setEnumeration((List<Object>) overrides.getOrDefault(OpenApiConstants.PROP_ENUM, JandexUtil.stringListValue(annotation, OpenApiConstants.PROP_ENUM))); | |||
| schema.setEnumeration((List<Object>) overrides.getOrDefault(OpenApiConstants.PROP_ENUMERATION, JandexUtil.stringListValue(annotation, OpenApiConstants.PROP_ENUMERATION))); | |||
There was a problem hiding this comment.
Good catch - the OpenAPI data model uses enum for this, which is probably why this was wrong. I guess there is/was no test coverage for this. :(
There was a problem hiding this comment.
This actually caused the TCK to fail with the consolidated version of this method. I think you're right that none of the tests within the implementation project hit this.
| Discriminator discriminator = new DiscriminatorImpl(); | ||
| AnnotationInstance[] nestedArray = value.asNestedArray(); | ||
| for (@SuppressWarnings("unused") AnnotationInstance nested : nestedArray) { | ||
| // TODO iterate the discriminator mappings and do something sensible with them! :( |
|
@msavy or @kenfinnigan - any comments before I merge this? |
…lrye-smallrye-parent-23 Bump smallrye-parent from 20 to 23
This is what I have for the change to use the @Schema name (and issue #25). Most of the real change is in the schema registry, but I also consolidated some duplicate functions and moved some that seemed better placed in the SchemaFactory than in the main scanner. As noted in the code comments, this does not handle merging Schema information found in @components with information found directly on the models.
I think we might need to have a bigger discussion around how best to merge/resolve things within the scanning phase (not to mention the higher-level phases) and whether or not warnings should be logged if the user codes annotations that are not valid or have undefined behavior. For example, a @Schema with a ref that also includes other properties - a good case to issue warning messages? I like the idea of being told that something in an annotation will be ignored/discarded or is otherwise not valid.