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:[EMAIL PROTECTED] Sent: Sunday, September 07, 2008 10:34 PM To: [email protected] 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
