openapi-generator icon indicating copy to clipboard operation
openapi-generator copied to clipboard

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

Open jpfinne opened this issue 10 months ago • 6 comments

Bug Report Checklist

  • [x] Have you provided a full/minimal spec to reproduce the issue?
  • [x] Have you validated the input using an OpenAPI validator (example)?
  • [x] Have you tested with the latest master to confirm the issue still exists?
  • [ ] Have you searched for related issues/PRs?
  • [x] 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

jpfinne avatar Apr 09 '24 07:04 jpfinne

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

wing328 avatar Apr 09 '24 07:04 wing328

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

jpfinne avatar Apr 09 '24 07:04 jpfinne

~~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?

wing328 avatar Apr 09 '24 07:04 wing328

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

jpfinne avatar Apr 09 '24 08:04 jpfinne

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 avatar Apr 26 '24 13:04 jpfinne

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 avatar Apr 26 '24 14:04 jpfinne

@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 avatar May 04 '24 15:05 jpfinne

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

wing328 avatar May 05 '24 14:05 wing328