rest: Extend HTTPRequest interface to support querying path (parameters)

stickies-v commented at 8:35 pm on July 30, 2022: contributor

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 behaving HTTPRequest::GetPath() and HTTPRequest::GetPathParameter().

Previously, path data was accessed through the strURIpart parameter which was passed to all endpoints. Since this data was already contained in the HTTPRequest req and we now have HTTPRequest::GetPath() and HTTPRequest::GetPathParameter() which are more intuitive to use, the strURIpart parameter is removed from the endpoint callback, simplifying the interface.

Finally, the ?count query parameter introduced by #24098 is now made required, unless -deprecatedrest=count is enabled. This way users understand the endpoint is updating, and we can reasonably remove it in some future update to simplify the code.

fanquake added the label RPC/REST/ZMQ on Jul 30, 2022

DrahtBot added the label Needs rebase on Jul 30, 2022

stickies-v force-pushed on Jul 30, 2022

DrahtBot removed the label Needs rebase on Jul 30, 2022

DrahtBot commented at 5:00 am on July 31, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25760 (rest: clean-up for mempool endpoints by brunoerg)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25721 (refactor: Replace BResult with util::Result by ryanofsky)
#25665 (refactor: Add util::Result class and use it in LoadChainstate by ryanofsky)
#25412 (rest: add /deploymentinfo endpoint by brunoerg)
#23599 ([WIP] DRAFT NOMERGE Tidy up RPCTxSerializationFlags by MarcoFalke)

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.

DrahtBot added the label Needs rebase on Aug 5, 2022

DrahtBot commented at 1:54 pm on August 5, 2022: contributor

🐙 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”.

refactor: separate format string parsing from ParseDataFormat

In future commits, we'll need to directly convert a format string
into RESTResponseFormat without all the other logic from ParseDataFormat

acb64e9f59

add -deprecatedrest startup option

Equivalent of -deprecatedrpc for the REST API. Allows the user to
keep the specified deprecated functionality unchanged

fee8638274

rest: move format string to query

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.

206a18452a

refactor: remove unused ParseDataFormat

No longer necessary since response format is a query parameter

901b1323b6

rest: store endpoint prefix data in HTTPRequest

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().

c71a1cff43

rest: add GetPath() and GetPathParameter()

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().

1bca36cee5

rest: use GetPath()

For readability, also standardize variable name into 'path'

a09781f662

rest: add explicit deprecation path for count

Unless -deprecatedrest=count is enabled, all count parameters are
now expected to be a query instead of path parameter.

aaf758420e

rest: remove dependency on strURIpart in handler

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

9c7b3035b4

rest: remove path from handler signature

Since we now exclusively access the path through the HTTPRequest,
we can remove the unused strURIpart path argument to simplify the
rest endpoint interface.

23fb5ca8ae

stickies-v force-pushed on Aug 5, 2022

DrahtBot commented at 1:40 am on December 12, 2022: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot commented at 1:17 am on March 12, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

maflcko commented at 10:17 am on March 13, 2023: member

@stickies-v Looks like this was opened as draft a year ago with no further activity from you. Can this be closed?

stickies-v commented at 11:28 am on March 13, 2023: contributor

Closing as per #25752 (comment)

stickies-v closed this on Mar 13, 2023

bitcoin locked this on Mar 12, 2024

rest: Extend HTTPRequest interface to support querying path (parameters) #25754

Brief summary

Reviews

Conflicts