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

[BUG] invalid allVars for allOf and properties at the same level #18340

Closed
4 of 6 tasks
jpfinne opened this issue Apr 9, 2024 · 8 comments
Closed
4 of 6 tasks

[BUG] invalid allVars for allOf and properties at the same level #18340

jpfinne opened this issue Apr 9, 2024 · 8 comments

Comments

@jpfinne
Copy link
Contributor

jpfinne commented Apr 9, 2024

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

allVars does not contain the properties when the properties are on the same level as the allOf
Simlar to Object3 in issue_16797.yaml

openapi-generator version

7.4

OpenAPI declaration file content or url
openapi: 3.0.1
info:
  title: OpenAPI definition
  version: '1.0'
paths:
  /pets:
    get:
      summary: Get all pets
      operationId: getAllPets
      responses:
        default:
          description: ok
components:
  schemas:
    Pet:
      type: object
      properties:
        petType:
          type: string    
      discriminator:
        propertyName: petType
    Cat:
      allOf:
        - $ref: '#/components/schemas/Pet'
      type: object
      properties:
        name:
          type: string

Ps: the issue also exists if there is no discriminator

Generation Details

Spring generator (probably all generators that support inheritance)

Steps to reproduce

use this templates/pojo.mustache for demonstration

/**
allVars
=======
{{#allVars}}{{name}} {{/allVars}}

vars
=======
{{#vars}}{{name}} {{/vars}}

parentVars
========
{{#parentVars}}{{name}} {{/parentVars}}
*/

openapi-generator-cli generate -g spring -i allvarSample.yaml -o out -t templates

Cat.java has an invalid allVars. It should contain name.

/**
allVars
========
petType 

vars
========
name 

parentVars
========
 petType
*/
Related issues/PRs
Suggest a fix

Add a normalizer that transforms the model into

    Cat:
      allOf:
        - $ref: '#/components/schemas/Pet'
        - type: object
          properties:
            name:
               type: string
@wing328
Copy link
Member

wing328 commented Apr 9, 2024

there's already a normalizer rule for that: REFACTOR_ALLOF_WITH_PROPERTIES_ONLY

https://github.com/openapitools/openapi-generator/blob/master/docs/customization.md#openapi-normalizer

@jpfinne
Copy link
Contributor Author

jpfinne commented Apr 9, 2024

there's already a normalizer rule for that: REFACTOR_ALLOF_WITH_PROPERTIES_ONLY

https://github.com/openapitools/openapi-generator/blob/master/docs/customization.md#openapi-normalizer

I don't think it works if REF_AS_PARENT_IN_ALLOF is enabled.

I meant a new normalizer that runs ** before ** REFACTOR_ALLOF_WITH_PROPERTIES_ONLY

@wing328
Copy link
Member

wing328 commented Apr 9, 2024

why not simply run REFACTOR_ALLOF_WITH_PROPERTIES_ONLY before REF_AS_PARENT_IN_ALLOF if possible?

I did a test and Cat seems correctly extending Pet

public class Cat extends Pet {

and allVars contains both name and petType

  static {
    // a set of all properties/fields (JSON key names)
    openapiFields = new HashSet<String>();
    openapiFields.add("petType");
    openapiFields.add("name");

command: java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g java -i /tmp/allof.yaml -o /tmp/alloftest/ --openapi-normalizer REFACTOR_ALLOF_WITH_PROPERTIES_ONLY=true,REF_AS_PARENT_IN_ALLOF=true

Did you get something similar in your end?

@jpfinne
Copy link
Contributor Author

jpfinne commented Apr 9, 2024

It seems the code on master has not the issue anymore.
I'll do more tests later today

@jpfinne
Copy link
Contributor Author

jpfinne commented Apr 26, 2024

I reproduce on master using this test in DefaultCodegenTest using the same contract:

    @Test
    public void testAllVars_issue_18340() {
        final OpenAPI openAPI = TestUtils.parseFlattenSpec("src/test/resources/3_0/spring/issue_18340.yaml");
        // the bug happens with or without this normalization
        new OpenAPINormalizer(openAPI, Map.of("REFACTOR_ALLOF_WITH_PROPERTIES_ONLY", " true")).normalize();
        
        Schema catModel = ModelUtils.getSchema(openAPI, "Cat");
        DefaultCodegen defaultCodegen = new DefaultCodegen();
        defaultCodegen.setOpenAPI(openAPI);
        CodegenModel defaultCat = defaultCodegen.fromModel("Cat", catModel);
        assertThat(getNames(defaultCat.allVars)).isEqualTo(List.of("name", "petType"));  // correct

        // same model gives an invalid var when using SpringCodegen.  name is missing
        SpringCodegen springCodegen = new SpringCodegen();
        springCodegen.setOpenAPI(openAPI);
        CodegenModel springCat = springCodegen.fromModel("Cat", catModel);
        assertThat(getNames(springCat.allVars)).isEqualTo(List.of("petType"));  // should be name,petType

        // Prove that supportsInheritance is the culprit
        SpringCodegen springCodegenNoSupportInheritance = new SpringCodegen() {
            {
                this.supportsInheritance = false;
            }
        };
        springCodegenNoSupportInheritance.setOpenAPI(openAPI);
        CodegenModel springCatNoSupportInheritance = springCodegenNoSupportInheritance.fromModel("Cat", catModel);
        assertThat(getNames(springCatNoSupportInheritance.allVars)).isEqualTo(List.of("name", "petType"));  // correct again
    }

@jpfinne
Copy link
Contributor Author

jpfinne commented Apr 26, 2024

Additional info. Moving the properties to the list of allOf fixes the issue:

    Cat:
      allOf:
        - $ref: '#/components/schemas/Pet'
        - type: object
          properties:
            name:
              type: string

allVars = petType,name (IMHO a much better order)

Using this contract and the DefaultCodegen (ie without supportInheriance)

    Cat:
      allOf:
        - $ref: '#/components/schemas/Pet'
      type: object
      properties:
        name:
          type: string

allVars = name,petType (IMHO not a good order. See #17542)

So I would "fix" REFACTOR_ALLOF_WITH_PROPERTIES_ONLY (or create another normalizer) to migrate to first version to the second version

jpfinne added a commit to jpfinne/openapi-generator that referenced this issue Apr 26, 2024
jpfinne added a commit to jpfinne/openapi-generator that referenced this issue Apr 26, 2024
wing328 added a commit that referenced this issue May 1, 2024
…#18538)

* allArgConstructor for java

* Remove leftover from builder pattern branch

* Rename generateConstructorWithAllArgs and use x- in vendorExtensions

* Test issue #18340

* Add evidences for allVars issue (#18340)

* remove eol

* update doc

---------

Co-authored-by: jpfinne <[email protected]>
@jpfinne
Copy link
Contributor Author

jpfinne commented May 4, 2024

@wing328 Sorry my mistake, in testAllVars_issue_18340() I made a stupid typo with an extra space for " true".

Map.of("REFACTOR_ALLOF_WITH_PROPERTIES_ONLY", " true")

Sorry that the typo is now on master

@jpfinne jpfinne closed this as completed May 4, 2024
@wing328
Copy link
Member

wing328 commented May 5, 2024

@jpfinne that's ok. please fix it later in another PR

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

No branches or pull requests

2 participants