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

[codegen][python-experimental] Composed schema with additionalProperties #6290

Merged
merged 146 commits into from
May 28, 2020

Conversation

sebastien-rosset
Copy link
Contributor

@sebastien-rosset sebastien-rosset commented May 13, 2020

Background

As per OAS specification:

  1. Undeclared properties should be allowed when the additionalProperties keyword is not specified, when additionalProperties: true or when additionalProperties: {}. The additionalProperties keyword can also be used to specify a schema for the undeclared properties.
  2. A composed schema may include the additionalProperties keyword, both as a sibling of allOf/oneOf/anyOf, and in the child schemas.
  3. The value of undeclared properties may be the null value.

Changes

  1. Add disallowAdditionalPropertiesIfNotPresent CLI option to control the interpretation of the additionalProperties keyword. See below for more details.
  2. Change the interpretation of the additionalProperties keyword to comply with the OAS 2.0 and 3.x specifications.
    1. Specifically, the following behavior changes have been made and are necessary to comply with the OAS spec:
      1. When the additionalProperties keyword is not present in the OAS document, that should be interpreted as if additionalProperties had been set to true.
      2. The additional properties should be nullable, i.e. the null value should be allowed.
    2. Some people may have built their OAS documents with the understanding that by default, additional properties are not allowed, because they incorrectly assumed the OpenAPITools implementation was correct. To retain the legacy behavior you can set the disallowAdditionalPropertiesIfNotPresent CLI option to true, which is the default for now.
    3. Limitation: due to issue swagger-parser #1369, achieving OAS compliance is not possible when the input document uses the OAS 2.0 schema. For OAS 2.0 documents, the legacy behavior should be used by setting the disallowAdditionalPropertiesIfNotPresent CLI option to true.
  3. By default the CLI option is set to true (i.e. non-compliant behavior, as originally implemented). Initially I wanted to change the default value to false for all code generators, but it's too much work. For now I am only enabling the compliant mode for python-experimental.
  4. Codegen and mustache templates were not properly processing composed schemas that have the additionalProperties keyword set. The issue is specifically resolved for python-experimental.
  5. codegen: add equals and hashCode methods for SemVer class.
  6. python-experimental: add unit test to validate a composed schema with additionalProperties and other use cases related to the additionalProperties keyword.
  7. python-experimental: correctly handle the OAS nullable keyword. When a schema is nullable, the client should accept the null value in the input data.
  8. python-experimental: correctly handle additionalProperties which is referencing a schema with a $ref. Generate imports
  9. python-experimental: correctly generate the data type when additionalProperties references a schema
  10. Add code comments in codegen.

Note: breaking changes are introduced in this PR, but they are necessary to be compliant with the OAS specification.

For example:

    Dog:
      allOf:
        - $ref: '#/components/schemas/Animal'
        - type: object
          properties:
            breed:
              type: string

In the above example, the additionalProperties keyword is not specified, which means by default undeclared properties should be allowed. This equivalent to:

    Dog:
      additionalProperties: true
      allOf:
        - $ref: '#/components/schemas/Animal'
        - type: object
          properties:
            breed:
              type: string

Related Issues and future work

While working on this PR, the following related issues have been identified:

  1. swagger-parser #1378 swagger-v2-converter incorrectly converts 2.0 schemas that do not have the 'type' keyword
  2. swagger-parser #1369: additionalProperties not set correctly if omitted
  3. Once the above issues have been fixed, some of the code in this PR can be disabled. Specifically it will no longer be necessary to differentiate between OAS 2.0 docs and OAS 3.0 docs.
  4. swagger-parser #1374: when a OAS 2.0 doc is converted, it would be helpful to retain the "2.0" version in a vendor extension, that would make it possible to know the original OAS doc was 2.0, not 3.x. That can help to drive some of the processing logic.
  5. Support for composed schemas that use the additionalProperties keyword is limited to python-experimental for now. Significant changes may have to be done in other languages. For example, in the Java clients, single inheritance is used, that causes a conflict between inheriting the allOf parent and the HashMap parent.
  6. Some of the names in samples OAS documents should be changed to better reflect the intent and behavior. See discussion in this PR for more details.
  7. Issue [BUG] [Java] Empty object property not working correctly #6400

