mtennoe / swagger-typescript-codegen

A Swagger Codegenerator tailored for typescript.
Apache License 2.0
140 stars 52 forks source link

Query parameters in POST request are converted to formdata #124

Closed WVerlaek closed 2 years ago

WVerlaek commented 3 years ago

It looks like it's not possible to have query parameters on POST requests, they are converted to formdata here: https://github.com/mtennoe/swagger-typescript-codegen/blob/master/templates/method.mustache#L179

Is there a specific reason this conversion is done? In our usecase, we generate a swagger spec from proto/grpc definitions using grpc-gateway, where query parameters are used to map fields in a Create request that aren't part of the actual resource to create: https://cloud.google.com/apis/design/standard_methods#create. When these fields are added as formdata in the request they are ignored when handled by the grpc-gateway.

It would be nice if there's a way to disable this conversion to formdata

mtennoe commented 3 years ago

Hey! The reason is that POST requests normally don't have query parameters, as all parameters are typically sent as part of the payload of the request (as all POST requests should have a payload containing this kind of stuff). To be clear though, AFAIK there is nothing in the spec saying that POST requests should not be allowed to have query parameters. Therefore, if this is handled explicitly in this generator I would be happy to take pull requests for this. If so, some questions:

WVerlaek commented 3 years ago

Yeah, having query parameters on POST requests seems to be the standard for grpc-gateway and https://cloud.google.com/apis/design/standard_methods#create

Should it just be an optional bool flag for toggling this?

I think that makes the most sense, to prevent any breaking changes

How do we handle a combination of form data and a query string?

Looking at https://github.com/mtennoe/swagger-typescript-codegen/blob/master/templates/method.mustache#L179 I was thinking we could remove

form = queryParameters;
queryParameters = {};

and pass both form and query parameters to the request?

Happy to open a PR if the above makes sense

mtennoe commented 3 years ago

and pass both form and query parameters to the request?

Hm, I would prefer it if we could separate what params should go into the formdata and what should go into the query parameters. That however would lead to a solution that is overly complex or verbose, so I guess if we name the bool property correctly that would be good enough. Something like duplicatePostFormDataInQueryParameters?

WVerlaek commented 3 years ago

Agree, what I meant was given the following example swagger spec for a POST request:

"post": {
  ...
  "parameters": [
    {
      "type": "boolean",
      "format": "boolean",
      "name": "some_query_parameter",
      "in": "query"
    },
    {
      "type": "boolean",
      "format": "boolean",
      "name": "some_form_parameter",
      "in": "formData"
    }
  ]
}

the current behaviour is that some_query_parameter is sent as form data, and some_form_parameter is not sent at all as form data gets overwritten by the query parameters. I think the solution would be to add a flag, and when enabled, will send some_query_parameter as a query parameter and some_form_parameter as form data in the same request.

From having a look at the mustache template I think this should only be a case of removing

    {{^isBodyParameter}}
        {{#isPOST}}
            form = queryParameters;
            queryParameters = {};
        {{/isPOST}}
    {{/isBodyParameter}}

when the flag is enabled (https://github.com/mtennoe/swagger-typescript-codegen/blob/master/templates/method.mustache#L179)

What do you think?

mtennoe commented 3 years ago

Ah yes, thanks for clarifying! Agree, that sounds like a good approach. It also ensures backwards compatibility for people relying on the formdata behavior

Emyrk commented 2 years ago

Hey any updates on this?

mtennoe commented 2 years ago

Sorry for the slow response, no updates here sadly @Emyrk. Did you want to give your proposal a shot @WVerlaek?

WVerlaek commented 2 years ago

Sorry didn't get a chance to finish this back then, we decided not to use query parameters. Have given it another shot, PR is here: https://github.com/mtennoe/swagger-typescript-codegen/pull/130

mtennoe commented 2 years ago

Great, taking a look!

mtennoe commented 2 years ago

Approved, merged, and published as version 3.2.4. Thanks @WVerlaek!