Correct and optimize custom serializers for stringized values, hashes, nullable and optional fields #1351
Conversation
Borrow string slices from the deserializer instead of necessarily allocating. Let the de::Error::custom constructor handle the Display formatting of the errors.
To be correct with all serializers, the serialize function always serializes an Option<T>.
Borrow strings from the deserializer instead of allocating, serialize nullable values with `Serializer::serialize_some` to match what the deserializer expects.
Make it correct with regards to its own serialization format when None is serialized. Add a round trip unit test to prove it.
mzabaluev
added
enhancement
Codecov Report
@@ Coverage Diff @@
## main #1351 +/- ##
=======================================
+ Coverage 59.5% 60.4% +0.9%
=======================================
Files 272 272
Lines 26974 26544 -430
=======================================
- Hits 16068 16052 -16
+ Misses 10906 10492 -414
... and 12 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
If the deserializer cannot provide a borrowed string value, deserialize into an owned string and bridge the two cases with Cow.
The serialize function is changed to serialize as `Option<T>` and serialize `None` as `Some(T::default()). This makes the round-trip work with any serializers, without breaking the schema produced by serde_json.
mzabaluev
changed the title
| //! Deserialize to `None` if the received value equals the `Default` value. | ||
| //! Serialize `None` as `Some` with the `Default` value for `T`. |
There was a problem hiding this comment.
The behaviour of this helper is questionable, before or after this change. If the type has a reasonable default value, why make the domain type field an Option? And in the other direction, why uselessly serialize a default value when a None would do as well?
There was a problem hiding this comment.
Yes, It's kind of a head-scratcher!
I see this is derived on fields of certain proto types with Option<…> types (e.g. here). Perhaps, those types do not inherently need to be Option. Consequently, they required serde to handle Option this way.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Co-authored-by: Farhad Shabani <[email protected]>
The custom field serializers in
tendermintandtendermint-protohave a few problems.The modules where the deserializers expect the values to be an
Option(i.e. nullable) do not consistently useSerializer::serialize_somewhen serializing a non-null value. This is not an issue withserde_json, but as the serde framework is generic, this implementation will break with other serializers, particularly for non-self-describing formats.Deserializer functions unconditionally allocate temporary strings where they can potentially be borrowed from the deserializer, achieving zero copies from source data.
These changes correct the serde schema issues and introduce optimization by deserializing strings into
Cow, allowing use of borrowed string values from deserializers that support it..changelog/