diff --git a/.changes/unreleased/fixed-20260903-113000.yaml b/.changes/unreleased/fixed-20260903-113000.yaml new file mode 100644 index 00000000..83c78b16 --- /dev/null +++ b/.changes/unreleased/fixed-20260903-113000.yaml @@ -0,0 +1,4 @@ +kind: Fixed +body: |- + **`DescriptorPool` rejects duplicate oneof names within a message** (#415), as `protoc` does. Only hand-built or synthesized descriptor sets can reach this state; the rejection is the new `PoolError::DuplicateOneofName` variant. +time: 2026-09-03T11:30:00+02:00 diff --git a/buffa-descriptor/src/pool.rs b/buffa-descriptor/src/pool.rs index 4ee79460..e0026e1d 100644 --- a/buffa-descriptor/src/pool.rs +++ b/buffa-descriptor/src/pool.rs @@ -206,6 +206,8 @@ pub enum PoolError { field: String, index: i32, }, + /// Two oneof declarations in one message have the same name. + DuplicateOneofName { message: String, name: String }, /// A field number is outside the valid range /// `[1, MAX_FIELD_NUMBER]` (`(1 << 29) - 1`). InvalidFieldNumber { field: String, number: i32 }, @@ -377,6 +379,12 @@ impl core::fmt::Display for PoolError { f, "field {field} in message {message} has invalid oneof index {index}" ), + Self::DuplicateOneofName { message, name } => { + write!( + f, + "message {message} declares oneof name {name:?} more than once" + ) + } Self::InvalidFieldNumber { field, number } => { write!(f, "field {field} has invalid field number {number}") } @@ -1477,16 +1485,23 @@ impl DescriptorPool { } // Build oneof descriptors. Track member field indices as we go. - let mut oneofs: Vec = msg - .oneof_decl - .iter() - .map(|o| OneofDescriptor { - name: o.name.clone().unwrap_or_default(), + let mut oneof_names: BTreeSet<&str> = BTreeSet::new(); + let mut oneofs = Vec::with_capacity(msg.oneof_decl.len()); + for o in &msg.oneof_decl { + let oneof_name = o.name.as_deref().unwrap_or(""); + if !oneof_names.insert(oneof_name) { + return Err(PoolError::DuplicateOneofName { + message: fqn, + name: oneof_name.to_string(), + }); + } + oneofs.push(OneofDescriptor { + name: oneof_name.to_string(), field_indices: Vec::new(), synthetic: false, options: clone_options(&o.options), - }) - .collect(); + }); + } // Build field descriptors. let mut fields = Vec::with_capacity(field_count); diff --git a/buffa-descriptor/tests/pool_e2e.rs b/buffa-descriptor/tests/pool_e2e.rs index 068998e1..d6d1f636 100644 --- a/buffa-descriptor/tests/pool_e2e.rs +++ b/buffa-descriptor/tests/pool_e2e.rs @@ -484,6 +484,94 @@ fn oneof_links() { assert_eq!(o.field_indices(), vec![0, 1, 2]); } +#[test] +fn duplicate_oneof_names_are_rejected_transactionally() { + use buffa_descriptor::generated::descriptor::field_descriptor_proto::Type; + use buffa_descriptor::generated::descriptor::{ + DescriptorProto, FieldDescriptorProto, OneofDescriptorProto, + }; + + // Each oneof gets a member so the descriptor is invalid for the duplicate + // name alone (protoc separately rejects an empty oneof). + let member = |name: &str, number: i32, oneof: i32| FieldDescriptorProto { + oneof_index: Some(oneof), + ..scalar_field(name, number, Type::TYPE_INT32) + }; + assert_rejected_without_mutating_pool( + "duplicate-oneof-name.proto", + "invalid.test.DuplicateOneof", + DescriptorProto { + name: Some("DuplicateOneof".into()), + field: vec![member("a", 1, 0), member("b", 2, 1)], + oneof_decl: vec![ + OneofDescriptorProto { + name: Some("choice".into()), + ..Default::default() + }, + OneofDescriptorProto { + name: Some("choice".into()), + ..Default::default() + }, + ], + ..Default::default() + }, + |err| { + assert!(matches!( + err, + PoolError::DuplicateOneofName { message, name } + if message == "invalid.test.DuplicateOneof" && name == "choice" + )); + assert_eq!( + err.to_string(), + "message invalid.test.DuplicateOneof declares oneof name \"choice\" more than once" + ); + }, + ); +} + +#[test] +fn distinct_oneof_names_are_accepted() { + use buffa_descriptor::generated::descriptor::{ + DescriptorProto, FileDescriptorProto, FileDescriptorSet, OneofDescriptorProto, + }; + + let mut p = DescriptorPool::decode(FDS_BYTES).unwrap(); + p.add_file_descriptor_set(FileDescriptorSet { + file: vec![FileDescriptorProto { + name: Some("distinct-oneof-names.proto".into()), + package: Some("valid.test".into()), + syntax: Some("proto3".into()), + message_type: vec![DescriptorProto { + name: Some("DistinctOneofs".into()), + oneof_decl: vec![ + OneofDescriptorProto { + name: Some("first".into()), + ..Default::default() + }, + OneofDescriptorProto { + name: Some("second".into()), + ..Default::default() + }, + ], + ..Default::default() + }], + ..Default::default() + }], + ..Default::default() + }) + .unwrap(); + + let message = p.message_by_name("valid.test.DistinctOneofs").unwrap(); + assert_eq!( + message + .oneofs() + .iter() + .map(|oneof| oneof.name()) + .collect::>(), + ["first", "second"] + ); +} + #[test] fn editions_feature_resolution() { let p = pool();