[thrift] problem with TBinaryProtocol.writeString

[Amit Sudharshan <am...@onellama.com>]

To All:
First off I'd like to introduce myself to the community. I am a project
manager of a small music startup which heavily utilizes thrift to connect
(and insulate) our PHP frontend folks from our Java backend recommendation
engineers.

I recently noticed a bug(feature?) in
com.facebook.thrift.protocol.TBinaryProtocol.writeString where if it is
passed a null pointer it will throw NPE.

Now, the autogenerated stub code tries to prevent this, however we recently
came across a case where we had a Set<String> which contained a "NULL"
(legal in java). Thrift tests to see if the set is non-null and implicitely
whether it has any elements, both of these pass in this case, and so the
null string is passed to the writeString method where we get the NPE.

If this is a bug, I can volunteer to fix it in most of the Thrift languages.
If it is a feature, I'd be happy with a one line explanation (we are coding
around this now).

-Amit
One Llama Media Inc.
http://www.onellama.com

Re: [thrift] problem with TBinaryProtocol.writeString

[Amit Sudharshan <am...@onellama.com>]

It seems that the general consensus is to immediately test for a null value
and throw a TProtocolException indicating that Null values/keys are not
allowed in collections .

On Tue, Sep 9, 2008 at 3:31 AM, Johan Stuyts <j....@zybber.nl> wrote:

> For this reason, my vote is that this is the correct behavior and we
>> should not support null values in containers.
>>
>
> I agree, and I also think it should be enforced in all languages. It is
> better to encounter limitations up front (even if your language does not
> have that limitation) instead of after lots of code has been written. For
> example, if a language that does not support null values in containers needs
> to access the services in the future, you won't be required to review all
> service implementations and modify the ones that put null values in
> containers.
>
> --
> Kind regards,
>
> Johan Stuyts
>

Re: [thrift] problem with TBinaryProtocol.writeString

[Johan Stuyts <j....@zybber.nl>]

> For this reason, my vote is that this is the correct behavior and we
> should not support null values in containers.

I agree, and I also think it should be enforced in all languages. It is  
better to encounter limitations up front (even if your language does not  
have that limitation) instead of after lots of code has been written. For  
example, if a language that does not support null values in containers  
needs to access the services in the future, you won't be required to  
review all service implementations and modify the ones that put null  
values in containers.

-- 
Kind regards,

Johan Stuyts

RE: [thrift] problem with TBinaryProtocol.writeString

[Mark Slee <ms...@facebook.com>]

Hmm, well, generally I think we should not be putting null elements into
containers, the reason being that this isn't supported cross-language.
For instance, in a C++ std::set, there is no way to place a null string
element into the set.

I think there are 2 courses of action here:
1/ Do what we do now -- throw an Exception when this happens (but
perhaps we should throw a more specific Thrift exception -- specifically
we should add a TProtocolException type for trying to serialize null
data fields)
2/ Treat null as equal to the empty string "" (this seems pretty risky
though...)

The Set case doesn't seem so bad, but the Map one is really problematic.
If you put a key in the map with a null value, it's not readily obvious
how we'd want to transport that over to C++. Would you expect to have
that key be present with an empty string? What if the data type is not a
string, but another object. Would we just default construct one? Or
would we remove the key from the map? Either of these seems to be
fundamentally changing the data in a nontrivial way.

For this reason, my vote is that this is the correct behavior and we
should not support null values in containers. We could strongly enforce
this by wrapping the container objects with checked subclasses, but I'd
rather not do that for the sake of simplicity and transparency.

Cheers,
Mark

-----Original Message-----
From: Amit Sudharshan [mailto:amit@onellama.com] 
Sent: Sunday, September 07, 2008 10:34 PM
To: thrift-dev@incubator.apache.org
Subject: [thrift] problem with TBinaryProtocol.writeString

To All:
First off I'd like to introduce myself to the community. I am a project
manager of a small music startup which heavily utilizes thrift to
connect (and insulate) our PHP frontend folks from our Java backend
recommendation engineers.

I recently noticed a bug(feature?) in
com.facebook.thrift.protocol.TBinaryProtocol.writeString where if it is
passed a null pointer it will throw NPE.

Now, the autogenerated stub code tries to prevent this, however we
recently came across a case where we had a Set<String> which contained a
"NULL"
(legal in java). Thrift tests to see if the set is non-null and
implicitely whether it has any elements, both of these pass in this
case, and so the null string is passed to the writeString method where
we get the NPE.

If this is a bug, I can volunteer to fix it in most of the Thrift
languages.
If it is a feature, I'd be happy with a one line explanation (we are
coding around this now).

-Amit
One Llama Media Inc.
http://www.onellama.com