PR checklist

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

@auto-labeler
Copy link

auto-labeler bot commented May 13, 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.

@sebastien-rosset
Copy link
Contributor Author

sebastien-rosset commented May 14, 2020

@wing328 , I think the addAdditionPropertiesToCodeGenModel function should be called for composed schemas as well, not just map schemas. AFAIK, the additionalProperties keyword can be used for type: object but also for composed schemas. What do you think?

@sebastien-rosset sebastien-rosset changed the title [python-experimental] Composed schema with additionalProperties [codegen][python-experimental] Composed schema with additionalProperties May 14, 2020
@sebastien-rosset
Copy link
Contributor Author

sebastien-rosset commented May 14, 2020

I think it's not a good idea to use single class inheritance to implement additionalProperties (e.g. Java client). That may work for non-composed schemas, but that does not work for composed 'allOf' schemas. For example, in Java, if additionalProperties is set to true (which it should be by default, per OAS spec), then the generated code has extends HashMap<String, V>. That clearly won't work for composed 'allOf' schemas.

Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you for the PR!

@wing328 wing328 merged commit f7f4141 into OpenAPITools:master May 28, 2020
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request May 30, 2020
* master:
  [PS] Refactor the http signing auth with ecdsa support (OpenAPITools#6397)
  [Rust Server] Hyper 0.13 + Async/Await support (OpenAPITools#6244)
  [Rust] set supportAsync to true as the default (OpenAPITools#6480)
  [php-symfony] Set required PHP version ^7.1.3 (OpenAPITools#6181)
  update doc
  [csharp] Rename netstandard to netstandard1.3 (OpenAPITools#6460)
  feat: support deprecated parameters for typescript-axios generator (OpenAPITools#6475)
  [REQ][typescript-axios] useSingleRequestParameter should mark parameter optional if all properties are optional (OpenAPITools#6477)
  better struct alias in rust (OpenAPITools#6470)
  Migrate Go server samples to OAS 3 only (OpenAPITools#6471)
  [Rust][reqwest] add async support (OpenAPITools#6464)
  [codegen][python-experimental] Composed schema with additionalProperties  (OpenAPITools#6290)
  [Java] Decommission Retrofit 1.x support (OpenAPITools#6447)
  remove scala oas3 samples (OpenAPITools#6446)
  [Java][jersey2] Fix RuntimeException when HTTP signature parameters are not configured (OpenAPITools#6457)
  [Java][jersey2] Improve http signature code comments (OpenAPITools#6463)
  [typescript-angular] drop support of angular below 6.0.0 (OpenAPITools#6360)
  [cli] new 'author template' command (OpenAPITools#6441)
  python-experimental updates ancestor + adds descendant discriminator tests (OpenAPITools#6417)
jimschubert added a commit that referenced this pull request May 30, 2020
* master: (36 commits)
  Improve handling spaces in example command (#6482)
  fix maven plugin snapshot version
  comment out erlang server test (#6499)
  Migrate Perl samples to use OAS v3 spec (#6490)
  [core] Refactor templating management (#6357)
  migrate apex samples to use oas3 spec (#6488)
  add new file in php-symfony sample
  [PS] Refactor the http signing auth with ecdsa support (#6397)
  [Rust Server] Hyper 0.13 + Async/Await support (#6244)
  [Rust] set supportAsync to true as the default (#6480)
  [php-symfony] Set required PHP version ^7.1.3 (#6181)
  update doc
  [csharp] Rename netstandard to netstandard1.3 (#6460)
  feat: support deprecated parameters for typescript-axios generator (#6475)
  [REQ][typescript-axios] useSingleRequestParameter should mark parameter optional if all properties are optional (#6477)
  better struct alias in rust (#6470)
  Migrate Go server samples to OAS 3 only (#6471)
  [Rust][reqwest] add async support (#6464)
  [codegen][python-experimental] Composed schema with additionalProperties  (#6290)
  [Java] Decommission Retrofit 1.x support (#6447)
  ...
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.

3 participants