From 85b0158b67d933061f5cc344a918dbf647e33b18 Mon Sep 17 00:00:00 2001 From: Brent Date: Thu, 27 Aug 2026 23:37:11 -0400 Subject: [PATCH] =?UTF-8?q?Rust:=20move=20the=20redirect=20option=20onto?= =?UTF-8?q?=20the=20builder=20=E2=80=94=203.7.0=20was=20breaking?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I shipped RequestInit.follow_redirects in 3.7.0 as a minor. Adding a public field to a struct consumers construct is a BREAKING change in Rust semver, and the evidence was immediate: building the SmooAI monorepo against 3.7.0 fails with error[E0063] in 129 exhaustive `RequestInit { .. }` constructors across about forty crates. I had that evidence an hour earlier and misread it. Adding the field broke the fetch repo's own tests; I patched them and treated it as test churn rather than the semver signal it was. The option is now FetchBuilder::with_follow_redirects, which is the shape Go and .NET already use, and RequestInit is back to its 3.6.2 fields. This is not just damage control — reqwest's redirect policy is per-Client rather than per-request, so the builder was the correct home from the start and the struct field was working against the grain. Kept fully additive so 3.6.2 -> 3.7.1 needs no consumer changes: client::fetch keeps its exact signature and delegates to a new client::fetch_with_redirect_policy that takes the extra argument. The wiring bug this repeats is worth naming. with_follow_redirects stored a value that reached FetchClient and then went nowhere, because both fetch methods still called the old entry point — the setter compiled, the option did nothing. That is the same failure as .NET's SmooFetchBuilder.Build() dropping FollowRedirects during its field-by-field copy, and in both languages only an end-to-end test that actually watched for the redirect hop caught it. A unit test asserting "the builder stored the flag" would have passed in both. 3.7.0 is yanked from crates.io. The other four languages are unaffected: Python added a defaulted dataclass field, Go and .NET added builder methods, and TypeScript's change was to stop overriding a caller's existing option. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/rust-redirect-on-builder.md | 21 +++++++++ rust/fetch/src/builder.rs | 39 ++++++++++++++--- rust/fetch/src/client.rs | 42 ++++++++++++++++-- rust/fetch/src/types.rs | 25 +++-------- rust/fetch/tests/builder_tests.rs | 4 -- rust/fetch/tests/fetch_tests.rs | 59 +++++++++----------------- 6 files changed, 118 insertions(+), 72 deletions(-) create mode 100644 .changeset/rust-redirect-on-builder.md diff --git a/.changeset/rust-redirect-on-builder.md b/.changeset/rust-redirect-on-builder.md new file mode 100644 index 0000000..cc9361d --- /dev/null +++ b/.changeset/rust-redirect-on-builder.md @@ -0,0 +1,21 @@ +--- +'@smooai/fetch': patch +--- + +Rust: move the redirect option off `RequestInit` and onto the builder + +3.7.0 added `RequestInit.follow_redirects`. Adding a public field to a struct +consumers construct is a **breaking change** in Rust semver, shipped under a +minor — building the SmooAI monorepo against 3.7.0 fails with +`error[E0063]: missing field` in **129 exhaustive constructors** across ~40 crates. + +The option is now `FetchBuilder::with_follow_redirects`, matching the shape Go +and .NET already use, and `RequestInit` is back to its 3.6.2 fields. Redirect +policy is per-Client in reqwest anyway, so the builder was the right home from +the start. + +`client::fetch` keeps its exact signature; a new `client::fetch_with_redirect_policy` +takes the extra argument, so nothing that compiled against 3.6.2 needs changing. + +3.7.0 is yanked from crates.io. The other four languages were unaffected — +Python added a defaulted dataclass field, Go and .NET added builder methods. diff --git a/rust/fetch/src/builder.rs b/rust/fetch/src/builder.rs index 2bfc454..44dfea3 100644 --- a/rust/fetch/src/builder.rs +++ b/rust/fetch/src/builder.rs @@ -55,6 +55,7 @@ pub struct FetchBuilder { hooks: LifecycleHooks, auth_token_provider: Option, auth_scheme: String, + follow_redirects: Option, } impl FetchBuilder { @@ -69,6 +70,7 @@ impl FetchBuilder { container_options: FetchContainerOptions::default(), default_init: None, hooks: LifecycleHooks::default(), + follow_redirects: None, auth_token_provider: None, auth_scheme: "Bearer".to_string(), } @@ -248,6 +250,31 @@ impl FetchBuilder { self } + /// Controls whether redirects are followed. Defaults to following, matching + /// reqwest. + /// + /// Set `false` when a redirect must not be followed. Two cases where + /// following one is wrong rather than merely surprising: + /// + /// - A caller that resolved the target hostname and checked it against an + /// SSRF allowlist has that guard defeated by a 302 to an internal + /// address, because the check was performed on the original host. + /// - RFC 8461 forbids fetching an MTA-STS policy through a redirect. + /// + /// With `false` the 3xx is returned as an ordinary response rather than + /// raised as an error. + /// + /// This lives on the builder, not on [`RequestInit`], for two reasons: + /// reqwest's redirect policy is per-Client rather than per-request, and a + /// new public field on `RequestInit` would break every exhaustive + /// constructor downstream — 129 of them in the SmooAI monorepo alone. The + /// Go and .NET ports expose the same client-level shape. + #[must_use] + pub fn with_follow_redirects(mut self, follow: bool) -> Self { + self.follow_redirects = Some(follow); + self + } + /// Build the configured fetch client. pub fn build(self) -> FetchClient { let rate_limiter = self @@ -272,6 +299,7 @@ impl FetchBuilder { hooks: Arc::new(self.hooks), auth_token_provider: self.auth_token_provider, auth_scheme: self.auth_scheme, + follow_redirects: self.follow_redirects, } } } @@ -293,6 +321,7 @@ pub struct FetchClient { hooks: Arc>, auth_token_provider: Option, auth_scheme: String, + follow_redirects: Option, } impl FetchClient { @@ -305,7 +334,7 @@ impl FetchClient { // Merge default init with per-request init let merged_init = self.apply_auth(self.merge_init(init)).await; - crate::client::fetch::( + crate::client::fetch_with_redirect_policy::( url, merged_init, Some(self.fetch_options.clone()), @@ -313,6 +342,7 @@ impl FetchClient { self.rate_limit_retry.as_ref(), self.circuit_breaker.as_ref(), Some(self.hooks.as_ref()), + self.follow_redirects, ) .await } @@ -326,7 +356,7 @@ impl FetchClient { ) -> Result, FetchError> { let merged_init = self.apply_auth(self.merge_init(init)).await; - crate::client::fetch::( + crate::client::fetch_with_redirect_policy::( url, merged_init, Some(options), @@ -334,6 +364,7 @@ impl FetchClient { self.rate_limit_retry.as_ref(), self.circuit_breaker.as_ref(), Some(self.hooks.as_ref()), + self.follow_redirects, ) .await } @@ -371,10 +402,6 @@ impl FetchClient { method: init.method, headers: merged_headers, body: init.body.or_else(|| default.body.clone()), - // Per-request wins, but only when it SAYS something — - // `None` inherits, so a client default of `Some(false)` - // survives a per-request `..Default::default()`. - follow_redirects: init.follow_redirects.or(default.follow_redirects), } } None => init, diff --git a/rust/fetch/src/client.rs b/rust/fetch/src/client.rs index 1894942..d69f2ea 100644 --- a/rust/fetch/src/client.rs +++ b/rust/fetch/src/client.rs @@ -173,12 +173,13 @@ async fn do_single_request( url: &str, init: &RequestInit, connect_timeout_ms: Option, + follow_redirects: Option, ) -> Result, FetchError> { let mut client_builder = reqwest::Client::builder(); if let Some(ms) = connect_timeout_ms { client_builder = client_builder.connect_timeout(Duration::from_millis(ms)); } - if init.follow_redirects == Some(false) { + if follow_redirects == Some(false) { // Per-client, not per-request, in reqwest — which is why a caller could // not previously express this without building its own client. client_builder = client_builder.redirect(reqwest::redirect::Policy::none()); @@ -255,7 +256,7 @@ async fn do_single_request( // without this leaves the option useless: reqwest returns the 302, and it // is then raised as an `HttpResponse` error because it is not 2xx. let is_unfollowed_redirect = - init.follow_redirects == Some(false) && (300..400).contains(&(status as u32)); + follow_redirects == Some(false) && (300..400).contains(&(status as u32)); if fetch_response.ok || is_unfollowed_redirect { Ok(fetch_response) @@ -285,6 +286,39 @@ pub async fn fetch( rate_limit_retry: Option<&RateLimitRetryOptions>, circuit_breaker: Option<&CircuitBreaker>, hooks: Option<&LifecycleHooks>, +) -> Result, FetchError> { + fetch_with_redirect_policy( + url, + init, + options, + rate_limiter, + rate_limit_retry, + circuit_breaker, + hooks, + None, + ) + .await +} + +/// [`fetch`], plus explicit control over redirect following. +/// +/// A separate entry point rather than an eighth parameter on [`fetch`], because +/// changing that signature would break every existing caller. `None` follows, +/// which is the default everywhere else. +/// +/// `Some(false)` returns a 3xx as an ordinary response instead of following it — +/// see [`crate::builder::FetchBuilder::with_follow_redirects`] for why that +/// matters (SSRF guards, RFC 8461). +#[allow(clippy::too_many_arguments)] +pub async fn fetch_with_redirect_policy( + url: &str, + init: RequestInit, + options: Option, + rate_limiter: Option<&SlidingWindowRateLimiter>, + rate_limit_retry: Option<&RateLimitRetryOptions>, + circuit_breaker: Option<&CircuitBreaker>, + hooks: Option<&LifecycleHooks>, + follow_redirects: Option, ) -> Result, FetchError> { let opts = options.unwrap_or_default(); @@ -348,7 +382,7 @@ pub async fn fetch( async move { timeout::with_timeout( timeout_ms, - do_single_request::(&url, &init, connect_timeout_ms), + do_single_request::(&url, &init, connect_timeout_ms, follow_redirects), ) .await } @@ -361,7 +395,7 @@ pub async fn fetch( // No retry, just execute once with timeout timeout::with_timeout( timeout_ms, - do_single_request::(&url, &init, connect_timeout_ms), + do_single_request::(&url, &init, connect_timeout_ms, follow_redirects), ) .await }; diff --git a/rust/fetch/src/types.rs b/rust/fetch/src/types.rs index 3e55397..bf52f13 100644 --- a/rust/fetch/src/types.rs +++ b/rust/fetch/src/types.rs @@ -215,6 +215,12 @@ impl Method { } /// Request initialization options, analogous to the JS `RequestInit`. +/// +/// Redirect behaviour is NOT here: it is a client-level concern in reqwest +/// (the policy lives on the Client, not the request), and adding a public +/// field to this struct would break every exhaustive constructor downstream. +/// Use [`crate::builder::FetchBuilder::with_follow_redirects`], which is what +/// the Go and .NET ports expose too. #[derive(Debug, Clone, Default)] pub struct RequestInit { /// HTTP method. @@ -223,23 +229,4 @@ pub struct RequestInit { pub headers: HashMap, /// Request body (serialized as a string). pub body: Option, - /// Whether to follow HTTP redirects. `None` inherits the client default, - /// which is to follow. - /// - /// Set `Some(false)` when a redirect must not be followed automatically. - /// Two cases where following one is wrong rather than merely surprising: - /// - /// - The caller resolved the target hostname and checked it against an - /// SSRF allowlist. A 302 to an internal address defeats that guard - /// entirely, because the check was performed on the original host. - /// - RFC 8461 forbids fetching an MTA-STS policy through a redirect. - /// - /// With `Some(false)` the 3xx is returned as an ordinary response. - /// - /// `Option` rather than `bool` for two reasons: it keeps - /// `#[derive(Default)]` correct (a bare `bool` would default to FALSE and - /// silently flip behaviour for every `..Default::default()` caller), and it - /// lets [`crate::builder`]'s `merge_init` tell "unset" from "explicitly - /// true" so a client-level default is not clobbered by a per-request init. - pub follow_redirects: Option, } diff --git a/rust/fetch/tests/builder_tests.rs b/rust/fetch/tests/builder_tests.rs index 0a734f3..9cb3ba0 100644 --- a/rust/fetch/tests/builder_tests.rs +++ b/rust/fetch/tests/builder_tests.rs @@ -76,7 +76,6 @@ async fn test_builder_with_default_headers() { method: Method::GET, headers: default_headers, body: None, - follow_redirects: None, }) .build(); @@ -219,7 +218,6 @@ async fn test_builder_merge_headers() { method: Method::GET, headers: default_headers, body: None, - follow_redirects: None, }) .build(); @@ -231,7 +229,6 @@ async fn test_builder_merge_headers() { method: Method::GET, headers: request_headers, body: None, - follow_redirects: None, }; let response = client.fetch(&url, init).await.unwrap(); @@ -265,7 +262,6 @@ async fn test_builder_post_with_body() { method: Method::POST, headers, body: Some(r#"{"name":"test"}"#.to_string()), - follow_redirects: None, }; let response = client.fetch(&url, init).await.unwrap(); diff --git a/rust/fetch/tests/fetch_tests.rs b/rust/fetch/tests/fetch_tests.rs index f395c0b..c703b19 100644 --- a/rust/fetch/tests/fetch_tests.rs +++ b/rust/fetch/tests/fetch_tests.rs @@ -77,7 +77,6 @@ async fn test_basic_post_request_with_body() { method: Method::POST, headers, body: Some(r#"{"key":"value"}"#.to_string()), - follow_redirects: None, }; let options = FetchOptions { connect_timeout_ms: None, @@ -346,12 +345,13 @@ async fn test_default_method_is_get() { // `Option` so that `None` inherits and a client-level default is not // clobbered by a per-request `..Default::default()`. #[test] -fn request_init_defaults_to_following_redirects() { +fn request_init_has_no_redirect_field() { + // Deliberate: a public field here would break every exhaustive + // `RequestInit { .. }` downstream. Redirect policy is per-Client in reqwest + // anyway, so it belongs on the builder — as it does in Go and .NET. let init = RequestInit::default(); - assert_eq!( - init.follow_redirects, None, - "unset must mean inherit, not 'do not follow'" - ); + assert_eq!(init.method, Method::GET); + assert!(init.body.is_none()); } #[tokio::test] @@ -368,24 +368,13 @@ async fn a_redirect_is_not_followed_when_the_caller_opts_out() { .mount(&server) .await; - let init = RequestInit { - follow_redirects: Some(false), - ..Default::default() - }; - let result = client::fetch::( - &format!("{}/start", server.uri()), - init, - Some(FetchOptions { - connect_timeout_ms: None, - timeout: Some(TimeoutOptions { timeout_ms: 5000 }), - retry: None, - }), - None, - None, - None, - None, - ) - .await; + let client = smooai_fetch::builder::FetchBuilder::::new() + .with_follow_redirects(false) + .without_retry() + .build(); + let result = client + .fetch(&format!("{}/start", server.uri()), RequestInit::default()) + .await; let response = result.expect("a 3xx is a response, not a transport error"); assert_eq!( @@ -406,21 +395,13 @@ async fn a_redirect_is_followed_by_default() { .mount(&server) .await; - let response = client::fetch::( - &format!("{}/start", server.uri()), - RequestInit::default(), - Some(FetchOptions { - connect_timeout_ms: None, - timeout: Some(TimeoutOptions { timeout_ms: 5000 }), - retry: None, - }), - None, - None, - None, - None, - ) - .await - .expect("follows by default"); + let client = smooai_fetch::builder::FetchBuilder::::new() + .without_retry() + .build(); + let response = client + .fetch(&format!("{}/start", server.uri()), RequestInit::default()) + .await + .expect("follows by default"); assert_eq!(response.status, 200, "default behaviour must be unchanged"); }