fix #13150 Do not add schema / class name mapping where custom mapping exists #13815

bernie-schelberg-mywave · 2022-10-25T10:40:33Z

Having multiple names for a model mapping can cause issues with clients that are expecting a particular types to always have the same model name. Refer #13150.

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
Commit all changed files.
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/configs/java*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master (6.1.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

…ustom mapping exists

bernie-schelberg-mywave · 2022-10-30T22:17:21Z

@cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09)

welshm · 2022-11-22T16:49:39Z

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

+                Optional.ofNullable(vendorExtensions)
+                    .map(ve -> ve.get("x-discriminator-value"))
+                    .map(discriminatorValue -> (String) discriminatorValue)
+                    .orElse(currentSchemaName);


This logic is changing - since this value previously would have been null

No, prior to this change currentSchemaName was added as a mapping name on line 3365, and then another mapping was added on line 3371 if an x-discriminator-value was set. With this change, it's one or the other, not both.

I wonder if this change is causing the issues you're seeing with Dart. I'd have to look more closely at how the schemas are being used later on, but this adds less schemas now; so perhaps some filterins logic is applied at some point?

I would try reverting this part of the change and see what the Dart output looks like.

welshm · 2022-11-22T16:51:56Z

Is this still required after #13958 ?

bernie-schelberg-mywave · 2022-11-22T23:16:51Z

Is this still required after #13958 ?

They seem unrelated to me. The other is about adding additional annotations to the model class / interface. This one is about having singular mapping names for oneOf subclasses.

wing328 · 2022-11-25T02:52:00Z

cc @OpenAPITools/generator-core-team as the change impacts default codegen.

jorgerod · 2023-02-13T12:32:52Z

Hello @bernie-schelberg-mywave @wing328

What is the status of this issue?
IMHO this is a bug and should be fixed as soon as possible. We have been blocked by this issue since 5.3.1 and I think this problem will happen to many teams.

bernie-schelberg-mywave · 2023-02-14T05:20:34Z

@jorgerod as far as I know, it is planned to merge this one into the version 7 branch. I don't really have any update on a timeline though, sorry.

jorgerod · 2023-02-15T13:20:52Z

Hi @bernie-schelberg-mywave

I have done a serialization test with your changes. The JsonSubTypes are no longer duplicated.
I used the spec from this PR and the test is as follows.

  @Test
  void testSerialization() throws JsonProcessingException {
    final ObjectMapper objectMapper = new ObjectMapper();

    final String foo = objectMapper.writeValueAsString(new FooDTO());
    assertThat(foo).contains("{\"type\":\"Foo\"}");

  }

The output is:

java.lang.AssertionError: 
Expecting actual:
  "{"type":"Foo"}"
to contain:
  "{"type":"foo"}"

I'm not a Jackson expert, but it seems that in the serialization Jackson is using the value of @JsonTypeName instead of the value defined in the mapping.

What do you think?

bernie-schelberg-mywave · 2023-02-16T06:54:27Z

Hi @jorgerod, the error message in your comment doesn't match your test. In your test you say that you expect foo to contain {"type":"Foo"}, but the assertion error states that it's expecting {"type":"foo"}. The actual value in the assertion error matches the assertion that you have in the test. Can you check and clarify what it is that you're expecting?

Also, what is FooDTO? It's not specified in the schema that is part of this PR. Have you defined it separately?

troehling · 2023-02-16T07:39:54Z

Hi @bernie-schelberg-mywave, I have experienced a similar issue as @jorgerod. Regarding your question of the FooDTO: In my case I have configured the suffix dto as modelNameSuffix in my generator. This leads to @JsonTypeName("Foo") being generated, which is used by Jackson for the serialization overriding the value configured in @JsonSubTypes.

jorgerod · 2023-02-16T08:29:38Z

Hi @bernie-schelberg-mywave

