From d6454f8fe2df21a73a39ab65009a65bccf1555c0 Mon Sep 17 00:00:00 2001 From: Javier Cano Date: Fri, 25 Mar 2022 22:32:02 -0700 Subject: [PATCH] feat: Add flag to skip unknown fields instead of error This is mostly required for forward compatibility, when new fields are added, any old instance will fail to deserialize a message serialized with a newer version. This is also aligned with prost that skips any unknown field too. I added a flag to to the builder, so this behavior is opt-in, keeping the old behavior (error out for unknown fields) as default. --- .circleci/config.yml | 3 ++ pbjson-build/src/generator/message.rs | 70 ++++++++++++++++++++++++--- pbjson-build/src/lib.rs | 11 ++++- pbjson-test/Cargo.toml | 3 ++ pbjson-test/build.rs | 14 ++++-- pbjson-test/src/lib.rs | 15 +++++- 6 files changed, 103 insertions(+), 13 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 0d3d387..27366fc 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -125,6 +125,9 @@ jobs: - run: name: Cargo test command: cargo test --workspace + - run: + name: Cargo test (skip unknown fields) + command: cargo test --workspace --features ignore-unknown-fields - cache_save workflows: diff --git a/pbjson-build/src/generator/message.rs b/pbjson-build/src/generator/message.rs index e0de7b8..8435bf1 100644 --- a/pbjson-build/src/generator/message.rs +++ b/pbjson-build/src/generator/message.rs @@ -37,6 +37,7 @@ pub fn generate_message( resolver: &Resolver<'_>, message: &Message, writer: &mut W, + ignore_unknown_fields: bool, ) -> Result<()> { let rust_type = resolver.rust_type(&message.path); @@ -47,7 +48,14 @@ pub fn generate_message( // Generate Deserialize write_deserialize_start(0, &rust_type, writer)?; - write_deserialize_message(resolver, 2, message, &rust_type, writer)?; + write_deserialize_message( + resolver, + 2, + message, + &rust_type, + writer, + ignore_unknown_fields, + )?; write_deserialize_end(0, writer)?; Ok(()) } @@ -426,8 +434,9 @@ fn write_deserialize_message( message: &Message, rust_type: &str, writer: &mut W, + ignore_unknown_fields: bool, ) -> Result<()> { - write_deserialize_field_name(2, message, writer)?; + write_deserialize_field_name(2, message, writer, ignore_unknown_fields)?; writeln!(writer, "{}struct GeneratedVisitor;", Indent(indent))?; @@ -486,14 +495,34 @@ fn write_deserialize_message( } } + if ignore_unknown_fields { + writeln!( + writer, + "{}GeneratedField::__SkipField__ => {{", + Indent(indent + 4), + )?; + writeln!( + writer, + "{}let _ = map.next_value::()?;", + Indent(indent + 5), + )?; + writeln!(writer, "{}}}", Indent(indent + 4))?; + } + writeln!(writer, "{}}}", Indent(indent + 3))?; writeln!(writer, "{}}}", Indent(indent + 2))?; } else { writeln!( writer, - "{}while map.next_key::()?.is_some() {{}}", + "{}while map.next_key::()?.is_some() {{", Indent(indent + 2) )?; + writeln!( + writer, + "{}let _ = map.next_value::()?;", + Indent(indent + 3) + )?; + writeln!(writer, "{}}}", Indent(indent + 2))?; } writeln!(writer, "{}Ok({} {{", Indent(indent + 2), rust_type)?; @@ -551,6 +580,7 @@ fn write_deserialize_field_name( indent: usize, message: &Message, writer: &mut W, + ignore_unknown_fields: bool, ) -> Result<()> { let fields: Vec<_> = message .all_fields() @@ -558,7 +588,12 @@ fn write_deserialize_field_name( .collect(); write_fields_array(writer, indent, fields.iter().map(|(name, _)| name.as_str()))?; - write_fields_enum(writer, indent, fields.iter().map(|(_, name)| name.as_str()))?; + write_fields_enum( + writer, + indent, + fields.iter().map(|(_, name)| name.as_str()), + ignore_unknown_fields, + )?; writeln!( writer, @@ -576,6 +611,7 @@ fn write_deserialize_field_name( {indent} write!(formatter, "expected one of: {{:?}}", &FIELDS) {indent} }} +{indent} #[allow(unused_variables)] {indent} fn visit_str(self, value: &str) -> std::result::Result {indent} where {indent} E: serde::de::Error, @@ -594,17 +630,31 @@ fn write_deserialize_field_name( type_name )?; } + if ignore_unknown_fields { + writeln!( + writer, + "{}_ => Ok(GeneratedField::__SkipField__),", + Indent(indent + 5) + )?; + } else { + writeln!( + writer, + "{}_ => Err(serde::de::Error::unknown_field(value, FIELDS)),", + Indent(indent + 5) + )?; + } + writeln!(writer, "{}}}", Indent(indent + 4))?; + } else if ignore_unknown_fields { writeln!( writer, - "{}_ => Err(serde::de::Error::unknown_field(value, FIELDS)),", + "{}Ok(GeneratedField::__SkipField__)", Indent(indent + 5) )?; - writeln!(writer, "{}}}", Indent(indent + 4))?; } else { writeln!( writer, "{}Err(serde::de::Error::unknown_field(value, FIELDS))", - Indent(indent + 4) + Indent(indent + 5) )?; } @@ -623,6 +673,7 @@ fn write_fields_enum<'a, W: Write, I: Iterator>( writer: &mut W, indent: usize, fields: I, + ignore_unknown_fields: bool, ) -> Result<()> { writeln!( writer, @@ -633,6 +684,11 @@ fn write_fields_enum<'a, W: Write, I: Iterator>( for type_name in fields { writeln!(writer, "{}{},", Indent(indent + 1), type_name)?; } + + if ignore_unknown_fields { + writeln!(writer, "{}__SkipField__,", Indent(indent + 1))?; + } + writeln!(writer, "{}}}", Indent(indent)) } diff --git a/pbjson-build/src/lib.rs b/pbjson-build/src/lib.rs index 43a1a9f..1504035 100644 --- a/pbjson-build/src/lib.rs +++ b/pbjson-build/src/lib.rs @@ -103,6 +103,7 @@ pub struct Builder { out_dir: Option, extern_paths: Vec<(String, String)>, retain_enum_prefix: bool, + ignore_unknown_fields: bool, } impl Builder { @@ -161,6 +162,14 @@ impl Builder { self } + /// Don't error out in the presence of unknown fields when deserializing, + /// instead skip the field. + pub fn ignore_unknown_fields(&mut self) -> &mut Self { + self.ignore_unknown_fields = true; + + self + } + /// Generates code for all registered types where `prefixes` contains a prefix of /// the fully-qualified path of the type pub fn build>(&mut self, prefixes: &[S]) -> Result<()> { @@ -238,7 +247,7 @@ impl Builder { } Descriptor::Message(descriptor) => { if let Some(message) = resolve_message(&self.descriptors, descriptor) { - generate_message(&resolver, &message, writer)? + generate_message(&resolver, &message, writer, self.ignore_unknown_fields)? } } } diff --git a/pbjson-test/Cargo.toml b/pbjson-test/Cargo.toml index 5e8d76d..e42465e 100644 --- a/pbjson-test/Cargo.toml +++ b/pbjson-test/Cargo.toml @@ -12,6 +12,9 @@ pbjson = { path = "../pbjson" } pbjson-types = { path = "../pbjson-types" } serde = { version = "1.0", features = ["derive"] } +[features] +ignore-unknown-fields = [] + [dev-dependencies] chrono = "0.4" serde_json = "1.0" diff --git a/pbjson-test/build.rs b/pbjson-test/build.rs index f1b7621..2ba448b 100644 --- a/pbjson-test/build.rs +++ b/pbjson-test/build.rs @@ -26,11 +26,17 @@ fn main() -> Result<()> { .protoc_arg("--experimental_allow_proto3_optional") .compile_protos(&proto_files, &[root])?; - let descriptor_set = std::fs::read(descriptor_path)?; - pbjson_build::Builder::new() + let descriptor_set = std::fs::read(&descriptor_path)?; + let mut builder = pbjson_build::Builder::new(); + builder .register_descriptors(&descriptor_set)? - .extern_path(".test.external", "crate") - .build(&[".test"])?; + .extern_path(".test.external", "crate"); + + if cfg!(feature = "ignore-unknown-fields") { + builder.ignore_unknown_fields(); + } + + builder.build(&[".test"])?; Ok(()) } diff --git a/pbjson-test/src/lib.rs b/pbjson-test/src/lib.rs index 8a6640d..ea35f93 100644 --- a/pbjson-test/src/lib.rs +++ b/pbjson-test/src/lib.rs @@ -43,7 +43,8 @@ mod tests { use test::syntax3::*; #[test] - fn test_empty() { + #[cfg(not(feature = "ignore-unknown-fields"))] + fn test_unknown_field_error() { let message = Empty {}; let encoded = serde_json::to_string(&message).unwrap(); @@ -62,6 +63,18 @@ mod tests { ); } + #[test] + #[cfg(feature = "ignore-unknown-fields")] + fn test_ignore_unknown_field() { + let message = Empty {}; + + let encoded = serde_json::to_string(&message).unwrap(); + let _decoded: Empty = serde_json::from_str(&encoded).unwrap(); + + let empty = serde_json::from_str::("{\n \"foo\": \"bar\"\n}").unwrap(); + assert_eq!(empty, Empty {}); + } + #[test] fn test_kitchen_sink() { let mut decoded: KitchenSink = serde_json::from_str("{}").unwrap();