Allow custom headers and extensions for twirp clients and servers; unify traits; unify error type #212
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a significant breaking change to the twirp-rs library by removing the Context abstraction and unifying client and server traits to use HTTP Request and Response types directly. The changes enable custom headers and extensions support while simplifying the API surface.
Key changes:
- Replaces
Contextwith direct HTTP request/response handling using extensions and headers - Unifies client and server traits to share the same interface
- Updates method signatures to use
http::Request<T>input andhttp::Response<T>output types
Reviewed Changes
Copilot reviewed 7 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/twirp/src/context.rs | Removes the entire Context implementation |
| crates/twirp/src/lib.rs | Updates exports to remove Context and add Request/Response |
| crates/twirp/src/server.rs | Refactors request handling to work with HTTP types directly |
| crates/twirp/src/client.rs | Updates client to handle HTTP Request/Response with headers and extensions |
| crates/twirp/src/details.rs | Updates router builder to use new function signatures |
| crates/twirp/src/test.rs | Updates test implementations to use new HTTP-based API |
| crates/twirp-build/src/lib.rs | Updates code generation to produce unified traits |
Comments suppressed due to low confidence (1)
There was a problem hiding this comment.
I like this more.
I also think that we should implement test-support directly into the twirp::Client, i.e. that one can register in-process backends which bypass all the http stack.
I guess this should be pretty straight-forward as well and essentially allow us to remove all the ugly wrapper code we currently have.
tclem
changed the title
crates/twirp/src/error.rs
Outdated
| @@ -231,6 +231,12 @@ impl TwirpErrorResponse { | |||
| d | |||
| } | |||
| }); | |||
| if let Some(ref retry_after) = self.retry_after { | |||
There was a problem hiding this comment.
since you insert the retry_after information into the map anyways, you could also read it from the map instead of having a separate field which you try to keep in sync?
| .header(header::CONTENT_TYPE, "application/json"); | ||
|
|
||
| if let Some(duration) = self.retry_after { | ||
| resp = resp.header(header::RETRY_AFTER, duration.as_secs().to_string()); |
There was a problem hiding this comment.
Isn't the retry_after information added twice now, once to the header and once in the metadata body?
There was a problem hiding this comment.
I ended up removing this entire and implementing manually in blackbird. I might revisit in a follow up.
This is an alternative to #201 with an expanded scope of capabilities. This is very much a breaking change so we should discuss if that's worth it.
Here are the highlights:
Context. The same capabilities now exist via http request and response Extensions and Headers.http::request::Request<In>andhttp::response::Response<Out>whereInandOutare the individual rpc message types.The trait interfaces do get slightly more complicated, but we get to drop context (was only implemented on the server trait). You can see in the examples how this works — I don't think it's too bad, but curious to hear feedback here.
EDIT:
One additional change is unifying and simplifying the error types. There is now just
TwirpErrorResponse, which isn't a true Rust error - it just models the twirp error response spec. This change dramatically simplified a bunch of code in our project. This error is really only to be used at the boundaries: it's one step away from an axumResponse.