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][protobuf-schema] Protobuf generator does not respect enableEnumPrefixRemoval #11484

Open
5 of 6 tasks
JulianGmp opened this issue Feb 1, 2022 · 3 comments
Open
5 of 6 tasks

Comments

@JulianGmp
Copy link
Contributor

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

When setting enableEnumPrefixRemoval=false, the protobuf-schema generator does not respect the flag and removes the enum value prefixes anyway.

openapi-generator version
openapi-generator-cli 6.0.0-SNAPSHOT
  commit : 8a2131f
  built  : 2022-02-01T15:07:00+01:00
  source : https://github.com/openapitools/openapi-generator
  docs   : https://openapi-generator.tech/
OpenAPI declaration file content or url

test.yaml

openapi: 3.0.0
info:
  title: Sample API
  description: Optional multiline or single-line description in [CommonMark](http://commonmark.org/help/) or HTML.
  version: 0.1.9

servers:
  - url: http://api.example.com/v1
    description: Optional server description, e.g. Main (production) server

components:
  schemas:
    userinfo:
      properties:
        something:
          type: string
          format: enum
          enum:
          - MY_ENUM_a
          - MY_ENUM_b
        somethingElse:
          type: string
          format: enum
          enum:
          - MY_OTHER_ENUM_a
          - MY_OTHER_ENUM_bla

paths:
  /users:
    get:
      summary: Returns a list of users.
      description: Optional extended description in CommonMark or HTML.
      responses:
        '200':
          description: A JSON array of user names
          content:
            application/json:
              schema:
                $ref: '#components/schemas/userinfo'
Generation Details

Generated output:

syntax = "proto3";

package openapitools;


message Userinfo {

  enum SomethingEnum {
    A = 0;
    B = 1;
  }

  SomethingEnum something = 28014471;

  enum SomethingElseEnum {
    A = 0;
    BLA = 1;
  }

  SomethingElseEnum somethingElse = 396934355;

}

Expected:

syntax = "proto3";

package openapitools;


message Userinfo {

  enum SomethingEnum {
    MY_ENUM_A = 0;
    MY_ENUM_B = 1;
  }

  SomethingEnum something = 28014471;

  enum SomethingElseEnum {
    MY_OTHER_ENUM_A = 0;
    MY_OTHER_ENUM_BLA = 1;
  }

  SomethingElseEnum somethingElse = 396934355;

}

NOTE: This is especially a problem with protobuf, because enum values have to be globally unique (imagine C enums instead of Java enums). This is a general issue of translating openapi to protobuf, but that's out of scope for this bug report.

I added a prefix to the values in my project, specifically to prevent this, but the generator removes it despite the option set.

Note that I tried the java generator too, which respected the setting.

Steps to reproduce
java -jar ./openapi-generator-cli-master.jar generate -g protobuf-schema -i test.yaml -o testout/ --additional-properties 'enableEnumPrefixRemoval=false'
Related issues/PRs
Suggest a fix
@Yurzel
Copy link
Contributor

Yurzel commented Feb 8, 2022

Yea I don't know about this. So far there is an option to put Unknown = 0 to the enum which always starts with a prefix of the enum name.

With that in mind I made the prefix across all the enum values. It should fix the collision issue aswell. I'll link PR to this issue.

The result would be:

enum SomethingEnum {
   SomethingEnum.A = 0;
   SomethingEnum.B = 1;
}

enum SomethingElseEnum {
   SomethingElseEnum.A = 0;
   SomethingElseEnum.BLA = 1;
}

There is actually a problem in Python code generated by grpcio, where you get collision between message name and enum value.😄

@bintoll
Copy link

bintoll commented Mar 17, 2023

Hello! I have just started using this generator and I feel that this implementation is not correct. This codegen changes actually breaks the initial idea of code generation based on schema, because after this change the schema is no longer valid. Here is me setup: nestjs, @grpc/grpc-js, @grpc/proto-loader, class-validator to validate incoming payload, swagger to generated openapi schema based on code, and this generator to generate protobuf based on openapi schema. In code my enum looks like:

enum IdentifierType {
  EMAIL = 'EMAIL',
  PHONE = 'PHONE'
}

After using this generator I get this enum in protobuf:

enum IdentifierType {
  IdentifierType_EMAIL = 0;
  IdentifierType_PHONE = 1;
}

Which is not the same. It results in having the request payload:

{
  identifierType: 'IdentifierType_EMAIL'
}

However it must be:

{
  identifierType: 'EMAIL'
}

to pass the validation pipe.

I completely understand the limitation that each enum name has to be globally unique. From my point of view generator with default config should keep track of all enum values during the generation process and throw an error if there are some non unique enums and it should NOT automatically add this prefixes leading to the fact that the generated protobuf schema is not completable with the initial openapi schema. And for those who really want generator to add this prefixes there should be a custom config option enabling this, because again, the idea of using a generator is to get the exactly same schema based on other schema, but now it just breaks initial complitability.

UPD:
This generally happens because @grpc/proto-loader transforms received enum number value to string using enum keys (this behavior was configured for @grpc/proto-loader which is quite usual I think). Using numeric enums solves this issue, how ever I still think that this feature to add prefixes in generated protobuf schema should be enabled with custom parameter and not by default.

@bintoll
Copy link

bintoll commented Mar 19, 2023

@wing328 Hello! Could you please comment this? I would not like to open another issue with same topic as long as there are thousands of them here. And as long as you maintain this generator, you know the specific better than no-one else.

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

3 participants