rest: Use query parameters to control resource loading

stickies-v commented at 5:37 pm on January 8, 2022: member

In RESTful APIs, typically path parameters (e.g. /some/unique/resource/) are used to represent resources, and query parameters (e.g. ?sort=asc) are used to control how these resources are being loaded through e.g. sorting, pagination, filtering, …

As first [discussed in #17631](https://github.com/bitcoin/bitcoin/pull/17631#discussion_r733031180), the current REST api contains two endpoints /headers/ and /blockfilterheaders/ that rather unexpectedly use path parameters to control how many (filter) headers are returned in the response. While this is no critical issue, it is unintuitive and we are still early enough to easily phase this behaviour out and ensure new endpoints (if any) do not have to stick to non-standard behaviour for just internal consistency.

In this PR, a new HTTPRequest::GetQueryParameter method is introduced to easily parse query parameters, as well as two new /headers/ and /blockfilterheaders/ endpoints that use a count query parameter are introduced. The old path parameter-based endpoints are kept without too much overhead, but the documentation now points to the new query parameter-based endpoints as the default interface to encourage standardness.

Behaviour change

New endpoints and default values

/headers/ and /blockfilterheaders/ now have 2 new endpoints that contain query parameters (?count=<count>) instead of path parameters (/<count>/), as described in REST-interface.md. Since query parameters can easily have default values, I have set this at 5 for both endpoints.

headers GET /rest/headers/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT=5> should now be used instead of GET /rest/headers/<COUNT>/<BLOCK-HASH>.<bin|hex|json>

blockfilterheaders GET /rest/blockfilterheaders/<FILTERTYPE>/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT=5> should now be used instead of GET /rest/blockfilterheaders/<FILTERTYPE>/<COUNT>/<BLOCK-HASH>.<bin|hex|json>

Some previously invalid API calls are now valid

API calls that contained query strings in the URI could not be parsed prior to this PR. This PR changes behaviour in that previously invalid calls (e.g. GET /rest/headers/5/somehash.json?someunusedparam=foo) would now become valid, as the query parameters are properly parsed, and discarded if unused. For example, prior to this PR, adding an irrelevant someparam parameter would be illegal:

0GET /rest/headers/5/0000004c6aad0c89c1c060e8e116dcd849e0554935cd78ff9c6a398abeac6eda.json?someparam=true
1->
2Invalid hash: 0000004c6aad0c89c1c060e8e116dcd849e0554935cd78ff9c6a398abeac6eda.json?someparam=true

This behaviour change affects all rest endpoints, not just the 2 new ones introduced here.

(Note: I’d be open to implementing additional logic to refuse requests containing unrecognized query parameters to minimize behaviour change, but for the endpoints that we currently have I don’t really see the point for that added complexity. E.g. I don’t see any scenarios where misspelling a parameter could lead to harmful outcomes)

To do

update doc/release-notes

Feedback

This is my first PR (hooray!). Please don’t hold back on any feedback/comments/nits/… you may have, big or small, whether they are code, process, language, … related. I welcome private messages too if there’s anything you don’t want to clutter the PR with. I’m here to learn and am grateful for everyone’s input.

DrahtBot added the label Build system on Jan 8, 2022

DrahtBot added the label RPC/REST/ZMQ on Jan 8, 2022

DrahtBot commented at 6:29 pm on January 8, 2022: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24054 (test: rest /tx with an invalid/unknown txid by brunoerg)
#22953 (refactor: introduce single-separator split helper (boost::split replacement) by theStack)
#21422 (Add feerate histogram to getmempoolinfo by kiminuo)

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.

stickies-v force-pushed on Jan 8, 2022

stickies-v marked this as a draft on Jan 8, 2022

stickies-v commented at 8:35 pm on January 8, 2022: member

Changing to draft while I sort out some issues with AddressSanitizer and Win64 builds.

stickies-v force-pushed on Jan 9, 2022

stickies-v force-pushed on Jan 10, 2022

stickies-v force-pushed on Jan 17, 2022

stickies-v force-pushed on Jan 18, 2022

Refactoring: move declarations to rest.h

This facilitates unit testing

59fb245049

Handle query string when parsing data format

URLs may contain a query string (prefixed with '?') and this should be ignored when parsing
the data format.

To facilitate testing this functionality, ParseDataFormat has been made non-static.

19e8f0dcfd

stickies-v force-pushed on Jan 18, 2022

Add GetQueryParameter helper function

Easily get the query parameter from the URI, with optional default value.

6109482698

Update /<count>/ endpoints to use a '?count=' query parameter instead

In most RESTful APIs, path parameters are used to represent resources, and
query parameters are used to control how these resources are being filtered/sorted/...

The old /<count>/ functionality is kept alive to maintain backwards compatibility,
but new paths with query parameters are introduced and documented as the default
interface so future API methods don't break consistency by using query parameters.

bdea37cd09

stickies-v force-pushed on Jan 18, 2022

stickies-v commented at 9:04 pm on January 18, 2022: member

Unit testing new functionality around libevent proved to be quite the challenge to get past LeakSanitizer, so for lack of a local CI setup I’ve had to push out a whole series of updates to test and get it to work. As all checks are passing now and there hasn’t been any conversation on this PR yet, I’ll close this and open a new fresh one to not confuse reviewers.

stickies-v closed this on Jan 18, 2022

DrahtBot locked this on Jan 18, 2023

rest: Use query parameters to control resource loading #24012

Behaviour change

New endpoints and default values

Some previously invalid API calls are now valid

To do

Feedback

Conflicts