Thrift
  1. Thrift
  2. THRIFT-1277

Node.js serializes false booleans as null

    Details

    • Patch Info:
      Patch Available

      Description

      When serializing boolean types, false booleans are serialized as null instead of false.

      1. nodejs_compiler_false_bools_loose.patch
        0.6 kB
        Hans Duedal
      2. nodejs_compiler_false_bools.patch
        0.6 kB
        Hans Duedal
      3. THRIFT-1277.patch
        9 kB
        Henrique Mendonça

        Activity

        Hans Duedal created issue -
        Hans Duedal made changes -
        Field Original Value New Value
        Attachment nodejs_compiler_false_bools.patch [ 12490792 ]
        Hans Duedal made changes -
        Issue Type Improvement [ 4 ] Bug [ 1 ]
        Hide
        Wade Simmons added a comment -

        This will serialize `undefined` as `true`. Perhaps use `!= null` instead which catches both `undefined` and `null`.

        Show
        Wade Simmons added a comment - This will serialize `undefined` as `true`. Perhaps use `!= null` instead which catches both `undefined` and `null`.
        Hide
        Hans Duedal added a comment - - edited

        But since all values are initialized with null during the "construct" will they ever be undefined? But it won't hurt to do loose compare anyway.

        Show
        Hans Duedal added a comment - - edited But since all values are initialized with null during the "construct" will they ever be undefined? But it won't hurt to do loose compare anyway.
        Hide
        Hans Duedal added a comment -

        Patch using loose compare instead

        Show
        Hans Duedal added a comment - Patch using loose compare instead
        Hans Duedal made changes -
        Attachment nodejs_compiler_false_bools_loose.patch [ 12490833 ]
        Bryan Duxbury made changes -
        Fix Version/s 0.8 [ 12316293 ]
        Fix Version/s 0.7 [ 12315360 ]
        Roger Meier made changes -
        Component/s Node.js - Compiler [ 12314319 ]
        Jake Farrell made changes -
        Fix Version/s 0.8 [ 12316293 ]
        Jake Farrell made changes -
        Fix Version/s 0.8 [ 12316293 ]
        Hide
        Jake Farrell added a comment -

        Committed

        Show
        Jake Farrell added a comment - Committed
        Jake Farrell made changes -
        Status Open [ 1 ] Closed [ 6 ]
        Assignee Hans Duedal [ cypres ]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in Thrift #317 (See https://builds.apache.org/job/Thrift/317/)
        Thrift-1277: Node.js serializes false booleans as null
        Client: js
        Patch: Hans Duedal

        When serializing boolean types, false booleans are serialized as null instead of false switching to using loose compare instead

        Show
        Hudson added a comment - Integrated in Thrift #317 (See https://builds.apache.org/job/Thrift/317/ ) Thrift-1277: Node.js serializes false booleans as null Client: js Patch: Hans Duedal When serializing boolean types, false booleans are serialized as null instead of false switching to using loose compare instead
        Hide
        Roger Meier added a comment -

        This breaks JavaScript jslint, see https://builds.apache.org/job/Thrift/317/console and unittest:

        unittest:
             [echo] Running Unit Tests with headless browser!
             [exec] Test testInsanity died, exception and test follows
             [exec] TypeError: Result of expression 'this.argument.write' [undefined] is not a function.
             [exec] function () {
             [exec]     var insanity = {
             [exec]       "1":{
             [exec]         "2":{
             [exec]           "userMap":{ "5":5, "8":8 },
             [exec]           "xtructs":[{
             [exec]               "string_thing":"Goodbye4",
             [exec]               "byte_thing":4,
             [exec]               "i32_thing":4,
             [exec]               "i64_thing":4
             [exec]             },
             [exec]             {
             [exec]               "string_thing":"Hello2",
             [exec]               "byte_thing":2,
             [exec]               "i32_thing":2,
             [exec]               "i64_thing":2
             [exec]             }
             [exec]           ]
             [exec]         },
             [exec]         "3":{
             [exec]           "userMap":{ "5":5, "8":8 },
             [exec]           "xtructs":[{
             [exec]               "string_thing":"Goodbye4",
             [exec]               "byte_thing":4,
             [exec]               "i32_thing":4,
             [exec]               "i64_thing":4
             [exec]             },
             [exec]             {
             [exec]               "string_thing":"Hello2",
             [exec]               "byte_thing":2,
             [exec]               "i32_thing":2,
             [exec]               "i64_thing":2
             [exec]             }
             [exec]           ]
             [exec]         }
             [exec]       },
             [exec]       "2":{ "6":{ "userMap":null, "xtructs":null } }
             [exec]     };
             [exec]     var res = client.testInsanity("");
             [exec]     ok(res, JSON.stringify(res));
             [exec]     ok(insanity, JSON.stringify(insanity));
             [exec]
             [exec]     checkRecursively(res, insanity);
             [exec]   }
             [exec] 'waitFor()' finished in 1644ms.
             [exec] Tests completed in 1654 milliseconds.
             [exec] 66 tests of 67 passed, 1 failed.
             [java] Timeout: killed the sub-process
             [java] Java Result: -1
        
        Show
        Roger Meier added a comment - This breaks JavaScript jslint, see https://builds.apache.org/job/Thrift/317/console and unittest: unittest: [echo] Running Unit Tests with headless browser! [exec] Test testInsanity died, exception and test follows [exec] TypeError: Result of expression 'this.argument.write' [undefined] is not a function. [exec] function () { [exec] var insanity = { [exec] "1":{ [exec] "2":{ [exec] "userMap":{ "5":5, "8":8 }, [exec] "xtructs":[{ [exec] "string_thing":"Goodbye4", [exec] "byte_thing":4, [exec] "i32_thing":4, [exec] "i64_thing":4 [exec] }, [exec] { [exec] "string_thing":"Hello2", [exec] "byte_thing":2, [exec] "i32_thing":2, [exec] "i64_thing":2 [exec] } [exec] ] [exec] }, [exec] "3":{ [exec] "userMap":{ "5":5, "8":8 }, [exec] "xtructs":[{ [exec] "string_thing":"Goodbye4", [exec] "byte_thing":4, [exec] "i32_thing":4, [exec] "i64_thing":4 [exec] }, [exec] { [exec] "string_thing":"Hello2", [exec] "byte_thing":2, [exec] "i32_thing":2, [exec] "i64_thing":2 [exec] } [exec] ] [exec] } [exec] }, [exec] "2":{ "6":{ "userMap":null, "xtructs":null } } [exec] }; [exec] var res = client.testInsanity(""); [exec] ok(res, JSON.stringify(res)); [exec] ok(insanity, JSON.stringify(insanity)); [exec] [exec] checkRecursively(res, insanity); [exec] } [exec] 'waitFor()' finished in 1644ms. [exec] Tests completed in 1654 milliseconds. [exec] 66 tests of 67 passed, 1 failed. [java] Timeout: killed the sub-process [java] Java Result: -1
        Roger Meier made changes -
        Resolution Fixed [ 1 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Hide
        Roger Meier added a comment -

        revert the changes

        Show
        Roger Meier added a comment - revert the changes
        Hide
        Hudson added a comment -

        Integrated in Thrift #318 (See https://builds.apache.org/job/Thrift/318/)
        THRIFT-1277 Node.js serializes false booleans as null
        revert patch => breaks js

        roger : http://svn.apache.org/viewvc/?view=rev&rev=1198334
        Files :

        • /thrift/trunk/compiler/cpp/src/generate/t_js_generator.cc
        Show
        Hudson added a comment - Integrated in Thrift #318 (See https://builds.apache.org/job/Thrift/318/ ) THRIFT-1277 Node.js serializes false booleans as null revert patch => breaks js roger : http://svn.apache.org/viewvc/?view=rev&rev=1198334 Files : /thrift/trunk/compiler/cpp/src/generate/t_js_generator.cc
        Jake Farrell made changes -
        Fix Version/s 0.9 [ 12316294 ]
        Fix Version/s 0.8 [ 12316293 ]
        Hide
        Christoph Tavan added a comment - - edited

        What was wrong with the original patch? As far as I can see all parameters get initialized to null or some default value in the constructor. The constructor then overwrites these defaults only if

        if (args.varname !== undefined) {
        }
        

        Consequently we cannot get properties that will be undefined. So we cannot have the case mentioned by Wade. I think we should do strict checking for null.

        It would be great if the strict checking patch could be applied again since it will also not break jslint.

        The lack of correct null checking however breaks quite a lot of stuff (like THRIFT-1297).

        Show
        Christoph Tavan added a comment - - edited What was wrong with the original patch? As far as I can see all parameters get initialized to null or some default value in the constructor. The constructor then overwrites these defaults only if if (args.varname !== undefined) { } Consequently we cannot get properties that will be undefined. So we cannot have the case mentioned by Wade. I think we should do strict checking for null. It would be great if the strict checking patch could be applied again since it will also not break jslint. The lack of correct null checking however breaks quite a lot of stuff (like THRIFT-1297 ).
        Hide
        Henrique Mendonça added a comment -

        Hi Christoph and Roger
        I think the loose option is a little more robust. Otherwise it would break if someone sets a property to "undefined".
        a jslint friendly version would be something like:

            out << indent() << "if (this." << (*f_iter)->get_name() << " !== null && this." << (*f_iter)->get_name() << " !== undefined) {" << endl;
        

        I can provide a patch if you prefer, and agree

        Show
        Henrique Mendonça added a comment - Hi Christoph and Roger I think the loose option is a little more robust. Otherwise it would break if someone sets a property to "undefined". a jslint friendly version would be something like: out << indent() << " if ( this ." << (*f_iter)->get_name() << " !== null && this ." << (*f_iter)->get_name() << " !== undefined) {" << endl; I can provide a patch if you prefer, and agree
        Hide
        Christoph Tavan added a comment -

        Henrique, I'm perfectly OK with your suggestion and it would be great if you could provide a patch!

        Show
        Christoph Tavan added a comment - Henrique, I'm perfectly OK with your suggestion and it would be great if you could provide a patch!
        Hide
        Roger Meier added a comment -

        Yes, Henrique please do so!

        Thanks,
        roger

        Show
        Roger Meier added a comment - Yes, Henrique please do so! Thanks, roger
        Hide
        Henrique Mendonça added a comment -

        A little delayed but here it is.
        THRIFT-1277.patch
        it also fixes depreciated qunit code (qunit trunk) and a little bug on test.js, an empty string was given as argument in testInsanity

        Show
        Henrique Mendonça added a comment - A little delayed but here it is. THRIFT-1277 .patch it also fixes depreciated qunit code (qunit trunk) and a little bug on test.js, an empty string was given as argument in testInsanity
        Henrique Mendonça made changes -
        Attachment THRIFT-1277.patch [ 12522701 ]
        Hide
        Christoph Tavan added a comment -

        Cool!

        +1 from my side for getting this patch merged to trunk! Thanks a lot, Henrique.

        Show
        Christoph Tavan added a comment - Cool! +1 from my side for getting this patch merged to trunk! Thanks a lot, Henrique.
        Hide
        Roger Meier added a comment -

        committed!
        thanks henrique
        ;-r

        Show
        Roger Meier added a comment - committed! thanks henrique ;-r
        Roger Meier made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Assignee Hans Duedal [ cypres ] Henrique Mendonca [ henrique ]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in Thrift #440 (See https://builds.apache.org/job/Thrift/440/)
        THRIFT-1277 Node.js serializes false booleans as null
        Patch: Henrique Mendonca (Revision 1326371)

        Result = SUCCESS
        roger : http://svn.apache.org/viewvc/?view=rev&rev=1326371
        Files :

        • /thrift/trunk/compiler/cpp/src/generate/t_js_generator.cc
        • /thrift/trunk/lib/js/test/test.js
        Show
        Hudson added a comment - Integrated in Thrift #440 (See https://builds.apache.org/job/Thrift/440/ ) THRIFT-1277 Node.js serializes false booleans as null Patch: Henrique Mendonca (Revision 1326371) Result = SUCCESS roger : http://svn.apache.org/viewvc/?view=rev&rev=1326371 Files : /thrift/trunk/compiler/cpp/src/generate/t_js_generator.cc /thrift/trunk/lib/js/test/test.js
        Jake Farrell made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Henrique Mendonça
            Reporter:
            Hans Duedal
          • Votes:
            2 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development