rest: Remove support for a number of -deprecatedrest options #25756

pull stickies-v wants to merge 16 commits into bitcoin:master from stickies-v:rest/query-params-3-cleanup changing 10 files +186 −228
  1. 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: remove support for -deprecatedrest=format.

    Brief summary

    #25752 introduced a number of -deprecatedrest options 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.

  2. fanquake added the label RPC/REST/ZMQ on Jul 30, 2022
  3. DrahtBot added the label Needs rebase on Jul 30, 2022
  4. stickies-v force-pushed on Jul 30, 2022
  5. stickies-v force-pushed on Jul 30, 2022
  6. stickies-v force-pushed on Jul 30, 2022
  7. DrahtBot removed the label Needs rebase on Jul 30, 2022
  8. DrahtBot commented at 4:57 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)

    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.

  9. DrahtBot added the label Needs rebase on Aug 5, 2022
  10. 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”.

  11. 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
  12. add -deprecatedrest startup option 
    Equivalent of -deprecatedrpc for the REST API. Allows the user to
keep the specified deprecated functionality unchanged
    fee8638274
  13. 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
  14. refactor: remove unused ParseDataFormat 
    No longer necessary since response format is a query parameter
    901b1323b6
  15. 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
  16. 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
  17. rest: use GetPath() 
    For readability, also standardize variable name into 'path'
    a09781f662
  18. 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
  19. 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
  20. 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
  21. rest: use from_blockhash query parameter 
    /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.
    7dc230ba3a
  22. rest: make txdetails a query parameter 
    instead of path parameter; reduces complexity
    00de819d92
  23. rest: remove support for -deprecatedrest=format 
    Format strings are now always assumed to be in the query string only
    80039c15a0
  24. rest: remove support for -deprecatedrest=count 35bd7675f6
  25. rest: remove support for -deprecatedrest=blockhash 2e4dbd06c5
  26. rest: remove support for -deprecatedrest=txdetails 3e9cb85008
  27. stickies-v force-pushed on Aug 5, 2022
  28. 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.
  29. 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.
  30. maflcko commented at 10:16 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?
  31. stickies-v commented at 11:28 am on March 13, 2023: contributor
    Closing as per #25752 (comment)
  32. stickies-v closed this on Mar 13, 2023
  33. bitcoin locked this on Mar 12, 2024


stickies-v DrahtBot maflcko

Labels
RPC/REST/ZMQ Needs rebase

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-27 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me