Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: request, response and response body hooks #2432

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GabMus
Copy link

@GabMus GabMus commented Sep 25, 2024

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

@GabMus
Copy link
Author

GabMus commented Sep 26, 2024

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

@seanmonstar
Copy link
Owner

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
Copy link
Author

GabMus commented Sep 27, 2024

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
Copy link
Author

GabMus commented Oct 14, 2024

@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
Copy link

MoritzS commented Oct 14, 2024

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
Copy link
Author

GabMus commented Oct 14, 2024

@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
Copy link
Owner

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.

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