WIP: Use a request builder pattern to allow clients to set headers and other per-request info #201
Conversation
tclem
force-pushed
the
tclem/client-request-builder
branch
from
8111525 to
05e4f6a
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a request builder pattern so clients can set headers and other per-request data on Twirp calls while remaining backward compatible.
- Adds a
RequestBuildertype andbuild_requestmethod to configure individual requests. - Updates the client’s
requestmethod to accept a builder and preserves existing behavior. - Enhances the code generator to emit
build_<method>functions alongside async RPC methods.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/twirp/src/test.rs | Updated test client to call build_request before request |
| crates/twirp/src/lib.rs | Exposed RequestBuilder in public exports |
| crates/twirp/src/client.rs | Added build_request, RequestBuilder, and header API |
| crates/twirp/Cargo.toml | Removed redundant license field |
| crates/twirp-build/src/lib.rs | Emit build_<method> functions in generated client trait |
| crates/twirp-build/Cargo.toml | Dropped unused proc-macro2 dependency |
Comments suppressed due to low confidence (5)
crates/twirp/src/client.rs:221
- Public API types like
RequestBuilderneed Rustdoc comments to explain their purpose and usage, especially theheaderandbuildmethods.
pub struct RequestBuilder<I, O> {
crates/twirp/src/test.rs:124
- [nitpick] The local variable
reqshadows the method parameter and now refers to a builder; consider renaming it tobuilderfor clarity.
let req = self.build_request("test.TestAPI/Ping", req)?;
crates/twirp/src/test.rs:124
- There's no test for the new header-injection path; consider adding a test that uses
build_requestandheaderto verify custom headers are sent.
let req = self.build_request("test.TestAPI/Ping", req)?;
crates/twirp/Cargo.toml:13
- Removing the
licensefield may break crate metadata validation; ensure that thelicense-filealone satisfies publishing requirements or re-add the SPDXlicensekey.
-license = "MIT"
crates/twirp-build/src/lib.rs:169
- [nitpick] Generated method names
build_<Method>may conflict with other conventions; verify that this naming is consistently documented or considerprepare_<method>for clarity.
let build_name = format_ident!("build_{}", name);
tclem
force-pushed
the
tclem/client-request-builder
branch
from
7292f1e to
3ecd726
Compare
|
Here's another iteration, but I ran into trouble downstream with some places where we use |
| }); | ||
|
|
||
| client_methods.push(quote! { | ||
| async fn #name(&self, req: #input_type) -> Result<#output_type, twirp::ClientError> { | ||
| self.request(#request_path, req).await | ||
| fn #name_request(&self, req: #input_type) -> Result<twirp::RequestBuilder<#input_type, #output_type>, twirp::ClientError> { |
There was a problem hiding this comment.
Would the purpose of this message be clearer if we postfixed this method name with _builder instead? A _request postfix doesn't suggest to me that a builder is involved.
There was a problem hiding this comment.
I'm taking inspiration from how reqwest does this:
https://docs.rs/reqwest/latest/reqwest/struct.Client.html#method.request
@look is working on something that requires adding custom headers to the twirp request. This is sort of possible today with middleware, but it is a bit awkward and not at all easy to do if you need those headers to be based on other application state.
This is one idea for allowing the twirp clients to set additional request data. I've tried to make this backward compatible.
I did consider some alternative designs:
Contextlike we do for the twirp servers. I don't love this pattern, it feels like Go, so I'm opting not to continue in that direction.