Skip to content

Remove auto-JSONification of query params, allowing them to be unquoted#2309

Merged
eddyashton merged 29 commits into
microsoft:mainfrom
eddyashton:non_json_query_parsing
Mar 16, 2021
Merged

Remove auto-JSONification of query params, allowing them to be unquoted#2309
eddyashton merged 29 commits into
microsoft:mainfrom
eddyashton:non_json_query_parsing

Conversation

@eddyashton
Copy link
Copy Markdown
Member

Resolves #2153.

The json_handler adapter no longer parses the query string. Any request parameters which are included in the query string must be extracted manually by the handler, and we will not try to read/parse them beforehand. We do add some helper functions in http_query.h to remove some of the boiler plate in query parsing.

The main benefit of this is that when those query parameters are strings they no longer need to be quoted. Additionally, this gives the app more precise control of the OpenAPI description (in a form we want to extend in future). It also makes parsing of GET and POST requests more consistent and removes the requirement that we are opinionated on GETs having no body (the pre-parsed JSON will now contain the body from either method, and query strings must be manually parsed in either method).

@eddyashton eddyashton requested a review from a team as a code owner March 11, 2021 18:12
Comment thread doc/schemas/app_openapi.json
@ghost
Copy link
Copy Markdown

ghost commented Mar 11, 2021

non_json_query_parsing@20717 aka 20210316.28 vs main ewma over 20 builds from 20228 to 20700
images

Comment thread src/node/rpc/endpoint_registry.h
Comment thread src/node/rpc/endpoint_registry.h
Comment thread CHANGELOG.md
Comment thread CHANGELOG.md Outdated
Comment thread CHANGELOG.md
Copy link
Copy Markdown
Member

@achamayou achamayou left a comment

Choose a reason for hiding this comment

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

I think it would be good to write some unit tests for the query parsing, to cover corner cases on separators etc.

@eddyashton
Copy link
Copy Markdown
Member Author

I think it would be good to write some unit tests for the query parsing, to cover corner cases on separators etc.

I've added a unit test in http_test.cpp. This confirms the behaviour of a few corner cases, let me know if you'd like anything else tested.

@eddyashton eddyashton merged commit 9292b82 into microsoft:main Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Accept plain strings for string query parameters of C++ endpoints

4 participants