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(ext/fetch): support fallible request-builder hooks #18116

Merged
merged 1 commit into from Mar 13, 2023

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Mar 10, 2023

This tweaks the signature of request_builder_hook in order to support fallible hooks.

The rationale for this is mostly on two sides:

This tweaks the signature of `request_builder_hook` in order to
support fallible hooks.

The rationale for this is mostly on two sides:
 * it allows a hook to inspect and possibly drop an outgoing request
   (e.g. for policying purposes), bubbling up a detailed error
   message to the user.
 * it wires into newer `reqwest` API which allows to split and
   then reassemble a `RequestBuilder`, although only in a fallible
   way.
@CLAassistant
Copy link

CLAassistant commented Mar 10, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@satyarohith satyarohith left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@satyarohith satyarohith left a comment

Choose a reason for hiding this comment

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

The description of the PR says it uses the new RequestBuilder::from_parts() but I don't see it in the code. I am probably misunderstanding the intent of the second point?

@lucab
Copy link
Contributor Author

lucab commented Mar 13, 2023

@satyarohith RequestBuilder::from_parts() is not yet in the latest reqwest released version, so it doesn't directly appear here. But the idea is that the consumer-provided hook logic can now split and re-assemble the RequestBuilder to manipulate the inner Request, something like this:

fn custom_hook(input: RequestBuilder) -> Result<RequestBuilder, AnyError> {
  let (client, inner_req) = input.build_split();
  let req = inner_req?;
  // ... Manipulate `req` as needed here ...
  let output = RequestBuilder::from_parts(client, req);
  Ok(output)
}

@satyarohith
Copy link
Member

Understood. Thanks for the explanation!

@lucab lucab merged commit 3f26ee8 into denoland:main Mar 13, 2023
@lucab lucab deleted the ups/fetch-hook-fallible branch March 13, 2023 10:29
kt3k pushed a commit that referenced this pull request Mar 16, 2023
This tweaks the signature of `request_builder_hook` in order to support
fallible hooks.

The rationale for this is mostly on two sides:
* it allows a hook to inspect and possibly drop an outgoing request
(e.g. for policying purposes), bubbling up a detailed error message to
the user.
* it wires into newer `reqwest` API which allows to split and then
reassemble a `RequestBuilder`, although only in a fallible way
(seanmonstar/reqwest#1770)
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.

None yet

4 participants