Skip to content
GitLab
Projects Groups Snippets
  • /
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in / Register
  • O openapi-generator
  • Project information
    • Project information
    • Activity
    • Labels
    • Members
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
  • Issues 3,476
    • Issues 3,476
    • List
    • Boards
    • Service Desk
    • Milestones
  • Merge requests 402
    • Merge requests 402
  • CI/CD
    • CI/CD
    • Pipelines
    • Jobs
    • Schedules
  • Deployments
    • Deployments
    • Environments
    • Releases
  • Packages and registries
    • Packages and registries
    • Package Registry
    • Infrastructure Registry
  • Monitor
    • Monitor
    • Incidents
  • Analytics
    • Analytics
    • Value stream
    • CI/CD
    • Repository
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Activity
  • Graph
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
Collapse sidebar
  • OpenAPI Tools
  • openapi-generator
  • Merge requests
  • !4906

[CORE] Fixes composed schema discriminator map

  • Review changes

  • Download
  • Email patches
  • Plain diff
Merged Administrator requested to merge github/fork/spacether/issue_4904_fix_composed_schema_discriminator_map into master Jan 01, 2020
  • Overview 0
  • Commits 18
  • Pipelines 0
  • Changes 171

Created by: spacether

This PR improves how we define and handle discriminators. We have added a feature flag legacyDiscriminatorBehavior When it is set to false we:

  • define the CodegenDiscriminator on all models that have a discriminator, including the ones that allOf inherit it from an ancestor
  • fill in the discriminator MappedModels map with data from data from 6 sources (see them described below)
  • validate the discriminator presence: validate that oneOf or anyOf models contain the required discriminator. If they do not contain it helpful errors are thrown.

I am doing this for the the below reasons:

Not all codegen models use extend to make composed models. On generators that don't do that, they could lack the inherited discriminator. Python experimental is an example where we don't use class inheritance to allOf inherit parent classes/models. In that generator, discriminator is defined in each codegenModel, so we should not rely on java's composed schema inheritance solution to set a granparent's discriminator on a child model and a grandchild model that allOf inherit their ancestors.

We also need to do this if we want the values of the CodegenDiscriminator MappedModels to differ from grandparent to parent to child. When we include the below 1-6 sources of MappedModels, the MappedModels map will differ depending upon which model you are looking at (grandparent/parent/child)

My solution is to have the code set the discriminator on each model that contains a discriminator. Prior to this PR models that inherit a discriminator do not contain a discriminator. Then on that CodegenDiscriminator add the following to MappedModels:

  1. any mappings from the current or inherited schema Discriminator Mapping
  2. any x-discriminator-value mappings in:
    • child models that allOf inherit from the the current schema
    • oneOf models in the current schema
    • anyOf models in the current schema
  3. any child models that allOf inherit from the the current schema
  4. any descendent models that allOf inherit from the above child models
  5. oneOf models in the current schema
  6. anyOf models in the current schema

Why Feature Flag This?

  • Some generators (javascript, probably others) implement discriminators differently and do not work if we set the flag to True. Cordoning this off behind the feature flag allows us to fix them later.
  • This is a really big change
  • I want to support legacy users who bring their old mustache files but use the latest generator code
  • Even though I could get all tests to pass, that does not mean that all models are being tested
    • When the flag is true, Java models Cat and Dog now have discriminators with empty maps. I do not know if that is okay.

Background Context:

When using oneOf or anyOf models with a discriminator in a parent composed schema, the discriminator map does not include the child oneOf models. This is because our existing java code was only covering the use case where there are descedant schemas which allOf inherit from the current schemas. It was not aware of and handling cases where the current schema is composed and there are oneOf any anyOf schemas that should be added to the discriminator MappedModels.

Below is an example spec where the oneOf models should be in FruitRequiredDesc's discriminator map:

    FruitType:
      properties:
        fruitType:
          type: string
      required:
      - fruitType
    FruitRequiredDesc:
      oneOf:
        - $ref: '#/components/schemas/AppleRequiredDesc'
        - $ref: '#/components/schemas/BananaRequiredDesc'
      discriminator:
        propertyName: fruitType
    AppleRequiredDesc:
      type: object
      required:
        - seeds
      properties:
        seeds:
          type: integer
      allOf:
        - $ref: '#/components/schemas/FruitType'
    BananaRequiredDesc:
      type: object
      required:
        - length
      properties:
        length:
          type: integer
      allOf:
        - $ref: '#/components/schemas/FruitType'

Test Verification

  • Many tests have been added, see the PR

Related Issues

If merged, this will close out https://github.com/OpenAPITools/openapi-generator/issues/4904

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

Core Team: @wing328 (2015/07) @jimschubert (2016/05) @cbornet (2016/05) @ackintosh (2018/02) @jmini (2018/04) @etherealjoy (2019/06)

Assignee
Assign to
Reviewers
Request review from
Time tracking
Source branch: github/fork/spacether/issue_4904_fix_composed_schema_discriminator_map