PookieBuns commented on code in PR #458:
URL: https://github.com/apache/avro-rs/pull/458#discussion_r2789323892
##########
avro/src/serde/de.rs:
##########
@@ -799,9 +850,19 @@ impl<'de> de::Deserializer<'de> for Deserializer<'de> {
Value::Record(fields) =>
visitor.visit_enum(EnumDeserializer::new(fields)),
Value::String(field) =>
visitor.visit_enum(EnumUnitDeserializer::new(field)),
Value::Union(idx, inner) => {
- if (*idx as usize) < variants.len() {
+ // if we came here from a some, we need to check if we are
deserializing a
+ // non-newtype enum
+ if self.deserializing_some
+ && let Value::Enum(_index, field) = inner.deref()
+ && variants.contains(&&**field)
+ {
+ return
visitor.visit_enum(EnumUnitDeserializer::new(field));
+ }
+ // Assume `null` is the first branch if deserializing some so
decrement the variant index
Review Comment:
Updated to use checked_sub to ensure underflow doesn't happen.
Regarding the topic about not being enforced by spec, even though it isn't
strictly enforced, it's highly recommended
https://avro.apache.org/docs/1.11.1/specification/#unions
> Note that when a default value is specified for a record field whose type
is a union, the type of the default value must match the first element of the
union. Thus, for unions containing “null”, the “null” is usually listed first,
since the default value of such unions is typically null.
For serialization we technically could get around this by resolving union
branches using variant name, but for deserialization cannot yet because it's
not schema aware and/or Value doesn't hold name information, which is why I
chose to use index bumping on serialization too for consistent behavior. This
seems like the closest we can get without a significant refactor or feature for
deserialization.
For now, the behavior will intentionally throw an error if you try to
(de)serialize an Option<T> with null not being the first branch. This doesn't
break existing behavior because the status quo is non-functional so although
not a perfect solution imo it's a step better than what we have now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
