RPC handlers can directly set HTTP response fields #921
Conversation
Codecov Report
@@ Coverage Diff @@
## master #921 +/- ##
===========================================
- Coverage 69.07% 52.24% -16.83%
===========================================
Files 100 95 -5
Lines 7836 6815 -1021
===========================================
- Hits 5412 3560 -1852
- Misses 2424 3255 +831
|
|
non_jsonrpc_response@5741 aka 20200305.24 vs master ewma over 30 builds from 5362 to 5751 |
| { | ||
| throw std::runtime_error( | ||
| "No result in verification response: " + response.dump()); | ||
| throw std::runtime_error(fmt::format( |
There was a problem hiding this comment.
Does anyone else want a THROW_FMT() or is it just me?
There was a problem hiding this comment.
Maybe - we do repeat this pattern a lot. I'd be tempted to keep the throw outside of the macro so that flow control is clear:
throw FMT_ERROR(std::runtime_error, "expected {}, got {}", expected, actual);
| return jsonrpc::unpack(ctx->get_request_body(), pack); | ||
| } | ||
|
|
||
| static nlohmann::json get_params_from_query( |
There was a problem hiding this comment.
This is an early sketch of what this might look like, and needs some more thought. Basically for GET requests, we should be able to write /users/LOG_get?id=42 rather than {"id": 42} in the body. This does that parsing, but we need to decide how complicated we allow these queries to be, how they're escaped, and how this hooks into the OpenAPI spec (each endpoint should declare whether it expects the params in the query or in the body).
There was a problem hiding this comment.
I think we should maybe look at off-the-shelf options, this one has tests and is allocation-free for example: https://github.com/jacketizer/libyuarel/blob/master/yuarel.h
There was a problem hiding this comment.
I agree we should look, but I don't think there's anything that gets us very far. yuarel for instance is merely splitting at &s - we can do that trivially, and what we're really interested in is handling escaped-values and type-converting the separated strings.
There was a problem hiding this comment.
You’re right, but perhaps we can at least get some tests that way.
Opening as a draft so I can do a sanity check on the diff, and update documentation.
Apologies for another monster-sized PR, hopefully this is the last for a while.
This PR lets RPC handlers set the status, headers, and body of the HTTP responses. As a result we no longer return JSON-RPC objects - clients must check the HTTP status and headers to know whether the body is an error (string) or successful result (generally JSON). The CCF version information (term, commit, global-commit) are now headers in the response, rather than part of the payload. I've converted all the existing JSON-RPC error codes to HTTP statuses (often
Bad Request, sometimesNot Found,Forbidden, orUnauthorized), but feel free to disagree with any of these mappings.Since the existing JSON requests/responses are still useful and widely used, we have
json_adapterfunctions which handle the parsing in a standard way. In particular, they read thecontent-typeheader to work out whether the payload is JSON or msgpack, and pack the result (if successful) in the same way.To be concrete, between this and #852, we've gone from this:
to this:
(NB: Handwritten samples, imprecise)