I have done a serialization test with your changes. The JsonSubTypes are no longer duplicated. I used the spec from this PR and the test is as follows.
  @Test
  void testSerialization() throws JsonProcessingException {
    final ObjectMapper objectMapper = new ObjectMapper();

    final String foo = objectMapper.writeValueAsString(new FooDTO());
    assertThat(foo).contains("{\"type\":\"Foo\"}");

  }
The output is:
java.lang.AssertionError: 
Expecting actual:
  "{"type":"Foo"}"
to contain:
  "{"type":"foo"}" 
I'm not a Jackson expert, but it seems that in the serialization Jackson is using the value of @JsonTypeName instead of the value defined in the mapping.

What do you think?

Hi @bernie-schelberg-mywave
In the example, in the hurry, there is a typo.

spec

openapi: '3.0.0'
info:
  version: '1.0.0'
  title: 'FooService'
paths:
  /parent:
    put:
      summary: put parent
      operationId: putParent
      parameters:
        - name: name
          in: path
          required: true
          description: Name of the account being updated.
          schema:
            type: string
      requestBody:
        description: The updated account definition to save.
        required: true
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/Parent'
      responses:
        '200':
          $ref: '#/components/responses/Parent'
components:
  schemas:
    Parent:
      type: object
      description: Defines an account by name.
      properties:
        name:
          type: string
          description: The account name.
        type:
          type: string
          description: The account type discriminator.
      required:
        - name
        - type
      discriminator:
        propertyName: type
        mapping:
          foo: '#/components/schemas/Foo'

    Foo:
      allOf:
        - $ref: "#/components/schemas/Parent"
        - type: object
          properties:
            fooType:
              type: string
  responses:
    Parent:
      description: The saved account definition.
      content:
        application/json:
          schema:
            $ref: '#/components/schemas/Parent'

Test:

  @Test
  void testSerialization() throws JsonProcessingException {
    final ObjectMapper objectMapper = new ObjectMapper();

    final String foo = objectMapper.writeValueAsString(new FooDTO());
    assertThat(foo).contains("{\"type\":\"foo\"}");

  }

Output:

java.lang.AssertionError: 
Expecting actual:
  "{"type":"foo"}"
to contain:
  "{"type":"Foo"}"

As @troehling says, Jackson use @JsonTypeName("Foo") for serialization.

I'm testing and I think that modifying pojo.mustache for the discriminator case with mapping will solve it.

Currently there is this:

openapi-generator/modules/openapi-generator/src/main/resources/JavaSpring/pojo.mustache

