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

[rest] add query parsing #1940

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

CodingRays
Copy link

This adds the capability to parse url query strings to the rest api.

It also contains a small refactoring of the parser and request class which moved state that is only required for parsing out of the request class. The only exception to this is the body field which is still built up in the request class.

@wgtdkp
Copy link
Member

wgtdkp commented Jul 16, 2023

It looks like there are formatting error with your PR: https://github.com/openthread/ot-br-posix/actions/runs/5553833818/jobs/10171787035?pr=1940

Please run command ./script/make-pretty clang-format to fix it locally.

Comment on lines 94 to 96
auto it = mHeaders.find(aQueryName);

return (it == mHeaders.end()) ? "" : it->second;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use mQueryParameters here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes we definitely should.
I've tested the parsing code but not the api. Looking at the automated tests its gonna be hard to test without having a endpoint that actually uses the query parameters. We have code under works that uses them but only for timeouts so that would not be testable either.

src/rest/parser.hpp Outdated Show resolved Hide resolved
CodingRays and others added 2 commits July 18, 2023 07:52
Co-authored-by: Kangping <wgtdkp@google.com>
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.

None yet

2 participants