Support models with multi-level hierarchy #4454

roxspring · 2019-11-11T17:00:50Z

Introduced BigCat extending Cat (which extends Animal) to the petstore-with-fake-endpoints spec.
ModelUtils.getAllParentsName() gains optional ability to include ancestor names, not just immediate parents.
ModelUtils considers composed interface a parent if it has a discriminator OR inherits one.
Discriminator mapped models includes all descendants, not just immediate ones.

Fixes #4423, #1685, #2928, #3058

Primarily for the attention of core team: @wing328 @jimschubert @cbornet @ackintosh @jmini @etherealjoy

Generator teams may also be interested - I've focused on checking the Java code myself.

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).
(actually ran java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar batch --includes-base-dir pwd --fail-fast -- bin/ci/* as recommended by @jimschubert)
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.

roxspring · 2019-11-16T00:37:12Z

Pushing this code change without regenerated samples fails CI because the samples no longer match what the code generates.
Pushing this code change with regenerated samples fails CI because existing generates have been generating invalid code for at least a month, probably a year [BUG][SCALAZ] tests are failing with generator 4.2.0 #4072

This is highly frustrating. What am I supposed to do to nurse this change through?

jimschubert · 2019-11-16T22:38:15Z

@roxspring I've looked over the recent CircleCI builds for your PR, and I'll provide analysis of these to help clarify for you:

https://circleci.com/gh/OpenAPITools/openapi-generator/10230#tests/containers/2
- Comparing the error from this build against changes I see locally on your branch, it looks like you hadn't committed your changes to samples.
https://circleci.com/gh/OpenAPITools/openapi-generator/10233
- scalaz verify failed. We don't see this failure on Master or other PRs, or at least I've never seen this failure (example, see https://circleci.com/gh/OpenAPITools/openapi-generator/tree/master)
- I'll look into this further. If it fails on this error again, please ignore and we'll look into it. This wouldn't be a blocker to merging your contribution (we'd temporarily skip this generator and "fix forward")
https://circleci.com/gh/OpenAPITools/openapi-generator/10247
- Same scalaz issue as above
https://circleci.com/gh/OpenAPITools/openapi-generator/10254#tests/containers/2
- Based on this output, I would guess that you had regenerated the samples at one point but didn't regenerate as the last commit for that CircleCI run. I say this because it shows some BigCat types as having changes (see test code changes to samples/client/petstore/php/OpenAPIClient-php/test/Model/BigCatTest.php
https://circleci.com/gh/OpenAPITools/openapi-generator/10364#tests/containers/2
- Your latest changes, without samples

I'm unable to repro verification issues with scalaz (see below), but I'd recommend commenting it out of pom.xml (~ lines 1101, 1193) if it occurs again and we'll look into it further.

Regarding your change, I'm confused a bit by this block:

String parentName = getSimpleRef(schema.get$ref());
Schema s = allSchemas.get(parentName);
if (s != null) {
    return hasOrInheritsDiscriminator(s, allSchemas);
}
else {
    LOGGER.error("Failed to obtain schema from {}", parentName);
}

I generated samples against your branch and opened #4503 to see how CI fares.

Here are the steps I follow before pushing to a branch (and running that bin/utils/ensure-up-to-date --batch script):

# (optional) Merge master
# git fetch
# git merge origin/master

# compile tooling
mvn install -am -Dmaven.javadoc.skip=true

# regenerate
./bin/utils/ensure-up-to-date --batch

# If there are changes, commit them + push

The last line will run the same batch command I provided you previously.

jimschubert · 2019-11-16T22:40:41Z

Commented too soon :)
The sister PR failed on scalaz due to Java version issue Unsupported major.minor version 52.0. There may be some misconfiguration with sbt/scala version in that project which would need to be evaluated (probably docker container issue). I'll skip there and rerun.

roxspring · 2019-11-16T23:34:26Z

@roxspring I've looked over the recent CircleCI builds for your PR, and I'll provide analysis of these to help clarify for you:

Thanks @jimschubert - really appreciate you taking a look!

https://circleci.com/gh/OpenAPITools/openapi-generator/10230#tests/containers/2

Comparing the error from this build against changes I see locally on your branch, it looks like you hadn't committed your changes to samples.

https://circleci.com/gh/OpenAPITools/openapi-generator/10233

Yes, this makes sense. I'd run ./bin/*-petstore.sh, because I hadn't yet discovered ./run-all-petstore.

scalaz verify failed. We don't see this failure on Master or other PRs, or at least I've never seen this failure (example, see https://circleci.com/gh/OpenAPITools/openapi-generator/tree/master)

Admittedly I'd never used scala before but I installed it this evening and attempted to compile master via mvn or sbt and reliably see the same failures as reported in #4072. samples/client/petstore/scalaz/.openapi-generator/VERSION suggests that it hasn't been generated since 3.0.0-SNAPSHOT, and suggests to me that this has been broken since the OpenAPI 3 upgrade. I really don't get what's going on here - why is this so obviously screwed up for me and my PRs but not for other people?!?

I'll look into this further. If it fails on this error again, please ignore and we'll look into it. This wouldn't be a blocker to merging your contribution (we'd temporarily skip this generator and "fix forward")

https://circleci.com/gh/OpenAPITools/openapi-generator/10247

Same scalaz issue as above

https://circleci.com/gh/OpenAPITools/openapi-generator/10254#tests/containers/2

Based on this output, I would guess that you had regenerated the samples at one point but didn't regenerate as the last commit for that CircleCI run. I say this because it shows some BigCat types as having changes (see test code changes to samples/client/petstore/php/OpenAPIClient-php/test/Model/BigCatTest.php

Yes, after the partial set of samples, I resubmitted with a full run-all-petstore.sh. Since that failed too, I recently force pushed without that last commit in to see if no samples would avoid triggering that portion of CI.

https://circleci.com/gh/OpenAPITools/openapi-generator/10364#tests/containers/2

Your latest changes, without samples

I'll rebuild the samples and push those too shortly.

I'm unable to repro verification issues with scalaz (see below), but I'd recommend commenting it out of pom.xml (~ lines 1101, 1193) if it occurs again and we'll look into it further.

Assuming all is still broken then I'll retry with that - thanks again for the pointers!

Regarding your change, I'm confused a bit by this block:

String parentName = getSimpleRef(schema.get$ref());
Schema s = allSchemas.get(parentName);
if (s != null) {
    return hasOrInheritsDiscriminator(s, allSchemas);
}
else {
    LOGGER.error("Failed to obtain schema from {}", parentName);
}

At some point since swagger-codegen 2.4.x this check was introduced to make sure that only composed schemas with a discriminator were considered as parents. This broke multi-level hierarchies though, because the intermediate parent schemas don't necessarily have a discriminator of their own if the one then inherit is good enough. In this particular case, we've successfully dereferenced a potential parent schema, but need to check that the parent or grandparent have a discriminator so that it qualifies. Any clearer?

I generated samples against your branch and opened #4503 to see how CI fares.

Here are the steps I follow before pushing to a branch (and running that bin/utils/ensure-up-to-date --batch script):
# (optional) Merge master
# git fetch
# git merge origin/master

# compile tooling
mvn install -am -Dmaven.javadoc.skip=true

# regenerate
./bin/utils/ensure-up-to-date --batch

# If there are changes, commit them + push
The last line will run the same batch command I provided you previously.

jimschubert · 2019-11-17T00:16:30Z

suggests that it hasn't been generated since 3.0.0-SNAPSHOT, and suggests to me that this has been broken since the OpenAPI 3 upgrade. I really don't get what's going on here - why is this so obviously screwed up for me and my PRs but not for other people?!?

To clarify our process a bit... We have historically only regenerated the more active generators, but verified all (with some exclusions commented on the pom) that can be built via Maven. This allows us to track breaking changes in the most popular generators regularly, but less used generators would get updated only when those generators change.

This was done because our script previously took 9 minutes or more to iterate only the popular generators. I'd tried once for all generators and it took over 30 minutes. This is a main reason I've introduced the batch command, so I could see this changing in the future to regenerate all samples on every build.

When we see a version file with 3.0.0, that does indicate this hasn't been generated, but also that we've had no major contributions to that specific generator since then.

roxspring · 2019-11-17T00:32:19Z

When we see a version file with 3.0.0, that does indicate this hasn't been generated, but also that we've had no major contributions to that specific generator since then.

That makes some sense. From what I can see though, regenerating scalaz against master (or my branch) results in the errors #4072. I'm seeing this under both Java 8 & 11, and not seeing java.lang.UnsupportedClassVersionError: scala/Option : Unsupported major.minor version 52.0 ~~anywhere~~ locally. Clearly that is the error blocking CircleCI though.

My speculation is some changes inherited from AbstractScalaCodegen have broken things, and this hasn't been spotted since scalaz has had no major contributions since.

roxspring · 2019-11-18T00:37:26Z

Rebased on master and making more progress through CI but not quite there yet.

The latest CI failure appears to be due to samples/server/petstore/java-pkmst/pom.xml getting generated without a meaningful version:

   <groupId>com.prokarma</groupId>
   <artifactId>pkmst-microservice</artifactId>
   <version></version>

The relevant pom.mustache contains <version>{{artifactVersion}}</version> like other pom.mustache files but it appears artifactVersion is blank in this case. It appears that JavaPKMSTServerCodegen is setting addtionalProperties[ARTIFACT_VERSION]=null and then AbstractJavaCodegen uses the value without checking it's non-null. I suspect that JavaPKMSTServerCodegen.java is in the wrong, given that it seems to be the only Codegen suffering, but I also suspect that AbstractJavaCodegen.java ought to be more defensive. Guess I should shave this yak in another MR rather than pollute this MR further.

jimschubert · 2019-11-18T02:08:56Z

@roxspring please feel free to revert the change to this file. I'm not sure, but it seems like it's leftover from when you ran all shell scripts under bin (unless you've intentionally run it manually). The fix looks simple enough, and I'll open a PR shortly.

@OpenAPITools/generator-core-team please review and provide feedback. I'd be happy to help normalize CI through my eval PR. I suspect something is corrupted in this PR's CircleCI cache, since my PR is the same code with regenerated samples and it's green as expected.

jimschubert · 2019-11-18T02:44:17Z

Opened #4520 to address the issue from java-pkmst. The issue would have affected all java clients and servers which don't explicitly pass an artifact version at generation time and don't have an API version specified in the OpenAPI Document.

@roxspring I really appreciate your diligence, as you're both directly and indirectly resolving multiple issues via this one contribution.

roxspring · 2019-11-18T22:18:39Z

Opened #4520 to address the issue from java-pkmst. The issue would have affected all java clients and servers which don't explicitly pass an artifact version at generation time and don't have an API version specified in the OpenAPI Document.

@OpenAPITools/generator-core-team please review and provide feedback. I'd be happy to help normalize CI through my eval PR. I suspect something is corrupted in this PR's CircleCI cache, since my PR is the same code with regenerated samples and it's green as expected.

I've rebased again to pick up this change but CircleCI is failing with yet more failures I don't understand. At this stage I'm choosing to believe that you're right about the CircleCI cache.

Hopefully we can discuss the code change itself as I think it's relatively straight forward and resolves a series of (near) duplicate issues raised over the past year or more.

@roxspring I really appreciate your diligence, as you're both directly and indirectly resolving multiple issues via this one contribution.

Thanks @jimschubert for all the help you've provided! Let me know if there's anything further to be done with this PR, happy to switch to your eval PR if it gets the job done.

jimschubert · 2019-11-18T23:56:23Z

@roxspring no problem. I've pinged the core team for review, mainly because of the change to the shared yaml and to have multiple eyes for any edge cases. If everything is cool, we can merge the PR I've opened and you'll still be associated as an author.

sebastien-rosset · 2019-11-20T07:20:15Z

I have executed the code generator from the multi-level-model-hierarchy branch. I confirm it generates the Java code as expected when there are multiple levels of inheritance hierarchy. I have tried with my own test model.

One caveat is that in the following scenario, the generated Java code will fail compilation:

The "discriminator" attribute and the discriminator property are specified in the one of the "parent" types
A sub-type specifies the discriminator again, but the sub-type does not specify the discriminator property. It is redundant and unnecessary to redefine the discriminator, but I don't think there is anything in the OpenAPI spec that states this is invalid.

In that case the Java compiler fails with "objectType has private access" because the generated code attempts to access the private member from the parent Java class.

   Vehicle:
      discriminator:
        propertyName: ObjectType
      properties:
          ObjectType:
            type: string
    Car:
      discriminator: # technically unnecessary because it is already defined in "Vehicle".
        propertyName: ObjectType
      allOf:
        - $ref: '#/components/schemas/Vehicle'  # Car inherits from Vehicle, which defines the discriminator property
        - type: object
          properties:
            #ObjectType:        # The generated Java will fail compilation unless this property is uncommented.
            #  type: string
            Wheels:
              type: integer

roxspring · 2019-11-24T23:59:29Z

One caveat is that in the following scenario, the generated Java code will fail compilation:

If I've understood correctly then you're saying:

Define discriminator attribute and property only in the root 👍
Redefine discriminator attribute and property in intermediate level 👍
Redefine discriminator attribute but not property in intermediate level 👎

I guess that also naturally leaves the question (4) about "Redefine discriminator property but not attribute in intermediate level".

Scenarios (3) and (4) seem pretty non-sensical to me. Is it absolutely clear what code should be generated in these cases? I think I can get my head around it making sense in a SQL schema but I'm struggling for a Java model.

For me they don't seem worth holding up the merge request but I'm aware that I'm biased! If folks really think it is worth holding up the merge then please let me know - ideally with some reasoned example of what should be produced in those cases.

(I'm ignoring conflicts in the generated samples until the code needs rebasing and/or I'm recommended otherwise)

sebastien-rosset · 2019-11-25T00:41:17Z

If I've understood correctly then you're saying:

IMO, your PR makes the code better, and the small caveat I pointed out should not block the merge. I was trying these combinations not because I think they are practical, but rather because I was trying to understand why the code was not generated as I expected (and also because the "discriminator" specification in OAS is not very clear). That's how I found the same bug that you addressed in this PR. I also empathize with the PR having a lot of file changes.

roxspring · 2019-12-02T09:59:54Z

Hi folks, is there anything I can do to drive this (or alternatively #4503) further forward towards review and integration?

sebastien-rosset · 2019-12-04T18:19:51Z

Hi folks, is there anything I can do to drive this (or alternatively #4503) further forward towards review and integration?

If there is anything I can do to test or validate, I would also be very interested to get this feature available.

spacether · 2019-12-05T16:08:48Z

@roxspring should we also add these ancestor models to the:
oneOf/anyOf/allOf info on the model?

cvgaviao · 2019-12-05T19:22:00Z

@roxspring, I tried to apply your change in my local clone in order to test with my inheritance models, but I received many conflicting errors...

roxspring · 2019-12-06T10:48:43Z

@roxspring should we also add these ancestor models to the:
oneOf/anyOf/allOf info on the model?

At this stage I'm still unclear how these are actually used so I'm really not sure. If somebody with a better understanding can explain the requirement clearly to me then I'll be happy to work on a solution. I'd prefer to get this MR in and open a subsequent one for any such improvements though as this addresses a painful regression against earlier behaviour and I don't want to delay it any further than it has been already.

@roxspring, I tried to apply your change in my local clone in order to test with my inheritance models, but I received many conflicting errors...

What conflicts? Conflicts with the samples are inevitable and I've already stated I'm not going to regenerate those again until that becomes a blocking issue - you can use ./run-all-petstore yourself to regenerate them in the meantime. I've not been made aware of any other conflicts.

cvgaviao · 2019-12-06T12:13:41Z

@roxspring, when I said conflict, I was referring about git merge conflicts that I had when I tried to apply your fixes. That happen because master branch had many commit after you have sent this PR. (Take a look in the ending of this page: There is a "This branch has conflicts that must be resolved" message.)
Normally in this case we need to rebase the branch we used to create the PR and repush it.

Anyway I was able to apply the fixes manually in the important classes and seems to be working well with my java model.

jimschubert · 2019-12-15T02:33:45Z

I've pinged the core team again and asked for a review of #4503, since it builds cleanly. My biggest question would be the impact of the change to the shared yaml which generates most samples.

roxspring · 2019-12-16T09:45:16Z

Given the headaches it caused, I'd have preferred not to modify the shared swagger and all the samples too... but I'm curious if that would have been acceptable? I got the impression that those samples were supposed to be exercising all supported features and therefore any successful merge request to add these features would have to modify them?

wing328 · 2019-12-27T07:44:06Z

@roxspring I believe we can close this with #4503 (merged). Thanks for the contribution 👍

roxspring force-pushed the feature/multi-level-model-hierarchy branch from a48b0d6 to b6a9b8f Compare November 11, 2019 23:51

roxspring mentioned this pull request Nov 12, 2019

Recreate petstore samples #4458

Closed

5 tasks

roxspring force-pushed the feature/multi-level-model-hierarchy branch 2 times, most recently from 725bfb0 to 1db0d6d Compare November 12, 2019 15:30

roxspring marked this pull request as ready for review November 12, 2019 15:49

roxspring mentioned this pull request Nov 14, 2019

Update samples against master code #4486

Closed

5 tasks

roxspring force-pushed the feature/multi-level-model-hierarchy branch from 1db0d6d to 2009cc4 Compare November 15, 2019 23:56

jimschubert mentioned this pull request Nov 16, 2019

Support models with multi-level hierarchy (via roxspring) #4503

Merged

5 tasks

roxspring force-pushed the feature/multi-level-model-hierarchy branch from ffa7493 to 20d1f9d Compare November 17, 2019 01:12

jimschubert mentioned this pull request Nov 17, 2019

Fixes issues with Scalaz outputs #4508

Merged

5 tasks

roxspring force-pushed the feature/multi-level-model-hierarchy branch 2 times, most recently from cb2ae72 to 547c401 Compare November 17, 2019 22:19

jimschubert mentioned this pull request Nov 18, 2019

[java] Fix java version defaulting #4520

Merged

5 tasks

roxspring added 5 commits November 18, 2019 19:35

Example of broken multi-level hierarchy

db2eefb

Support for multiple levels of hierarchy in model objects

b59f5ae

Support for multiple levels of hierarchy in generators

7a77492

Updated samples (./bin/utils/ensure-up-to-date)

c50fd9b

Update samples (./bin/run-all-petstore)

4470630

roxspring force-pushed the feature/multi-level-model-hierarchy branch 2 times, most recently from f8c88a4 to 4470630 Compare November 18, 2019 20:59

sebastien-rosset mentioned this pull request Nov 25, 2019

[BUG][Go] Support allOf inheritance with embedded go structs #4606

Closed

6 tasks

sebastien-rosset mentioned this pull request Dec 3, 2019

[Python] Add 'supportLargeModel' tag to handle unusually large OAS data type #4628

Closed

5 tasks

spacether mentioned this pull request Dec 5, 2019

[Python] Adds allOf/oneOf/anyOf composed models #4446

Merged

5 tasks

cvgaviao mentioned this pull request Dec 6, 2019

[BUG] ModelUtils uses wrong variable to log inheritance problem #4702

Open

sebastien-rosset mentioned this pull request Dec 17, 2019

[Golang] generate embedded structs and set supportsInheritance to true #4607

Closed

5 tasks

wing328 closed this Dec 27, 2019

Uh oh!

Support models with multi-level hierarchy #4454

Support models with multi-level hierarchy #4454

Uh oh!

Conversation

roxspring commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

roxspring commented Nov 16, 2019

Uh oh!

jimschubert commented Nov 16, 2019

Uh oh!

jimschubert commented Nov 16, 2019

Uh oh!

roxspring commented Nov 16, 2019

Uh oh!

jimschubert commented Nov 17, 2019

Uh oh!

roxspring commented Nov 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roxspring commented Nov 18, 2019

Uh oh!

jimschubert commented Nov 18, 2019

Uh oh!

jimschubert commented Nov 18, 2019

Uh oh!

roxspring commented Nov 18, 2019

Uh oh!

jimschubert commented Nov 18, 2019

Uh oh!

sebastien-rosset commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roxspring commented Nov 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebastien-rosset commented Nov 25, 2019

Uh oh!

roxspring commented Dec 2, 2019

Uh oh!

sebastien-rosset commented Dec 4, 2019

Uh oh!

spacether commented Dec 5, 2019

Uh oh!

cvgaviao commented Dec 5, 2019

Uh oh!

roxspring commented Dec 6, 2019

Uh oh!

cvgaviao commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimschubert commented Dec 15, 2019

Uh oh!

roxspring commented Dec 16, 2019

Uh oh!

wing328 commented Dec 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

roxspring commented Nov 11, 2019 •

edited

Loading

roxspring commented Nov 17, 2019 •

edited

Loading

sebastien-rosset commented Nov 20, 2019 •

edited

Loading

roxspring commented Nov 24, 2019 •

edited

Loading

cvgaviao commented Dec 6, 2019 •

edited

Loading