From 3ed393628366eb0b056bfc3b8524b99e680fdb91 Mon Sep 17 00:00:00 2001 From: jackylee-ch Date: Mon, 7 Sep 2026 17:33:41 +0800 Subject: [PATCH 1/2] fix(cloud): parse OpenDAL boolean properties the way object_store does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cos` and `oss` compared the raw property string against `"true"`, so `True`, `1` and `yes` — all accepted by `object_store` for its own configuration, and by the `allow_http` sibling in this crate — read as `false` without a warning. Add a shared `property_as_bool` next to `property_or_env` that trims, lowercases and matches `object_store`'s set, and warns on a value that is not a boolean instead of silently answering `false`. Signed-off-by: jackylee-ch --- vortex-cloud/src/opendal/cos.rs | 24 +++++++++++++-- vortex-cloud/src/opendal/mod.rs | 54 +++++++++++++++++++++++++++++++++ vortex-cloud/src/opendal/oss.rs | 23 +++++++++++++- 3 files changed, 98 insertions(+), 3 deletions(-) diff --git a/vortex-cloud/src/opendal/cos.rs b/vortex-cloud/src/opendal/cos.rs index b35978d53bc..e3c9f1bc260 100644 --- a/vortex-cloud/src/opendal/cos.rs +++ b/vortex-cloud/src/opendal/cos.rs @@ -13,6 +13,7 @@ use vortex_utils::aliases::hash_map::HashMap; use crate::opendal::OpenDALStoreError; use crate::opendal::build_operator; +use crate::opendal::property_as_bool; use crate::opendal::property_or_env; use crate::opendal::warn_on_unknown_properties; @@ -129,8 +130,7 @@ where &env_lookup, ), root: properties.get("root").cloned(), - disable_config_load: properties.get("disable_config_load").map(String::as_str) - == Some("true"), + disable_config_load: property_as_bool(properties, "disable_config_load"), }) } @@ -228,4 +228,24 @@ mod tests { Err(OpenDALStoreError::MissingConfig("endpoint")) )); } + + /// `disable_config_load` asks OpenDAL not to pick up ambient credentials, so a spelling the + /// caller reasonably wrote must not be dropped: reading it as `false` would leave the + /// implicit config loading the caller asked to turn off. + #[rstest::rstest] + #[case("True")] + #[case("1")] + #[case("yes")] + fn cos_reads_disable_config_load_beyond_lowercase_true(#[case] value: &str) { + let url = Url::parse("cos://my-bucket/path").unwrap(); + let env = |key: &str| match key { + "COS_ENDPOINT" => Some("https://example.com".to_string()), + _ => None, + }; + let mut props = HashMap::new(); + props.insert("disable_config_load".to_string(), value.to_string()); + + let config = url_and_properties_to_config(&url, &props, env).expect("config"); + assert!(config.disable_config_load, "{value} should read as true"); + } } diff --git a/vortex-cloud/src/opendal/mod.rs b/vortex-cloud/src/opendal/mod.rs index 857adb78070..e4b01e46799 100644 --- a/vortex-cloud/src/opendal/mod.rs +++ b/vortex-cloud/src/opendal/mod.rs @@ -204,6 +204,26 @@ where properties.get(key).cloned().or_else(|| env_lookup(env_var)) } +/// Take `key` from `properties` as a boolean, accepting the spellings `object_store` accepts in +/// its own configuration: `1`/`true`/`on`/`yes`/`y` and their negatives, case-insensitively. +/// +/// An absent key is `false`. A value that is not a boolean is warned about and read as `false`, +/// which is how [`warn_on_unknown_properties`] already treats a key the service cannot use. +#[cfg(any(feature = "cos", feature = "oss"))] +pub(crate) fn property_as_bool(properties: &HashMap, key: &str) -> bool { + let Some(value) = properties.get(key) else { + return false; + }; + match value.trim().to_ascii_lowercase().as_str() { + "1" | "true" | "on" | "yes" | "y" => true, + "0" | "false" | "off" | "no" | "n" => false, + _ => { + warn!("ignoring OpenDAL store property {key}: `{value}` is not a boolean"); + false + } + } +} + /// Log a warning for every property key the service does not recognize. #[cfg(any(feature = "cos", feature = "goosefs", feature = "oss"))] pub(crate) fn warn_on_unknown_properties(properties: &HashMap, known: &[&str]) { @@ -265,4 +285,38 @@ mod tests { ); } } + + /// The spellings `object_store` accepts for its own boolean configuration, so that the same + /// property means the same thing whether a URL resolves to a native store or an OpenDAL one. + /// Values reach us verbatim from the caller's property map, hence the case and whitespace + /// cases; anything that is not a boolean stays `false` rather than becoming an error. + #[cfg(any(feature = "cos", feature = "oss"))] + #[rstest::rstest] + #[case("true", true)] + #[case("True", true)] + #[case("TRUE", true)] + #[case(" true ", true)] + #[case("1", true)] + #[case("on", true)] + #[case("yes", true)] + #[case("y", true)] + #[case("false", false)] + #[case("False", false)] + #[case("0", false)] + #[case("off", false)] + #[case("no", false)] + #[case("n", false)] + #[case("maybe", false)] + #[case("", false)] + fn property_as_bool_matches_object_store(#[case] value: &str, #[case] expected: bool) { + let mut props = HashMap::new(); + props.insert("skip_signature".to_string(), value.to_string()); + assert_eq!(property_as_bool(&props, "skip_signature"), expected); + } + + #[cfg(any(feature = "cos", feature = "oss"))] + #[test] + fn property_as_bool_is_false_when_absent() { + assert!(!property_as_bool(&HashMap::new(), "skip_signature")); + } } diff --git a/vortex-cloud/src/opendal/oss.rs b/vortex-cloud/src/opendal/oss.rs index 94ff47230e4..bb264329a2a 100644 --- a/vortex-cloud/src/opendal/oss.rs +++ b/vortex-cloud/src/opendal/oss.rs @@ -13,6 +13,7 @@ use vortex_utils::aliases::hash_map::HashMap; use crate::opendal::OpenDALStoreError; use crate::opendal::build_operator; +use crate::opendal::property_as_bool; use crate::opendal::property_or_env; use crate::opendal::warn_on_unknown_properties; @@ -140,7 +141,7 @@ where &env_lookup, ), root: properties.get("root").cloned(), - skip_signature: properties.get("skip_signature").map(String::as_str) == Some("true"), + skip_signature: property_as_bool(properties, "skip_signature"), }) } @@ -254,4 +255,24 @@ mod tests { Err(OpenDALStoreError::MissingConfig("endpoint")) )); } + + /// `skip_signature` is what makes a public read-only bucket reachable without credentials, so + /// a spelling the caller reasonably wrote must not be dropped: reading it as `false` keeps + /// signing the request and the bucket answers 403. + #[rstest::rstest] + #[case("True")] + #[case("1")] + #[case("yes")] + fn oss_reads_skip_signature_beyond_lowercase_true(#[case] value: &str) { + let url = Url::parse("oss://public-bucket/path").unwrap(); + let env = |key: &str| match key { + "OSS_ENDPOINT" => Some("https://oss-cn-hangzhou.aliyuncs.com".to_string()), + _ => None, + }; + let mut props = HashMap::new(); + props.insert("skip_signature".to_string(), value.to_string()); + + let config = url_and_properties_to_config(&url, &props, env).expect("config"); + assert!(config.skip_signature, "{value} should read as true"); + } } From 553b455bcfdaedee5b2322fc8d7d290f827a8616 Mon Sep 17 00:00:00 2001 From: jackylee-ch Date: Thu, 10 Sep 2026 10:54:56 +0800 Subject: [PATCH 2/2] test(cloud): drop the per-service boolean property tests The `property_as_bool` cases cover the parsing; the cos and oss tests only asserted that each call site reaches it, which is a one-line change per service. Signed-off-by: jackylee-ch --- vortex-cloud/src/opendal/cos.rs | 20 -------------------- vortex-cloud/src/opendal/oss.rs | 20 -------------------- 2 files changed, 40 deletions(-) diff --git a/vortex-cloud/src/opendal/cos.rs b/vortex-cloud/src/opendal/cos.rs index e3c9f1bc260..b1a4f07f0a9 100644 --- a/vortex-cloud/src/opendal/cos.rs +++ b/vortex-cloud/src/opendal/cos.rs @@ -228,24 +228,4 @@ mod tests { Err(OpenDALStoreError::MissingConfig("endpoint")) )); } - - /// `disable_config_load` asks OpenDAL not to pick up ambient credentials, so a spelling the - /// caller reasonably wrote must not be dropped: reading it as `false` would leave the - /// implicit config loading the caller asked to turn off. - #[rstest::rstest] - #[case("True")] - #[case("1")] - #[case("yes")] - fn cos_reads_disable_config_load_beyond_lowercase_true(#[case] value: &str) { - let url = Url::parse("cos://my-bucket/path").unwrap(); - let env = |key: &str| match key { - "COS_ENDPOINT" => Some("https://example.com".to_string()), - _ => None, - }; - let mut props = HashMap::new(); - props.insert("disable_config_load".to_string(), value.to_string()); - - let config = url_and_properties_to_config(&url, &props, env).expect("config"); - assert!(config.disable_config_load, "{value} should read as true"); - } } diff --git a/vortex-cloud/src/opendal/oss.rs b/vortex-cloud/src/opendal/oss.rs index bb264329a2a..3d2ae4d41b3 100644 --- a/vortex-cloud/src/opendal/oss.rs +++ b/vortex-cloud/src/opendal/oss.rs @@ -255,24 +255,4 @@ mod tests { Err(OpenDALStoreError::MissingConfig("endpoint")) )); } - - /// `skip_signature` is what makes a public read-only bucket reachable without credentials, so - /// a spelling the caller reasonably wrote must not be dropped: reading it as `false` keeps - /// signing the request and the bucket answers 403. - #[rstest::rstest] - #[case("True")] - #[case("1")] - #[case("yes")] - fn oss_reads_skip_signature_beyond_lowercase_true(#[case] value: &str) { - let url = Url::parse("oss://public-bucket/path").unwrap(); - let env = |key: &str| match key { - "OSS_ENDPOINT" => Some("https://oss-cn-hangzhou.aliyuncs.com".to_string()), - _ => None, - }; - let mut props = HashMap::new(); - props.insert("skip_signature".to_string(), value.to_string()); - - let config = url_and_properties_to_config(&url, &props, env).expect("config"); - assert!(config.skip_signature, "{value} should read as true"); - } }