diff --git a/.changes/unreleased/added-20260628-210901.yaml b/.changes/unreleased/added-20260628-210901.yaml new file mode 100644 index 00000000..6e00f4ab --- /dev/null +++ b/.changes/unreleased/added-20260628-210901.yaml @@ -0,0 +1,3 @@ +kind: Added +body: '`DescriptorPool` now implements `Clone`. (#253)' +time: 2026-06-28T21:09:01.000000000Z diff --git a/.changes/unreleased/fixed-20260628-210900.yaml b/.changes/unreleased/fixed-20260628-210900.yaml new file mode 100644 index 00000000..e01a59b9 --- /dev/null +++ b/.changes/unreleased/fixed-20260628-210900.yaml @@ -0,0 +1,3 @@ +kind: Fixed +body: '`DescriptorPool::add_file_descriptor_set` is now transactional: a failed add (e.g. an unresolvable type name) leaves the pool unchanged instead of retaining placeholder descriptors and name entries that made a corrected retry fail with `DuplicateName`. (#253)' +time: 2026-06-28T21:09:00.000000000Z diff --git a/buffa-descriptor/src/pool.rs b/buffa-descriptor/src/pool.rs index 3d0b0d65..dae14dc3 100644 --- a/buffa-descriptor/src/pool.rs +++ b/buffa-descriptor/src/pool.rs @@ -154,7 +154,7 @@ enum Definition { /// or accumulated via [`DescriptorPool::add_file_descriptor_set`]. Once built, /// the pool is immutable — descriptor handles are pool indices and all data /// is stored in flat `Vec`s. -#[derive(Debug, Default)] +#[derive(Clone, Debug, Default)] pub struct DescriptorPool { /// Original file descriptors, retained for raw access. files: Vec, @@ -224,11 +224,37 @@ impl DescriptorPool { /// Add the files in a `FileDescriptorSet` to the pool, registering and /// linking new types. Files already in the pool (by filename) are skipped. + /// If linking fails, the pool is left unchanged. + /// + /// Failure atomicity is implemented by staging the add against a clone of + /// the pool, so each call that introduces new files costs a deep copy of + /// the existing pool. Callers loading many files should batch them into a + /// single `FileDescriptorSet` rather than adding files one set at a time. /// /// # Errors /// /// Returns a [`PoolError`] on resolution failure. pub fn add_file_descriptor_set(&mut self, set: FileDescriptorSet) -> Result<(), PoolError> { + // Fast path for no-op re-adds without cloning the existing pool. + let has_new_files = set.file.iter().any(|f| { + f.name + .as_deref() + // MSRV: `Option::is_none_or` requires 1.82. + .map_or(true, |n| !self.file_by_name.contains_key(n)) + }); + if !has_new_files { + return Ok(()); + } + + // Build against a staged clone so fallible later passes cannot leave + // the live pool partially mutated. + let mut staged = self.clone(); + staged.add_file_descriptor_set_staged(set)?; + *self = staged; + Ok(()) + } + + fn add_file_descriptor_set_staged(&mut self, set: FileDescriptorSet) -> Result<(), PoolError> { // Filter out files already present (idempotent re-add). let new_files: Vec = set .file diff --git a/buffa-descriptor/tests/pool_e2e.rs b/buffa-descriptor/tests/pool_e2e.rs index 4a96eee3..e602a31c 100644 --- a/buffa-descriptor/tests/pool_e2e.rs +++ b/buffa-descriptor/tests/pool_e2e.rs @@ -206,6 +206,87 @@ fn idempotent_re_add() { ); } +#[test] +fn failed_add_does_not_mutate_pool_and_retry_succeeds() { + use buffa_descriptor::generated::descriptor::field_descriptor_proto::{Label, Type}; + use buffa_descriptor::generated::descriptor::{ + DescriptorProto, FieldDescriptorProto, FileDescriptorProto, FileDescriptorSet, + }; + + let mut p = DescriptorPool::decode(FDS_BYTES).unwrap(); + let baseline_message_count = p.messages().len(); + let baseline_file_count = p.files().len(); + + let broken = FileDescriptorSet { + file: vec![FileDescriptorProto { + name: Some("poison.proto".into()), + package: Some("poison.test".into()), + syntax: Some("proto3".into()), + message_type: vec![DescriptorProto { + name: Some("RetryMe".into()), + field: vec![FieldDescriptorProto { + name: Some("broken".into()), + number: Some(1), + label: Some(Label::LABEL_OPTIONAL), + r#type: Some(Type::TYPE_MESSAGE), + type_name: Some(".poison.test.Missing".into()), + ..Default::default() + }], + ..Default::default() + }], + ..Default::default() + }], + ..Default::default() + }; + + let err = p.add_file_descriptor_set(broken).unwrap_err(); + assert!( + err.to_string().contains("unresolved type name"), + "unexpected error: {err}" + ); + assert!( + p.message_by_name("poison.test.RetryMe").is_none(), + "failed add must not register placeholder descriptors" + ); + assert!( + p.file_by_name("poison.proto").is_none(), + "failed add must not record the file as loaded" + ); + assert_eq!(p.messages().len(), baseline_message_count); + assert_eq!(p.files().len(), baseline_file_count); + + let fixed = FileDescriptorSet { + file: vec![FileDescriptorProto { + name: Some("poison.proto".into()), + package: Some("poison.test".into()), + syntax: Some("proto3".into()), + message_type: vec![DescriptorProto { + name: Some("RetryMe".into()), + field: vec![FieldDescriptorProto { + name: Some("ok".into()), + number: Some(1), + label: Some(Label::LABEL_OPTIONAL), + r#type: Some(Type::TYPE_STRING), + ..Default::default() + }], + ..Default::default() + }], + ..Default::default() + }], + ..Default::default() + }; + + p.add_file_descriptor_set(fixed) + .expect("retry with a valid descriptor set succeeds"); + + let retry = p + .message_by_name("poison.test.RetryMe") + .expect("message loads after the corrected retry"); + assert_eq!(retry.field(1).unwrap().name(), "ok"); + assert_eq!(p.messages().len(), baseline_message_count + 1); + assert_eq!(p.files().len(), baseline_file_count + 1); +} + #[test] fn service_descriptor_links() { let p = pool();