Thrift
  1. Thrift
  2. THRIFT-135

Nulls in set<string> throw an exception in Java

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: 0.3
    • Component/s: Java - Compiler
    • Labels:
      None

      Description

      From Amit Sudharshan:

      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.

        Issue Links

          Activity

          Hide
          David Reiss added a comment -

          I'm open to suggestions on how we should deal with this. My instinct is that we should not allow nulls in sets since they cannot be represented in C++ or PHP (though they can in Python). If we take that approach, I guess the choices are:

          1/ Continue throwing an exception.
          2/ Silently convert to "".
          3/ Silently drop from the set.

          Thoughts?

          Show
          David Reiss added a comment - I'm open to suggestions on how we should deal with this. My instinct is that we should not allow nulls in sets since they cannot be represented in C++ or PHP (though they can in Python). If we take that approach, I guess the choices are: 1/ Continue throwing an exception. 2/ Silently convert to "". 3/ Silently drop from the set. Thoughts?
          Hide
          Johan Stuyts added a comment -

          I prefer the first approach. I'd rather have the error be noticed as soon as possible instead of wondering what I did wrong and/or thinking Thrift does not work correctly.

          If the documentation does not cleary state that null is not allowed in collections it should be added.

          Show
          Johan Stuyts added a comment - I prefer the first approach. I'd rather have the error be noticed as soon as possible instead of wondering what I did wrong and/or thinking Thrift does not work correctly. If the documentation does not cleary state that null is not allowed in collections it should be added.
          Hide
          Chad Walters added a comment -

          I agree that #1 is best. Perhaps we should make sure this error condition throws a Thrift exception rather than a Java NPE, though.

          Show
          Chad Walters added a comment - I agree that #1 is best. Perhaps we should make sure this error condition throws a Thrift exception rather than a Java NPE, though.
          Hide
          Bryan Duxbury added a comment -

          How about we make a wrapper Set implementation that throws IllegalArgumentException if you try to add null, and use that as the implementation in generated structs?

          Show
          Bryan Duxbury added a comment - How about we make a wrapper Set implementation that throws IllegalArgumentException if you try to add null, and use that as the implementation in generated structs?
          Hide
          David Reiss added a comment -

          Normally we try to use the default collection types that the language provides to make Thrift feel more native. However, I think this is a bigger deal in C++ which uses nonvirtual methods. I think this would be okay in Java since the focus is more on the Set interface than any particular Set class. However, we should keep in mind that an application developer can put any Set implementation into a structure, so we would still have to do checking on serialization (unless we're okay with just throwing a null pointer exception).

          Show
          David Reiss added a comment - Normally we try to use the default collection types that the language provides to make Thrift feel more native. However, I think this is a bigger deal in C++ which uses nonvirtual methods. I think this would be okay in Java since the focus is more on the Set interface than any particular Set class. However, we should keep in mind that an application developer can put any Set implementation into a structure, so we would still have to do checking on serialization (unless we're okay with just throwing a null pointer exception).
          Hide
          Bryan Duxbury added a comment -

          I'm thinking that this is the right behavior.

          Show
          Bryan Duxbury added a comment - I'm thinking that this is the right behavior.

            People

            • Assignee:
              Unassigned
              Reporter:
              David Reiss
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development