Uh oh!
There was an error while loading. Please reload this page.
[Java/Core] Adds properties to ComposedSchema models when they exist - #4482
Conversation
spacether
commented
Nov 18, 2019
Ah, didn't know that. I just cced them, thanks! |
Core Team: @wing328@jimschubert@cbornet@ackintosh@jmini@etherealjoy
|
spacether
commented
Nov 30, 2019
@wing328 these changes were approved 8 days ago. Is there a milestone that they can be added to? Is any more work required in the PR? |
| # additionalProperties: | ||
| # type: string | ||
| # uncomment this when https://github.com/swagger-api/swagger-parser/issues/1252 is resolved |
There was a problem hiding this comment.
I wouldn't imagine this will get fixed anytime soon in swagger-parser for the 3.0 specification (the revision versions 3.0.1/3.0.2 are supposedly documentation-only changes).
The behavior you're seeing where this returns MapSchema is defined behavior: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#model-with-mapdictionary-properties
The requested behavior is undefined, as there's not enough clarity in the specification around constraints of using oneOf. It should be that oneOf can only coexist with discriminator.
oneOf is taken from JSON Schema's specification Section 9.2.1.3, which says:
This keyword's value MUST be a non-empty array. Each item of the array MUST be a valid JSON Schema.
An instance validates successfully against this keyword if it validates successfully against exactly one schema defined by this keyword's value.
Meaning your example is only a valid object if it is exactly apple or exactly banana, and no dynamic properties via additionalProperties are allowed due to the use of MUST rather than SHOULD in the JSON Schema specification.
The discriminator support for oneOf is a JSON Schema extension defined in the OAS Composition and Inheritance (Polymorphism) section.
Ideally, a spec with oneOf any anything besides discriminator should fail validation. We could consider adding this strict validation to OpenAPI Generator, or at least as a recommendation via the --recommend flag to the validate command.
| addVars(m, unaliasPropertySchema(schema.getProperties()), schema.getRequired(), null, null); | ||
| } | ||
| // uncomment this when https://github.com/swagger-api/swagger-parser/issues/1252 is resolved |
There was a problem hiding this comment.
See my comment on the example spec document in this PR.
| // if schema has properties outside of allOf/oneOf/anyOf also add them to m | ||
| if (schema.getProperties() != null) { | ||
| addVars(m, unaliasPropertySchema(schema.getProperties()), schema.getRequired(), null, null); |
There was a problem hiding this comment.
@wing328 See my other comment in this PR. Constraints around structures containing oneOf aren't very clearly defined in OAS 3.x, since the extension to include discriminator makes it look like any Schema properties are valid.
Do you think we should log a warning here, or wrap it the a ! strict spec check, or both? I've commented elsewhere as well that we might want to add to the validate and/or recommends about this.
jimschubert
commented
Dec 4, 2019
Just a heads up that our CLI also validates: |
This PR fixes this issue:
#4515
Currently, when we process composedSchemas, we are not including any properties that are described in the
propertiessection of that model's schema. An example is the below fruit model, which leaves out the color property when we ingest it and make a model.This spec passes validation at: https://apitools.dev/swagger-parser/online/#
This PR fixes this issue, adding properties to the composed model.
We also add a test in DefaultCodegenTest.java where we check that the fruit model contains the color property.
This issue was also recently noticed and fixed in swagger-codegen at: swagger-api/swagger-codegen#9827
Note: additional properties should also be allowed in composed schemas, but that feature is blocked by an upstream bug in swagger-parser. I've included a commented out additionalproperties property in the fruit spec, commented out code to process it in DefaultCodegen.java. Data in both locations links to the open swagger-parser issue which I created at: swagger-api/swagger-parser#1252
./bin/(or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run./bin/{LANG}-petstore.sh,./bin/openapi3/{LANG}-petstore.shif updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).master,4.3.x,5.0.x. Default:master.Java Technical Committee
@bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10)
Core Team
@wing328 (2015/07) ❤️
@jimschubert (2016/05) ❤️
@cbornet (2016/05)
@ackintosh (2018/02) ❤️
@jmini (2018/04) ❤️
@etherealjoy (2019/06)