Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 4, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c79a557
Adds comment in java test where we need to check the fruit model
spacether Nov 13, 2019
bb6321b
Adds oneOf test to DefaultCodegenTest.java, adds code to include prop…
spacether Nov 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// uncomment this when https://github.com/swagger-api/swagger-parser/issues/1252 is resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// if schema has additionalproperties outside of allOf/oneOf/anyOf also add it to m
// if (schema.getAdditionalProperties() != null) {
// addAdditionPropertiesToCodeGenModel(m, schema);
// }

// parent model
final String parentName = ModelUtils.getParentName(composed, allDefinitions);
final List<String> allParents = ModelUtils.getAllParentsName(composed, allDefinitions);
Expand Down
35 changes: 35 additions & 0 deletions modules/openapi-generator/src/test/java/org/openapitools/codegen/DefaultCodegenTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,41 @@ public void testGetSchemaTypeWithComposedSchemaWithOneOf() {
Assert.assertEquals(type, "oneOf<ObjA,ObjB>");
}

@Test
public void testComposedSchemaOneOfWithProperties() {
final OpenAPI openAPI = TestUtils.parseSpec("src/test/resources/3_0/oneOf.yaml");
final DefaultCodegen codegen = new DefaultCodegen();

final Schema schema = openAPI.getComponents().getSchemas().get("fruit");
codegen.setOpenAPI(openAPI);
CodegenModel fruit = codegen.fromModel("Fruit", schema);

Set<String> oneOf = new TreeSet<String>();
oneOf.add("Apple");
oneOf.add("Banana");
Assert.assertEquals(fruit.oneOf, oneOf);
Assert.assertEquals(fruit.optionalVars.size(), 3);
Assert.assertEquals(fruit.vars.size(), 3);
// make sure that fruit has the property color
boolean colorSeen = false;
for (CodegenProperty cp : fruit.vars) {
if (cp.name.equals("color")) {
colorSeen = true;
break;
}
}
Assert.assertTrue(colorSeen);
colorSeen = false;
for (CodegenProperty cp : fruit.optionalVars) {
if (cp.name.equals("color")) {
colorSeen = true;
break;
}
}
Assert.assertTrue(colorSeen);
}


@Test
public void testEscapeText() {
final DefaultCodegen codegen = new DefaultCodegen();
Expand Down
3 changes: 3 additions & 0 deletions modules/openapi-generator/src/test/resources/3_0/oneOf.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ components:
oneOf:
- $ref: '#/components/schemas/apple'
- $ref: '#/components/schemas/banana'
# additionalProperties:
# type: string
# uncomment this when https://github.com/swagger-api/swagger-parser/issues/1252 is resolved
Comment on lines +25 to +27
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

apple:
title: apple
type: object
Expand Down