seanmonstar / reqwest

An easy and powerful Rust HTTP Client
https://docs.rs/reqwest
Apache License 2.0
9.95k stars 1.13k forks source link

feat: request, response and response body hooks #2432

Closed GabMus closed 1 week ago

GabMus commented 1 month ago

This is my personal rendition (albeit a little primitive) of event hooks as detailed in #155

GabMus commented 1 month ago

Fixed the formatting issue, I was unaware of this issue and assumed a regular cargo fmt would be all I needed.

seanmonstar commented 1 month ago

Thanks for the PR!

Wow that's an old issue 😅 I believe my thinking on the matter has shifted a bit. I now desire to pop open reqwest, to expose it as a stack of middleware, and I think doing that would allow people to insert "hooks" anywhere in the stack.

GabMus commented 1 month ago

Sounds good, but do you have an ETA on this? Or even some document explaining how the new API would work? The unfortunate reality is that this missing feature is the only thing keeping me from using reqwest, and other alternatives don't look any better in this regard, not with an API as simple as this one.

Do you think there might be a way to accomodate for the proposed hooks system for the time being, while the new solution is being worked on?

GabMus commented 1 month ago

@seanmonstar any updates on this? I'm sorry to be pressing, but this feature, or something that accomplishes the same goal, is essential for my usecase. I'm very willing to put in the work in case this solution is not to your liking, but I'd need a little direction on how you want to do this.

MoritzS commented 1 month ago

Hi @GabMus, thanks for the PR! I was looking for something very similar and was glad to see that you had already thought of a solution.

I have tested your changes and have one comment: For my particular use case, the Request hook is executed too early. What I mean is that the Request does not yet contain any of the automatically added headers like Content-Length or Accept. In my application, I need this because I want to add an Authorization header that contains a cryptographic signature of all the other headers.

I'm not sure if you intended this kind of use case or if it's even possible with reqwest, so just ignore my comment if it doesn't make sense.

GabMus commented 1 month ago

@MoritzS You're right, I probably need to move the request hook execution somewhere closer to here

Will definitely make and test this change whenever possible, thanks for the heads up!

seanmonstar commented 1 month ago

any updates on this?

Event hooks like this still likely aren't what I want to add directly to reqwest. My goal is to make all the layers of reqwest available as tower middleware (described a tiny bit more in the "Level Up Client Middleware" section of https://seanmonstar.com/blog/2023-in-review/). I'm working on a design, and I'm happy to collaborate on it, but I also have a few other priorities first that I must get to.

The only thing that might fit and could unblock you is if we added the ability to ClientBuilder to pass in a Box<dyn Service> as the inner pooled client stack, which could let anyone wrap an implementation with hooks external to reqwest. It'd require some finagling to make it still work with a bunch of the client builder options that currently configure the internal stack.

GabMus commented 1 week ago

Alright, I think the approach you want to see, particularly after reading the blog post you liked to, makes a lot more sense. Unfortunately I don't know much about tower internals right now, so I doubt I'll be able to help directly (although feel free to reach out, I can definitely try to help).

Regardless, I think I'll close this one, as it doesn't fit the vision you have, hopefully this is gonna come sooner rather than later :crossed_fingers: