Skip to content
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.

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

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

Merged
merged 6 commits into from
Aug 4, 2020

Conversation

jimschubert
Copy link
Member

Fixes #6950

The model flattening for composed schemas was not copying all attributes of the schema being flattened. This broke inheritance across composed schemas, which is a feature that is supported by the specification.

cc @OpenAPITools/generator-core-team
cc @jeff9finger

Example output showing proper inheritance using spec from #6950:

image

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
  • Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@auto-labeler
Copy link

auto-labeler bot commented Aug 1, 2020

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

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

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

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.

* master:
  [core] Add type and format properties to model of inline response (#6153)
  [PowerShell] better publishing workflow (#7114)
  [aspnetcore] Typo issues in docs and generated code (#7094)
  fix http signaure auth in build.sbt (#7110)
  fix for the issue facing spec invlolving arrayschema structure with ref (#6310)
  [csharp-netcore] added cancellation tokens to async calls (#7077)
  [PS] Allow CI to publish the module (#7091)
  [Dart] Treat float as double (#6924)
  [Java][jersey2]Fix gradle HttpSignatureAuth dependencies (#7096)
  move maven,gradle tests to shipppable ci (#7108)
@jimschubert
Copy link
Member Author

This PR adds to some work done in #6153, which caused master CircleCI to break due to two new properties in a test yaml file and type: object added to generated code for those properties.

These are fixed by this PR, so I'll go ahead and merge. If any reviewers have concerns we can address in a follow up change.

@jimschubert jimschubert merged commit 7a846a1 into master Aug 4, 2020
@jimschubert jimschubert deleted the jersey-inheritance branch August 4, 2020 01:16
jimschubert added a commit that referenced this pull request Aug 12, 2020
* master: (129 commits)
  [typescript-axios] add promise to bearer and oauth tokens (#7132)
  update doc
  [REQ] Added enumClassPrefix option to Go server generation (#7008)
  [Java][jersey2] Add helper methods for oneOf Java classes (#7115)
  [Kotlin][Retrofit2] fix missing import for file (#7121)
  adding handling for epoch dates in javascript ApiClient mustache file (#6497) (#6504)
  update doc
  comment out cpanm in travis
  [Kotlin] Rxjava3 support (#6998)
  [BUG][JAVA] Fix error handling in okhttp-gson async api client (#7089)
  Update to reset httpRepsonse.Body (#6948)
  [php-lumen] Set required PHP version to ^7.2.5 (#6949)
  [contrib][docs] Assert importance of title/description/repro steps (#7103)
  ISSUE-4222: Prevent conflicts with accept(s) local variables in generated Java RestTemplate ApiClient (#7101)
  [bug][core] Copy all attributes (not properties) on composed schemas when flattening models (#7106)
  [core] Add type and format properties to model of inline response (#6153)
  [PowerShell] better publishing workflow (#7114)
  [aspnetcore] Typo issues in docs and generated code (#7094)
  fix http signaure auth in build.sbt (#7110)
  fix for the issue facing spec invlolving arrayschema structure with ref (#6310)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Multi level inheritance (with discriminator declared) does not generate classes with inheritance
2 participants