rest: Extend HTTPRequest interface to support querying path (parameters) #25754
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4d37532 to
fee9bc0
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
This was referenced Jul 31, 2022
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
In future commits, we'll need to directly convert a format string into RESTResponseFormat without all the other logic from ParseDataFormat
Equivalent of -deprecatedrpc for the REST API. Allows the user to keep the specified deprecated functionality unchanged
Instead of specifying the response format as an e.g. ".json" path parameter, the format string is now expected to be in the query string "?format=json". This allows for a more standardized URI format, and removes the dependency on too tightly coupled code like ParseDataFormat(). Can be overridden with -deprecatedrest=format, which enables automatic parsing of the URI to move format strings from the path to the query.
No longer necessary since response format is a query parameter
By storing the endpoint prefix (e.g. "/rest/headers/") in the HTTPRequest, we can more easily extract the (relative) path from a URI in subsequent commits. Currently, this is done in http_request_cb().
Utility functions to easily get the query path. GetPath() returns the entire path as a vector of strings, and GetPathParameter() allows to query for individual path parameters with a similar interface as GetQueryParameter().
For readability, also standardize variable name into 'path'
Unless -deprecatedrest=count is enabled, all count parameters are now expected to be a query instead of path parameter.
strURIpart was generally used to access path data, even though this data was already container inside the HTTPRequest. With can now use the new GetPath() and GetPathParameter() interface. Renamed some parameters in function signature to minimize LoC changes
Since we now exclusively access the path through the HTTPRequest, we can remove the unused strURIpart path argument to simplify the rest endpoint interface.
fee9bc0 to
23fb5ca
Compare
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
1 similar comment
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
@stickies-v Looks like this was opened as draft a year ago with no further activity from you. Can this be closed? |
|
Closing as per #25752 (comment) |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Draft, for reference only to support #25752 for discussion on concept and approach. Code and tests should be fully functional, but still to be considered rough. Commits specific to this PR start from rest: store endpoint prefix data in HTTPRequest.
Brief summary
#24098 introduced a simple
HTTPRequest::GetQueryParameter()function to access query parameters directly from the request, which is passed to every endpoint function. This PR extends this interface by introducing a similarly behavingHTTPRequest::GetPath()andHTTPRequest::GetPathParameter().Previously, path data was accessed through the
strURIpartparameter which was passed to all endpoints. Since this data was already contained in theHTTPRequest reqand we now haveHTTPRequest::GetPath()andHTTPRequest::GetPathParameter()which are more intuitive to use, thestrURIpartparameter is removed from the endpoint callback, simplifying the interface.Finally, the
?countquery parameter introduced by #24098 is now made required, unless-deprecatedrest=countis enabled. This way users understand the endpoint is updating, and we can reasonably remove it in some future update to simplify the code.