rest: Move format string from path-like parameter to query parameter #25753

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.

   Brief summary

   All REST API endpoints require the user to specify the response format by including a format string at the end of the path (e.g. /rest/chaininfo.json). This PR removes the path-like parameter and updates all endpoints to use a ?format query parameter, which defaults to json if unspecified.

   Previous behaviour can be preserved through -deprecatedrest=format

   Benefits:

   • only use (standard) path and query parameters, no custom dot-separated parameter
   • a default json response format seems reasonable, and was trivial to implement with a query parameter
   • simplify code since we don’t need bespoke logic to parse the format, e.g. remove ParseDataFormat() function which had too many responsibilities
   • Introduce -deprecatedrest startup option, similar to -deprecatedrpc. With this option, we keep API changes backwards compatible for as long as the -deprecatedrest option is available (usually 1 version?).
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. DrahtBot removed the label Needs rebase on Jul 30, 2022
6. 
   DrahtBot commented at 5:02 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.

   Type	Reviewers
   Concept NACK	luke-jr

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   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)
   • #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.

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

9. 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
10. add -deprecatedrest startup option

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

    fee8638274
11. 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
12. refactor: remove unused ParseDataFormat

    No longer necessary since response format is a query parameter

    901b1323b6
13. stickies-v force-pushed on Aug 5, 2022
14. 
    luke-jr commented at 11:34 pm on August 9, 2022: member

    Concept NACK. Not only does it break the API, the current interface is what is expected for format specification everywhere.
15. 
    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.
16. 
    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.
17. 
    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?
18. 
    stickies-v commented at 11:28 am on March 13, 2023: contributor

    Closing as per #25752 (comment)
19. stickies-v closed this on Mar 13, 2023

    ---

20. bitcoin locked this on Mar 12, 2024

Contributors
stickies-v DrahtBot luke-jr maflcko

Labels
RPC/REST/ZMQ Needs rebase