Correct allOf with only one child schema (no discriminator) #6901

wing328 · 2020-07-09T12:14:40Z

Correct allOf with only one child schema (no discriminator) by removing the old (incorrect) behavior.

Now it works correctly as model composition.

cc @OpenAPITools/generator-core-team

PR checklist

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/config/java*. For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

m-mohr · 2020-07-09T14:12:21Z

How to do enable inheritance without discriminator?

wing328 · 2020-07-09T14:36:39Z

Don't think you can do that according to the spec

The OpenAPI Specification allows combining and extending model definitions using the allOf property of JSON Schema, in effect offering model composition. allOf takes an array of object definitions that are validated independently but together compose a single object.

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

Ref: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.3.md#composition-and-inheritance-polymorphism

jeff9finger · 2020-07-09T17:30:05Z

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

+                        // TOOD to be removed in 6.x release
+                        LOGGER.warn("[deprecated] inheritance without use of 'discriminator.propertyName' has been deprecated" +
+                                        " in the 5.x release. Composed schema name: {}. Title: {}",
+                                composedSchema.getName(), composedSchema.getTitle()); 
                    }
                } else {
                    // not a ref, doing nothing, except counting the number of times the 'null' type


This block is no longer needed at all right? It only increments an unused variable nullSchemaChildrenCount

spacether

Looks good

jeff9finger · 2020-07-09T17:33:44Z

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

@@ -1239,8 +1237,10 @@ public static String getParentName(ComposedSchema composedSchema, Map<String, Sc
                        return parentName;
                    } else {
                        // not a parent since discriminator.propertyName is not set
-                        hasAmbiguousParents = true;
-                        refedWithoutDiscriminator.add(parentName);
+                        // TOOD to be removed in 6.x release


:-) Misspelling: TOOD --> TODO

This case indicates composition, not inheritance right? Especially in the edge case of an allOf reference to a model with no properties nor additionalProperties So why is it being deprecated? This seems like a valid use case for composition.

The log warning message seems to indicate that I will not be able to do composition in this way. Is that what is intended?

This reverts commit 99b8533.

wing328 · 2020-07-12T13:42:12Z

@jeff9finger I've pushed some update to minimize the change and only fix the incorrect behavior. Please have another look when you've time.

typhoon2k · 2020-08-10T06:22:17Z

Hi @wing328,

~~as far as I understand the only change applied here is change in log message content (and log level).~~
We have a very simple case:

base_schemas.yaml

baseSchema:
  title: BaseSchema
  type: object
  description: Base schema description
  properties:
    property1:
      type: string
    property2:
      type: string

extension_schemas.yaml:

extensionSchema
  title: ExtensionSchema
  type: object
  description: Extension schema description
  allOf:
    - $ref: 'base_schemas.yaml#/baseSchema'
  example:
    property1: 'value1'
    property2: 'value2'

With this definition (we have only one child schema here) we are still getting inheritance instead of composition and generation log contains a lot of WARN (previously)/INFO (with this update) messages.

So, behavior is not fixed for us...

best regards,
ty

typhoon2k · 2020-08-10T07:33:00Z

I've investigated this a bit firther and it seems that some generators are using own logic for allOf cases. In our case we are using typescript-axios and plantuml generators and both are generating it as inheritance (for example, typescript-axios generator is using special allOf template).

But what worries me is that we are getting many many "inheritance without use of 'discriminator.propertyName' has been deprecated in the 5.x release" messages in generator log. Do we need to produce this spam now?

Otherwise looks good for me.

best regards,
ty

typhoon2k · 2020-08-25T12:18:08Z

Hi @wing328 ,

what are the plans for this change? Can we get it in master sometime soon?

ty

P.S. It would be great to get rid of "inheritance without use of 'discriminator.propertyName' has been deprecated in the 5.x release" spamming message as well before merging this PR.

jimschubert · 2020-08-31T01:20:27Z

@typhoon2k I don't think we can add a breaking change and deprecation without informing the user.

@wing328 can you wrap the above message in once as other spammy logs in ModelUtils? This way, users could tweak logging by changing the system properties and defaults:

-Dorg.openapitools.codegen.utils.oncelogger.cachesize=200
-Dorg.openapitools.codegen.utils.oncelogger.expiry=2000

jimschubert

LGTM

typhoon2k · 2020-08-31T11:14:31Z

@jimschubert , I just tried to play locally with once logger. I've made the following change to logging code:

            once(LOGGER).info("[deprecated] inheritance without use of 'discriminator.propertyName' has been deprecated" +
                    " in the 5.x release. Composed schema name: " + composedSchema.getName() + ". Title: " + composedSchema.getTitle());

With this change I still get spam in logs. Do I need to configure once logger somehow additionally?

Update:
I understand why - composed schema name and title are different each time in our case. But as we have many of them, then we get a lot of such messages in logs.

So it would be nice to include message without composed schema name and title to reduce spam in logs.

jimschubert · 2020-09-01T01:46:51Z

Excellent suggestion. I'll go ahead and merge this, then change the info log to be generic and add a debug log with the name and title.

…6901)" This reverts commit 972120c.

jimschubert · 2020-09-01T02:51:57Z

@wing328 I'd merged this, but had to revert via #7323 as it caused all CI checks to fail in master. If you have a chance, could you merge master and take a look? It failed on JavaClientCodegenTest#testAllowModelWithNoProperties and I didn't readily see a way to allow for empty/marker classes on this change as the spec allows.

* master: (89 commits) add structPrefix support to go-experimental (#7327) Add a link to SmartHR Tech Blog (#7324) Revert "Correct allOf with only one child schema (no discriminator)" (#7323) Correct allOf with only one child schema (no discriminator) (#6901) [Go]: Interface definitions for api functions (#5914) Update bug_report.md (#7320) update samples [Java][Client] Use java8 OffsetDateTime for clients (#7190) [java] Intro openApiNullable property to enable/disable OpenAPI Jackson Nullable library (#6154) [Spring Boot] update dependencies, mark java8 option as deprecated (#7306) Remove dot in golang type (#7307) [doc] Document usage of post-process file feature (#7315) fix http bear auth documentation for go clinets (#7312) [Extensions][Go][Java] Test x-auth-id-alias (#6642) [php-slim4] Move config to a separate file (#6971) [C][Client][Clang Static Analyzer] Remove the useless free operation for (#7309) Fix typescript-node generation when only models are generated (#7127) update spring config to use java8 (#7308) [C][Client][Clang Static Analyzer] Fix uninitialized argument value (#7305) [Java] remove deprecated jackson classes (#7304) ...

* master: (984 commits) add structPrefix support to go-experimental (OpenAPITools#7327) Add a link to SmartHR Tech Blog (OpenAPITools#7324) Revert "Correct allOf with only one child schema (no discriminator)" (OpenAPITools#7323) Correct allOf with only one child schema (no discriminator) (OpenAPITools#6901) [Go]: Interface definitions for api functions (OpenAPITools#5914) Update bug_report.md (OpenAPITools#7320) update samples [Java][Client] Use java8 OffsetDateTime for clients (OpenAPITools#7190) [java] Intro openApiNullable property to enable/disable OpenAPI Jackson Nullable library (OpenAPITools#6154) [Spring Boot] update dependencies, mark java8 option as deprecated (OpenAPITools#7306) Remove dot in golang type (OpenAPITools#7307) [doc] Document usage of post-process file feature (OpenAPITools#7315) fix http bear auth documentation for go clinets (OpenAPITools#7312) [Extensions][Go][Java] Test x-auth-id-alias (OpenAPITools#6642) [php-slim4] Move config to a separate file (OpenAPITools#6971) [C][Client][Clang Static Analyzer] Remove the useless free operation for (OpenAPITools#7309) Fix typescript-node generation when only models are generated (OpenAPITools#7127) update spring config to use java8 (OpenAPITools#7308) [C][Client][Clang Static Analyzer] Fix uninitialized argument value (OpenAPITools#7305) [Java] remove deprecated jackson classes (OpenAPITools#7304) ...

typhoon2k · 2020-09-17T14:23:42Z

hi @wing328 , @jimschubert , is there any chance this update will get into master sometime soon?

wing328 added 2 commits July 9, 2020 18:45

fix single allof resulting in inheritance

99b8533

clear cache

1bf3e7d

wing328 added Feature: Generator Issue: Bug labels Jul 9, 2020

wing328 added this to the 5.0.0 milestone Jul 9, 2020

This was referenced Jul 9, 2020

[BUG] Code is not generated correctly for allOf. #6815

Open

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

Closed

jeff9finger reviewed Jul 9, 2020

View reviewed changes

spacether approved these changes Jul 9, 2020

View reviewed changes

jeff9finger reviewed Jul 9, 2020

View reviewed changes

wing328 added 3 commits July 11, 2020 11:12

Revert "fix single allof resulting in inheritance"

2576ef1

This reverts commit 99b8533.

revise message

920ae0c

Merge remote-tracking branch 'origin/master' into fix-allof-single

1baf2b5

jeff9finger mentioned this pull request Jul 15, 2020

[BUG] Multi level inheritance (with discriminator declared) does not generate classes with inheritance #6950

Closed

6 tasks

jimschubert approved these changes Aug 31, 2020

View reviewed changes

jimschubert merged commit 972120c into master Sep 1, 2020

jimschubert deleted the fix-allof-single branch September 1, 2020 01:47

jimschubert restored the fix-allof-single branch September 1, 2020 02:45

jimschubert added a commit that referenced this pull request Sep 1, 2020

Revert "Correct allOf with only one child schema (no discriminator) (#…

0232606

…6901)" This reverts commit 972120c.

jimschubert mentioned this pull request Sep 1, 2020

Revert "Correct allOf with only one child schema (no discriminator)" #7323

Merged

wing328 deleted the fix-allof-single branch September 2, 2020 07:49

wing328 restored the fix-allof-single branch September 2, 2020 07:49

wing328 deleted the fix-allof-single branch November 3, 2020 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct allOf with only one child schema (no discriminator) #6901

Correct allOf with only one child schema (no discriminator) #6901

wing328 commented Jul 9, 2020 •

edited

Loading

m-mohr commented Jul 9, 2020

wing328 commented Jul 9, 2020

jeff9finger Jul 9, 2020

spacether left a comment

jeff9finger Jul 9, 2020

wing328 commented Jul 12, 2020

typhoon2k commented Aug 10, 2020 •

edited

Loading

typhoon2k commented Aug 10, 2020 •

edited

Loading

typhoon2k commented Aug 25, 2020

jimschubert commented Aug 31, 2020

jimschubert left a comment

typhoon2k commented Aug 31, 2020 •

edited

Loading

jimschubert commented Sep 1, 2020

jimschubert commented Sep 1, 2020

typhoon2k commented Sep 17, 2020

Correct allOf with only one child schema (no discriminator) #6901

Correct allOf with only one child schema (no discriminator) #6901

Conversation

wing328 commented Jul 9, 2020 • edited Loading

PR checklist

m-mohr commented Jul 9, 2020

wing328 commented Jul 9, 2020

jeff9finger Jul 9, 2020

Choose a reason for hiding this comment

spacether left a comment

Choose a reason for hiding this comment

jeff9finger Jul 9, 2020

Choose a reason for hiding this comment

wing328 commented Jul 12, 2020

typhoon2k commented Aug 10, 2020 • edited Loading

typhoon2k commented Aug 10, 2020 • edited Loading

typhoon2k commented Aug 25, 2020

jimschubert commented Aug 31, 2020

jimschubert left a comment

Choose a reason for hiding this comment

typhoon2k commented Aug 31, 2020 • edited Loading

jimschubert commented Sep 1, 2020

jimschubert commented Sep 1, 2020

typhoon2k commented Sep 17, 2020

wing328 commented Jul 9, 2020 •

edited

Loading

typhoon2k commented Aug 10, 2020 •

edited

Loading

typhoon2k commented Aug 10, 2020 •

edited

Loading

typhoon2k commented Aug 31, 2020 •

edited

Loading