Lines 16 to 20 in 12e76ec

    
           {{#jackson}} 
        
           {{#isClassnameSanitized}} 
        
           @JsonTypeName("{{name}}") 
        
           {{/isClassnameSanitized}} 
        
           {{/jackson}}

It would be to put this:

{{#jackson}}
{{#isClassnameSanitized}}
    {{^hasDiscriminatorWithNonEmptyMapping}}
@JsonTypeName("{{name}}")
    {{/hasDiscriminatorWithNonEmptyMapping}}
{{/isClassnameSanitized}}
{{/jackson}}

If there is a discriminator with mapping, don't add @JsonTypeName

I also think this bugfix should go in a separate PR from this one as they are different problems.

How do you see it @bernie-schelberg-mywave @troehling @wing328 @welshm ?

EDIT: With the change I mention, this issue would be solved.
Thank you

bernie-schelberg-mywave · 2023-02-16T22:01:41Z

@jorgerod I think I understand now. I think it may be related, but a different issue to this one. I think the issue you describe is present in the master branch, is it not?

welshm · 2023-02-16T22:57:00Z

Sorry this fell off my radar for a bit! Can you regenerate the samples to include with the PR? Or did no samples change? This looks fine to me from a SpringCodegen point of view - but the changes are in the default codegen so may need more reviewers - although @wing328 may be able to take a look as well

welshm · 2023-02-16T22:59:49Z

@jorgerod I agree with your suggestion that the fix should be in a separate PR - it will also be quicker to merge and simpler to review as it will just be touching a Spring codegen mustache.

bernie-schelberg-mywave · 2023-02-17T01:29:20Z

@welshm I merged master again, and re-generated samples. There were no changes.

welshm · 2023-02-17T01:46:36Z

@welshm I merged master again, and re-generated samples. There were no changes.

Might want to add some samples then that show this behavior being used. Not required though.

bernie-schelberg-mywave · 2023-02-17T04:03:45Z

@welshm I modified one of the examples to have a discriminator mapping, renaming className to type in the process. That changed more than was really necessary, but className didn't seem right when the class name was no longer being used as the discriminator.
Generated all samples and pushed the changes. I noticed that the cs generator seems not to use the discriminator mapping values, but that's a separate issue I think.

jorgerod · 2023-02-17T13:40:21Z

Hi @bernie-schelberg-mywave @welshm
I have created this PR

welshm

Thank you for updating the samples (and modifying them to use excercise the code change!)

Minor suggestion on parameter naming, but otherwise I think this is good.

If you are not on the Slack, I would recommend joining - Link

You will need to get someone from the default codegen maintainers to review still

welshm · 2023-02-17T14:40:21Z

...penapi-generator/src/test/resources/3_0/petstore-with-fake-endpoints-models-for-testing.yaml

      properties:
-        className:
+        type:


nit: I would not use type here since it's the name of another property of the YAML - maybe animalType ?

agree, it's confusing. I've changed it to species.

welshm · 2023-02-17T14:41:21Z

.../client/petstore/java-helidon-client/mp/src/main/java/org/openapitools/client/model/Cat.java

@@ -21,6 +21,8 @@
 import com.fasterxml.jackson.annotation.JsonTypeName;
 import com.fasterxml.jackson.annotation.JsonValue;
 import org.openapitools.client.model.Animal;
+import org.openapitools.client.model.Cat;


Seems weird to import itself, but separate problem

Yeah, that's weird. I've seen that some subclasses have the @JsonSubTypes annotation as well (here for example), which I don't think is necessary (not because of this change, the master branch shows this behaviour as well, but it wasn't in the samples before because the source schema didn't use discriminator values). Because of this annotation, every subclass refers to every other subclass. This may be the reason for the unnecessary import here.

agreed it's something we can deal with later.

welshm · 2023-02-17T14:41:51Z

samples/client/petstore/java/apache-httpclient/api/openapi.yaml

      properties:
-        className:
+        type:


Same nit - would probably call this animalType or animalSpecies

changed to species

i've cherry-pick some commits and created #14984 instead.

i didn't rename className to something else

# Conflicts: # modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java

bernie-schelberg-mywave · 2023-02-20T01:16:35Z

@OpenAPITools/generator-core-team please review.

@wing328 , @welshm , I'm not sure I'm using the right handle, how do I cc / request review from the core team?

wing328 · 2023-03-16T16:11:29Z

I've updated the target branch to 7.0.x as I think that's what this change is based on.

wing328 · 2023-03-20T09:04:09Z

closed via #14984 instead

fix OpenAPITools#13150 Do not add schema / class name mapping where c…

de1566c

…ustom mapping exists

This was referenced Oct 25, 2022

fix #13150 Add uniqueModelMappings option #13802

Closed

Spring generates duplicated JsonSubTypes #13150

Closed

Fix Ruby tests

2bf0ea2

bernie-schelberg-mywave mentioned this pull request Nov 7, 2022

Use enum type for discriminator #13846

Merged

5 tasks

welshm approved these changes Nov 22, 2022

View reviewed changes

Merge branch 'master' into issue13150-2

18200c3

bernie-schelberg-mywave added 2 commits February 17, 2023 13:37

Add Java example

4571231

Generate other samples

a1a26ea

welshm reviewed Feb 17, 2023

View reviewed changes

bernie-schelberg-mywave added 2 commits February 20, 2023 09:56

Change animal type property to species

17f8260

Merge branch 'master' into issue13150-2

742ff17

# Conflicts: # modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java

wing328 added Enhancement: Feature Feature: Generator labels Feb 20, 2023

WojciechZankowski mentioned this pull request Mar 11, 2023

[BUG][JAVA] AllOf with mapping generates wrong JsonSubTypes annotations #14917

Closed

6 tasks

bernie-schelberg-mywave added 2 commits March 16, 2023 14:15

Merge branch '7.0.x' into issue13150-2

6899d4b

Generated samples after merging latest 7.0.x

3247451

bernie-schelberg-mywave requested a review from jimschubert as a code owner March 16, 2023 04:51

wing328 changed the base branch from master to 7.0.x March 16, 2023 13:59

wing328 added this to the 7.0.0 milestone Mar 16, 2023

wing328 mentioned this pull request Mar 17, 2023

Do not add schema / class name mapping where custom mapping exists #14984

Merged

6 tasks

wing328 changed the base branch from 7.0.x to master March 17, 2023 12:12

wing328 changed the base branch from master to 7.0.x March 17, 2023 12:14

wing328 closed this Mar 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #13150 Do not add schema / class name mapping where custom mapping exists #13815

fix #13150 Do not add schema / class name mapping where custom mapping exists #13815

bernie-schelberg-mywave commented Oct 25, 2022

bernie-schelberg-mywave commented Oct 30, 2022

welshm Nov 22, 2022

bernie-schelberg-mywave Nov 22, 2022

welshm Feb 20, 2023

welshm commented Nov 22, 2022

bernie-schelberg-mywave commented Nov 22, 2022

wing328 commented Nov 25, 2022

jorgerod commented Feb 13, 2023

bernie-schelberg-mywave commented Feb 14, 2023

jorgerod commented Feb 15, 2023

bernie-schelberg-mywave commented Feb 16, 2023

troehling commented Feb 16, 2023

jorgerod commented Feb 16, 2023 •

edited

Loading

bernie-schelberg-mywave commented Feb 16, 2023

welshm commented Feb 16, 2023

welshm commented Feb 16, 2023

bernie-schelberg-mywave commented Feb 17, 2023

welshm commented Feb 17, 2023

bernie-schelberg-mywave commented Feb 17, 2023

jorgerod commented Feb 17, 2023

welshm left a comment

welshm Feb 17, 2023

bernie-schelberg-mywave Feb 20, 2023

welshm Feb 17, 2023

bernie-schelberg-mywave Feb 20, 2023 •

edited

Loading

wing328 Mar 17, 2023

welshm Feb 17, 2023

bernie-schelberg-mywave Feb 20, 2023

wing328 Mar 17, 2023

bernie-schelberg-mywave commented Feb 20, 2023

wing328 commented Mar 16, 2023

wing328 commented Mar 20, 2023

fix #13150 Do not add schema / class name mapping where custom mapping exists #13815

fix #13150 Do not add schema / class name mapping where custom mapping exists #13815

Conversation

bernie-schelberg-mywave commented Oct 25, 2022

PR checklist

bernie-schelberg-mywave commented Oct 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

welshm commented Nov 22, 2022

bernie-schelberg-mywave commented Nov 22, 2022

wing328 commented Nov 25, 2022

jorgerod commented Feb 13, 2023

bernie-schelberg-mywave commented Feb 14, 2023

jorgerod commented Feb 15, 2023

bernie-schelberg-mywave commented Feb 16, 2023

troehling commented Feb 16, 2023

jorgerod commented Feb 16, 2023 • edited Loading

bernie-schelberg-mywave commented Feb 16, 2023

welshm commented Feb 16, 2023

welshm commented Feb 16, 2023

bernie-schelberg-mywave commented Feb 17, 2023

welshm commented Feb 17, 2023

bernie-schelberg-mywave commented Feb 17, 2023

jorgerod commented Feb 17, 2023

welshm left a 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

bernie-schelberg-mywave Feb 20, 2023 • edited Loading

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

bernie-schelberg-mywave commented Feb 20, 2023

wing328 commented Mar 16, 2023

wing328 commented Mar 20, 2023

jorgerod commented Feb 16, 2023 •

edited

Loading

bernie-schelberg-mywave Feb 20, 2023 •

edited

Loading