Skip to content

Conversation

@er-vin
Copy link

@er-vin er-vin commented Jul 22, 2025

This is convenient in debug and testing scenario for code using reqwest. In particular for introspecting to know what will be sent without introducing a MITM.

@er-vin er-vin force-pushed the expose-client-default-headers branch from f3e4fd2 to a3cf7f0 Compare July 23, 2025 06:14
@er-vin
Copy link
Author

er-vin commented Jul 24, 2025

@seanmonstar One failure with the MSRV check, it looks related to the 0.1.16 update of hyper-util. Paths forward:

  • bumping rust-version to 1.70 or later
  • pin hyper-util to 0.1.15

Any preference? Want me to address it in this PR or a separate one?

@seanmonstar
Copy link
Owner

Thanks for the PR! I'm curious, you mentioned for debugging and inspecting. Is there a significant benefit to having programmatic access vs just including them in the fmt::Debug output of the Client?

@er-vin
Copy link
Author

er-vin commented Jul 28, 2025

Thanks for the PR! I'm curious, you mentioned for debugging and inspecting. Is there a significant benefit to having programmatic access vs just including them in the fmt::Debug output of the Client?

Yeah, my comment was maybe a bit misleading. It's really for the testing part that it comes in handy. In particular if you want to integrate with vcr cassettes. You can't put in them all the headers which will be used in practice if you can't know the ones in the client.

@er-vin er-vin force-pushed the expose-client-default-headers branch from a3cf7f0 to 435dbb9 Compare July 28, 2025 20:29
@er-vin
Copy link
Author

er-vin commented Jul 28, 2025

Looks like it can be merged now.

@seanmonstar once this is merged, any idea on when there will be a release containing this patch? I'm unsure about the release cycle used for this project.

@er-vin er-vin force-pushed the expose-client-default-headers branch 2 times, most recently from 591a486 to ffa5cd1 Compare August 1, 2025 08:17
@er-vin
Copy link
Author

er-vin commented Aug 1, 2025

@seanmonstar Since I noticed other PRs getting in, I've rebased this one to keep it ready. Do you have any concerns left with it?

@seanmonstar
Copy link
Owner

It's really for the testing part that it comes in handy. In particular if you want to integrate with vcr cassettes. You can't put in them all the headers which will be used in practice if you can't know the ones in the client.

Hm, I feel like I don't quite understand. Can you say more?

My hesitation is around taking caution when exposing references, since those expose internal details (you can't factor away to something else, since you can't produce a reference only within a function).

@er-vin
Copy link
Author

er-vin commented Aug 1, 2025

It's really for the testing part that it comes in handy. In particular if you want to integrate with vcr cassettes. You can't put in them all the headers which will be used in practice if you can't know the ones in the client.

Hm, I feel like I don't quite understand. Can you say more?

Sure, let me expand. I'm having integration tests which serialize requests sent in the VCS cassette format (using vcr_cassette). This is done by taking the built Request and calling (among other things) headers() on it.

Now some of the headers are supposed to be used by all the requests, so it would make sense to move then in the default headers of the client. In my test, I can get access to the client and the request, but the request doesn't carry the default headers of the client. So my tests become "blind" to any header moved from requests to client.

That's why I'd like my tests to be able to get access to the client's headers to have a complete view again.

An alternative would be for the Request to provide all the headers, but if I understand correctly it wasn't the case when default_headers() was introduced on the ClientBuilder and that was on purpose. Also, changing this now means a behavior change which might not be desirable.

My hesitation is around taking caution when exposing references, since those expose internal details (you can't factor away to something else, since you can't produce a reference only within a function).

Note that I went with references because that's what headers() was already exposing on Request. But I admit I was hesitant to switch to value instead. If you prefer I could do this.

@er-vin er-vin force-pushed the expose-client-default-headers branch from ffa5cd1 to 28f24bb Compare August 1, 2025 17:12
@er-vin
Copy link
Author

er-vin commented Aug 7, 2025

@seanmonstar Did my previous comment help make things clearer?

If not feel free to ask questions and I'll expand.

@er-vin er-vin force-pushed the expose-client-default-headers branch from 28f24bb to 5a3d53f Compare August 14, 2025 09:31
@er-vin
Copy link
Author

er-vin commented Aug 14, 2025

@seanmonstar Looks like I missed the boat for the latest release. Any chance to attract your attention again and see what to do with this patch? Thanks in advance.

@er-vin er-vin force-pushed the expose-client-default-headers branch from 5a3d53f to 759078a Compare August 19, 2025 10:07
@er-vin er-vin force-pushed the expose-client-default-headers branch from 759078a to d608ec4 Compare September 22, 2025 14:33
This is convenient in debug and testing scenario for code using
reqwest. In particular for introspecting to know what will be sent
without introducing a MITM.
@er-vin er-vin force-pushed the expose-client-default-headers branch from d608ec4 to 181f3c4 Compare October 2, 2025 21:00
@er-vin
Copy link
Author

er-vin commented Oct 2, 2025

@seanmonstar Any chance this PR could be evaluated again? I don't mind approaching it differently but having a conclusive conversation about it would help. Thanks in advance.

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.

2 participants