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

Add support of model inheritance as a standard java single class extension without requirement to add discriminator (see #6127 ) #7593

Closed

Conversation

SergeyLyakhov
Copy link
Contributor

@SergeyLyakhov SergeyLyakhov commented Feb 6, 2018

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 and ./bin/security/{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\.
[+] Filed the PR against the correct branch: master.
[+] Copied the technical committee to review the pull request if your PR is targeting a particular programming language.
@wing328 @bbdouglas @JFCote @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger

Description of the PR

This change adds new codegen cli option supportsModelExtension=true|false which allows generation of class extension without defining discriminator.
This is fully backward compatible and does not break any previous logic.
Implemented on DefaultCodegen level. Allow Java-style single class inheritance for any language which doesn't support mixin.

@SergeyLyakhov SergeyLyakhov changed the title Add support of model inheritance as a standard java single class extension without requirement to add discriminator Add support of model inheritance as a standard java single class extension without requirement to add discriminator (see #6127 ) Feb 6, 2018
…nsion. Exclude incorrect references from inheritance.
@SergeyLyakhov
Copy link
Contributor Author

SergeyLyakhov commented Feb 13, 2018

@cbornet
The order doesn't matter.

  1. This is designed for the models which have single-model inheritance.
  2. Codegen already supports inheritance in the same way. But requires x-discriminator-value.
    This fix just extends existing inheritance model but without requirement to have discriminator value.

@@ -1394,7 +1394,7 @@ public CodegenModel fromModel(String name, Model model, Map<String, Model> allDe
}
// set first interface with discriminator found as parent
if (parent == null
&& (supportsModelExtension
&& ((supportsModelExtension && (interfaceModel instanceof ModelImpl || interfaceModel instanceof ComposedModel))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added validation of parent model similar to the existing one for discriminator case.

Copy link
Contributor

@jeff9finger jeff9finger left a comment

Choose a reason for hiding this comment

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

A few questions.

  • It appears that this option makes all uses of "allOf" create a sub class. This may not be desirable in all cases. So, if the spec needs to use either composition or the existing use of discriminator, can this option be enabled per definition by use of a vendor extension or some other mechanism?
  • How will this work with existing uses of discriminator?
  • What happens when more than 2 elements exist in the "allOf" declaration?
  • What happens if there are at least 2 inline objects in the "allOf" declaration

@SergeyLyakhov
Copy link
Contributor Author

@jeff9finger

First of all:
I. This change is intended for single-parent model (Java-like style).
II. type: object definition is not a model interface. And can't be inherited as parent class (even before fix).

#1. The following definition is single-parent model (object never inherits):

   Child:
    allOf:
      - $ref: '#/definitions/SingleParent'
      - type: object
  • for discriminator case - this works exactly as before.
  • no discriminator - Child still extends SingleParent.

#2. The following definition is multi-parent (two-parent) model:

   Child:
    allOf:
      - $ref: '#/definitions/FirstParent'       # let assume this model does not have discriminator
      - $ref: '#/definitions/SecondParent' # let assume this model have discriminator
      - type: object
  • this case may work not properly:
    Before the fix, Child will extend SecondParent (because there is discriminator).
    After the fix, Child will extend FirstParent (first "real" model).
  • this kind of model is out of scope of schema with single-model inheritance. This fix not intended for such models.

It appears that this option makes all uses of "allOf" create a sub class. This may not be desirable in all cases.

Probably I do not understand such requirements. But in this case users should not use this option.

How will this work with existing uses of discriminator?

For #1 this will work as previously.
For #2 collisions possible. Regardless swager-core preserving the order of models, as I understood @cbornet - this is not guaranteed.

What happens if there are at least 2 inline objects in the "allOf" declaration

One model will be extended. For second model - fields will be inherited.
For this fix - the first model is inherited (regardless with or w/o discriminator).
Before the fix - first model with discriminator is inherited.

@jeff9finger
Copy link
Contributor

I am not too concerned about the multi parent model as it is not a realistic model for Java. But if the behavior is different than before when the supportsModelExtension = true, we need to ensure that it is documented clearly.

What I am concerned about is a case where the API design calls for both inheritance* and composition. It appears that when supportsModelExtension = true, there is no option for composition and vise versa. Composition still needs to be allowed when supportsModelExtension = true.

I will try to illustrate my point with an example.

The specification below has 3 different hierarchies. The one with a discriminator will work the same way as it has always worked.

But let's say that I want inheritance for

  • ParentWithoutDiscriminator --> ChildOneWithoutParentDiscriminator
  • ParentWithoutDiscriminator --> ChildTwoWithoutParentDiscriminator --> SubChildTwoWithoutDiscriminator

but I want composition for

  • ParentTwoWithoutDiscriminator : ChildOneWithoutParentTwoDiscriminator

If I understand correctly, there is not a way to allow for both of these options when supportsModelExtension=true.

I would like to specify which hierarchy to apply the supportsModelExtension option to.

definitions:

# I would like *inheritance* for this hierarchy: 
# ParentWithoutDiscriminator --> ChildOneWithoutParentDiscriminator
# ParentWithoutDiscriminator --> ChildTwoWithoutParentDiscriminator --> SubChildTwoWithoutDiscriminator

  ParentWithoutDiscriminator:
    description: A parent object - no discriminator
    type: object
    properties:
      prop_wo_disc_one:
        type: string
      
  ChildOneWithoutParentDiscriminator:
    allOf:
      - $ref: '#/definitions/ParentWithoutDiscriminator'
      - type: object
        properties:
          child_w_disc_prop:
            type: string

  ChildTwoWithoutParentDiscriminator:
    allOf:
      - $ref: '#/definitions/ParentWithoutDiscriminator'
      - type: object
        properties:
          child_w_disc_prop:
            type: string

  SubChildTwoWithoutDiscriminator:
    allOf:
      - $ref: '#/definitions/ChildTwoWithoutParentDiscriminator'
      - type: object
        properties:
          sub_child_w_disc_prop:
            type: string

# I would like *composition for this hierarchy
# ParentTwoWithoutDiscriminator : ChildOneWithoutParentTwoDiscriminator
  ParentTwoWithoutDiscriminator:
    description: A parent object - no discriminator
    type: object
    properties:
      prop_wo_disc_two:
        type: string

  ChildOneWithoutParentTwoDiscriminator:
    allOf:
      - $ref: '#/definitions/ParentTwoWithoutDiscriminator'
      - type: object
        properties:
          child_w_disc_prop:
            type: string

# The following always works the same as before 
  ParentWithDiscriminator:
    description: A parent object - with discriminator
    discriminator: parent_type
    type: object
    required:
    - parent_type
    properties:
      parent_type:
        type: string
      prop_w_disc_one:
        type: string

  ChildWithDiscriminator:
    allOf:
      - $ref: '#/definitions/ParentWithDiscriminator'
      - type: object
        properties:
          child_w_disc_prop:
            type: string

What if there was an option (vendor extension ?) on the parent or on the child that specifies whether to use inheritance or composition?

@SergeyLyakhov
Copy link
Contributor Author

@jeff9finger

What if there was an option (vendor extension ?)

If we add a vendor extension, would we than validate it in codegen code, or use in mustache templates?
I suppose it would be very complicate fix to implement this functionality using only vendor extension + template changes.

@jeff9finger
Copy link
Contributor

For the vendor extension, I think it would need changes to both the codegen code and the mustache templates.

@SergeyLyakhov
Copy link
Contributor Author

@jeff9finger
Implemented it in the way proposed. Was not able to re-open this PR. Added a new one #7910.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants