[
https://issues.apache.org/jira/browse/THRIFT-735?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12875325#action_12875325
]
Jeff DeCew commented on THRIFT-735:
-----------------------------------
This seems odd / bad to me. When deserializing an enum with an unrecognized
value, as long as the field is optional it is set to null rather than causing
an exception. The same should be true of unions, which have the same "exactly
one of" semantic. So a union with an unrecognized value should be null, rather
than empty or always failing. Then, it is up to the context to decide if null
is an acceptable value.
It sounds like this patch is analogous to making MyEnum.findByValue() throw an
exception rather than return null for an unrecognized value. This means that
even if I declare my union optional, I will get protocol exceptions when I try
to expand the union to include new types, rather than just having the field set
to null. If we preserve the "exactly one" guarantee, we need to also ensure
that unions are still able to be used as flexible types. The logical way to do
this (in Java) is to set the field of type Union to null when optional, and
throw a protocol exception only when the union field is required (or when the
context says that null values are not allowed).
If unions were read with a static method, like enums are, then this would
probably be more intuitive.
{code:java|title=Reading a Struct}
if (field.type == TType.STRUCT) {
this.myField = new MyStruct();
this.myField.read(iprot);
} else {
TProtocolUtil.skip(iprot, field.type);
}
{code}
{code:java|title=Reading an Enum}
if (field.type == TType.I32) {
this.myField = MyEnum.findByValue(iprot.readI32());
} else {
TProtocolUtil.skip(iprot, field.type);
}
{code}
{code:java|title=Proposed Reading a Union}
if (field.type == TType.STRUCT) {
this.myField = MyUnion.staticRead(iprot);
} else {
TProtocolUtil.skip(iprot, field.type);
}
{code}
I don't know how this ports to reading into lists, sets, or maps or to reading
return values, but it makes sense that the behavior should match exactly with
enums for all those cases.
Once enums and unions are handled symmetrically, changes to their behavior
should be considered together.
> Required field checking is broken when the field type is a Union struct
> -----------------------------------------------------------------------
>
> Key: THRIFT-735
> URL: https://issues.apache.org/jira/browse/THRIFT-735
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (Java), Library (Java)
> Affects Versions: 0.2
> Reporter: Bryan Duxbury
> Assignee: Bryan Duxbury
> Fix For: 0.4
>
> Attachments: thrift-735-v2.patch, thrift-735.patch
>
>
> The validate() method on generated structs verifies that required fields are
> set after validation. However, if the type of the field is a Union struct,
> then just checking that the field isn't null is not a valid check. The value
> may be a non-null union, but have an unset field. (We encountered this when
> deserializing a type that had a union for a field, and the union's set value
> was an enum value that had been removed from the definition, making it a
> skip.)
> In order to perform the correct validation, if the value is a Union, then we
> must also check that the set field and value are non-null.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
