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

[csharp] Fix string enum models sometimes losing attributes #8765

Closed

Conversation

spanglerco
Copy link

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Depending on HashMap iteration order, the isString flag added to string enum values can be lost due to copying a reference to the allowableValues map to properties that reference the enum. The result is that the code fails to compile.

This should fix #7656, fix #7657 (regarded as a duplicate of 7656), and fix #8027 (which also appears to be the same issue).

Both the base updateCodegenPropertyEnum method and later in postProcessEnumRefs replace the enumVars entry in the allowableValues map. Depending on the order the two methods run for the same enum, it's possible for the "CodegenProperty" values such as isString added by postProcessEnumRefs to get lost when updateCodegenPropertyEnum replaces the list. By passing a copy, the order no longer matters.

I attempted to add a test case to the existing enum tests, but was unable to make the problem reproduce there. But all three linked issues' reproduction documents no longer reproduce the issue with this change. My local test case where I ran into this originally also no longer reproduces with this change.

/cc @mandrean

Depending on HashMap iteration order, the isString flag added to string
enum values can be lost due to copying a reference to the
allowableValues map to properties that reference the enum. The result is
that the code fails to compile.

This should fix swagger-api#7656, fix swagger-api#7657 (regarded as a duplicate of 7656), and
fix swagger-api#8027 (which also appears to be the same issue).
@spanglerco
Copy link
Author

All of the changes to the samples are from before my change. There were no additional changes to the samples after my change.

@ghost
Copy link

ghost commented Jan 16, 2019

Hello @mandrean is there any chance this could be merged into the next 2.x release?

This bug is still present in 2.4.1. I am able to reproduce it just by changing the names of my enums (which changes the hashing order, as described by @spanglerco.

For example the below generates an invalid .cs file:

swagger: '2.0'
info:
  version: 9000.0.1
  title: Repro API
definitions:

  G:
    type: object
    properties:
      SomeProp:
        $ref: '#/definitions/AEnum'


  AEnum:
    type: string
    enum:
      - a
      - b
    
paths:
  /test1:
    get:
      responses:
        200:
          description: The enum referenced from this response will be generated incorrectly with the csharp generator
          schema:
            $ref: '#/definitions/G'

while the following generates a valid .cs file for the enum:

swagger: '2.0'
info:
  version: 9000.0.1
  title: Repro API
definitions:

  G:
    type: object
    properties:
      SomeProp:
        $ref: '#/definitions/ZEnum'


  ZEnum:
    type: string
    enum:
      - a
      - b
    
paths:
  /test1:
    get:
      responses:
        200:
          description: The enum referenced from this response will be generated correctly with the csharp generator
          schema:
            $ref: '#/definitions/G'

@raiworld81
Copy link

Hi @mandrean, this bug is really frustrating for us, how many chances are there to solve it in the next release?

@frantuma
Copy link
Member

Thanks! change applied in #9936

@frantuma frantuma closed this Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants