Skip to content

[bug][core] Copy all attributes (not properties) on composed schemas when flattening models #7106

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 6 commits into from
Aug 4, 2020
Merged

[bug][core] Copy all attributes (not properties) on composed schemas when flattening models #7106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fa69a7f
[bug] Copy all validations on composed schemas when flattening models
jimschubert Aug 1, 2020
f7f5788
Regenerate samples
jimschubert Aug 1, 2020
eb85d80
Add regression test for attribute copy of flattened composed schemas
jimschubert Aug 2, 2020
3770a2b
Ensure consistent model structure keying with sorted properties in Ja…
jimschubert Aug 2, 2020
410ec31
Merge branch 'master' into jersey-inheritance
jimschubert Aug 3, 2020
a034b52
Remove unnecessary null checks and duplicate setters in InlineModelRe…
jimschubert Aug 4, 2020
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
75 changes: 62 additions & 13 deletions modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@

package org.openapitools.codegen;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.core.util.DefaultPrettyPrinter;
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import io.swagger.v3.core.util.Json;
import io.swagger.v3.oas.models.*;
import io.swagger.v3.oas.models.callbacks.Callback;
Expand All @@ -25,6 +29,7 @@
import io.swagger.v3.oas.models.parameters.RequestBody;
import io.swagger.v3.oas.models.responses.ApiResponse;
import io.swagger.v3.oas.models.responses.ApiResponses;
import org.apache.commons.lang3.StringUtils;
import org.openapitools.codegen.utils.ModelUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -36,6 +41,17 @@ public class InlineModelResolver {
private OpenAPI openapi;
private Map<String, Schema> addedModels = new HashMap<String, Schema>();
private Map<String, String> generatedSignature = new HashMap<String, String>();

// structure mapper sorts properties alphabetically on write to ensure models are
// serialized consistently for lookup of existing models
private static ObjectMapper structureMapper;

static {
structureMapper = Json.mapper().copy();
structureMapper.configure(MapperFeature.SORT_PROPERTIES_ALPHABETICALLY, true);
structureMapper.writer(new DefaultPrettyPrinter());
}

static Logger LOGGER = LoggerFactory.getLogger(InlineModelResolver.class);

void flatten(OpenAPI openapi) {
Expand Down Expand Up @@ -488,15 +504,25 @@ private String resolveModelName(String title, String key) {
}

private String matchGenerated(Schema model) {
String json = Json.pretty(model);
if (generatedSignature.containsKey(json)) {
return generatedSignature.get(json);
try {
String json = structureMapper.writeValueAsString(model);
if (generatedSignature.containsKey(json)) {
return generatedSignature.get(json);
}
} catch (JsonProcessingException e) {
e.printStackTrace();
}

return null;
}

private void addGenerated(String name, Schema model) {
generatedSignature.put(Json.pretty(model), name);
try {
String json = structureMapper.writeValueAsString(model);
generatedSignature.put(json, name);
} catch (JsonProcessingException e) {
e.printStackTrace();
}
}

/**
Expand Down Expand Up @@ -620,22 +646,45 @@ private Schema modelFromProperty(OpenAPI openAPI, Schema object, String path) {
}
XML xml = object.getXml();
Map<String, Schema> properties = object.getProperties();

// NOTE:
// No need to null check setters below. All defaults in the new'd Schema are null, so setting to null would just be a noop.
Schema model = new Schema();
if (object.getType() != null) {
model.setType(object.getType());
}
if (object.getFormat() != null) {
// Even though the `format` keyword typically applies to primitive types only,
// the JSON schema specification states `format` can be used for any model type instance
// including object types.
model.setFormat(object.getFormat());
}
model.setType(object.getType());

// Even though the `format` keyword typically applies to primitive types only,
// the JSON schema specification states `format` can be used for any model type instance
// including object types.
model.setFormat(object.getFormat());

model.setDescription(description);
model.setExample(example);
model.setName(object.getName());
model.setXml(xml);
model.setRequired(object.getRequired());
model.setNullable(object.getNullable());
model.setDiscriminator(object.getDiscriminator());
Copy link
Contributor

@spacether spacether Aug 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So I have some questions about how our code works here. Can you help answer them?

  1. This copies the property from the inline schema into the extracted schema that we put into openapi component schemas, right?
    So it sets the discriminator on this schema:
      - type: object
        discriminator: party_type
        required:
          - party_type
        properties:
          party_type:
            readOnly: true
            $ref: '#/definitions/PartyType'
          tax_id_number:
            type: string

Which is extracted and used to create the schema with the key Party_allOf, correct?
2. Is only one Party_allOf class made for each composed schema or is each inline schema extracted out into its own Party_allOf1, Party_allOf2 etc?
3. How about adding a javadoc to this method so one can understand this by reading the doc in the future?

Copy link
Member Author

@jimschubert jimschubert Aug 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Party_allOf will be created once, then any properties it references will be flattened. Later, we "lookup" the transient Party_allOf by serializing and deserializing the structure as the key of a map, and the value as a name. This is how we basically deduplicate those dynamic structures and associate them all with a generated name. See structureMapper in this PR which I've updated to have determinate property order. As it was, Jackson would probably serialize in the same way maybe 99% of the time, but without explicitly sorting it'll follow the same rules as JSON where map properties are technically unordered.

model.setWriteOnly(object.getWriteOnly());
model.setUniqueItems(object.getUniqueItems());
model.setTitle(object.getTitle());
model.setReadOnly(object.getReadOnly());
model.setPattern(object.getPattern());
model.setNot(object.getNot());
model.setMinProperties(object.getMinProperties());
model.setMinLength(object.getMinLength());
model.setMinItems(object.getMinItems());
model.setMinimum(object.getMinimum());
model.setMaxProperties(object.getMaxProperties());
model.setMaxLength(object.getMaxLength());
model.setMaxItems(object.getMaxItems());
model.setMaximum(object.getMaximum());
model.setExternalDocs(object.getExternalDocs());
model.setExtensions(object.getExtensions());
model.setExclusiveMinimum(object.getExclusiveMinimum());
model.setExclusiveMaximum(object.getExclusiveMaximum());
model.setExample(object.getExample());
model.setDeprecated(object.getDeprecated());

if (properties != null) {
flattenProperties(openAPI, properties, path);
model.setProperties(properties);
Expand Down
108 changes: 108 additions & 0 deletions ...les/openapi-generator/src/test/java/org/openapitools/codegen/InlineModelResolverTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -785,6 +785,109 @@ public void objectComposedWithInline() {
checkComposedChildren(openAPI, schema.getOneOf(), "oneOf");
}


@Test
public void inheritanceWithInlineDiscriminator() {
OpenAPI openAPI = TestUtils.parseSpec("src/test/resources/2_0/regression_6905.yaml");
new InlineModelResolver().flatten(openAPI);

assertTrue(openAPI.getComponents().getSchemas().get("PartyType") instanceof StringSchema);
assertTrue(openAPI.getComponents().getSchemas().get("CustomerType") instanceof StringSchema);
assertTrue(openAPI.getComponents().getSchemas().get("Entity") instanceof ObjectSchema);

assertTrue(openAPI.getComponents().getSchemas().get("Party") instanceof ComposedSchema);
assertTrue(openAPI.getComponents().getSchemas().get("Contact") instanceof ComposedSchema);
assertTrue(openAPI.getComponents().getSchemas().get("Customer") instanceof ComposedSchema);
assertTrue(openAPI.getComponents().getSchemas().get("Person") instanceof ComposedSchema);
assertTrue(openAPI.getComponents().getSchemas().get("Organization") instanceof ComposedSchema);

assertTrue(openAPI.getComponents().getSchemas().get("ApiError") instanceof ObjectSchema);

assertFalse(openAPI.getComponents().getSchemas().get("Party_allOf") instanceof ComposedSchema);
assertFalse(openAPI.getComponents().getSchemas().get("Contact_allOf") instanceof ComposedSchema);
assertFalse(openAPI.getComponents().getSchemas().get("Customer_allOf") instanceof ComposedSchema);
assertFalse(openAPI.getComponents().getSchemas().get("Person_allOf") instanceof ComposedSchema);
assertFalse(openAPI.getComponents().getSchemas().get("Organization_allOf") instanceof ComposedSchema);

// Party
ComposedSchema party = (ComposedSchema) openAPI.getComponents().getSchemas().get("Party");
List<Schema> partySchemas = party.getAllOf();
Schema entity = ModelUtils.getReferencedSchema(openAPI, partySchemas.get(0));
Schema partyAllOf = ModelUtils.getReferencedSchema(openAPI, partySchemas.get(1));

assertEquals(partySchemas.get(0).get$ref(), "#/components/schemas/Entity");
assertEquals(partySchemas.get(1).get$ref(), "#/components/schemas/Party_allOf");

assertNull(party.getDiscriminator());
assertNull(entity.getDiscriminator());
assertNotNull(partyAllOf.getDiscriminator());
assertEquals(partyAllOf.getDiscriminator().getPropertyName(), "party_type");
assertEquals(partyAllOf.getRequired().get(0), "party_type");


// Contact
ComposedSchema contact = (ComposedSchema) openAPI.getComponents().getSchemas().get("Contact");
Schema contactAllOf = ModelUtils.getReferencedSchema(openAPI, contact.getAllOf().get(1));

assertEquals(contact.getExtensions().get("x-discriminator-value"), "contact");
assertEquals(contact.getAllOf().get(0).get$ref(), "#/components/schemas/Party");
assertEquals(contact.getAllOf().get(1).get$ref(), "#/components/schemas/Contact_allOf");
assertNull(contactAllOf.getDiscriminator());


// Customer
ComposedSchema customer = (ComposedSchema) openAPI.getComponents().getSchemas().get("Customer");
List<Schema> customerSchemas = customer.getAllOf();
Schema customerAllOf = ModelUtils.getReferencedSchema(openAPI, customerSchemas.get(1));

assertEquals(customerSchemas.get(0).get$ref(), "#/components/schemas/Party");
assertNull(customer.getDiscriminator());
assertEquals(customer.getExtensions().get("x-discriminator-value"), "customer");

// Discriminators are not defined at this level in the schema doc
assertNull(customerSchemas.get(0).getDiscriminator());
assertEquals(customerSchemas.get(1).get$ref(), "#/components/schemas/Customer_allOf");
assertNull(customerSchemas.get(1).getDiscriminator());

// Customer -> Party where Customer defines it's own discriminator
assertNotNull(customerAllOf.getDiscriminator());
assertEquals(customerAllOf.getDiscriminator().getPropertyName(), "customer_type");
assertEquals(customerAllOf.getRequired().get(0), "customer_type");


// Person
ComposedSchema person = (ComposedSchema) openAPI.getComponents().getSchemas().get("Person");
List<Schema> personSchemas = person.getAllOf();
Schema personAllOf = ModelUtils.getReferencedSchema(openAPI, personSchemas.get(1));

// Discriminators are not defined at this level in the schema doc
assertEquals(personSchemas.get(0).get$ref(), "#/components/schemas/Customer");
assertNull(personSchemas.get(0).getDiscriminator());
assertEquals(personSchemas.get(1).get$ref(), "#/components/schemas/Person_allOf");
assertNull(personSchemas.get(1).getDiscriminator());

// Person -> Customer -> Party, so discriminator is not at this level
assertNull(person.getDiscriminator());
assertEquals(person.getExtensions().get("x-discriminator-value"), "person");
assertNull(personAllOf.getDiscriminator());


// Organization
ComposedSchema organization = (ComposedSchema) openAPI.getComponents().getSchemas().get("Organization");
List<Schema> organizationSchemas = organization.getAllOf();
Schema organizationAllOf = ModelUtils.getReferencedSchema(openAPI, organizationSchemas.get(1));

// Discriminators are not defined at this level in the schema doc
assertEquals(organizationSchemas.get(0).get$ref(), "#/components/schemas/Customer");
assertNull(organizationSchemas.get(0).getDiscriminator());
assertEquals(organizationSchemas.get(1).get$ref(), "#/components/schemas/Organization_allOf");
assertNull(organizationSchemas.get(1).getDiscriminator());

// Organization -> Customer -> Party, so discriminator is not at this level
assertNull(organizationAllOf.getDiscriminator());
assertEquals(organization.getExtensions().get("x-discriminator-value"), "organization");
}

@Test
public void arbitraryObjectModelWithArrayInlineWithTitle() {
OpenAPI openAPI = TestUtils.parseSpec("src/test/resources/3_0/inline_model_resolver.yaml");
Expand Down Expand Up @@ -888,4 +991,9 @@ public void callbacks() {
assertTrue(properties.get("action") instanceof StringSchema);
assertTrue(properties.get("data") instanceof StringSchema);
}

@Test
public void regresssion_6905() {

}
}
Loading