From 24a84e1da086a83bd021e61cbcb2dde4e6aba823 Mon Sep 17 00:00:00 2001 From: wayne warren Date: Wed, 19 Feb 2025 19:03:28 -0700 Subject: [PATCH 1/5] feat: implement GetOptions extensions --- object_store/src/extensions.rs | 191 +++++++++++++++++++++++++++++++++ object_store/src/lib.rs | 10 +- 2 files changed, 200 insertions(+), 1 deletion(-) create mode 100644 object_store/src/extensions.rs diff --git a/object_store/src/extensions.rs b/object_store/src/extensions.rs new file mode 100644 index 000000000000..4a3a3de2274a --- /dev/null +++ b/object_store/src/extensions.rs @@ -0,0 +1,191 @@ +use std::{ + any::{Any, TypeId}, + collections::HashMap, + sync::Arc, +}; + +/// Trait that must be implemented by extensions. +pub trait Extension: std::fmt::Debug + Send + Sync { + /// Return a &Any for this type. + fn as_any(&self) -> &dyn std::any::Any; + + /// Ensure that [`Extensions`] can implement [`std::cmp::PartialEq`] by requiring [`ExtTrait`] + /// implementors to implement a dyn-compatible partial equality operation. + /// + /// This is necessary because [`std::cmp::PartialEq`] uses a `Self` type parameter, which + /// violates dyn-compatibility rules: https://doc.rust-lang.org/error_codes/E0038.html#trait-uses-self-as-a-type-parameter-in-the-supertrait-listing + fn partial_eq(&self, other: &dyn std::any::Any) -> bool; +} + +type ExtensionMap = HashMap>; +type Ext = Arc; + +/// Type that holds opaque instances of types referenced by their [`std::any::TypeId`]. +/// +/// Types are stored as instances of the [`Extension`] trait in order to enable implementation of +/// [`std::fmt::Debug`] and [`std::cmp::PartialEq`]. +#[derive(Debug, Default, Clone)] +pub struct Extensions { + inner: ExtensionMap, +} + +impl PartialEq for Extensions { + fn eq(&self, other: &Self) -> bool { + for (k, v) in &self.inner { + if let Some(ov) = other.inner.get(&k) { + if !v.partial_eq(ov.as_any()) { + return false; + } + } else { + return false; + } + } + + true + } +} + +impl Extensions { + pub(crate) fn set_ext(&mut self, t: T) { + let id = t.type_id(); + let a = Arc::new(t); + self.inner.insert(id, a); + } + + pub(crate) fn get_ext(&self) -> Option<&T> { + let id = TypeId::of::(); + self.inner + .get(&id) + .map(|e| e.as_any().downcast_ref()) + .flatten() + } +} + +impl From> for Extensions { + fn from(other: HashMap) -> Self { + let mut s = Self::default(); + for (k, v) in other { + let v = Arc::new(ExtWrapper::new(v)); + s.inner.insert(k, v); + } + s + } +} + +/// Type that implements [`Extension`] for the sake of converting to [`Extensions`] from external +/// instances of `HashMap'. +#[derive(Debug)] +struct ExtWrapper { + inner: Arc, +} + +impl ExtWrapper { + fn new(v: T) -> Self { + Self { inner: Arc::new(v) } + } +} + +impl Extension for ExtWrapper { + fn as_any(&self) -> &dyn std::any::Any { + self.inner.as_ref() + } + + fn partial_eq(&self, other: &dyn std::any::Any) -> bool { + true + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[derive(Debug, PartialEq)] + struct MyExt { + x: u8, + } + + impl Extension for MyExt { + fn as_any(&self) -> &dyn std::any::Any { + self + } + + fn partial_eq(&self, other: &dyn std::any::Any) -> bool { + let self_id = self.type_id(); + let other_id = other.type_id(); + println!("MyExt.partial_eq"); + println!( + " self : {self_id:?}, {}", + std::any::type_name_of_val(self) + ); + println!( + " other : {other_id:?}, {}", + std::any::type_name_of_val(other) + ); + other + .downcast_ref::() + .inspect(|v| println!("{v:?}")) + .map(|other| self.x == other.x) + .unwrap_or_default() + } + } + + #[test] + fn custom_ext_trait_impl() { + let mut exts1 = Extensions::default(); + let myext1 = MyExt { x: 0 }; + exts1.set_ext(myext1); + let t1 = TypeId::of::(); + println!("type id of MyExt: {t1:?}"); + + let mut exts2 = Extensions::default(); + let myext2 = MyExt { x: 1 }; + exts2.set_ext(myext2); + + assert_ne!( + exts1, exts2, + "extensions of the same type with different values should not be equal" + ); + + let mut exts3 = Extensions::default(); + let myext3 = MyExt { x: 0 }; + exts3.set_ext(myext3); + + assert_eq!( + exts1, exts3, + "extensions of the same type with the same values should be equal" + ); + } + + #[test] + fn ext_wrapper() { + let v1 = 0; + let v2 = 1; + let v3 = 0; + + let wrapped1 = ExtWrapper::new(v1); + let wrapped2 = ExtWrapper::new(v2); + let wrapped3 = ExtWrapper::new(v3); + + let t1 = wrapped1.type_id(); + println!("type id of ExtWrapper: {t1:?}"); + + let t2 = wrapped2.type_id(); + println!("type id of ExtWrapper: {t2:?}"); + + let mut exts1 = Extensions::default(); + exts1.set_ext(wrapped1); + + let mut exts2 = Extensions::default(); + exts2.set_ext(wrapped2); + + assert_eq!( + exts1, exts2, + "extentions of type ExtWrapper are always equal even if their values aren't" + ); + + let mut exts3 = Extensions::default(); + exts3.set_ext(wrapped3); + + assert_eq!(exts1, exts3, "extentions of type ExtWrapper are equal"); + } +} diff --git a/object_store/src/lib.rs b/object_store/src/lib.rs index 4d8d8f02a0bc..940e2fd6596a 100644 --- a/object_store/src/lib.rs +++ b/object_store/src/lib.rs @@ -536,6 +536,7 @@ mod config; mod tags; +pub use extensions::{Extensions, Extension}; pub use tags::TagSet; pub mod multipart; @@ -545,6 +546,7 @@ mod upload; mod util; mod attributes; +mod extensions; #[cfg(any(feature = "integration", test))] pub mod integration; @@ -910,7 +912,6 @@ pub struct ObjectMeta { /// A version indicator for this object pub version: Option, } - /// Options for a get request, such as range #[derive(Debug, Default, Clone)] pub struct GetOptions { @@ -963,6 +964,13 @@ pub struct GetOptions { /// /// pub head: bool, + + /// Implementation-specific extensions. but + /// intended for use by [`ObjectStore`] implementations that need to pass context-specific + /// information (like tracing spans) via trait methods. + /// + /// These extensions are ignored entirely by backends offered through this crate. + pub extensions: Extensions, } impl GetOptions { From d51b2de8218393894b775dc1a82b2eb8308a19c9 Mon Sep 17 00:00:00 2001 From: wayne warren Date: Thu, 20 Feb 2025 15:27:44 -0700 Subject: [PATCH 2/5] test: add tests and get ExtWrapper working --- object_store/src/extensions.rs | 159 +++++++++++++++++++++++++-------- 1 file changed, 121 insertions(+), 38 deletions(-) diff --git a/object_store/src/extensions.rs b/object_store/src/extensions.rs index 4a3a3de2274a..71ae2a472134 100644 --- a/object_store/src/extensions.rs +++ b/object_store/src/extensions.rs @@ -7,14 +7,15 @@ use std::{ /// Trait that must be implemented by extensions. pub trait Extension: std::fmt::Debug + Send + Sync { /// Return a &Any for this type. - fn as_any(&self) -> &dyn std::any::Any; + fn as_any(self: Arc) -> Ext; - /// Ensure that [`Extensions`] can implement [`std::cmp::PartialEq`] by requiring [`ExtTrait`] + /// Ensure that [`Extensions`] can implement [`std::cmp::PartialEq`] by requiring [`Extension`] /// implementors to implement a dyn-compatible partial equality operation. /// /// This is necessary because [`std::cmp::PartialEq`] uses a `Self` type parameter, which - /// violates dyn-compatibility rules: https://doc.rust-lang.org/error_codes/E0038.html#trait-uses-self-as-a-type-parameter-in-the-supertrait-listing - fn partial_eq(&self, other: &dyn std::any::Any) -> bool; + /// violates dyn-compatibility rules: + /// https://doc.rust-lang.org/error_codes/E0038.html#trait-uses-self-as-a-type-parameter-in-the-supertrait-listing + fn partial_eq(self: Arc, other: Ext) -> bool; } type ExtensionMap = HashMap>; @@ -33,7 +34,7 @@ impl PartialEq for Extensions { fn eq(&self, other: &Self) -> bool { for (k, v) in &self.inner { if let Some(ov) = other.inner.get(&k) { - if !v.partial_eq(ov.as_any()) { + if !v.clone().partial_eq(ov.clone().as_any()) { return false; } } else { @@ -52,12 +53,54 @@ impl Extensions { self.inner.insert(id, a); } - pub(crate) fn get_ext(&self) -> Option<&T> { + fn set_ext_wrapper( + &mut self, + t: ExtWrapper, + ) { let id = TypeId::of::(); - self.inner - .get(&id) - .map(|e| e.as_any().downcast_ref()) - .flatten() + // println!("inserting: {id:?}: {}", std::any::type_name::()); + let a = Arc::new(t); + self.inner.insert(id, a); + } + + pub(crate) fn get_ext(&self) -> Option> { + let id = TypeId::of::(); + // println!( + // "looking for : {id:?}: {}", + // std::any::type_name::() + // ); + self.inner.get(&id).map(|e| { + // let id = e.type_id(); + // println!( + // "found value : {id:?}: {}", + // std::any::type_name_of_val(e) + // ); + let v = Arc::clone(&e); + // let id = v.type_id(); + // println!( + // "cloned value : {id:?}: {}", + // std::any::type_name_of_val(&v) + // ); + let v = v.as_any(); + // let id = v.type_id(); + // println!( + // "as_any value : {id:?}: {}", + // std::any::type_name_of_val(&v) + // ); + let v: Arc = + Arc::downcast::(v).expect("must be able to downcast to type if found in map"); + // let id = v.type_id(); + // println!( + // "found value : {id:?}: {}", + // std::any::type_name_of_val(&v) + // ); + // let id = v.as_ref().type_id(); + // println!( + // "found value (inner): {id:?}: {}", + // std::any::type_name_of_val(v.as_ref()) + // ); + v + }) } } @@ -74,23 +117,32 @@ impl From> for Extensions { /// Type that implements [`Extension`] for the sake of converting to [`Extensions`] from external /// instances of `HashMap'. +/// +/// NOTE: Different instances of the same type held by ExtWrapper are considered equal for the sake +/// of the `Extensions.partial_eq` trait since its intended use case is for wrapping dyn-compatible +/// trait objects; because [`std::cmp::PartialEq`] has `Self` as a generic type parameter, the type +/// system won't let us set it as a trait bound on incoming trait objects when constructing +/// ExtWrapper. #[derive(Debug)] -struct ExtWrapper { +struct ExtWrapper { inner: Arc, } -impl ExtWrapper { +impl ExtWrapper { fn new(v: T) -> Self { Self { inner: Arc::new(v) } } } -impl Extension for ExtWrapper { - fn as_any(&self) -> &dyn std::any::Any { - self.inner.as_ref() +impl Extension for ExtWrapper { + fn as_any(self: Arc) -> Ext { + self.inner.clone() } - fn partial_eq(&self, other: &dyn std::any::Any) -> bool { + fn partial_eq(self: Arc, _: Ext) -> bool { + // this is necessary because ExtWrapper is a generic impl of Extensions necessary for + // converting from external Arc types where none of the trait bounts can + // implement PartialEq due to dyn-compatibility requirements. true } } @@ -105,21 +157,21 @@ mod test { } impl Extension for MyExt { - fn as_any(&self) -> &dyn std::any::Any { + fn as_any(self: Arc) -> Ext { self } - fn partial_eq(&self, other: &dyn std::any::Any) -> bool { + fn partial_eq(self: Arc, other: Ext) -> bool { let self_id = self.type_id(); let other_id = other.type_id(); println!("MyExt.partial_eq"); println!( " self : {self_id:?}, {}", - std::any::type_name_of_val(self) + std::any::type_name_of_val(self.as_ref()) ); println!( " other : {other_id:?}, {}", - std::any::type_name_of_val(other) + std::any::type_name_of_val(other.as_ref()) ); other .downcast_ref::() @@ -130,7 +182,7 @@ mod test { } #[test] - fn custom_ext_trait_impl() { + fn equality_custom_ext_trait_impl() { let mut exts1 = Extensions::default(); let myext1 = MyExt { x: 0 }; exts1.set_ext(myext1); @@ -157,35 +209,66 @@ mod test { } #[test] - fn ext_wrapper() { - let v1 = 0; - let v2 = 1; - let v3 = 0; - - let wrapped1 = ExtWrapper::new(v1); - let wrapped2 = ExtWrapper::new(v2); - let wrapped3 = ExtWrapper::new(v3); - + fn equality_ext_wrapper() { + let mut exts1 = Extensions::default(); + let wrapped1 = ExtWrapper::new(0); let t1 = wrapped1.type_id(); println!("type id of ExtWrapper: {t1:?}"); + exts1.set_ext_wrapper(wrapped1); + let mut exts2 = Extensions::default(); + let wrapped2 = ExtWrapper::new(1); let t2 = wrapped2.type_id(); println!("type id of ExtWrapper: {t2:?}"); - let mut exts1 = Extensions::default(); - exts1.set_ext(wrapped1); - - let mut exts2 = Extensions::default(); - exts2.set_ext(wrapped2); + exts2.set_ext_wrapper(wrapped2); assert_eq!( exts1, exts2, - "extentions of type ExtWrapper are always equal even if their values aren't" + // this behavior is necessary because ExtWrapper is a generic impl of Extensions + // necessary for converting from external Arc types where none of the + // trait bounts can implement PartialEq due to dyn-compatibility requirements. + "extensions of type ExtWrapper are always equal even if their values aren't" ); let mut exts3 = Extensions::default(); - exts3.set_ext(wrapped3); + let wrapped3 = ExtWrapper::new(0); + exts3.set_ext_wrapper(wrapped3); + + assert_eq!(exts1, exts3, "extensions of type ExtWrapper are equal"); + } + + #[test] + fn one_instance_of_same_type_custom_impl() { + let mut exts = Extensions::default(); + + println!("validate MyExt"); + let myext = MyExt { x: 0 }; + exts.set_ext(myext); + let expected = exts.get_ext::().expect("must get a value"); + assert_eq!(0, expected.x, "return the same instance we just added"); + + println!("validate replacing previous MyExt"); + let myext = MyExt { x: 1 }; + exts.set_ext(myext); + let expected = exts.get_ext::().expect("must get a value"); + assert_eq!(1, expected.x, "return the same instance we just added"); + } + + #[test] + fn one_instance_of_same_type_ext_wrapper() { + let mut exts = Extensions::default(); + + println!("validate ExtWrapper with value 0"); + let wrapped = ExtWrapper::new(0i32); + exts.set_ext_wrapper(wrapped); + let expected = exts.get_ext::().expect("must get a value"); + assert_eq!(0, *expected, "return the same instance we just added"); - assert_eq!(exts1, exts3, "extentions of type ExtWrapper are equal"); + println!("validate replacing previous ExtWrapper with value 1"); + let wrapped = ExtWrapper::new(1i32); + exts.set_ext_wrapper(wrapped); + let expected = exts.get_ext::().expect("must get a value"); + assert_eq!(1, *expected, "return the same instance we just added"); } } From c273220927c302a21591f7d849f614b80fa9162a Mon Sep 17 00:00:00 2001 From: wayne warren Date: Thu, 20 Feb 2025 16:05:08 -0700 Subject: [PATCH 3/5] chore: minor cleanup of ExtWrapper usage --- object_store/src/extensions.rs | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/object_store/src/extensions.rs b/object_store/src/extensions.rs index 71ae2a472134..c595697f909b 100644 --- a/object_store/src/extensions.rs +++ b/object_store/src/extensions.rs @@ -53,13 +53,10 @@ impl Extensions { self.inner.insert(id, a); } - fn set_ext_wrapper( - &mut self, - t: ExtWrapper, - ) { + fn set_ext_wrapped(&mut self, t: T) { let id = TypeId::of::(); - // println!("inserting: {id:?}: {}", std::any::type_name::()); - let a = Arc::new(t); + //println!("inserting: {id:?}: {}", std::any::type_name::()); + let a = Arc::new(ExtWrapper::new(t)); self.inner.insert(id, a); } @@ -211,17 +208,10 @@ mod test { #[test] fn equality_ext_wrapper() { let mut exts1 = Extensions::default(); - let wrapped1 = ExtWrapper::new(0); - let t1 = wrapped1.type_id(); - println!("type id of ExtWrapper: {t1:?}"); - exts1.set_ext_wrapper(wrapped1); + exts1.set_ext_wrapped(0); let mut exts2 = Extensions::default(); - let wrapped2 = ExtWrapper::new(1); - let t2 = wrapped2.type_id(); - println!("type id of ExtWrapper: {t2:?}"); - - exts2.set_ext_wrapper(wrapped2); + exts2.set_ext_wrapped(1); assert_eq!( exts1, exts2, @@ -232,8 +222,7 @@ mod test { ); let mut exts3 = Extensions::default(); - let wrapped3 = ExtWrapper::new(0); - exts3.set_ext_wrapper(wrapped3); + exts3.set_ext_wrapped(0); assert_eq!(exts1, exts3, "extensions of type ExtWrapper are equal"); } @@ -260,14 +249,12 @@ mod test { let mut exts = Extensions::default(); println!("validate ExtWrapper with value 0"); - let wrapped = ExtWrapper::new(0i32); - exts.set_ext_wrapper(wrapped); + exts.set_ext_wrapped(0i32); let expected = exts.get_ext::().expect("must get a value"); assert_eq!(0, *expected, "return the same instance we just added"); println!("validate replacing previous ExtWrapper with value 1"); - let wrapped = ExtWrapper::new(1i32); - exts.set_ext_wrapper(wrapped); + exts.set_ext_wrapped(1i32); let expected = exts.get_ext::().expect("must get a value"); assert_eq!(1, *expected, "return the same instance we just added"); } From 3c4a84376a25af362597dad66821ff55777fd370 Mon Sep 17 00:00:00 2001 From: wayne warren Date: Thu, 20 Feb 2025 20:12:47 -0700 Subject: [PATCH 4/5] test: add test for impl From for Extensions, fix bug in ExtWrapper --- object_store/src/extensions.rs | 127 +++++++++++++++------------------ object_store/src/lib.rs | 5 +- 2 files changed, 61 insertions(+), 71 deletions(-) diff --git a/object_store/src/extensions.rs b/object_store/src/extensions.rs index c595697f909b..ba0a957c942c 100644 --- a/object_store/src/extensions.rs +++ b/object_store/src/extensions.rs @@ -6,14 +6,14 @@ use std::{ /// Trait that must be implemented by extensions. pub trait Extension: std::fmt::Debug + Send + Sync { - /// Return a &Any for this type. + /// Return a `Arc` for this type. fn as_any(self: Arc) -> Ext; /// Ensure that [`Extensions`] can implement [`std::cmp::PartialEq`] by requiring [`Extension`] /// implementors to implement a dyn-compatible partial equality operation. /// - /// This is necessary because [`std::cmp::PartialEq`] uses a `Self` type parameter, which - /// violates dyn-compatibility rules: + /// This is necessary rather than using a [`std::cmp::PartialEq`] trait bound because uses a + /// `Self` type parameter, which violates dyn-compatibility rules: /// https://doc.rust-lang.org/error_codes/E0038.html#trait-uses-self-as-a-type-parameter-in-the-supertrait-listing fn partial_eq(self: Arc, other: Ext) -> bool; } @@ -53,50 +53,11 @@ impl Extensions { self.inner.insert(id, a); } - fn set_ext_wrapped(&mut self, t: T) { - let id = TypeId::of::(); - //println!("inserting: {id:?}: {}", std::any::type_name::()); - let a = Arc::new(ExtWrapper::new(t)); - self.inner.insert(id, a); - } - pub(crate) fn get_ext(&self) -> Option> { let id = TypeId::of::(); - // println!( - // "looking for : {id:?}: {}", - // std::any::type_name::() - // ); self.inner.get(&id).map(|e| { - // let id = e.type_id(); - // println!( - // "found value : {id:?}: {}", - // std::any::type_name_of_val(e) - // ); - let v = Arc::clone(&e); - // let id = v.type_id(); - // println!( - // "cloned value : {id:?}: {}", - // std::any::type_name_of_val(&v) - // ); - let v = v.as_any(); - // let id = v.type_id(); - // println!( - // "as_any value : {id:?}: {}", - // std::any::type_name_of_val(&v) - // ); - let v: Arc = - Arc::downcast::(v).expect("must be able to downcast to type if found in map"); - // let id = v.type_id(); - // println!( - // "found value : {id:?}: {}", - // std::any::type_name_of_val(&v) - // ); - // let id = v.as_ref().type_id(); - // println!( - // "found value (inner): {id:?}: {}", - // std::any::type_name_of_val(v.as_ref()) - // ); - v + let v = Arc::clone(&e).as_any(); + Arc::downcast::(v).unwrap() }) } } @@ -105,7 +66,7 @@ impl From> for Extensions { fn from(other: HashMap) -> Self { let mut s = Self::default(); for (k, v) in other { - let v = Arc::new(ExtWrapper::new(v)); + let v = Arc::new(ExtWrapper { inner: v }); s.inner.insert(k, v); } s @@ -120,18 +81,17 @@ impl From> for Extensions { /// trait objects; because [`std::cmp::PartialEq`] has `Self` as a generic type parameter, the type /// system won't let us set it as a trait bound on incoming trait objects when constructing /// ExtWrapper. -#[derive(Debug)] -struct ExtWrapper { - inner: Arc, +struct ExtWrapper { + inner: Arc, } -impl ExtWrapper { - fn new(v: T) -> Self { - Self { inner: Arc::new(v) } +impl std::fmt::Debug for ExtWrapper { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "ExtWrapper2") } } -impl Extension for ExtWrapper { +impl Extension for ExtWrapper { fn as_any(self: Arc) -> Ext { self.inner.clone() } @@ -159,20 +119,8 @@ mod test { } fn partial_eq(self: Arc, other: Ext) -> bool { - let self_id = self.type_id(); - let other_id = other.type_id(); - println!("MyExt.partial_eq"); - println!( - " self : {self_id:?}, {}", - std::any::type_name_of_val(self.as_ref()) - ); - println!( - " other : {other_id:?}, {}", - std::any::type_name_of_val(other.as_ref()) - ); other .downcast_ref::() - .inspect(|v| println!("{v:?}")) .map(|other| self.x == other.x) .unwrap_or_default() } @@ -183,8 +131,6 @@ mod test { let mut exts1 = Extensions::default(); let myext1 = MyExt { x: 0 }; exts1.set_ext(myext1); - let t1 = TypeId::of::(); - println!("type id of MyExt: {t1:?}"); let mut exts2 = Extensions::default(); let myext2 = MyExt { x: 1 }; @@ -205,6 +151,28 @@ mod test { ); } + impl Extensions { + fn set_ext_wrapped(&mut self, t: T) { + let id = TypeId::of::(); + let a = Arc::new(ExtWrapper { inner: Arc::new(t) }); + self.inner.insert(id, a); + } + } + + #[test] + fn not_equal_if_missing_entry() { + let mut exts1 = Extensions::default(); + exts1.set_ext_wrapped(0); + exts1.set_ext_wrapped(String::from("meow")); + + let mut exts2 = Extensions::default(); + exts2.set_ext_wrapped(1); + + assert_ne!( + exts1, exts2, + "two different Extensions cannot be equal if they don't carry the same types" + ); + } #[test] fn equality_ext_wrapper() { let mut exts1 = Extensions::default(); @@ -248,14 +216,37 @@ mod test { fn one_instance_of_same_type_ext_wrapper() { let mut exts = Extensions::default(); - println!("validate ExtWrapper with value 0"); + println!("validate ExtWrapper with value 0"); exts.set_ext_wrapped(0i32); let expected = exts.get_ext::().expect("must get a value"); assert_eq!(0, *expected, "return the same instance we just added"); - println!("validate replacing previous ExtWrapper with value 1"); + println!("validate replacing previous ExtWrapper with value 1"); exts.set_ext_wrapped(1i32); let expected = exts.get_ext::().expect("must get a value"); assert_eq!(1, *expected, "return the same instance we just added"); } + + #[test] + fn from_hashmap_of_exts() { + let mut map: HashMap = HashMap::new(); + + let v = 0i32; + let id = v.type_id(); + assert!(map.insert(id, Arc::new(v)).is_none()); + + let s: String = "meow".to_string(); + let id = s.type_id(); + assert!(map.insert(id, Arc::new(s)).is_none()); + + assert!(map.len() == 2); + + let exts: Extensions = map.into(); + + let v = exts.get_ext::().expect("must get a value"); + assert_eq!(0i32, *v.as_ref()); + + let v = exts.get_ext::().expect("must get a value"); + assert_eq!(String::from("meow"), *v.as_ref()); + } } diff --git a/object_store/src/lib.rs b/object_store/src/lib.rs index 940e2fd6596a..cb27990764b0 100644 --- a/object_store/src/lib.rs +++ b/object_store/src/lib.rs @@ -965,9 +965,8 @@ pub struct GetOptions { /// pub head: bool, - /// Implementation-specific extensions. but - /// intended for use by [`ObjectStore`] implementations that need to pass context-specific - /// information (like tracing spans) via trait methods. + /// Implementation-specific extensions. Intended for use by [`ObjectStore`] implementations + /// that need to pass context-specific information (like tracing spans) via trait methods. /// /// These extensions are ignored entirely by backends offered through this crate. pub extensions: Extensions, From 167afbb69f3ed0b3513e420ede2f4ec38c02b547 Mon Sep 17 00:00:00 2001 From: wayne warren Date: Thu, 20 Feb 2025 22:57:52 -0700 Subject: [PATCH 5/5] fix: bug in impl PartialEq for Extensions --- object_store/src/extensions.rs | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/object_store/src/extensions.rs b/object_store/src/extensions.rs index ba0a957c942c..38a9325b7b5b 100644 --- a/object_store/src/extensions.rs +++ b/object_store/src/extensions.rs @@ -32,17 +32,15 @@ pub struct Extensions { impl PartialEq for Extensions { fn eq(&self, other: &Self) -> bool { - for (k, v) in &self.inner { - if let Some(ov) = other.inner.get(&k) { - if !v.clone().partial_eq(ov.clone().as_any()) { - return false; - } - } else { - return false; - } + if self.inner.len() != other.inner.len() { + return false; } - true + self.inner.iter().all(|(key, value)| { + other.inner.get(key).map_or(false, |other| { + value.clone().partial_eq(other.clone().as_any()) + }) + }) } } @@ -166,13 +164,20 @@ mod test { exts1.set_ext_wrapped(String::from("meow")); let mut exts2 = Extensions::default(); - exts2.set_ext_wrapped(1); + exts2.set_ext_wrapped(0); assert_ne!( exts1, exts2, "two different Extensions cannot be equal if they don't carry the same types" ); + + // validate the PartialEq impl is commutative + assert_ne!( + exts2, exts1, + "two different Extensions cannot be equal if they don't carry the same types" + ); } + #[test] fn equality_ext_wrapper() { let mut exts1 = Extensions::default();