gnostic icon indicating copy to clipboard operation
gnostic copied to clipboard

protoc-gen-open generates a bad path for some starred paths

Open asavageiv opened this issue 3 months ago • 1 comments

Given this service definitions:

service WorkspaceService {
  rpc GetWorkspaceSetting(GetWorkspaceSettingRequest) returns (WorkspaceSetting) {
    option (google.api.http) = {get: "/api/v1/{name=workspace/settings/*}"};
    option (google.api.method_signature) = "name";
  }

  message GetWorkspaceSettingRequest {
    // The resource name of the workspace setting.
    // Format: workspace/settings/{setting}
    string name = 1 [
      (google.api.field_behavior) = REQUIRED,
      (google.api.resource_reference) = {type: "api.memos.dev/WorkspaceSetting"}
    ];
  }

I would expect a path to be generated like this:

paths:
    /api/v1/{name}:
        get:
            tags:
                - WorkspaceService
            description: Gets a workspace setting.
            operationId: WorkspaceService_GetWorkspaceSetting
            parameters:
                - name: name
                  in: path
                  description: The setting id.
                  required: true
                  schema:
                    type: string

This is the offending code: https://github.com/google/gnostic/blob/e0e09f70628157dc0db87ed60a109465ae62723b/cmd/protoc-gen-openapi/generator/generator.go#L508-L516

Here is the description of the feature from the Transcoding proto: https://github.com/googleapis/googleapis/blob/f8776fec04e336527ba7279d960105533a1c4e21/google/api/http.proto#L68

asavageiv avatar Sep 12 '25 00:09 asavageiv

This is a tricky case but I think this part of the spec is more clear:

// The syntax `Variable` matches part of the URL path as specified by its
// template. A variable template must not contain other variables. If a variable
// matches a single path segment, its template may be omitted, e.g. `{var}`
// is equivalent to `{var=*}`.

Right now it doesn't generate the same YAML. The latter form will not give anything at all. I think the intention of the {var=something} form is to always to make var a path variable with a given format. I can see how the rule came about based on the resource naming guidance here: https://google.aip.dev/122#guidance. Namely

Resource name components should usually alternate between collection identifiers (example: publishers, books, users) and resource IDs (example: 123, les-miserables, vhugo1802).
Resource names must use the / character to separate individual segments of the resource name.

    Non-terminal segments of a resource name must not contain a / character.
    The terminal segment of a resource name should not contain a / character.

The WorkspaceService is violating that. (See also https://github.com/usememos/memos/issues/4684)

What I want is for it to generate YAML like this:

paths:
    /api/v1/workspace/settings/{name}:
        get:
            tags:
                - WorkspaceService
            description: Gets a workspace setting.
            operationId: WorkspaceService_GetWorkspaceSetting
            parameters:
                - name: name
                  in: path
                  description: The setting id.
                  required: true
                  schema:
                    type: string

That lets the openapi-generator-cli tool at least generate a client that works, and doesn't result in path conflicts if there's another service like the one below. That would result in a conflict on /api/v1/{name} if we went that route.

  rpc GetActivity(GetActivityRequest) returns (Activity) {
    option (google.api.http) = {get: "/api/v1/{name=activities/*}"};
    option (google.api.method_signature) = "name";
  }

asavageiv avatar Sep 12 '25 05:09 asavageiv