[core] [regression] set parentName when a single possible parent exists #3771

mnahkies · 2019-08-27T21:19:00Z

PR checklist

Read the contribution guidelines.
Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Whilst the spec states that the 'allOf' relationship does not imply a hierarchy:

While composition offers model extensibility, it does not imply a hierarchy between the models.
To support polymorphism, the OpenAPI Specification adds the discriminator field.

Unfortunately this does not make sense for many existing use cases, that were supported by older
versions of the generator. Therefore, I've restored the older behavior, specifically
in the case that only a single possible parent schema is present.

I think a more complete solution would generate interfaces for the composed schemas,
and mark the generated class as implementing these.

@wing328 I guess you're probably the best person to review this, given that you overhauled the support for anyOf, allOf, oneOf originally.

I'd be interested if you had an alternative suggestion for achieving this without requiring the parent schema to know the identity of all the extending schema's - in our real definition collection, we have a set of common models that many different entities extend from in other definition files, and we have also build some common library functions that we use on the generated sub classes that accept objects extending from the parent.

Admittedly, our method of using allOf to express this feels awkward. Ideally I'd like a way to generate a generic Page<T> class that could then be used as Page<Cat>, Page<Dog>, etc but I'm not aware of anyway to do this with the openapi spec / code gen tooling.

Example YAML:

openapi: 3.0.0
info:
  title: example
  description: example
  version: 0.0.0
paths:
  /test:
    get:
      tags:
        - Test
      summary: test
      description: test
      operationId: getCats
      responses:
        '200':
          description: successful
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/PagedCats'
components:
  schemas:
    Page:
      properties:
        page:
          type: integer
      required:
        - page
    Cat:
      properties:
        id:
          type: string
          format: uuid
        required:
          - id
    Dog:
      properties:
        id:
          type: string
          format: uuid
        required:
          - id
    PagedCats:
      allOf:
        - $ref: '#/components/schemas/Page'
        - properties:
            items:
              type: array
              items:
                $ref: '#/components/schemas/Cat'
          required:
            - items
    PagedDogs:
      allOf:
        - $ref: '#/components/schemas/Page'
        - properties:
            items:
              type: array
              items:
                $ref: '#/components/schemas/Dog'
          required:
            - items

fixes #2845, and fixes #3523

auto-labeler · 2019-08-27T21:19:05Z

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

macjohnny

LGTM

macjohnny · 2019-08-28T04:48:18Z

@mnahkies thanks a lot for your PR!

macjohnny · 2019-08-28T04:49:30Z

cc @OpenAPITools/generator-core-team

modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java

wing328 · 2019-08-28T06:59:04Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java

                    }
                } else {
                    // not a ref, doing nothing
                }
            }
        }

