Thrift
  1. Thrift
  2. THRIFT-33

Unset fields will still be serialized if they are primitives

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Labels:
      None

      Description

      Since primitive types (int, bool, byte, etc.) cannot be null in Java, the Java libraries will serialize "empty" values into their output. This is very bad when you have sparse Thrift structs with many possible fields and only one actually used.

      The Isset logic appears to be partially implemented, but is apparently unused. It seems like the required action is using getters and setters and honoring the isset information appropriately.

        Activity

        Hide
        Mark Slee added a comment -

        We originally did this to keep the library as lightweight as possible (i.e. to not require the use of get/set for all members). I agree that this is problematic when relying upon the isset feature to keep the structures lightweight over the wire – and would endorse changing this as such.

        This has actually already been implemented through the optional use of the -javabean generator flag. If you do this, then get/set methods, and an unset method, are generated for each field which keep the __isset up to date.

        Also, if you specify the "optional" keyword on these fields, the javabean generated code will NOT send them over the wire when they are not set. Take a look at this code in the generator:

        for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter) {
        bool null_allowed = type_can_be_null((*f_iter)->get_type());
        if (null_allowed) {
        out <<
        indent() << "if (this." << (*f_iter)->get_name() << " != null)

        {" << endl; indent_up(); }

        bool optional = bean_style_ && (*f_iter)->get_req() == t_field::T_OPTIONAL;
        if (optional) {
        out <<
        indent() << "if (this.__isset." << (*f_iter)->get_name() << ")

        {" << endl; }
        Show
        Mark Slee added a comment - We originally did this to keep the library as lightweight as possible (i.e. to not require the use of get/set for all members). I agree that this is problematic when relying upon the isset feature to keep the structures lightweight over the wire – and would endorse changing this as such. This has actually already been implemented through the optional use of the -javabean generator flag. If you do this, then get/set methods, and an unset method, are generated for each field which keep the __isset up to date. Also, if you specify the "optional" keyword on these fields, the javabean generated code will NOT send them over the wire when they are not set. Take a look at this code in the generator: for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter) { bool null_allowed = type_can_be_null((*f_iter)->get_type()); if (null_allowed) { out << indent() << "if (this." << (*f_iter)->get_name() << " != null) {" << endl; indent_up(); } bool optional = bean_style_ && (*f_iter)->get_req() == t_field::T_OPTIONAL; if (optional) { out << indent() << "if (this.__isset." << (*f_iter)->get_name() << ") {" << endl; }
        Hide
        Bryan Duxbury added a comment -

        Does this mean that we should just use the javabean generator if we want to avoid serializing primitives? That is, do we want the standard Java generated code NOT to be sparse with primitive types? This may just be a documentation issue at root, but still, I find it confusing what I get out of the box with the java vs javabean generator.

        Show
        Bryan Duxbury added a comment - Does this mean that we should just use the javabean generator if we want to avoid serializing primitives? That is, do we want the standard Java generated code NOT to be sparse with primitive types? This may just be a documentation issue at root, but still, I find it confusing what I get out of the box with the java vs javabean generator.
        Hide
        David Reiss added a comment -

        I think that the java (without beans) code should work the same way as C++: only respect __isset if the field is marked optional.

        Show
        David Reiss added a comment - I think that the java (without beans) code should work the same way as C++: only respect __isset if the field is marked optional.
        Hide
        Stuart Sierra added a comment -

        David said: I think that the java (without beans) code should work the same way as C++: only respect __isset if the field is marked optional.

        Does that mean that C++ and (non-bean) Java code requires manually keeping __isset up to date?

        Show
        Stuart Sierra added a comment - David said: I think that the java (without beans) code should work the same way as C++: only respect __isset if the field is marked optional. Does that mean that C++ and (non-bean) Java code requires manually keeping __isset up to date?
        Hide
        David Reiss added a comment -

        Yes. That is why we ignore __isset in those cases: we don't want users to have to maintain it manually.

        Show
        David Reiss added a comment - Yes. That is why we ignore __isset in those cases: we don't want users to have to maintain it manually.

          People

          • Assignee:
            Unassigned
            Reporter:
            Bryan Duxbury
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development