nytimes / openapi2proto

A tool for generating Protobuf v3 schemas and gRPC service definitions from OpenAPI specifications
Apache License 2.0
964 stars 98 forks source link

Array Enum Definition #48

Closed garyluu closed 6 years ago

garyluu commented 6 years ago

I'm currently encountering an issue involving a definition containing an array of string enum. Here's my swagger.yaml:

swagger: '2.0'
info:
  title: Example
  version: "1.0.0"
paths:
  /endpoint/{id}:
    get:
      tags:
        - ExampleTag
      parameters:
        - name: id
          in: path
          required: true
          type: string
      responses:
        '200':
          description: Example description
          schema:
            type: array
            items:
              $ref: '#/definitions/FirstDefinition'

definitions:
  FirstDefinition:
    type: object
    properties:
      first_property:
        type: array
        items:
          type: string
          enum:
            - A
            - B

Here's what I get:

syntax = "proto3";

import "google/api/annotations.proto";

package example;

message GetEndpointIdRequest {
    string id = 1;
}

message GetEndpointIdResponse {
    repeated FirstDefinition items = 1;
}

message FirstDefinition {
    repeated string first_property = 1;
}

service ExampleService {
    rpc GetEndpointId(GetEndpointIdRequest) returns (GetEndpointIdResponse) {
      option (google.api.http) = {
        get: "/endpoint/{id}"
      };
    }
}

I'm kind of expecting my enum ["A","B"] to be present somewhere.

garyluu commented 6 years ago

My current workaround:

swagger: '2.0'
info:
  title: Example
  version: "1.0.0"
paths:
  /endpoint/{id}:
    get:
      tags:
        - ExampleTag
      parameters:
        - name: id
          in: path
          required: true
          type: string
      responses:
        '200':
          description: Example description
          schema:
            type: array
            items:
              $ref: '#/definitions/FirstDefinition'

definitions:
  EnumThingy:
    type: string
    enum:
      - A
      - B
  FirstDefinition:
    type: object
    properties:
      first_property:
        type: array
        items:
          $ref: '#/definitions/EnumThingy'
jprobinson commented 6 years ago

Yikes! At least we have a work around, but I'll try and get to this today.

Thanks for reporting, @garyluu!

lestrrat commented 6 years ago

as of d8a7802e51f4cdb2af1c50afbfe9e5f14187fdb9, the first schema creates a proto file like this:

syntax = "proto3";

package example;

message GetEndpointIdRequest {
    string id = 1;
}

message GetEndpointIdResponse {
    repeated FirstDefinition items = 1;
}

message FirstDefinition {
    enum First_property {
        FIRST_PROPERTY_A = 0;
        FIRST_PROPERTY_B = 1;
    }
    repeated First_property first_property = 1;
}

service ExampleService {
    rpc GetEndpointId(GetEndpointIdRequest) returns (GetEndpointIdResponse) {}
}

This would be almost right, except the enum names are not right (should be A and B)

lestrrat commented 6 years ago

Well, actually it says in the README

* To prevent enum collisions and to match the [protobuf style guide](https://developers.google.com/protocol-buffers/docs/style#enums), enum values will be `CAPITALS_WITH_UNDERSCORES` and nested enum values and will have their parent types prepended.

I don't use enums much, is that FIRST_PROPERTY_A thing equivalent from the workaround?

jprobinson commented 6 years ago

I think part of the logic to do this:

nested enum values and will have their parent types prepended.

unfortunately ended up leaking into a higher order of logic than we wanted to and no one has really swoop in to fix it.

In the end, I think I have had several similar enum types with indentical values so the prefix has helped avoid collisions so you could swing it as an unexpected "feature".

Would you prefer it to spit out something like this?

    enum First_property {
        A = 0;
        B = 1;
    }
jprobinson commented 6 years ago

@lestrrat If you want the result above, you can unnest the enum definition, similar to the work around above. Pretty much all nested definitions will get a prefix with their parent definition.

I realize now that this issue has been resolved via some other PRs that I've recently made. I'm going to close this issue for now if no other complaints.