[BUG] enum parameters are no longer enum types since 4.0.0-beta3

mboogerd commented 5 years ago

Description

I am using a custom mustache template to generate an alternative controller whose operations use nullable enums for optional enum query parameters. This has been working fine until 4.0.0 beta2, but has started breaking from beta3 (and is still broken in the 4.0.0 release). As far as I have narrowed down the issue, the {{isEnum}} expression no longer evaluates to true. This happens at least when used in the context of a query parameter, but still works fine for models. From the code differences between the two releases I could not find out why it has stopped working for (query) parameters

As a result of the above, my custom mustache template dataTypeNullable: {{&dataType}}{{#isEnum}}{{^required}}?{{/required}}{{/isEnum}}

no longer functions as desired when used in the context of generating operation parameters for my controllers. If this isn't a bug, it would be much appreciated if someone could point me in the direction of a fix!

openapi-generator version

OK <= 4.0.0-beta2 NOK > 4.0.0-beta2

Minimal OpenAPI declaration to reproduce

openapi: "3.0.0"
info:
  version: 1.0.0
  title: "Example API"
servers:
  - url: http://example/
paths:
  /:
    get:
      parameters:
        - in: query
          name: param
          schema:
            $ref: "#/components/schemas/SomeEnum"
          required: false
      operationId: exampleOperation
      responses:
        default:
          description: "not implemented error"
components:
  schemas:
    SomeEnum:
      type: string
      enum:
        - item1
        - item2

Command line used for generation

We have been using the generator as part of a multi-stage docker build, as follows (although I am pretty sure this doesn't matter).

FROM openapitools/openapi-generator-cli:v4.0.0-beta2 AS generator

WORKDIR /local
COPY . .

RUN /usr/local/bin/docker-entrypoint.sh generate \
    -DpackageName=TestService \
    -DdebugOperations=true \
    -i test-service.yaml \
    -g aspnetcore \
    -o out/

Steps to reproduce

Run the 4.0.0-beta2 generator with debugOperations set to true and capture the debug log
Run the 4.0.0-beta3 generator with debugOperations set to true and capture the debug log
Diff the log and obtain something like this.
Observe that the isEnum property of the param parameter has moved from true to false between beta2 and beta3.

Suggest a fix

auto-labeler[bot] commented 5 years ago

👍 Thanks for opening this issue! 🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

mboogerd commented 5 years ago

Updated title as I believe from my investigations that the CodegenOperations produced will affect all generators. I can confirm that I also have the issue in the csharp-netcore client generator, besides aspnetcore server generator.

wing328 commented 5 years ago

@mboogerd Thanks for reporting the issue with the details. Do you mind doing a git-bisect to determine which commit has caused the issue?

4.0.0-beta commit: de2923f

Ref: https://stackoverflow.com/questions/4713088/how-to-use-git-bisect

mboogerd commented 5 years ago

(rewrote my previous message as it was based on poorly disecting the error) Bisect results: c4d982f77584b8370ba16a33c5443b4b3af68fa2 is the first bad commit. If I pass my above yaml to:

cd openapi-generator/modules/openapi-generator-cli

mvn clean install -DskipTests

java -jar ./target/openapi-generator-cli.jar generate \
    -DdebugOperations=true \
    -i test.yaml \
    -g aspnetcore \
    -o out/ > testlog

We can see that the log reports allParams to contain an element with isEnum: true on beta2, and isEnum: false on c4d982f77584b8370ba16a33c5443b4b3af68fa2.

@wing328: Hope this helps!

DonDi94 commented 5 years ago

@wing328 I reported the same issue a while back (https://github.com/OpenAPITools/openapi-generator/issues/2645).

I investigated more and it seems to be a problem with DefaultCodegen.java, in the fromProperty function. Around line 2119 the referenced schema for enums is processed, but it is missing the property.isEnum = true assignment which the "inline enum case" just above has.

//Inline enum case:
if (p.getEnum() != null && !p.getEnum().isEmpty()) {
    List<Object> _enum = p.getEnum();
    property._enum = new ArrayList<String>();
    for (Object i : _enum) {
        property._enum.add(String.valueOf(i));
    }
    property.isEnum = true; //isEnum is set to true for the inline but not for the referenced case

    Map<String, Object> allowableValues = new HashMap<String, Object>();
    allowableValues.put("values", _enum);
    if (allowableValues.size() > 0) {
        property.allowableValues = allowableValues;
    }
}

Schema referencedSchema = ModelUtils.getReferencedSchema(this.openAPI, p);

//Referenced enum case:
if (referencedSchema.getEnum() != null && !referencedSchema.getEnum().isEmpty()) {
    List<Object> _enum = referencedSchema.getEnum();

    Map<String, Object> allowableValues = new HashMap<String, Object>();
    allowableValues.put("values", _enum);
    if (allowableValues.size() > 0) {
        property.allowableValues = allowableValues;
        //missing "property.isEnum = true;" statement
    }
}

Also the type assignment for the referenced enum is missing. A solution that I tested and works is to move the type checking done at the beginning of the function into a helper function, and use that for both property and referencedSchema. See gist https://gist.github.com/DonDi94/ab604d29f6f1391d7cba4c5e5caac7f9/revisions

With this change the issue seems to be solved: https://gist.github.com/DonDi94/77aadb10ab9e4f4a32ad610a34313cd8/revisions

wing328 commented 5 years ago

@DonDi94 can you please file a PR with the suggested fix?

DonDi94 commented 5 years ago

@wing328 I have just published a pull request. I didn't manage to test it with mvn verify -Psamples, but the tests at compile time all pass and the problem in this issue seems to be fixed. I placed the isEnum=true in a slightly different position from what I previously suggested to keep a consistent logic with the inline enum definition's code.

DonDi94 commented 5 years ago

@wing328 After a fist unsuccessful commit, I fixed a bug and now it works with all the integration tests passing. It is ready for merge if you want.

jmini commented 5 years ago

This issue described here seems to be really specific to aspnetcore and has something to do with the nullable support. It was working before and did not work after this change: https://github.com/OpenAPITools/openapi-generator/commit/c4d982f77584b8370ba16a33c5443b4b3af68fa2 PR #2529

@drl-max, @wing328 maybe you can have a look at the specific example with the specific generator.

@DonDi94: I suggest we continue the discussion for isEnum=true in #2645

drl-max commented 5 years ago

I've been investigating this for a few hours now and, from what I can tell, the root cause of the regression was when I removed the isNullable and required evaluation logic from the csharp templates in PR #2528, but failed to realize that aspnetcore depended on that.

As @DonDi94 pointed out, enum schemas appear to be handled very differently if they are referenced versus inline. I added a second optional enum query parameter to the example Spec but this time it was defined inline.

openapi: "3.0.0"
info:
  version: 1.0.0
  title: "Example API"
servers:
  - url: http://example/
paths:
  /:
    get:
      parameters:
        - in: query
          name: param
          schema:
            $ref: "#/components/schemas/SomeEnum"
          required: false
        - in: query
          name: param2
          required: false
          schema:
            type: string
            nullable: true
            enum:
              - item1
              - item2
      operationId: exampleOperation
      responses:
        default:
          description: "not implemented error"
components:
  schemas:
    SomeEnum:
      type: string
      nullable: true
      enum:
        - item1
        - item2

This parameter ultimately came out with isEnum being true, but the baseType being string, which is what showed up in the generated code.

public virtual IActionResult ExampleOperation([FromQuery]SomeEnum param, [FromQuery]string param2)

I will continue to investigate this as best I can.

jmini commented 5 years ago

paths:
  /:
    get:
      parameters:
        - in: query
          name: param
          schema:
            $ref: "#/components/schemas/SomeEnum"
          required: false
        - in: query
          name: param2
          required: false
          schema:
            type: string
            nullable: true
            enum:
              - item1
              - item2
      operationId: exampleOperation
      responses:
        default:
          description: "not implemented error"

Nice example.

In https://github.com/OpenAPITools/openapi-generator/issues/2645#issuecomment-509544274 I have tried to explain why even if semantically the schemas defined in param and in param2 are the same (from an OpenAPI Spec point of view), OpenAPI-Generator needs to interpret cases both differently. This is the expected behavior, in particular for languages where SomeEnum is generated as enum class in the model package, when the object is shared between 2 definitions.

OpenAPITools / openapi-generator