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

[typescript-fetch] support explode query parameters #6364

Conversation

bobvanderlinden
Copy link

@bobvanderlinden bobvanderlinden commented May 19, 2020

Related to #3781

Currently multiple query parameters that are serialized as CSV. param: [1,2,3] becomes param=1,2,3.

The OpenAPI specification https://swagger.io/docs/specification/serialization/#query says the default serialization for query parameters with multiple values is style: form with explode: true. That means that means that serialization for param: [1,2,3] should be param=1&param=2&param=3.

This PR attempts to add such support for explode for typescript-fetch.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@auto-labeler
Copy link

auto-labeler bot commented May 19, 2020

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@bobvanderlinden bobvanderlinden force-pushed the pr-typescript-fetch-support-explode branch from 079ffa8 to d85110a Compare May 19, 2020 12:35
@bobvanderlinden
Copy link
Author

bobvanderlinden commented May 19, 2020

I'm not sure whether this is a breaking change. It depends whether explode was used in the specification, but the mustache template ignored explode.

None of the petstore examples have changed, because none of the examples use explode: true. Note that the default for explode should be true, but it is currently false in openapi-generator, which I think is wrong. See https://swagger.io/docs/specification/serialization/#query

@bobvanderlinden
Copy link
Author

@leonyu Could you have a look?

@leonyu
Copy link
Contributor

leonyu commented May 20, 2020

Sorry, I no longer work for the company that used this. I don't really have the setup or context to develop for this. If it is a trivial PR, I can code review, that's about it. Beyond that, I can't contribute in any meaningful capacity.

@bobvanderlinden
Copy link
Author

You should probably remove yourself from the technical committee. That said, who should I tag for this PR to review?

@leonyu
Copy link
Contributor

leonyu commented May 20, 2020

I am not on the committee for this project. I never contributed directly to this project. My contributions were for the upstream at https://github.com/swagger-api/swagger-codegen .

My guess is you should probably ping one of the TypeScript person on the committee.

@bobvanderlinden
Copy link
Author

Ah I wasn't aware. Sorry for the inconvenience.

Pinging technical committee of typescript:

@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02)

@amakhrov
Copy link
Contributor

@bobvanderlinden could you please clarify why the existing logic that supports isCollectionFormatMulti doesn't work for you?

@bobvanderlinden
Copy link
Author

You are totally right. It is already supported, but the default of explode is considered to be false by openapi-generator, whereas the standard describes the default to be true. That's probably where the confusion came from. Sorry for the PR and thanks for the clarification!

@jonnekleijer
Copy link

jonnekleijer commented Jul 17, 2020

@amakhrov I am having a similar issue, I could not find (something similar to) isCollectionFormatMulti via the openapi-generator-cli: version openapi-generator-cli-3.3.4.jar?

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

Successfully merging this pull request may close these issues.

4 participants