Skip to content

Commit

Permalink
feat: Add flag to skip unknown fields instead of error
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jknovi authored and tustvold committed May 9, 2022
1 parent ce8cb7b commit d6454f8
Show file tree
Hide file tree
Showing 6 changed files with 103 additions and 13 deletions.
3 changes: 3 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
70 changes: 63 additions & 7 deletions pbjson-build/src/generator/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ pub fn generate_message<W: Write>(
resolver: &Resolver<'_>,
message: &Message,
writer: &mut W,
ignore_unknown_fields: bool,
) -> Result<()> {
let rust_type = resolver.rust_type(&message.path);

Expand All @@ -47,7 +48,14 @@ pub fn generate_message<W: Write>(

// 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(())
}
Expand Down Expand Up @@ -426,8 +434,9 @@ fn write_deserialize_message<W: Write>(
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))?;

Expand Down Expand Up @@ -486,14 +495,34 @@ fn write_deserialize_message<W: Write>(
}
}

if ignore_unknown_fields {
writeln!(
writer,
"{}GeneratedField::__SkipField__ => {{",
Indent(indent + 4),
)?;
writeln!(
writer,
"{}let _ = map.next_value::<serde::de::IgnoredAny>()?;",
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::<GeneratedField>()?.is_some() {{}}",
"{}while map.next_key::<GeneratedField>()?.is_some() {{",
Indent(indent + 2)
)?;
writeln!(
writer,
"{}let _ = map.next_value::<serde::de::IgnoredAny>()?;",
Indent(indent + 3)
)?;
writeln!(writer, "{}}}", Indent(indent + 2))?;
}

writeln!(writer, "{}Ok({} {{", Indent(indent + 2), rust_type)?;
Expand Down Expand Up @@ -551,14 +580,20 @@ fn write_deserialize_field_name<W: Write>(
indent: usize,
message: &Message,
writer: &mut W,
ignore_unknown_fields: bool,
) -> Result<()> {
let fields: Vec<_> = message
.all_fields()
.map(|field| (field.json_name(), field.rust_type_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,
Expand All @@ -576,6 +611,7 @@ fn write_deserialize_field_name<W: Write>(
{indent} write!(formatter, "expected one of: {{:?}}", &FIELDS)
{indent} }}
{indent} #[allow(unused_variables)]
{indent} fn visit_str<E>(self, value: &str) -> std::result::Result<GeneratedField, E>
{indent} where
{indent} E: serde::de::Error,
Expand All @@ -594,17 +630,31 @@ fn write_deserialize_field_name<W: Write>(
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)
)?;
}

Expand All @@ -623,6 +673,7 @@ fn write_fields_enum<'a, W: Write, I: Iterator<Item = &'a str>>(
writer: &mut W,
indent: usize,
fields: I,
ignore_unknown_fields: bool,
) -> Result<()> {
writeln!(
writer,
Expand All @@ -633,6 +684,11 @@ fn write_fields_enum<'a, W: Write, I: Iterator<Item = &'a str>>(
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))
}

Expand Down
11 changes: 10 additions & 1 deletion pbjson-build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ pub struct Builder {
out_dir: Option<PathBuf>,
extern_paths: Vec<(String, String)>,
retain_enum_prefix: bool,
ignore_unknown_fields: bool,
}

impl Builder {
Expand Down Expand Up @@ -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<S: AsRef<str>>(&mut self, prefixes: &[S]) -> Result<()> {
Expand Down Expand Up @@ -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)?
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions pbjson-test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
14 changes: 10 additions & 4 deletions pbjson-test/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
}
15 changes: 14 additions & 1 deletion pbjson-test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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::<Empty>("{\n \"foo\": \"bar\"\n}").unwrap();
assert_eq!(empty, Empty {});
}

#[test]
fn test_kitchen_sink() {
let mut decoded: KitchenSink = serde_json::from_str("{}").unwrap();
Expand Down

0 comments on commit d6454f8

Please sign in to comment.