[Java/Core] Adds properties to ComposedSchema models when they exist #4482

spacether · 2019-11-13T18:32:05Z

This PR fixes this issue:
#4515

Currently, when we process composedSchemas, we are not including any properties that are described in the properties section 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.

openapi: 3.0.1
info:
   title: fruity
   version: 0.0.1
paths:
   /:
      get:
         responses:
            '200':
               description: desc
               content:
                  application/json:
                     schema:
                        $ref: '#/components/schemas/fruit'
components:
   schemas:
      fruit:
         title: fruit
         properties:
            color:
               type: string
         oneOf:
            - $ref: '#/components/schemas/apple'
            - $ref: '#/components/schemas/banana'
resolved
      apple:
         title: apple
         type: object
         properties:
            kind:
               type: string
      banana:
         title: banana
         type: object
         properties:
            count:
               type: number

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

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./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.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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)

…erties in composed schema

lwlee2608

Looks good to me. It is is identical with my PR #4516 . I shall cancel my PR since this is done earlier.

I believe you should cc the core member instead of java technical committee.

spacether · 2019-11-18T04:42:21Z

Looks good to me. It is is identical with my PR #4515. I shall cancel my PR since this is done earlier.

I believe you should cc the core member instead of java technical committee.

Ah, didn't know that. I just cced them, thanks!

spacether · 2019-11-22T17:41:41Z

Core Team: @wing328 @jimschubert @cbornet @ackintosh @jmini @etherealjoy
Gentle reminder that this still needs a review

it fixes [Core] oneOf schema properties #4515
the PR includes a test at modules/openapi-generator/src/test/java/org/openapitools/codegen/DefaultCodegenTest.java
this fixes an issue that swagger-codegen already fixed here Adding support for properties in ComposedModel swagger-api/swagger-codegen#9827

jeff9finger

LGTM 👍

spacether · 2019-11-30T19:45:50Z

@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?

jimschubert

Awesome.

jimschubert · 2019-12-04T02:31:20Z

modules/openapi-generator/src/test/resources/3_0/oneOf.yaml

+         # additionalProperties:
+         #    type: string
+         # uncomment this when https://github.com/swagger-api/swagger-parser/issues/1252 is resolved


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.

jimschubert · 2019-12-04T02:32:04Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

+                addVars(m, unaliasPropertySchema(schema.getProperties()), schema.getRequired(), null, null);
+            }
+
+            // uncomment this when https://github.com/swagger-api/swagger-parser/issues/1252 is resolved


See my comment on the example spec document in this PR.

jimschubert · 2019-12-04T02:39:58Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

@@ -1787,6 +1787,17 @@ public CodegenModel fromModel(String name, Schema schema) {
            Map<String, Schema> allProperties = new LinkedHashMap<String, Schema>();
            List<String> allRequired = new ArrayList<String>();

+            // 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);


@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 · 2019-12-04T02:40:36Z

This spec passes validation at: https://apitools.dev/swagger-parser/online/#

Just a heads up that our CLI also validates:

openapi-generator validate -i openapi.yml

spacether added 2 commits November 12, 2019 18:17

Adds comment in java test where we need to check the fruit model

c79a557

Adds oneOf test to DefaultCodegenTest.java, adds code to include prop…

bb6321b

…erties in composed schema

spacether changed the title ~~Adds properties to ComposedSchema models when they exist~~ [Java] Adds properties to ComposedSchema models when they exist Nov 13, 2019

spacether mentioned this pull request Nov 17, 2019

[Core] oneOf schema properties #4515

Closed

5 tasks

lwlee2608 reviewed Nov 18, 2019

View reviewed changes

spacether changed the title ~~[Java] Adds properties to ComposedSchema models when they exist~~ [Java/Core] Adds properties to ComposedSchema models when they exist Nov 18, 2019

lwlee2608 mentioned this pull request Nov 18, 2019

Fix oneOf schema properties #4516

Closed

5 tasks

jeff9finger approved these changes Nov 22, 2019

View reviewed changes

jimschubert added Client: Java Enhancement: General labels Dec 4, 2019

jimschubert added this to the 4.2.3 milestone Dec 4, 2019

jimschubert approved these changes Dec 4, 2019

View reviewed changes

jimschubert reviewed Dec 4, 2019

View reviewed changes

jimschubert merged commit c882338 into OpenAPITools:master Dec 4, 2019

spacether deleted the fixes_oneof_class branch December 4, 2019 03:01

jimschubert mentioned this pull request Dec 4, 2019

[core] Provide User feedback on confusing use of oneOf #4695

Merged

5 tasks

spacether restored the fixes_oneof_class branch February 3, 2020 17:48

spacether deleted the fixes_oneof_class branch April 30, 2020 03:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Java/Core] Adds properties to ComposedSchema models when they exist #4482

[Java/Core] Adds properties to ComposedSchema models when they exist #4482

spacether commented Nov 13, 2019 •

edited

Loading

lwlee2608 left a comment •

edited

Loading

spacether commented Nov 18, 2019

spacether commented Nov 22, 2019 •

edited

Loading

jeff9finger left a comment

spacether commented Nov 30, 2019

jimschubert left a comment

jimschubert Dec 4, 2019

jimschubert Dec 4, 2019

jimschubert Dec 4, 2019

jimschubert commented Dec 4, 2019

[Java/Core] Adds properties to ComposedSchema models when they exist #4482

[Java/Core] Adds properties to ComposedSchema models when they exist #4482

Conversation

spacether commented Nov 13, 2019 • edited Loading

lwlee2608 left a comment • edited Loading

Choose a reason for hiding this comment

spacether commented Nov 18, 2019

spacether commented Nov 22, 2019 • edited Loading

jeff9finger left a comment

Choose a reason for hiding this comment

spacether commented Nov 30, 2019

jimschubert left a comment

Choose a reason for hiding this comment

jimschubert Dec 4, 2019

Choose a reason for hiding this comment

jimschubert Dec 4, 2019

Choose a reason for hiding this comment

jimschubert Dec 4, 2019

Choose a reason for hiding this comment

jimschubert commented Dec 4, 2019

spacether commented Nov 13, 2019 •

edited

Loading

lwlee2608 left a comment •

edited

Loading

spacether commented Nov 22, 2019 •

edited

Loading