Skip to content

Conversation

@itsibitzi
Copy link
Contributor

@itsibitzi itsibitzi commented Oct 13, 2025

A work around for an outstanding issue where HTTP client doesn't copy the default headers across.

We need to decide long term if we want to have a different approach for HTTP and direct or you just pass in a HTTP client to a direct to give default headers.

It does feel a bit weird to pass in a HTTP client to a direct TwirpClient, but we have one already.

Right now there are some runtime panics if you use the wrong methods on the builder (e.g. if you try to add a handler to a http client). We could get around this by adding a marker type to be builder and only implementing direct mode functions on e.g. ClientBuilder<DirectMode>

@itsibitzi itsibitzi force-pushed the itsibitzi/20251013-default-headers-workaround branch 3 times, most recently from 715437e to 7fa4373 Compare October 13, 2025 21:20
@itsibitzi itsibitzi marked this pull request as ready for review October 14, 2025 08:11
@itsibitzi itsibitzi requested a review from a team as a code owner October 14, 2025 08:11
Copilot AI review requested due to automatic review settings October 14, 2025 08:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements a workaround for an issue where HTTP clients don't copy default headers across requests in Twirp client configurations. The change adds support for setting default headers in direct mode and ensures they are properly applied to requests.

Key changes:

  • Added with_default_header method to ClientBuilder for setting default headers in direct mode
  • Modified request execution to apply default headers when they don't already exist on the request
  • Refactored ClientBuilder to use struct spread syntax for cleaner code

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

if let Some(handlers) = &mut self.handlers {
handlers.default_headers.insert(key, value);
} else {
panic!("you must use `ClientBuilder::direct()` to register handler headers");
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message mentions 'handler headers' but the method is for 'default headers'. Consider changing to 'you must use ClientBuilder::direct() to register default headers'.

Suggested change
panic!("you must use `ClientBuilder::direct()` to register handler headers");
panic!("you must use `ClientBuilder::direct()` to register default headers");

Copilot uses AI. Check for mistakes.
if let Some(handlers) = &mut self.handlers {
handlers.default_headers.insert(key, value);
} else {
panic!("you must use `ClientBuilder::direct()` to register handler headers");
}
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using panic! for API misuse creates a poor developer experience. Consider returning a Result or using a builder pattern with marker types to prevent this at compile time, as mentioned in the PR description.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@aneubeck aneubeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense to me

@itsibitzi itsibitzi force-pushed the itsibitzi/20251013-default-headers-workaround branch from 7fa4373 to d40edd0 Compare October 14, 2025 09:51
@itsibitzi itsibitzi enabled auto-merge October 14, 2025 09:53
@itsibitzi itsibitzi added this pull request to the merge queue Oct 14, 2025
Merged via the queue into main with commit a89fc3e Oct 14, 2025
5 checks passed
@itsibitzi itsibitzi deleted the itsibitzi/20251013-default-headers-workaround branch October 14, 2025 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants