Skip to content

rest: Further API and code cleanup with query parameters #25752

@stickies-v

Description

@stickies-v

Introduction

In RESTful APIs, path parameters (e.g. /some/unique/resource/) are typically 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, ...

The design of the current REST API endpoints is quite non-standard in a few ways, I've listed some examples below under "Undesirable behaviour examples". Generally, it relies on a number of path (or path-like) parameters that really should be query parameters. Besides being unintuitive for users, I also found the code can be much cleaner when using a generalized interface for query and path parameters. The first work towards this was already done in #24098, but there are still a number of issues outstanding, as e.g. discussed here and here.

Suggested changes

I suggest (and have already prepared the code) to clean up the interface and the code. Besides perhaps the -deprecatedrest startup flag, no new user-facing functionality is introduced. Specifically, the relevant proposed changes are:

  • introduce a -deprecatedrest startup option to allow users to preserve REST endpoint functionality while they upgrade their dependencies, similar to -deprecatedrpc
  • expand the HTTPRequest interface to include easy-to-read functions to access path parameters, like rest: Use query parameters to control resource loading #24098 introduced for query parameters: HTTPRequest::GetPath() and HTTPRequest::GetPathParameter(). This allows cleaning up the interface quite a bit, including removing the duplicated strURIpart that was passed as a third argument to each endpoint (but this is unintuitive and duplicated: the path data is already in the HTTPRequest object).
  • further streamline the endpoint URIs:
    • in all endpoints: remove the path-like dot-separated format strings (e.g. .json) in favour of a query parameter (?format=json), which can default to json (especially useful in endpoints that only accept json)
    • use ?from_blockhash=<blockhash> query parameter instead of a path parameter in /rest/headers and /rest/blockfilterheaders/, since it is a collection endpoint (multiple resources are returned)
    • deprecate /rest/block/notxdetails in favour of a ?txdetails=<true|false> parameter in /rest/block

The suggested implementation keeps everything backwards compatible (with the -deprecatedrest functions enabled). Even though it means there is a bit more review overhead, I think the tradeoff is acceptable - I did my best to minimize code complexity due to this compatibility.

PRs

The suggested changes can largely be separated into 4 small-ish PRs (there are some dependencies, so they build sequentially - but shouldn't be too hard to rebase/reorder/leave things out). I have released the PRs as draft already, just to make it easier to look at the proposed changes.

  1. rest: Move format string from path-like parameter to query parameter #25753
  2. rest: Extend HTTPRequest interface to support querying path (parameters) #25754
  3. rest: Use from_blockhash and txdetails query parameters #25755
  4. rest: Remove support for a number of -deprecatedrest options #25756

Discussion questions/notes

I've already written most of the code, but before fleshing out the PRs I'm first looking to get some feedback on the general concept and approach.

A couple of notes/things to discuss:

  • the LoC change is quite significant, is this a wortwhile upgrade?
  • should we maintain backwards compatibility with -deprecatedrest, not have backwards compatibility at all, or hide the backwards compatibility from the user?
  • -deprecatedrest is documented to, like the RPC, operate on a per-endpoint basis. However, since the first PR (format string) affects all endpoints, I thought that would not be user-friendly and instead opted to group them by functional change. Going forward, since we now have a rather extensible interface with the query parameters, I expect we'll use -deprecatedrest on a per-endpoint basis indeed.

Undesirable behaviour examples

  • Require the user to explicitly define parameters where only 1 value is allowed anyway. For example, rest/chaininfo/ only accepts json as a format but throws if we do not explicitly define json to be the format
  • duplicate deploymentinto endpoints required
  • Unnecessary code coupling in ParseDataFormat() where we need one function to get the entire query path and then magically remove the format string (e.g. .json) from it and return it separately.
  • Two different endpoints to support the notxdetails option in /rest/block, this could just be one endpoint with an optional default query parameter
  • APIs can now be upgraded in a backwards-compatible way more easily, since query parameters are named and thus do not rely on position and can be optional

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions