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 4 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
58 changes: 54 additions & 4 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 @@ -627,6 +653,30 @@ private Schema modelFromProperty(OpenAPI openAPI, Schema object, String path) {
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.setType(object.getType());
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.setFormat(object.getFormat());
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 @@ -768,6 +768,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 @@ -871,4 +974,9 @@ public void callbacks() {
assertTrue(properties.get("action") instanceof StringSchema);
assertTrue(properties.get("data") instanceof StringSchema);
}

@Test
public void regresssion_6905() {

}
}
172 changes: 172 additions & 0 deletions modules/openapi-generator/src/test/resources/2_0/regression_6905.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
swagger: '2.0'

info:
title: Test Command model generation
description: Test Command model generation
version: 1.0.0
definitions:
PartyType:
description: type
type: string
enum:
- customer
- contact

CustomerType:
description: type
type: string
enum:
- person
- organization

Entity:
type: object
properties:
id:
type: string
readOnly: true

Party:
allOf:
- $ref: '#/definitions/Entity'
- type: object
discriminator: party_type
required:
- party_type
properties:
party_type:
readOnly: true
$ref: '#/definitions/PartyType'
tax_id_number:
type: string

Contact:
x-discriminator-value: contact
allOf:
- $ref: '#/definitions/Party'
- type: object
properties:
first_name:
type: string
last_name:
type: string
suffix:
type: string
dob:
type: string
format: date

Customer:
x-discriminator-value: customer
allOf:
- $ref: '#/definitions/Party'
- type: object
discriminator: customer_type
required:
- customer_type
properties:
customer_type:
readOnly: true
$ref: '#/definitions/CustomerType'
customer_num:
type: string
external_customer_num:
type: string
Person:
x-discriminator-value: person
allOf:
- $ref: '#/definitions/Customer'
- type: object
properties:
first_name:
type: string
last_name:
type: string

Organization:
x-discriminator-value: organization
allOf:
- $ref: '#/definitions/Customer'
- type: object
required:
- organization_name
properties:
organization_name:
type: string

ApiError:
type: object
required:
- code
- message
properties:
code:
type: string
readOnly: true
message:
type: string
readOnly: true

paths:
/customers:
get:
consumes: []
operationId: queryCustomers
tags:
- Customer
summary: Get customers
responses:
200:
description: Success
schema:
type: array
items:
$ref: '#/definitions/Customer'
400:
description: Bad request.
schema:
$ref: '#/definitions/ApiError'
default:
description: Unknown error.
schema:
$ref: '#/definitions/ApiError'
/contacts:
get:
consumes: []
operationId: queryContacts
tags:
- Contact
summary: Get contact
responses:
200:
description: Success
schema:
type: array
items:
$ref: '#/definitions/Contact'
400:
description: Bad request.
schema:
$ref: '#/definitions/ApiError'
default:
description: Unknown error.
schema:
$ref: '#/definitions/ApiError'
/parties:
get:
consumes: []
responses:
200:
description: Success
schema:
type: array
items:
$ref: '#/definitions/Party'
400:
description: Bad request.
schema:
$ref: '#/definitions/ApiError'
default:
description: Unknown error.
schema:
$ref: '#/definitions/ApiError'
Loading