sudorandom / protoc-gen-connect-openapi

Plugin for generating OpenAPIv3 from protobufs matching the Connect RPC interface
MIT License
102 stars 7 forks source link

Invalid OpenAPI Definition for Path & Body Combination #27

Closed plunkettscott closed 2 months ago

plunkettscott commented 2 months ago

According to google.api.http documentation, specifying the body of * means any fields that are not matched in the path template, however, the following schema is incorrectly generated in the OpenAPI definition according to that specification:

rpc SuspendUser(SuspendUserRequest) returns (SuspendUserResponse) {
    option (google.api.http) = {
        post: "/v1/users/{user_id}/suspend"
        body: "*"
        response_body: "user"
    };
}

message SuspendUserRequest {
    string user_id = 1;
    string reason = 2;
    string internal_notes = 3;
    google.protobuf.Timestamp effective_from = 4;
    google.protobuf.Timestamp effective_to = 5;
    bool cannot_appeal = 6;
}

message SuspendUserResponse {
    User user = 1;
}

This results in this OpenAPI path definition:

  /v1/users/{user_id}/suspend:
    post:
      tags:
        - user.v1.UserService
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/user.v1.SuspendUserRequest'
          application/proto:
            schema:
              $ref: '#/components/schemas/user.v1.SuspendUserRequest'
        required: true
      responses:
        default:
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/connect.error'
        "200":
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/user.v1.SuspendUserResponse'

And the following is the SuspendUserRequest from the schema:

user.v1.SuspendUserRequest:
      type: object
      properties:
        userId:
          type: string
          title: user_id
          additionalProperties: false
        reason:
          type: string
          title: reason
          additionalProperties: false
        internalNotes:
          type: string
          title: internal_notes
          additionalProperties: false
        effectiveFrom:
          $ref: '#/components/schemas/google.protobuf.Timestamp'
        effectiveTo:
          $ref: '#/components/schemas/google.protobuf.Timestamp'
        cannotAppeal:
          type: boolean
          title: cannot_appeal
          additionalProperties: false
      title: SuspendUserRequest
      additionalProperties: false

The expected behavior here would be for the path to contain a parameters definition with user_id mapping to the path variable in the URL and it should be excluded from the SuspendUserRequest payload.

sudorandom commented 2 months ago

Thanks for this report!

The first part of this should be fixed, with used_id showing up as a path parameter.

The second part, with excluding these parameters from the request types will take bit longer. I'll have to change how the response is declared; instead of using a reference I'll probably have to declare it inline, which may be a bit messy because path parameters can be nested deep inside of protobuf messages, so resolving to a filtered type will be a bit... fun.

plunkettscott commented 2 months ago

Thanks! I've verified the first part in the other issue, but let me know if/when you need me to test on this second part. Truthfully, I don't have any schemas where the path parameter is nested deeply, it's always a top-level field of the message, so a solution for nested messages is not super important to me if you need more time to evaluate that one.

sudorandom commented 2 months ago

@plunkettscott Hey, this should be resolved by v0.10.0. As discussed, this only deals with the top-level fields for a message. If it is requested that we filter out attributes from nested messages I'd welcome that issue. I'm just not sure it's really desired/needed.