+        // parent name only makes sense when there is a single obvious parent
+        if (refedParentNames.size() == 1) {
+            return refedParentNames.get(0);


Can we throw a warning about this behavior and advise the user to use "discriminator.propertyName" in OAS v3 instead to set a proper parent model as such behavior will be removed in the future?

Our goal is to move users to adopt OAS v3 instead of staying with OAS v2 (which is not the best specification to work with inheritance/composition)

Sure, I'd still be interested in how you would recommend applying the discriminator approach to a collection of schema's like I describe in the MR description - the parent in this case should not have knowledge of its children, and also I don't particularly want to add an arbitrary field like type: string solely for code generation purposes.

I'd be more than happy to refactor our definitions to do things the OAS v3 way over time - in general I've found v3 much better to work with than v2 ever was.

we should raise an issue at the openapi 3.0 specification to allow this type of inheritance without discriminator

Another way is to use vendor extension to describe this if the OAS 3.0 spec doesn't officially support it at the moment.

(nullable was not supported in OAS v2 and we'd to use x-nullable: true before OAS v3 officially supports it)

this is a great idea, and probably the easiest way to support this behavior in the future.

That sounds like a good plan. @macjohnny would you be comfortable raising such an issue? I don't really have the bandwidth to write a well thought proposal at this time.

Also I've added a warning, and fixed the commit author (thanks for pointing that out). Let me know if you're happy with the wording

Btw, if this fallback is breaking some existing use cases (allOf with just one schema), we will need to add an option later to cater to both cases.

@wing328 in regards to your #3771 (comment) and mine #3771 (comment) I assume that's what is happening in my case.

wing328 · 2019-08-28T07:02:48Z

@mnahkies thanks a lot for the PR 👍

I've left some comments. Please take a look when you've time.

wing328 · 2019-08-28T08:04:00Z

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

Whilst the spec states that the 'allOf' relationship does not imply a hierarchy: > While composition offers model extensibility, it does not imply a hierarchy between the models. > To support polymorphism, the OpenAPI Specification adds the discriminator field. Unfortunately this does not make sense for many existing use cases, that were supported by older versions of the generator. Therefore, I've restored the older behavior, specifically in the case that only a single possible parent schema is present. I think a more complete solution would generate interfaces for the composed schemas, and mark the generated class as implementing these. fixes issue 2845, and fixes issue OpenAPITools#3523

macjohnny · 2019-08-28T12:06:51Z

@wing328 can we merge this?

wing328 · 2019-08-28T12:31:33Z

Yes, the revised change/message looks good.

smasala · 2019-10-04T10:25:10Z

@wing328 @mnahkies this commit breaks typescript generation for models which only have a single $ref item listed

Example

MyModel:
      description: a model
      allOf:
        - $ref: '#/components/schemas/MyOtherModel'

Generated output

import { SomeImportPropertyType } from './propModel';    // this is correct

/**
 * MyModel
 */
export interface MyModel { 
}
export namespace MyModel {
}

wing328 · 2020-04-20T04:25:24Z

Unfortunately this does not make sense for many existing use cases, that were supported by older
versions of the generator. Therefore, I've restored the older behavior, specifically
in the case that only a single possible parent schema is present.

Heads-up: we'll remove this "old" behavior in 5.0.x (the upcoming major release)

auto-labeler bot added the Enhancement: General label Aug 27, 2019

mnahkies mentioned this pull request Aug 27, 2019

[BUG] Inheritance is broken #2845

Closed

5 tasks

macjohnny approved these changes Aug 28, 2019

View reviewed changes

macjohnny added this to the 4.1.2 milestone Aug 28, 2019

wing328 reviewed Aug 28, 2019

View reviewed changes

modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java Show resolved Hide resolved

wing328 reviewed Aug 28, 2019

View reviewed changes

mnahkies force-pushed the issue-2845-inheritance-regression branch 2 times, most recently from fd05c24 to d9ec32b Compare August 28, 2019 08:09

mnahkies force-pushed the issue-2845-inheritance-regression branch from d9ec32b to c8f9263 Compare August 28, 2019 08:25

wing328 merged commit 34ec98d into OpenAPITools:master Aug 28, 2019

wing328 added Enhancement: Feature Feature: OAS 3.0 spec support and removed Enhancement: General labels Aug 28, 2019

mnahkies deleted the issue-2845-inheritance-regression branch August 28, 2019 16:55

smasala mentioned this pull request Oct 14, 2019

allOf ignored when the property is used on its own #4146

Open

jimschubert mentioned this pull request Feb 2, 2020

[BUG] [Java] inheritance is generated without discriminator field #5097

Closed

amakhrov mentioned this pull request Apr 19, 2020

[BUG] [Spring] [4.3.0] allOf forces inheritance #5876

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] [regression] set parentName when a single possible parent exists #3771

[core] [regression] set parentName when a single possible parent exists #3771

mnahkies commented Aug 27, 2019 •

edited by macjohnny

Loading

auto-labeler bot commented Aug 27, 2019

macjohnny left a comment

macjohnny commented Aug 28, 2019

macjohnny commented Aug 28, 2019

wing328 Aug 28, 2019

mnahkies Aug 28, 2019

macjohnny Aug 28, 2019

wing328 Aug 28, 2019

macjohnny Aug 28, 2019

mnahkies Aug 28, 2019

wing328 Aug 28, 2019

smasala Oct 4, 2019

wing328 commented Aug 28, 2019

wing328 commented Aug 28, 2019

macjohnny commented Aug 28, 2019

wing328 commented Aug 28, 2019

smasala commented Oct 4, 2019

wing328 commented Apr 20, 2020

[core] [regression] set parentName when a single possible parent exists #3771

[core] [regression] set parentName when a single possible parent exists #3771

Conversation

mnahkies commented Aug 27, 2019 • edited by macjohnny Loading

PR checklist

Description of the PR

auto-labeler bot commented Aug 27, 2019

macjohnny left a comment

Choose a reason for hiding this comment

macjohnny commented Aug 28, 2019

macjohnny commented Aug 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wing328 commented Aug 28, 2019

wing328 commented Aug 28, 2019

macjohnny commented Aug 28, 2019

wing328 commented Aug 28, 2019

smasala commented Oct 4, 2019

wing328 commented Apr 20, 2020

mnahkies commented Aug 27, 2019 •

edited by macjohnny

Loading