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

Bryan Duxbury commented on THRIFT-735:
--------------------------------------

bq. How does that work with containers?
That depends on the container. Generally, we can always read the list itself, 
since we read the type header and size. However, if some of the values are 
invalid, things get a little icky. In a list, you'd get one or more null 
fields. In a set, you'd get exactly one null field, thus reducing the size 
reported on the wire. In a map, if the key was null, then the entire entry 
would go away (at least in java, which doesn't allow null in HashMap keys); if 
the value was null, only that part of the entry would be null.

bq. The idea of an annotation on enum and union types that indicates whether an 
unexpected value should trigger a validation error is appealing to me.
We already have this annotation - it's called "required". If it's an unexpected 
value, you'll get null back, and if that field is required, that's a validation 
error. If you don't want the validation error, don't make the field required. 
Can you imagine a scenario where you'd want a field of type enum to be required 
yet not care if the value is recognizable?

I don't think that "unset" and "unexpected value" have a meaningful difference 
to any user of Thrift, since in either case the "value" will just appear to be 
a null. If we treat them differently, then perhaps "unset" could be determined 
whether or not the value is null. However, I don't really know how you would 
make use of this new feature other than in debugging a null value that occurred 
unexpectedly. Also, trying to achieve this duality would eliminate some 
optimizations we've made to the memory footprint of structs. 

I should also point out that in Java, our enums are actual enums, not ints. 
This means that if we don't recognize the value's id on the wire, we literally 
have no way other than null to represent that it was unrecognized.

> 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.3
>
>         Attachments: 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