Create RequestExtension trait that allows for ureq-specific methods on http::Request
#1011
Conversation
…f HTTP requests without having to construct an agent.
| @@ -553,6 +554,8 @@ pub mod tls; | |||
|
|
|||
| #[cfg(feature = "cookies")] | |||
| mod cookies; | |||
| mod request_ext; | |||
There was a problem hiding this comment.
I wasn't sure whether to create a new file or not - I figured the request file maybe was long enough already? Would be happy to move it into that file if you want.
| use crate::typestate::HttpCrateScope; | ||
| use crate::{http, Agent, AsSendBody}; | ||
|
|
||
| /// Extension trait for [`http::Request<impl AsSendBody>`]. |
There was a problem hiding this comment.
I've tried to match the style of documentation and examples to the existing styles, but lmk if you want any changes made.
| /// | ||
| /// This function will return a `ConfigBuilder` that can be used to set ureq-specific options. | ||
| /// The resulting method can be used with `ureq::run` or with an existing agent. | ||
| /// Configuration values set on the request takes precedence over configuration values set on the agent. |
There was a problem hiding this comment.
I think this is correct based on a glance of the source code? I wasn't quite sure, because ureq somewhere also describes it might be an error to run something created with one agent on another agent, and I'm not quite sure how this plays into this - as technically it's not created with any agent.
There was a problem hiding this comment.
Yeah, I was just going to say. This is where it breaks down. When you do configure_request(), we clone the agent level config to make the request level config. And this config is further manipulated and later used in run.rs.
With this patch, this sequence of events would fail:
- Configure a bespoke agent with a global timeout of 10 seconds.
- Use RequestExt::configure() to set something else, like
https_only(true). - Run the configured Request on agent made in 1.
This would result in a request that does not have the 10 second timeout because we are below using Agent::new_with_defaults() as the starting point for the request level config.
I'm toying with the idea whether we can make an API that borrows the agent until the request is executed.
fn configure<'a>(self, agent: &'a Agent) -> ConfigBuilder<HttpCrateScope<'a, S>> {
...
}and combine that with that instead of build() followed by Agent::run(), we expose run() straight on the builder. The call would be:
let request: http::Request<()> = http::Request::builder()
.method(http::Method::GET)
.uri("http://foo.bar")
.body(())
.unwrap();
request
.configure()
.https_only(false)
.run() // <---- run hereThere was a problem hiding this comment.
Yes I agree with the issue!
I've seen your other suggestion, but I think this is probably the crux of the issue, so will respond here first - I think it makes sense to finish this discussion before any of the other stuff becomes relevant.
Does the same issue not also happen if you use Agent::configure_request and then use ureq::run on the request afterwards?
I'm wondering if the more intuitive way to do this isn't to have each field on the request-based config be an Option and then default to the agent-based configuration if it's not set on the request level?
| use crate::config::RequestLevelConfig; | ||
|
|
||
| #[test] | ||
| fn set_https_only_to_true_on_get_request() { |
There was a problem hiding this comment.
I've just tested that the request.extensions() get set properly. To be fair, seeing as this mostly calls into existing ureq-machinery, I consider these tests more "does the API work" tests, than actual test of the functionality :)
| @@ -553,6 +554,8 @@ pub mod tls; | |||
|
|
|||
| #[cfg(feature = "cookies")] | |||
| mod cookies; | |||
| mod request_ext; | |||
| /// | ||
| /// This function will return a `ConfigBuilder` that can be used to set ureq-specific options. | ||
| /// The resulting method can be used with `ureq::run` or with an existing agent. | ||
| /// Configuration values set on the request takes precedence over configuration values set on the agent. |
There was a problem hiding this comment.
Yeah, I was just going to say. This is where it breaks down. When you do configure_request(), we clone the agent level config to make the request level config. And this config is further manipulated and later used in run.rs.
With this patch, this sequence of events would fail:
- Configure a bespoke agent with a global timeout of 10 seconds.
- Use RequestExt::configure() to set something else, like
https_only(true). - Run the configured Request on agent made in 1.
This would result in a request that does not have the 10 second timeout because we are below using Agent::new_with_defaults() as the starting point for the request level config.
I'm toying with the idea whether we can make an API that borrows the agent until the request is executed.
fn configure<'a>(self, agent: &'a Agent) -> ConfigBuilder<HttpCrateScope<'a, S>> {
...
}and combine that with that instead of build() followed by Agent::run(), we expose run() straight on the builder. The call would be:
let request: http::Request<()> = http::Request::builder()
.method(http::Method::GET)
.uri("http://foo.bar")
.body(())
.unwrap();
request
.configure()
.https_only(false)
.run() // <---- run here| use crate::config::RequestLevelConfig; | ||
|
|
||
| #[test] | ||
| fn set_https_only_to_true_on_get_request() { |
|
Sorry I used my work github user. 🙈 |
Creates a RequestExtension trait that will allow attaching ureq-specific methods to an
http::Request.Currently the only trait method is
configure, which allows the creation of aConfigurationBuilderfrom anhttp::Request. This method fixes #998.