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

Generated OpenAPI document fails validation on parameters in path #2084

Closed
someone1 opened this issue Apr 12, 2021 · 5 comments
Closed

Generated OpenAPI document fails validation on parameters in path #2084

someone1 opened this issue Apr 12, 2021 · 5 comments
Labels

Comments

@someone1
Copy link
Contributor

🐛 Bug Report

Take the example a little bit of everything swagger file and pop it into swagger editor/UI - it will fail validation checks

This feels like the issues raised in #720 and #1015 though I didn't want to bump an older issue or hijack another issue that seemed more specific.

Sometimes the path annotations generate fine in the OpenAPI document and other times not. For example, the following annotrations seem to work:

option (google.api.http) = {
      get: "/v1alpha/{user=users/*}/noun:verb"
    };

option (google.api.http) = {
      post: "/v1alpha/{user=users/*}/noun2:verb"
      body: "*"
    };

Generates:

"/v1alpha/{user}/noun:verb": {
      "get": {
        "parameters": [
          {
            "name": "user",
            "in": "path",
            "required": true,
            "type": "string"
          }
        ],
    }
}
"/v1alpha/{user}/advice:generate": {
      "post": {
        "parameters": [
          {
            "name": "user",
            "in": "path",
            "required": true,
            "type": "string"
          },
          {
            "name": "body",
            "in": "body",
            "required": true,
            "schema": {
              "$ref": "#/definitions/v1alphaVerbRequest"
            }
          }
        ],
    }
}

However, the following does not:

    option (google.api.http) = {
      get: "/v1alpha/{parent=users/*}/accounts"
    };
    option (google.api.http) = {
      get: "/v1alpha/{name=users/*/accounts/*}"
    };

Generates:

 "/v1alpha/{parent=users/*}/accounts": {
      "get": {
        "parameters": [
          {
            "name": "parent",
            "in": "path",
            "required": true,
            "type": "string"
          }
        ],
    }
}
 "/v1alpha/{name=users/*/accounts/*}": {
      "get": {
        "parameters": [
          {
            "name": "name",
            "in": "path",
            "required": true,
            "type": "string"
          }
        ],
    }
}

Interestingly, in the example above that doesn't' work, renaming parent= to user= generates properly!

    option (google.api.http) = {
      get: "/v1alpha/{user=users/*}/accounts"
    };
  "/v1alpha/{user}/accounts": {
      "get": {
        "parameters": [
          {
            "name": "user",
            "in": "path",
            "required": true,
            "type": "string"
          }
        ],
   }
}

Does the generator expect specific values in which it will trigger the correct generation in the OpenAPI spec for the path annotations? Switching from parent to user to fix one such case of issues sounds buggy to me. I'm not certain how the second example /v1alpha/{name=users/*/accounts/*} should map to query params in the OpenAPI spec, but the URL should probably look like /v1alpha/users/*/accounts/* - I think the gateway itself processes all this okay, its just the generated OpenAPI spec that's falling short.

Generated w/ v2.3.0

@someone1
Copy link
Contributor Author

someone1 commented Apr 12, 2021

Some light digging and it seems that name and parent are explicitly flagged as resource names and behave differently.

This is probably the right check, but maybe there should be a different outcome? Replacing /v1alpha/{name=users/*/accounts/*} with /v1alpha/{name} is wrong, and I don't think there's a version where you can get the path and named parameter to both exist. I think the OpenAPI spec will have to be generated where the path parameter is broken up for the sake of the path itself, otherwise you run the risk of duplicate paths (e.g. /v1/{name=projects/*/subscription/*} vs /v1/{name=projects/*/topics/*} would be indistinguishable).

This is my first time looking at the code so apologies if this sounds appalling, but what do you think about the following?

  • If the resource name has 1 part, then this can just become the named part of the URL: /v1alpha/{parent=users/*}/accounts -> /v1alpha/users/{parent}/accounts
  • If there are more than 2 parts, then each part is a named parameter: /v1alpha/{account.name=users/*/accounts/*} -> /v1alpha/users/{name_users}/accounts/{name_accounts}

We sacrifice the parameter's intended format in exchange for being able to generate an OpenAPI file.

@johanbrandhorst
Copy link
Collaborator

Hi @someone1, thanks for the detailed issue! This area of the code is somewhat unfamiliar to me as well, so I would be happy to discuss changing it so that it works as expected in the generator. We'll want to be wary that whatever changes we make actually correspond to how it's being parsed by the gateway, but I think you suggestion makes sense. Then the question becomes whether our existing parser is clever enough to be able to make that translation, or if we need to rework it.

Would you be interested in contributing a fix for this?

@stale
Copy link

stale bot commented Jun 16, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@tenevdev
Copy link

I'd like to contribute a fix if the draft I referenced is a few commits away from an acceptable solution.

@stale stale bot removed the wontfix label Aug 12, 2021
@stale
Copy link

stale bot commented Oct 12, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Oct 12, 2021
@stale stale bot closed this as completed Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants