Skip to content

[typescript-angular] support for object query parameters as nested key/json string#5790

Merged
macjohnny merged 1 commit intoOpenAPITools:masterfrom
dougal83:typescript-angular-query-param-object-options
Jun 18, 2020
Merged

[typescript-angular] support for object query parameters as nested key/json string#5790
macjohnny merged 1 commit intoOpenAPITools:masterfrom
dougal83:typescript-angular-query-param-object-options

Conversation

@dougal83
Copy link
Contributor

@dougal83 dougal83 commented Apr 1, 2020

fixes: #5781

Extend support to include an option to configure how objects are encoded in query parameters. To include options for nested keys and JSON string.

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.

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @nicokoenig @topce @akehir @petejohansonxo @amakhrov

@auto-labeler
Copy link

auto-labeler bot commented Apr 1, 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.

Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

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

LGTM

@macjohnny
Copy link
Member

macjohnny commented Apr 1, 2020

according to the CI (https://circleci.com/gh/OpenAPITools/openapi-generator/14068#tests/containers/2) the docs file needs to be changed

@macjohnny macjohnny added this to the 4.3.1 milestone Apr 1, 2020
@dougal83
Copy link
Contributor Author

dougal83 commented Apr 1, 2020

according to the CI (https://circleci.com/gh/OpenAPITools/openapi-generator/14068#tests/containers/2) the docs file needs to be changed

@macjohnny Thank you for the review.

I added the following line:

|queryParamObjectFormat|Query parameter object value format: 'dot', 'json', 'key'.| |dot|

to docs/generators/typescript-angular.md. Is this the correct way to update the docs/web page for the parameters?

EDIT: ran bin/utils/ensure-up-to-date

@amakhrov
Copy link
Contributor

amakhrov commented Apr 1, 2020

@dougal83
After reviewing the spec, I also realized that OAS3 has a standard way to define the format of nested objects:
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.3.md#style-values (see deepObject style)

However, it's not really supported by any of the existing generators. Only php generators do smth like that, but they do not respect the style=deepObject setting anyway.

I'm really curious if there is any "standard-ish" approach implemented by the majority of other generators for object params serialization that we can consider a de-facto standard?
@TiFu @macjohnny any insights on that?

@dougal83
Copy link
Contributor Author

dougal83 commented Apr 1, 2020

@dougal83
After reviewing the spec, I also realized that OAS3 has a standard way to define the format of nested objects:
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.3.md#style-values (see deepObject style)

I'm interested in using openapi-generator with loopback, there is an issue over there regarding supporting deep objects. Am interested in helping if I can.

If anything, I would personally make the key option the default but the dot notation was already in place. key is the closest match to the official standard but I've not looked into it beyond my personal project.

@amakhrov
Copy link
Contributor

amakhrov commented Apr 2, 2020

Looks like json serialization is supported by the spec, as mentioned in this comment: OAI/OpenAPI-Specification#1706 (comment)

should we support this in the typescript generator instead of a custom generator cli option?

@dougal83
Copy link
Contributor Author

dougal83 commented Apr 2, 2020

Looks like json serialization is supported by the spec, as mentioned in this comment: OAI/OpenAPI-Specification#1706 (comment)

should we support this in the typescript generator instead of a custom generator cli option?

I'll leave this up to the tech. committee to decide. Feel free to do as you see fit with this PR depending on the outcome of the discussion. 👍

Copy link
Contributor

@TiFu TiFu left a comment

Choose a reason for hiding this comment

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

Maybe to add my two cents:

As long as this adheres to the spec - and it seems like it does because the spec leaves this detail undefined - I'm fine with this change. It just makes more options available to whoever is using the generator.

@wing328 wing328 removed this from the 4.3.1 milestone May 6, 2020
@dougal83
Copy link
Contributor Author

dougal83 commented May 6, 2020

Looks like json serialization is supported by the spec, as mentioned in this comment: OAI/OpenAPI-Specification#1706 (comment)

should we support this in the typescript generator instead of a custom generator cli option?

Perhaps we should close this PR in the light of missing the latest release and focus on the Typescript avenue for improvement.

@macjohnny
Copy link
Member

I will merge this once #6360 is merged, since there will be conflicts

@mrbatista
Copy link

@macjohnny any update?

@macjohnny
Copy link
Member

@dougal83 could you please merge the latest master into your branch, resolve the conflicts and re-generated the samples?

@dougal83 dougal83 force-pushed the typescript-angular-query-param-object-options branch from 3560c11 to 49d5198 Compare June 8, 2020 19:21
@dougal83
Copy link
Contributor Author

dougal83 commented Jun 8, 2020

Performed simple rebase and resolved conflict. Untested and samples not regenerated(don't think they are affected). I need to install JDK & Maven... try to do shortly.

@dougal83 dougal83 force-pushed the typescript-angular-query-param-object-options branch from 49d5198 to f3a5ac7 Compare June 8, 2020 19:33
@dougal83
Copy link
Contributor Author

dougal83 commented Jun 9, 2020

@macjohnny Tests pass. Sample regeneration run. 👍

fixes: OpenAPITools#5781

Signed-off-by: Douglas McConnachie <dougal83+git@gmail.com>
@dougal83 dougal83 force-pushed the typescript-angular-query-param-object-options branch from b4d5433 to b085ed8 Compare June 18, 2020 15:04
@dougal83
Copy link
Contributor Author

Removed the last commit with sample regen as nothing material was added. I hope this is ready to go after checks...

@macjohnny macjohnny added this to the 5.0.0 milestone Jun 18, 2020
@macjohnny macjohnny merged commit 4e352cb into OpenAPITools:master Jun 18, 2020
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.

[REQ] [typescript-angular] support for object query parameters as nested key/json string

6 participants