feat: support deserializing explicit nulls (#73)#74
Conversation
| @@ -753,151 +753,207 @@ fn write_deserialize_field<W: Write>( | |||
| json_name | |||
There was a problem hiding this comment.
I recommend reviewing without whitespace
https://github.com/influxdata/pbjson/pull/74/files?w=1
| Some(deserializer) => { | ||
| write!( | ||
| writer, | ||
| "map.next_value::<::std::option::Option<{}>>()?.map(|x| {}::{}(x.0))", |
There was a problem hiding this comment.
This is the major "theme" of the change for optional fields rather than decoding T we decode to Option<T>
| _ => { | ||
| match one_of { | ||
| Some(one_of) => match &field.field_type { | ||
| FieldType::Scalar(s) => match override_deserializer(*s) { |
There was a problem hiding this comment.
Fields within a oneof can't have labels, and so we don't need to worry about repeated, etc...
crepererum
left a comment
crepererum
left a comment
There was a problem hiding this comment.
It's tested and I believe this will fly, but I have some minor nits (see comments).
| decoded.string_value = None; | ||
| verify(&decoded, r#"{}"#); | ||
|
|
||
| // Test explicit null optional scalar |
There was a problem hiding this comment.
The statefulness makes it a bit hard to read, because what is decoded at this point? Would be nice if decoded would be specified fully before every verify[_decode] block.
There was a problem hiding this comment.
The line above verifies that decoded is {}?
| verify_decode(&decoded, r#"{"stringValue":null}"#); | ||
| verify_decode(&decoded, r#"{"uint32Value":null}"#); | ||
| verify_decode(&decoded, r#"{"uint64Value":null}"#); | ||
| } |
There was a problem hiding this comment.
Is "List Elements are not Nullable" not tested?
| @@ -427,6 +427,34 @@ mod tests { | |||
|
|
|||
There was a problem hiding this comment.
I'm wondering if checking in the generated file for the test setup would be helpful, because reviewing a bunch of writeln! calls is a bit tricky (I have to admit that I have about zero mental context for each of the writes).
There was a problem hiding this comment.
The difficulty is we compile the tests with various different feature flags, to test various different builder options, so I'm not really sure how to achieve this without checking in 5 different versions of the code...
| writer, | ||
| "::pbjson::private::NumberDeserialize<{}>", | ||
| key.rust_type() | ||
| "Some(map.next_value::<Vec<{}>>()?.into_iter().map(|x| x as i32).collect())", |
There was a problem hiding this comment.
I really got confused about Vec<{}> and it took me a while to realize that {} is NOT Rust code, but placeholder for write!. I've said this below, but I think checking in the generated test code would help a lot.
There was a problem hiding this comment.
Also: you're really careful to use fully-qualified names for all types including Option. I wonder if this should also be done for Vec (::std::vec::Vec)?
2 participants
Closes #73
Adds support for explicit null values where the corresponding protobuf values are nullable.
It is worth highlighting that JSON supports greater nullability than protobuf, and this leads to a couple of places where
nullis still invalid.List Elements are not Nullable
Oneof nulls are not typed
Will get converted to
{foo: None}which loses the fact the null was encoded ona