From 87f3788582a3809d1eb29fd6a54d7eb119ff6e28 Mon Sep 17 00:00:00 2001 From: Raphael Taylor-Davies Date: Fri, 30 Sep 2022 12:46:07 +0100 Subject: [PATCH 1/2] feat: support deserializing explicit nulls (#73) --- pbjson-build/src/generator/message.rs | 248 ++++++++++++++++---------- pbjson-test/src/lib.rs | 28 +++ 2 files changed, 180 insertions(+), 96 deletions(-) diff --git a/pbjson-build/src/generator/message.rs b/pbjson-build/src/generator/message.rs index 4fd13fd..094a2cc 100644 --- a/pbjson-build/src/generator/message.rs +++ b/pbjson-build/src/generator/message.rs @@ -753,151 +753,207 @@ fn write_deserialize_field( json_name )?; writeln!(writer, "{}}}", Indent(indent + 1))?; - write!(writer, "{}{}__ = Some(", Indent(indent + 1), field_name)?; + write!(writer, "{}{}__ = ", Indent(indent + 1), field_name)?; - if let Some(one_of) = one_of { - write!( - writer, - "{}::{}(", - resolver.rust_type(&one_of.path), - field.rust_type_name() - )?; - } - - match &field.field_type { - FieldType::Scalar(scalar) => { - write_encode_scalar_field(indent + 1, *scalar, field.field_modifier, writer)?; - } - FieldType::Enum(path) => match field.field_modifier { - FieldModifier::Repeated => { - write!( - writer, - "map.next_value::>()?.into_iter().map(|x| x as i32).collect()", - resolver.rust_type(path) - )?; - } - _ => { + match one_of { + Some(one_of) => match &field.field_type { + FieldType::Scalar(s) => match override_deserializer(*s) { + Some(deserializer) => { + write!( + writer, + "map.next_value::<::std::option::Option<{}>>()?.map(|x| {}::{}(x.0))", + deserializer, + resolver.rust_type(&one_of.path), + field.rust_type_name() + )?; + } + None => { + write!( + writer, + "map.next_value::<::std::option::Option<_>>()?.map({}::{})", + resolver.rust_type(&one_of.path), + field.rust_type_name() + )?; + } + }, + FieldType::Enum(path) => { write!( writer, - "map.next_value::<{}>()? as i32", - resolver.rust_type(path) + "map.next_value::<::std::option::Option<{}>>()?.map(|x| {}::{}(x as i32))", + resolver.rust_type(path), + resolver.rust_type(&one_of.path), + field.rust_type_name() )?; } + FieldType::Message(_) => writeln!( + writer, + "map.next_value::<::std::option::Option<_>>()?.map({}::{})", + resolver.rust_type(&one_of.path), + field.rust_type_name() + )?, + FieldType::Map(_, _) => unreachable!("one of cannot contain map fields"), }, - FieldType::Map(key, value) => { - writeln!(writer)?; - match btree_map { - true => write!( - writer, - "{}map.next_value:: write!( - writer, - "{}map.next_value:: { - panic!("protobuf disallows maps with floating point or bytes keys") + None => match &field.field_type { + FieldType::Scalar(scalar) => { + write_encode_scalar_field(indent + 1, *scalar, field.field_modifier, writer)?; + } + FieldType::Enum(path) => match field.field_modifier { + FieldModifier::Optional => { + write!( + writer, + "map.next_value::<::std::option::Option<{}>>()?.map(|x| x as i32)", + resolver.rust_type(path) + )?; } - _ if key.is_numeric() => { + FieldModifier::Repeated => { write!( writer, - "::pbjson::private::NumberDeserialize<{}>", - key.rust_type() + "Some(map.next_value::>()?.into_iter().map(|x| x as i32).collect())", + resolver.rust_type(path) )?; - "k.0" } _ => { - write!(writer, "_")?; - "k" - } - }; - write!(writer, ", ")?; - let map_v = match value.as_ref() { - FieldType::Scalar(scalar) if scalar.is_numeric() => { write!( writer, - "::pbjson::private::NumberDeserialize<{}>", - scalar.rust_type() + "Some(map.next_value::<{}>()? as i32)", + resolver.rust_type(path) )?; - "v.0" - } - FieldType::Scalar(ScalarType::Bytes) => { - write!(writer, "::pbjson::private::BytesDeserialize<_>",)?; - "v.0" } - FieldType::Enum(path) => { - write!(writer, "{}", resolver.rust_type(path))?; - "v as i32" - } - FieldType::Map(_, _) => panic!("protobuf disallows nested maps"), - _ => { - write!(writer, "_")?; - "v" + }, + FieldType::Map(key, value) => { + write!(writer, "Some(")?; + writeln!(writer)?; + match btree_map { + true => write!( + writer, + "{}map.next_value:: write!( + writer, + "{}map.next_value::>()?")?; - if map_k != "k" || map_v != "v" { - writeln!( - writer, - "{}.into_iter().map(|(k,v)| ({}, {})).collect()", - Indent(indent + 3), - map_k, - map_v, - )?; + let map_k = match key { + ScalarType::Bytes | ScalarType::F32 | ScalarType::F64 => { + panic!("protobuf disallows maps with floating point or bytes keys") + } + _ if key.is_numeric() => { + write!( + writer, + "::pbjson::private::NumberDeserialize<{}>", + key.rust_type() + )?; + "k.0" + } + _ => { + write!(writer, "_")?; + "k" + } + }; + write!(writer, ", ")?; + let map_v = match value.as_ref() { + FieldType::Scalar(scalar) if scalar.is_numeric() => { + write!( + writer, + "::pbjson::private::NumberDeserialize<{}>", + scalar.rust_type() + )?; + "v.0" + } + FieldType::Scalar(ScalarType::Bytes) => { + write!(writer, "::pbjson::private::BytesDeserialize<_>",)?; + "v.0" + } + FieldType::Enum(path) => { + write!(writer, "{}", resolver.rust_type(path))?; + "v as i32" + } + FieldType::Map(_, _) => panic!("protobuf disallows nested maps"), + _ => { + write!(writer, "_")?; + "v" + } + }; + + writeln!(writer, ">>()?")?; + if map_k != "k" || map_v != "v" { + writeln!( + writer, + "{}.into_iter().map(|(k,v)| ({}, {})).collect()", + Indent(indent + 3), + map_k, + map_v, + )?; + } + write!(writer, "{})", Indent(indent + 1))?; } - write!(writer, "{}", Indent(indent + 1))?; - } - _ => { - write!(writer, "map.next_value()?",)?; - } - }; - - if one_of.is_some() { - write!(writer, ")")?; + FieldType::Message(_) => { + write!(writer, "map.next_value()?")?; + } + }, } - - writeln!(writer, ");")?; + writeln!(writer, ";")?; writeln!(writer, "{}}}", Indent(indent)) } +fn override_deserializer(scalar: ScalarType) -> Option<&'static str> { + match scalar { + ScalarType::Bytes => Some("::pbjson::private::BytesDeserialize<_>"), + _ if scalar.is_numeric() => Some("::pbjson::private::NumberDeserialize<_>"), + _ => None, + } +} + fn write_encode_scalar_field( indent: usize, scalar: ScalarType, field_modifier: FieldModifier, writer: &mut W, ) -> Result<()> { - let deserializer = match scalar { - ScalarType::Bytes => "BytesDeserialize", - _ if scalar.is_numeric() => "NumberDeserialize", - _ => return write!(writer, "map.next_value()?",), + let deserializer = match override_deserializer(scalar) { + Some(deserializer) => deserializer, + None => { + return match field_modifier { + FieldModifier::Optional => { + write!(writer, "map.next_value()?") + } + _ => write!(writer, "Some(map.next_value()?)"), + } + } }; writeln!(writer)?; match field_modifier { + FieldModifier::Optional => { + writeln!( + writer, + "{}map.next_value::<::std::option::Option<{}>>()?.map(|x| x.0)", + Indent(indent + 1), + deserializer + )?; + } FieldModifier::Repeated => { writeln!( writer, - "{}map.next_value::>>()?", + "{}Some(map.next_value::>()?", Indent(indent + 1), deserializer )?; writeln!( writer, - "{}.into_iter().map(|x| x.0).collect()", + "{}.into_iter().map(|x| x.0).collect())", Indent(indent + 2) )?; } _ => { writeln!( writer, - "{}map.next_value::<::pbjson::private::{}<_>>()?.0", + "{}Some(map.next_value::<{}>()?.0)", Indent(indent + 1), deserializer )?; diff --git a/pbjson-test/src/lib.rs b/pbjson-test/src/lib.rs index f4a22a8..7c7c6c9 100644 --- a/pbjson-test/src/lib.rs +++ b/pbjson-test/src/lib.rs @@ -427,6 +427,34 @@ mod tests { decoded.string_value = None; verify(&decoded, r#"{}"#); + + // Test explicit null optional scalar + verify_decode(&decoded, r#"{"optionalU32":null}"#); + verify_decode(&decoded, r#"{"optionalU64":null}"#); + verify_decode(&decoded, r#"{"optionalString":null}"#); + + // Test explicit null optional enum + verify_decode(&decoded, r#"{"optionalValue":null}"#); + + // Test explicit null message + verify_decode(&decoded, r#"{"empty":null}"#); + + // Test explicit null in oneof + verify_decode(&decoded, r#"{"oneOfI32":null}"#); + verify_decode(&decoded, r#"{"oneOfBool":null}"#); + verify_decode(&decoded, r#"{"oneOfValue":null}"#); + verify_decode(&decoded, r#"{"oneOfMessage":null}"#); + + // Test explicit null value type + verify_decode(&decoded, r#"{"boolValue":null}"#); + verify_decode(&decoded, r#"{"bytesValue":null}"#); + verify_decode(&decoded, r#"{"doubleValue":null}"#); + verify_decode(&decoded, r#"{"floatValue":null}"#); + verify_decode(&decoded, r#"{"int32Value":null}"#); + verify_decode(&decoded, r#"{"int64Value":null}"#); + verify_decode(&decoded, r#"{"stringValue":null}"#); + verify_decode(&decoded, r#"{"uint32Value":null}"#); + verify_decode(&decoded, r#"{"uint64Value":null}"#); } #[test] From 7b0f22bfe59a9933cdecc1046a9259e9e1b7caa4 Mon Sep 17 00:00:00 2001 From: Raphael Taylor-Davies Date: Wed, 26 Oct 2022 17:24:41 +1300 Subject: [PATCH 2/2] chore: more tests --- pbjson-build/src/generator/message.rs | 20 ++++++---- pbjson-test/protos/syntax3.proto | 3 ++ pbjson-test/src/lib.rs | 55 +++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 8 deletions(-) diff --git a/pbjson-build/src/generator/message.rs b/pbjson-build/src/generator/message.rs index 33d1218..c7cb46e 100644 --- a/pbjson-build/src/generator/message.rs +++ b/pbjson-build/src/generator/message.rs @@ -358,7 +358,7 @@ fn write_serialize_scalar_variable( Indent(indent), field_name, variable.as_ref - ) + ); } }; @@ -611,9 +611,9 @@ fn write_deserialize_message( writeln!( writer, "{indent}{field}: {field}__.ok_or_else(|| serde::de::Error::missing_field(\"{json_name}\"))?,", - indent=Indent(indent + 3), - field= field.rust_field_name(), - json_name= field.json_name() + indent = Indent(indent + 3), + field = field.rust_field_name(), + json_name = field.json_name() )?; } FieldModifier::UseDefault | FieldModifier::Repeated => { @@ -967,9 +967,13 @@ fn write_deserialize_field( } write!(writer, "{})", Indent(indent + 1))?; } - FieldType::Message(_) => { - write!(writer, "map.next_value()?")?; - } + FieldType::Message(_) => match field.field_modifier { + FieldModifier::Repeated => { + // No explicit presence for repeated fields + write!(writer, "Some(map.next_value()?)")?; + } + _ => write!(writer, "map.next_value()?")?, + }, }, } writeln!(writer, ";")?; @@ -998,7 +1002,7 @@ fn write_encode_scalar_field( write!(writer, "map.next_value()?") } _ => write!(writer, "Some(map.next_value()?)"), - } + }; } }; diff --git a/pbjson-test/protos/syntax3.proto b/pbjson-test/protos/syntax3.proto index 6be284f..fec84a5 100644 --- a/pbjson-test/protos/syntax3.proto +++ b/pbjson-test/protos/syntax3.proto @@ -108,4 +108,7 @@ message KitchenSink { google.protobuf.StringValue string_value = 54; google.protobuf.UInt32Value uint32_value = 55; google.protobuf.UInt64Value uint64_value = 56; + + repeated google.protobuf.Int32Value repeated_int32_value = 57; + map map_int32_value = 58; } \ No newline at end of file diff --git a/pbjson-test/src/lib.rs b/pbjson-test/src/lib.rs index 5b98f54..a2f604a 100644 --- a/pbjson-test/src/lib.rs +++ b/pbjson-test/src/lib.rs @@ -28,14 +28,17 @@ pub mod test { include!(concat!(env!("OUT_DIR"), "/test.syntax3.rs")); include!(concat!(env!("OUT_DIR"), "/test.syntax3.serde.rs")); } + pub mod common { include!(concat!(env!("OUT_DIR"), "/test.common.rs")); include!(concat!(env!("OUT_DIR"), "/test.common.serde.rs")); } + pub mod duplicate_name { include!(concat!(env!("OUT_DIR"), "/test.duplicate_name.rs")); include!(concat!(env!("OUT_DIR"), "/test.duplicate_name.serde.rs")); } + pub mod escape { include!(concat!( env!("OUT_DIR"), @@ -80,6 +83,7 @@ mod tests { } } } + impl From<(&'static str, &'static str)> for EncodedStrings { fn from((expected, expected_preserved_proto): (&'static str, &'static str)) -> Self { EncodedStrings { @@ -132,6 +136,14 @@ mod tests { } } + fn verify_decode_err(encoded: &str, error: &str) { + let err = serde_json::from_str::(encoded) + .unwrap_err() + .to_string(); + + assert!(err.contains(error), "{}", err); + } + fn verify(decoded: &KitchenSink, expected: impl Into) { let expected = expected.into(); verify_encode(decoded, expected); @@ -690,6 +702,49 @@ mod tests { verify_decode(&decoded, r#"{"stringValue":null}"#); verify_decode(&decoded, r#"{"uint32Value":null}"#); verify_decode(&decoded, r#"{"uint64Value":null}"#); + + // Test primitives are not nullable + verify_decode_err(r#"{"i32":null}"#, "data did not match any variant"); + verify_decode_err(r#"{"u64":null}"#, "data did not match any variant"); + verify_decode_err(r#"{"value":null}"#, "invalid type: null"); + verify_decode_err(r#"{"bool":null}"#, "invalid type: null"); + verify_decode_err(r#"{"string":null}"#, "invalid type: null"); + + // Test lists are not nullable + verify_decode_err( + r#"{"repeatedI32":null}"#, + "invalid type: null, expected a sequence", + ); + verify_decode_err( + r#"{"repeatedI32":[null]}"#, + "data did not match any variant", + ); + verify_decode_err( + r#"{"repeatedInt32Value":null}"#, + "invalid type: null, expected a sequence", + ); + verify_decode_err( + r#"{"repeatedInt32Value":[null]}"#, + "data did not match any variant", + ); + + // Test maps are not nullable + verify_decode_err( + r#"{"stringDict":null}"#, + "invalid type: null, expected a map", + ); + verify_decode_err( + r#"{"stringDict": {"foo": null}}"#, + "invalid type: null, expected a string ", + ); + verify_decode_err( + r#"{"mapInt32Value":null}"#, + "invalid type: null, expected a map", + ); + verify_decode_err( + r#"{"mapInt32Value":{"foo": null}}"#, + "data did not match any variant", + ); } #[test]