[ 
https://issues.apache.org/jira/browse/THRIFT-735?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12875783#action_12875783
 ] 

Jeff DeCew commented on THRIFT-735:
-----------------------------------

I think it is a great point, but I ask: what do we do in the same case with 
enums?
If we can find satisfactory solutions to all these problems for enums, the same 
solutions will work just fine for Unions.
If we can't find the solutions for enums then maybe enums should be stored as 
integers, and unions should always be allowed to be empty.

Getting back to my question, it looks like the deserialization leaves unknown 
enums values as null in lists.  This means that the data can't be immediately 
reserialized.  Certainly, this is preferable to receiving an exception on 
deserialization and not even getting the list.  I agree with David, leaving 
null entries is better than shortening the list.  However, if we did shorten 
the list, we could also (intuitively) shorten sets and maps (rather than 
throwing NPE).

But all this complexity is weighed against simply changing the definition of a 
union such that it is allowed to be serialized and deserialized with no value 
at all.  What was the argument against that again? We don't want a Null type in 
thrift?  Isn't that what an empty struct is?  I still think it's worth trying 
to make unions like enums rather than structs, but if things get much messier, 
I might have to change my mind.

> 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.

Reply via email to