This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-11638-0a2b0498c96121812686d8b626591a929c87ee06 in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
commit a2b405fe22b6657fd4f69eb139e6836cafa3b55f Author: ump45nose <[email protected]> AuthorDate: Sun Oct 18 02:32:15 2026 +0000 fix(arrow-schema): validate `UnionFields` when deserializing (#11638) # Which issue does this PR close? - Closes #11576. # Rationale for this change `UnionFields` derived `Deserialize`, so deserializing could build a value with duplicate or negative type ids, skipping the checks that #11533 added to the other constructors. # What changes are included in this PR? `Serialize` is still derived (transparent), but `Deserialize` is now implemented by hand: it reads the same sequence of `(type_id, field)` pairs and builds the value with `UnionFields::try_new`, returning a serde error if validation fails. The wire format doesn't change, so anything serialized before still deserializes as long as it was valid. I kept serde rather than removing it, since `DataType` (and therefore `Field` and `Schema`) derive serde and need `UnionFields` to support it. # Are these changes tested? Yes, two new tests in `fields.rs` (behind the `serde` feature, using `postcard` like the existing `Field` serde tests): a round trip of a valid `UnionFields`, and deserializing input with a duplicate type id and with a negative type id, both of which now fail. The second test fails without this change. # Are there any significant user-facing changes? Deserializing an invalid `UnionFields` (or a `DataType::Union` containing one) now returns an error instead of succeeding. AI disclosure: I used an AI coding assistant to write the change, the tests and this description, and I reviewed all of it. --------- Co-authored-by: ump45nose <[email protected]> --- arrow-schema/Cargo.toml | 1 + arrow-schema/src/fields.rs | 51 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/arrow-schema/Cargo.toml b/arrow-schema/Cargo.toml index ae62aa4c42..3b5bcffd02 100644 --- a/arrow-schema/Cargo.toml +++ b/arrow-schema/Cargo.toml @@ -55,6 +55,7 @@ all-features = true criterion = { workspace = true, default-features = false } insta = { workspace = true, default-features = true } postcard = { version = "1.0.10", default-features = false, features = ["use-std"] } +serde_json = "1.0" [[bench]] name = "ffi" diff --git a/arrow-schema/src/fields.rs b/arrow-schema/src/fields.rs index 60e560293e..c68f179fcb 100644 --- a/arrow-schema/src/fields.rs +++ b/arrow-schema/src/fields.rs @@ -332,10 +332,23 @@ impl<'a> IntoIterator for &'a Fields { /// - All type ids are unique /// - All type ids are non-negative #[derive(Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] -#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +#[cfg_attr(feature = "serde", derive(serde::Serialize))] #[cfg_attr(feature = "serde", serde(transparent))] pub struct UnionFields(Arc<[(i8, FieldRef)]>); +// Manual implementation to enforce invariants per docstring above +#[cfg(feature = "serde")] +impl<'de> serde::Deserialize<'de> for UnionFields { + fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> + where + D: serde::Deserializer<'de>, + { + let entries = Vec::<(i8, FieldRef)>::deserialize(deserializer)?; + let (type_ids, fields): (Vec<i8>, Vec<FieldRef>) = entries.into_iter().unzip(); + Self::try_new(type_ids, fields).map_err(serde::de::Error::custom) + } +} + impl std::fmt::Debug for UnionFields { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { self.0.as_ref().fmt(f) @@ -1118,4 +1131,40 @@ mod tests { ]; UnionFields::from_iter(fields); } + + #[cfg(feature = "serde")] + #[test] + fn test_union_fields_serde_round_trip() { + let fields = UnionFields::try_new( + vec![1, 3], + vec![ + Field::new("a", DataType::Int32, true), + Field::new("b", DataType::Utf8, true), + ], + ) + .unwrap(); + let serialized = postcard::to_stdvec(&fields).unwrap(); + let deserialized: UnionFields = postcard::from_bytes(&serialized).unwrap(); + assert_eq!(fields, deserialized); + } + + #[cfg(feature = "serde")] + #[test] + fn test_union_fields_deserialize_should_validate() { + let field = Arc::new(Field::new("a", DataType::Int32, true)); + + let duplicate: Vec<(i8, FieldRef)> = vec![(1, Arc::clone(&field)), (1, Arc::clone(&field))]; + let serialized = serde_json::to_string(&duplicate).unwrap(); + let err = serde_json::from_str::<UnionFields>(&serialized).unwrap_err(); + assert!(err.to_string().contains("duplicate type id: 1"), "{err}"); + + let negative: Vec<(i8, FieldRef)> = vec![(0, Arc::clone(&field)), (-1, field)]; + let serialized = serde_json::to_string(&negative).unwrap(); + let err = serde_json::from_str::<UnionFields>(&serialized).unwrap_err(); + assert!( + err.to_string() + .contains("type ids must be non-negative: -1"), + "{err}" + ); + } }
