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

feat(protoc-gen-openapi): remove path parameters from body #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

istvan-hevele
Copy link
Owner

@istvan-hevele istvan-hevele commented Mar 25, 2022

When a request has both path parameters and body: "*", the path parameters are being repeated in the body in the resulting OpenAPI spec.
Note how the following example has name both in parameters and requestBody in the generated openapi spec.

protobuf:

service LibraryService {
  rpc MergeShelves(MergeShelvesRequest) return (Shelf) {
    option (google.api.http) = {
      post: "/v1/{name=shelves/*}:merge"
      body: "*"
    };
  }
}

// Describes the shelf being removed (other_shelf_name) and updated
// (name) in this merge.
message MergeShelvesRequest {
  // The name of the shelf we're adding books to.
  string name = 1 [
    (google.api.field_behavior) = REQUIRED,
    (google.api.resource_reference).type = "Shelf"
  ];

  // The name of the shelf we're removing books from and deleting.
  string other_shelf_name = 2 [
    (google.api.field_behavior) = REQUIRED,
    (google.api.resource_reference).type = "Shelf"
  ];
}

openapi spec:

paths:
    /v1/shelves/{shelf}:merge:
        post:
            tags:
                - LibraryService
            description: |-
                Merges two shelves by adding all books from the shelf named
                 `other_shelf_name` to shelf `name`, and deletes
                 `other_shelf_name`. Returns the updated shelf.
                 The book ids of the moved books may not be the same as the original books.
                 Returns NOT_FOUND if either shelf does not exist.
                 This call is a no-op if the specified shelves are the same.
            operationId: LibraryService_MergeShelves
            parameters:
                - name: shelf
                  in: path
                  description: The shelf id.
                  required: true
                  schema:
                    type: string
            requestBody:
                content:
                    application/json:
                        schema:
                            $ref: '#/components/schemas/MergeShelvesRequest'
                required: true
            responses:
                "200":
                    description: OK
                    content:
                        application/json:
                            schema:
                                $ref: '#/components/schemas/Shelf'
components:
    schemas:
        MergeShelvesRequest:
            required:
                - name
                - other_shelf_name
            type: object
            properties:
                name:
                    type: string
                    description: The name of the shelf we're adding books to.
                other_shelf_name:
                    type: string
                    description: The name of the shelf we're removing books from and deleting.
            description: Describes the shelf being removed (other_shelf_name) and updated (name) in this merge.

According to use_wildcard_in_body, every field not bound by the path template should be mapped to the request body.
So fields that are bound by the path template shouldn't be in the request body.

This PR creates a new type named {method_name}RequestBody which doesn't contain the fields bound by the path template in these cases.
The resulting openapi spec looks like:

paths:
    /v1/shelves/{shelf}:merge:
        post:
            tags:
                - LibraryService
            description: |-
                Merges two shelves by adding all books from the shelf named
                 `other_shelf_name` to shelf `name`, and deletes
                 `other_shelf_name`. Returns the updated shelf.
                 The book ids of the moved books may not be the same as the original books.

                 Returns NOT_FOUND if either shelf does not exist.
                 This call is a no-op if the specified shelves are the same.
            operationId: LibraryService_MergeShelves
            parameters:
                - name: shelf
                  in: path
                  description: The shelf id.
                  required: true
                  schema:
                    type: string
            requestBody:
                content:
                    application/json:
                        schema:
                            $ref: '#/components/schemas/MergeShelvesRequestBody'
                required: true
            responses:
                "200":
                    description: OK
                    content:
                        application/json:
                            schema:
                                $ref: '#/components/schemas/Shelf'
components:
    schemas:
        MergeShelvesRequestBody:
            required:
                - other_shelf_name
            type: object
            properties:
                other_shelf_name:
                    type: string
                    description: The name of the shelf we're removing books from and deleting.
            description: Describes the shelf being removed (other_shelf_name) and updated (name) in this merge.

@istvan-hevele istvan-hevele force-pushed the remove-path-parameters-from-body branch from 96e6109 to d7cdf97 Compare March 25, 2022 09:22
@@ -470,6 +482,7 @@ func (g *OpenAPIv3Generator) buildOperationV3(
file *protogen.File,
operationID string,
tagName string,
methodName string,
description string,
defaultHost string,
path string,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not required of course, but you can clean up a longer list of params like this by consolidating to:
operationID, tagName, methodName, description, defaultHost, path string


// getMessageFieldNames gets the names of all fields from a message
func (g *OpenAPIv3Generator) getMessageFieldNames(message *protogen.Message) []string {
fieldNames := []string{}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fieldNames := make([]string, len(message.Fields))

@@ -127,6 +135,10 @@ func (g *OpenAPIv3Generator) buildDocumentV3() *v3.Document {
g.requiredSchemas = g.requiredSchemas[count:len(g.requiredSchemas)]
}

for _, schema := range g.modifiedSchemas {
g.addSchemaToDocumentV3(d, schema.Message, schema.Name, schema.FieldNames)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what best practice is here, but you can pass the entire schema here since you seem to need all of its attributes

// Pass the entire request message as the request body.
typeName := fullMessageTypeName(inputMessage.Desc)
requestSchema = g.schemaOrReferenceForType(typeName)
if string(inputMessage.Desc.FullName()) != "google.api.HttpBody" && haveCommonItem(coveredParameters, g.getMessageFieldNames(inputMessage)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have access to the actual type? may be more safe to check that against something that isn't hard coded

@daanschipper
Copy link

Hi @istvan-hevele, are you (still) planning on merging this upstream to google/gnostic? The pull request is to your own fork of gnostic, so most likely the maintainers didn't see your changes. I'm also facing the same issue, it would be great if this gets merged upstream, thanks!

@istvan-hevele
Copy link
Owner Author

Hi @daanschipper, I'd like to get this merged, but unfortunately I don't have enough spare time currently to get this rebased and opened against the upstream repo.
I may look on this again in the future though, unless anyone else has the time to help this get merged.

@daanschipper
Copy link

I can help with that! I'll use your changes to generate my openapi specs to validate the changes, address the comments above and open a pull request upstream.

@jagobagascon
Copy link

Hi! I came across this issue (google#323) and was wondering if you ever got the chance to make the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants