From 5c103885681132ee582a4445719c86ae0acfab2e Mon Sep 17 00:00:00 2001 From: ogarciarevett Date: Sat, 27 Jun 2026 06:23:08 +0200 Subject: [PATCH] fix(view): preserve packed closed-enum unknowns --- README.md | 1 - buffa-codegen/src/view.rs | 26 +++++++++++++++++++++- buffa-test/src/tests/closed_enum.rs | 31 +++++++++++++++++++++++--- buffa/src/view.rs | 34 +++++++++++++++++++++++++++-- 4 files changed, 85 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index fc5c525c..2f21f7e9 100644 --- a/README.md +++ b/README.md @@ -56,7 +56,6 @@ These are intentionally out of scope: These are gaps we intend to address in future releases: -- **Closed-enum unknown values in packed-repeated view decode** are silently dropped (not routed to unknown fields). The owned decoder handles this correctly; the view decoder handles singular, optional, oneof, and unpacked repeated correctly. Packed blobs have no per-element tag to borrow, so the zero-copy `UnknownFieldsView<'a>` has no span to reference. - **Closed-enum unknown values in map values** are silently dropped (not routed to unknown fields). The proto spec requires the entire map entry (key + value) to go to unknown fields, which requires re-encoding. This affects proto2 schemas with `map` where an evolved sender adds new enum values. ## Semver and API stability diff --git a/buffa-codegen/src/view.rs b/buffa-codegen/src/view.rs index 90570470..6f8106d3 100644 --- a/buffa-codegen/src/view.rs +++ b/buffa-codegen/src/view.rs @@ -44,6 +44,24 @@ fn closed_enum_view_unknown_route(preserve_unknown_fields: bool) -> TokenStream } } +/// Token stream that records a closed-enum unknown packed element as an owned +/// varint unknown field in `view.__buffa_unknown_fields`. +fn closed_enum_view_unknown_varint_route( + field_number: u32, + preserve_unknown_fields: bool, +) -> TokenStream { + if preserve_unknown_fields { + quote! { + view.__buffa_unknown_fields.push_owned(::buffa::UnknownField { + number: #field_number, + data: ::buffa::UnknownFieldData::Varint(__raw as u64), + }); + } + } else { + quote! {} + } +} + /// Convert a borrowed bytes view to the owned field type. /// /// When `use_bytes_type()` is active for this field, emits @@ -1055,7 +1073,13 @@ fn repeated_decode_arm( let push_known = quote! { view.#ident.push(__v); }; let packed_elem = if ty == Type::TYPE_ENUM { if closed { - closed_enum_decode("e! { &mut pcur }, push_known.clone()) + let unknown_route = + closed_enum_view_unknown_varint_route(field_number, preserve_unknown_fields); + closed_enum_decode_with_unknown( + "e! { &mut pcur }, + push_known.clone(), + unknown_route, + ) } else { quote! { view.#ident.push(::buffa::EnumValue::from(::buffa::types::decode_int32(&mut pcur)?)); } } diff --git a/buffa-test/src/tests/closed_enum.rs b/buffa-test/src/tests/closed_enum.rs index a4fcbc1a..f44e8069 100644 --- a/buffa-test/src/tests/closed_enum.rs +++ b/buffa-test/src/tests/closed_enum.rs @@ -195,8 +195,8 @@ fn test_view_closed_enum_optional_unknown_to_unknown_fields() { #[test] fn test_view_closed_enum_repeated_unpacked_unknown_preserved() { - use crate::proto2::__buffa::view::ClosedEnumContextsView; use crate::proto2::Priority; + use crate::proto2::__buffa::view::ClosedEnumContextsView; use buffa::MessageView; // Field 2 (unpacked): [LOW=0, 99, HIGH=2] let mut wire = Vec::new(); @@ -218,6 +218,31 @@ fn test_view_closed_enum_repeated_unpacked_unknown_preserved() { assert!(!view2.__buffa_unknown_fields.is_empty()); } +#[test] +fn test_view_closed_enum_repeated_packed_unknown_preserved() { + use crate::proto2::__buffa::view::ClosedEnumContextsView; + use crate::proto2::{ClosedEnumContexts, Priority}; + use buffa::encoding::{encode_varint, Tag, WireType}; + use buffa::{Message, MessageView}; + + let mut payload = Vec::new(); + encode_varint(0, &mut payload); + encode_varint(99, &mut payload); + encode_varint(2, &mut payload); + let mut wire = Vec::new(); + Tag::new(3, WireType::LengthDelimited).encode(&mut wire); + encode_varint(payload.len() as u64, &mut wire); + wire.extend_from_slice(&payload); + + let owned_direct = ClosedEnumContexts::decode(&mut wire.as_slice()).unwrap(); + let view = ClosedEnumContextsView::decode_view(&wire).unwrap(); + let vals: Vec<_> = view.rep_packed.iter().copied().collect(); + assert_eq!(vals, vec![Priority::LOW, Priority::HIGH]); + + let via_view = view.to_owned_message(); + assert_eq!(via_view.encode_to_vec(), owned_direct.encode_to_vec()); +} + #[test] fn test_view_closed_enum_oneof_unknown_to_unknown_fields() { use crate::proto2::__buffa::view::ClosedEnumContextsView; @@ -232,8 +257,8 @@ fn test_view_closed_enum_oneof_unknown_to_unknown_fields() { #[test] fn test_view_closed_enum_known_not_routed() { - use crate::proto2::__buffa::view::ClosedEnumContextsView; use crate::proto2::Priority; + use crate::proto2::__buffa::view::ClosedEnumContextsView; use buffa::MessageView; let wire = varint_field(1, 2); // HIGH = 2 let view = ClosedEnumContextsView::decode_view(&wire).unwrap(); @@ -245,8 +270,8 @@ fn test_view_closed_enum_known_not_routed() { fn test_view_owned_parity_for_closed_enum_unknowns() { // Whatever the owned decoder produces, the view path must produce // byte-identical output after to_owned_message().encode_to_vec(). - use crate::proto2::__buffa::view::ClosedEnumContextsView; use crate::proto2::ClosedEnumContexts; + use crate::proto2::__buffa::view::ClosedEnumContextsView; use buffa::{Message, MessageView}; let mut wire = Vec::new(); wire.extend(varint_field(1, 99)); // optional unknown diff --git a/buffa/src/view.rs b/buffa/src/view.rs index 8da31afb..99c1c191 100644 --- a/buffa/src/view.rs +++ b/buffa/src/view.rs @@ -834,6 +834,9 @@ impl<'a, K, V> IntoIterator for MapView<'a, K, V> { pub struct UnknownFieldsView<'a> { /// Raw (tag, value) byte spans from the input buffer. raw_spans: alloc::vec::Vec<&'a [u8]>, + /// Synthetic unknown fields used when a view decoder cannot borrow an + /// original wire span but still needs to preserve the unknown value. + owned_fields: crate::UnknownFields, } impl<'a> UnknownFieldsView<'a> { @@ -847,14 +850,19 @@ impl<'a> UnknownFieldsView<'a> { self.raw_spans.push(span); } + #[doc(hidden)] + pub fn push_owned(&mut self, field: crate::UnknownField) { + self.owned_fields.push(field); + } + /// Returns `true` if no unknown fields were recorded. pub fn is_empty(&self) -> bool { - self.raw_spans.is_empty() + self.raw_spans.is_empty() && self.owned_fields.is_empty() } /// Total byte length of all unknown field data. pub fn encoded_len(&self) -> usize { - self.raw_spans.iter().map(|s| s.len()).sum() + self.raw_spans.iter().map(|s| s.len()).sum::() + self.owned_fields.encoded_len() } /// Write all unknown-field bytes verbatim. Each span is a complete @@ -864,6 +872,7 @@ impl<'a> UnknownFieldsView<'a> { for span in &self.raw_spans { buf.put_slice(span); } + self.owned_fields.write_to(buf); } /// Convert to an owned [`UnknownFields`](crate::UnknownFields) by parsing all stored raw byte spans. @@ -886,6 +895,9 @@ impl<'a> UnknownFieldsView<'a> { let field = decode_unknown_field(tag, &mut cur, crate::RECURSION_LIMIT)?; out.push(field); } + for field in self.owned_fields.iter() { + out.push(field.clone()); + } Ok(out) } } @@ -1696,6 +1708,24 @@ mod tests { assert!(uf.to_owned().is_err()); } + #[test] + fn unknown_fields_view_to_owned_includes_owned_fields() { + let mut uf = UnknownFieldsView::new(); + uf.push_owned(crate::UnknownField { + number: 3, + data: crate::UnknownFieldData::Varint(99), + }); + + let owned = uf.to_owned().expect("owned unknown field"); + let fields: Vec<_> = owned.iter().collect(); + assert_eq!(fields.len(), 1); + assert_eq!(fields[0].number, 3); + assert!(matches!( + fields[0].data, + crate::UnknownFieldData::Varint(99) + )); + } + // ── OwnedView ────────────────────────────────────────────────────── // Minimal types to test OwnedView without depending on generated code.