rest: Remove support for a number of -deprecatedrest options
#25756
Conversation
This file contains 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
stickies-v
force-pushed
the
rest/query-params-3-cleanup
branch
3 times, most recently
from
b5919e2
to
03f266a
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.
/rest/headers and /rest/blockfilterheaders are collection endpoints, i.e. they return an array of objects. One of the filters they support is to define the block hash from which to return objects. In REST APIs, filter parameters are usually implemented as query parameters instead of path parameters, for example so they can be more easily combined with other filters.
instead of path parameter; reduces complexity
Format strings are now always assumed to be in the query string only
stickies-v
force-pushed
the
rest/query-params-3-cleanup
branch
from
03f266a
to
3e9cb85
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: remove support for -deprecatedrest=format.
Brief summary
#25752 introduced a number of
-deprecatedrestoptions to keep old behaviour on most REST endpoints. With this PR, we remove this support to clean up the code.This PR should only be merged when we want to remove backwards compatibility, which I expect will be at least 1 release after merging the last PR in #25752. I've just prepared the code already to show how it can clean up the endpoint logic.