Thrift
  1. Thrift
  2. THRIFT-815

Deserialization of lists is critically broken.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.4
    • Fix Version/s: 0.4
    • Component/s: JavaScript - Compiler
    • Labels:
      None

      Description

      Edit the test code that comes with the js language target:

      var list = [1,2,3];
      var ret = client.testList(list);
      debugger;

      ret comes back as [3,3,3] when it should be echoed back as [1,2,3]
      The test case never caught this because it only verified the size, and not the contents of the returned array.
      I cannot find an immediate workaround, but I will try wrapping it in a dummy type like ListServiceResponse.

      1. 813-1.patch
        1 kB
        T Jake Luciani
      2. test.html
        8 kB
        Roger Meier
      3. THRIFT_815_TestFramework_test.html_json2.patch
        30 kB
        Roger Meier
      4. THRIFT-815_thrift.js.patch
        1 kB
        Roger Meier

        Activity

        Hide
        Jordan added a comment -

        Is anyone interested in fixing this? I'm afraid I don't know much about the code base. I'd love to help fix this if anyone can provide me some direction.

        Show
        Jordan added a comment - Is anyone interested in fixing this? I'm afraid I don't know much about the code base. I'd love to help fix this if anyone can provide me some direction.
        Hide
        Bryan Duxbury added a comment -

        Well for starters, you should amend the test to check not just the length but the contents. From there, I would look into the protocol implementation for bugs. I'm neither a Thrift-JS contributor nor user, so I can't provide much more detail than that. If you find yourself with questions about the architecture of Thrift in general, feel free to toss questions up on the dev mailing list or find me in #thrift on freenode.

        Show
        Bryan Duxbury added a comment - Well for starters, you should amend the test to check not just the length but the contents. From there, I would look into the protocol implementation for bugs. I'm neither a Thrift-JS contributor nor user, so I can't provide much more detail than that. If you find yourself with questions about the architecture of Thrift in general, feel free to toss questions up on the dev mailing list or find me in #thrift on freenode.
        Hide
        T Jake Luciani added a comment -

        Hi Jordan,

        This is most likely a bug on the javascript protocol side. if you step through the de-serialization in a browser I'm sure you'll see the issue.

        Sorry I'm unable to help out more at the moment.

        -Jake

        Show
        T Jake Luciani added a comment - Hi Jordan, This is most likely a bug on the javascript protocol side. if you step through the de-serialization in a browser I'm sure you'll see the issue. Sorry I'm unable to help out more at the moment. -Jake
        Hide
        Jordan added a comment -

        Jake,
        I'll try to track down where this is occurring exactly to provide enough info to pinpoint where the problem is coming from.

        Show
        Jordan added a comment - Jake, I'll try to track down where this is occurring exactly to provide enough info to pinpoint where the problem is coming from.
        Hide
        Jordan added a comment -

        Jake,

        I stepped through the browser using firefox, and I could see that the first element from a list is obtained correctly and the second one is incorrect. I'm not sure what is causing it. There's all sorts of crazy variable names such as rstack, rpos, r, fid, ftype that have no explanation or documentation. I've exhausted a couple hours trying to see what's going on, and I can't seem to make sense of the code. Can you take a look this weekend? I think that this is the most critical bug so far in the js-compiler.

        Show
        Jordan added a comment - Jake, I stepped through the browser using firefox, and I could see that the first element from a list is obtained correctly and the second one is incorrect. I'm not sure what is causing it. There's all sorts of crazy variable names such as rstack, rpos, r, fid, ftype that have no explanation or documentation. I've exhausted a couple hours trying to see what's going on, and I can't seem to make sense of the code. Can you take a look this weekend? I think that this is the most critical bug so far in the js-compiler.
        Hide
        T Jake Luciani added a comment -

        Fixed, thanks for finding this.

        Show
        T Jake Luciani added a comment - Fixed, thanks for finding this.
        Hide
        Jordan added a comment - - edited

        Thanks Jake, but I don't think that this works when the things in the list contains objects.

        I changed the testList method to accept a list of LoginResponse objects (that's one of my types) and tried the following code example:
        On the server side, it should just return the same list, as before but now the list contains objects.

        var arr = new Array();
        var lr = new TheService.LoginResponse(

        {validCredentials:true, sessionId:"asdf"}

        );
        arr.push(lr);
        lr = new TheService.LoginResponse(

        {validCredentials:false, sessionId:"ccc"}

        );
        arr.push(lr);
        lr = new TheService.LoginResponse(

        {validCredentials:false, sessionId:"KKLK"}

        );
        arr.push(lr);
        var ret = client.testList(arr);
        debugger;

        What I observe is that I get an empty (everything set to false etc.) first object, the second object is the last object of the input list* and everything else is empty (everything set to false/0/etc).

        Show
        Jordan added a comment - - edited Thanks Jake, but I don't think that this works when the things in the list contains objects. I changed the testList method to accept a list of LoginResponse objects (that's one of my types) and tried the following code example: On the server side, it should just return the same list, as before but now the list contains objects. var arr = new Array(); var lr = new TheService.LoginResponse( {validCredentials:true, sessionId:"asdf"} ); arr.push(lr); lr = new TheService.LoginResponse( {validCredentials:false, sessionId:"ccc"} ); arr.push(lr); lr = new TheService.LoginResponse( {validCredentials:false, sessionId:"KKLK"} ); arr.push(lr); var ret = client.testList(arr); debugger; What I observe is that I get an empty (everything set to false etc.) first object, the second object is the last object of the input list* and everything else is empty (everything set to false/0/etc).
        Hide
        Jordan added a comment -

        See my last comment. (I think the provided fix won't fix the case when lists contain objects. )

        Show
        Jordan added a comment - See my last comment. (I think the provided fix won't fix the case when lists contain objects. )
        Hide
        T Jake Luciani added a comment -

        hmm, could you provide a testcase?

        Show
        T Jake Luciani added a comment - hmm, could you provide a testcase?
        Hide
        Jordan added a comment -

        Yes, but it will involve actually changing the service to include a method which accepts and returns a list of objects.
        Do you just use diff to make the patches?

        Show
        Jordan added a comment - Yes, but it will involve actually changing the service to include a method which accepts and returns a list of objects. Do you just use diff to make the patches?
        Hide
        T Jake Luciani added a comment -

        yeah, or you can just attach a tar of the test dir. thanks

        Show
        T Jake Luciani added a comment - yeah, or you can just attach a tar of the test dir. thanks
        Hide
        Roger Meier added a comment -

        The patch fixes the list type!

        attached is a qunit based test, that also contains utf-8 string tests, etc, not complete yet but it might be a starting point.

        Currently I use test/cpp/srcTestServer.cpp using the C++ HttpServer, provided by THRIFT-247

        https://svn.apache.org/repos/asf/incubator/thrift/trunk/test/ThriftTest.thrift
        I'm not sure if all supported Languages are doing the same within UnitTests....
        e.g echo a list, etc.

        Show
        Roger Meier added a comment - The patch fixes the list type! attached is a qunit based test, that also contains utf-8 string tests, etc, not complete yet but it might be a starting point. Currently I use test/cpp/srcTestServer.cpp using the C++ HttpServer, provided by THRIFT-247 https://svn.apache.org/repos/asf/incubator/thrift/trunk/test/ThriftTest.thrift I'm not sure if all supported Languages are doing the same within UnitTests.... e.g echo a list, etc.
        Hide
        Roger Meier added a comment - - edited

        The patch THRIFT-815_thrift.js.patch fixes wrong order on wire:

        detected with the Unit Test provided above and Firebug

        var list = new Array(1,2,3)
            test("List", function() {
              ok(client.testList(list), list);
            });
        

        Wire:

        out: [1,"testList",1,0,{"1":{"lst":["i32",3,3,2,1]}}]
        in:   [1,"testList",2,0,{"0":{"lst":["i32",3,3,2,1]}}]
        

        With that patch:

        out: [1,"testList",1,0,{"1":{"lst":["i32",3,1,2,3]}}]
        in:   [1,"testList",2,0,{"0":{"lst":["i32",3,1,2,3]}}]
        
        Show
        Roger Meier added a comment - - edited The patch THRIFT-815_thrift.js.patch fixes wrong order on wire: detected with the Unit Test provided above and Firebug var list = new Array(1,2,3) test("List", function () { ok(client.testList(list), list); }); Wire: out: [1, "testList" ,1,0,{ "1" :{ "lst" :[ "i32" ,3,3,2,1]}}] in: [1, "testList" ,2,0,{ "0" :{ "lst" :[ "i32" ,3,3,2,1]}}] With that patch: out: [1, "testList" ,1,0,{ "1" :{ "lst" :[ "i32" ,3,1,2,3]}}] in: [1, "testList" ,2,0,{ "0" :{ "lst" :[ "i32" ,3,1,2,3]}}]
        Hide
        T Jake Luciani added a comment -

        Cool! this test framework you added is nice.

        I've got a number of other tests failing, let me tinker with this a bit... Thanks

        Show
        T Jake Luciani added a comment - Cool! this test framework you added is nice. I've got a number of other tests failing, let me tinker with this a bit... Thanks
        Hide
        Jordan added a comment -

        If you're adding test cases, don't forget the null being translated into dummy objects:
        https://issues.apache.org/jira/browse/THRIFT-816

        Show
        Jordan added a comment - If you're adding test cases, don't forget the null being translated into dummy objects: https://issues.apache.org/jira/browse/THRIFT-816
        Hide
        Jordan added a comment -

        I just tested objects that are declared as "Set"s of objects, and I experience the same problem. The fix doesn't work for Sets of complex (non-primitive) objects either.
        The symptoms are exactly the same as with Lists of objects.

        Show
        Jordan added a comment - I just tested objects that are declared as "Set"s of objects, and I experience the same problem. The fix doesn't work for Sets of complex (non-primitive) objects either. The symptoms are exactly the same as with Lists of objects.
        Hide
        Jordan added a comment -

        For the record, I've applied both patches, and, while it may fix the order and primitive objects, it doesn't seem to work for lists/sets that contain objects (as defined in the thrift models.)
        For instance, if one of my operations returns a list<i32> it works fine, but if I change the service to return a list<Car> it will not work.
        I just wanted to clarify that I had tried both patches. I will look some more into the code, but I'm not very optimistic about being able to understand what's going on in thrift.js without some simple documentation.

        Show
        Jordan added a comment - For the record, I've applied both patches, and, while it may fix the order and primitive objects, it doesn't seem to work for lists/sets that contain objects (as defined in the thrift models.) For instance, if one of my operations returns a list<i32> it works fine, but if I change the service to return a list<Car> it will not work. I just wanted to clarify that I had tried both patches. I will look some more into the code, but I'm not very optimistic about being able to understand what's going on in thrift.js without some simple documentation.
        Hide
        Jordan added a comment - - edited

        I don't know what's supposed to happen, but around line 510-530 in thrift.js seems wrong. When we read a list, we delete an element off of the front of the list, and pop it on top of the rstack, then around the the following part of the code, we treat the object that we just popped on top of the stack, as if it were a field of an object itself. It is NOT. It is an object, not a field of an object. So things are failing because of this, and I'm not sure why we were popping an object on top in the first place. Can someone look at this?

        Bar.java
        
                    //remove so we don't see it again
                    delete this.rstack[this.rstack.length-1][fid]
        
                    this.rstack.push(field)      
        
                    break
                }
        
                if(fid != -1){ 
        
                    //should only be 1 of these but this is the only
                    //way to match a key
                    for(var f in (this.rstack[this.rstack.length-1])){
                        if(Thrift.Protocol.RType[f] == null ) continue;
        
                        ftype = Thrift.Protocol.RType[f];
        
                        this.rstack[this.rstack.length-1] = this.rstack[this.rstack.length-1][f];
        
                    }
                }
        
        Show
        Jordan added a comment - - edited I don't know what's supposed to happen, but around line 510-530 in thrift.js seems wrong. When we read a list, we delete an element off of the front of the list, and pop it on top of the rstack, then around the the following part of the code, we treat the object that we just popped on top of the stack, as if it were a field of an object itself. It is NOT. It is an object, not a field of an object. So things are failing because of this, and I'm not sure why we were popping an object on top in the first place. Can someone look at this? Bar.java //remove so we don't see it again delete this .rstack[ this .rstack.length-1][fid] this .rstack.push(field) break } if (fid != -1){ //should only be 1 of these but this is the only //way to match a key for ( var f in ( this .rstack[ this .rstack.length-1])){ if (Thrift.Protocol.RType[f] == null ) continue ; ftype = Thrift.Protocol.RType[f]; this .rstack[ this .rstack.length-1] = this .rstack[ this .rstack.length-1][f]; } }
        Hide
        Roger Meier added a comment -

        Hi Jordan!
        code snippets? just use the question mark and preview function on the right side of the input box to get help on formating text

        Cool, you did some tests with both patches and they work! What do you think about the QUnit based test? Is there another framework you would recommend?

        @Jake:
        Could you commit the patch THRIFT-815_thrift.js.patch , that fixes the wire order? ... I would like to commit, but I can't....
        I would appreciate if you could commit the JavaScript test.html with the Unit tests as well.
        This might be a place where Jordan's likes to contribute?

        Adding test cases for all defined functions at ThriftTest.thrift is an important thing, for finding bugs inside the List<Object> serialization and other.

        Another BIG question is the server side for the tests, I use test/cpp/srcTestServer.cpp using the C++ HttpServer, provided by THRIFT-247 (please commit...). It seems, that many different tests for several Languages exist, but they do different things inside and are therefore not useful for interacting with eachother.....I think this is an important thing to standardize.

        THRIFT-35 mentions that all language tests have to be moved into their appropriate library directory, this will cleanup the test directory and it can be used for describing standard tests cases, that have to be implemented in every language.

        Show
        Roger Meier added a comment - Hi Jordan! code snippets? just use the question mark and preview function on the right side of the input box to get help on formating text Cool, you did some tests with both patches and they work! What do you think about the QUnit based test? Is there another framework you would recommend? @Jake: Could you commit the patch THRIFT-815_thrift.js.patch , that fixes the wire order? ... I would like to commit, but I can't.... I would appreciate if you could commit the JavaScript test.html with the Unit tests as well. This might be a place where Jordan's likes to contribute? Adding test cases for all defined functions at ThriftTest.thrift is an important thing, for finding bugs inside the List<Object> serialization and other. Another BIG question is the server side for the tests, I use test/cpp/srcTestServer.cpp using the C++ HttpServer, provided by THRIFT-247 (please commit...). It seems, that many different tests for several Languages exist, but they do different things inside and are therefore not useful for interacting with eachother.....I think this is an important thing to standardize. THRIFT-35 mentions that all language tests have to be moved into their appropriate library directory, this will cleanup the test directory and it can be used for describing standard tests cases, that have to be implemented in every language.
        Hide
        Jordan added a comment -

        Roger,

        Thanks for the tips on formatting.
        You are correct that I tried both patches, but they do not fix the major problem. Do you think we should wait until we figure out what's going wrong and fix it before submitting the patch?

        The patches do not fix lists of objects and sets of objects. My previous comment was the result of me stepping though the code in firebug.

        Show
        Jordan added a comment - Roger, Thanks for the tips on formatting. You are correct that I tried both patches, but they do not fix the major problem. Do you think we should wait until we figure out what's going wrong and fix it before submitting the patch? The patches do not fix lists of objects and sets of objects. My previous comment was the result of me stepping though the code in firebug.
        Hide
        Roger Meier added a comment -

        Jordan,

        my philosophy would be: Every thing that goes into the right direction and fixes bugs should be commited.
        Now Lists with simple types work properly with these patches. => That's a good thing!

        I really do not like to work with a local code base, that contains many patches! see my svn status

        M      test/cpp/Thrift-test.mk
        M      test/cpp/src/TestClient.cpp
        M      test/cpp/src/main.cpp
        M      test/cpp/src/TestServer.cpp
        M      lib/cpp/src/protocol/TProtocol.h
        A      lib/cpp/src/transport/THttpServer.h
        M      lib/cpp/src/transport/THttpClient.cpp
        M      lib/cpp/src/transport/THttpClient.h
        A      lib/cpp/src/transport/THttpTransport.cpp
        A      lib/cpp/src/transport/THttpTransport.h
        A      lib/cpp/src/transport/THttpServer.cpp
        M      lib/cpp/Makefile.am
        M      lib/js/test/test.html
        M      lib/js/thrift.js
        M      compiler/cpp/src/generate/t_js_generator.cc
        

        We should proceed with enhancing the Test Framework to have a good basis for finding the pending bug's.

        I'm especially interested in C++ and JavaScript bindings and I would like to help fixing bugs and enhancing Thrift!

        Show
        Roger Meier added a comment - Jordan, my philosophy would be: Every thing that goes into the right direction and fixes bugs should be commited. Now Lists with simple types work properly with these patches. => That's a good thing! I really do not like to work with a local code base, that contains many patches! see my svn status M test/cpp/Thrift-test.mk M test/cpp/src/TestClient.cpp M test/cpp/src/main.cpp M test/cpp/src/TestServer.cpp M lib/cpp/src/protocol/TProtocol.h A lib/cpp/src/transport/THttpServer.h M lib/cpp/src/transport/THttpClient.cpp M lib/cpp/src/transport/THttpClient.h A lib/cpp/src/transport/THttpTransport.cpp A lib/cpp/src/transport/THttpTransport.h A lib/cpp/src/transport/THttpServer.cpp M lib/cpp/Makefile.am M lib/js/test/test.html M lib/js/thrift.js M compiler/cpp/src/generate/t_js_generator.cc We should proceed with enhancing the Test Framework to have a good basis for finding the pending bug's. I'm especially interested in C++ and JavaScript bindings and I would like to help fixing bugs and enhancing Thrift!
        Hide
        Jordan added a comment -

        Would someone like to join me in a phone call, in order to explain how the javascript code works? That way I can be more productive in fixing these bugs.

        Show
        Jordan added a comment - Would someone like to join me in a phone call, in order to explain how the javascript code works? That way I can be more productive in fixing these bugs.
        Hide
        T Jake Luciani added a comment -

        Ok, I've committed the fixes and test cases all pass now. Please take a look and let me know if this addresses your issues.

        Show
        T Jake Luciani added a comment - Ok, I've committed the fixes and test cases all pass now. Please take a look and let me know if this addresses your issues.
        Hide
        Jordan added a comment -

        Jake,

        Thank you for making these fixes and tidying up the test cases, but this issue shouldn't be resolved until returning lists of objects is fixed. Currently I believe this to still be broken unless you added a fix that I was unaware of..

        I'm going to reassign this ticket, please re-resolve if I am mistaken.

        Show
        Jordan added a comment - Jake, Thank you for making these fixes and tidying up the test cases, but this issue shouldn't be resolved until returning lists of objects is fixed. Currently I believe this to still be broken unless you added a fix that I was unaware of.. I'm going to reassign this ticket, please re-resolve if I am mistaken.
        Hide
        Jordan added a comment -

        Reopening ticket, as I believe returning lists of objects is still broken.

        Show
        Jordan added a comment - Reopening ticket, as I believe returning lists of objects is still broken.
        Hide
        Roger Meier added a comment -

        Thanks Jake, for applying the QUnit based test.html and the bugfixes. I appreciate that very much!

        From my perspective, Lists and Sets with Basetypes are fixed!

        But I still have issues with other Types (Map, MapMap and testInsanity from ThriftTest.thrift):
        see http://www.bufferoverflow.ch/thrift/test/test.html (r980253)
        and http://www.bufferoverflow.ch/thrift/test/test_roger.html with more unit tests.

        I'm using TestServer.cpp in combination with THttpServer.cpp

        Show
        Roger Meier added a comment - Thanks Jake, for applying the QUnit based test.html and the bugfixes. I appreciate that very much! From my perspective, Lists and Sets with Basetypes are fixed! But I still have issues with other Types (Map, MapMap and testInsanity from ThriftTest.thrift): see http://www.bufferoverflow.ch/thrift/test/test.html (r980253) and http://www.bufferoverflow.ch/thrift/test/test_roger.html with more unit tests. I'm using TestServer.cpp in combination with THttpServer.cpp
        Hide
        T Jake Luciani added a comment -

        Hmm, all these work for me.

        Have you tried it with the java server? it's running here: http://3.rdrail.net:8088/test/test.html

        This includes a list of objects in testInsanity()

        I guess there could be an encoding issue with the c++ server...

        Show
        T Jake Luciani added a comment - Hmm, all these work for me. Have you tried it with the java server? it's running here: http://3.rdrail.net:8088/test/test.html This includes a list of objects in testInsanity() I guess there could be an encoding issue with the c++ server...
        Hide
        Jordan added a comment -

        Jake,

        I'll have to check, but I remember the symptoms of the problem with lists of objects, was that the results would have maybe one or two valid objects, and the rest were missing. Or even worse, and object would be duplicated in the place of all of the objects, and the others would be missing. I recall that testInsanity didn't check these edge cases (unless you fixed it.)
        I'm not currently able to get the most recent Thrift to build on my desktop, but I'll see if I can pinpoint where the test is lacking.

        Show
        Jordan added a comment - Jake, I'll have to check, but I remember the symptoms of the problem with lists of objects, was that the results would have maybe one or two valid objects, and the rest were missing. Or even worse, and object would be duplicated in the place of all of the objects, and the others would be missing. I recall that testInsanity didn't check these edge cases (unless you fixed it.) I'm not currently able to get the most recent Thrift to build on my desktop, but I'll see if I can pinpoint where the test is lacking.
        Hide
        Roger Meier added a comment -

        I did not test with the Java Server. It seems, that it returns other values as the C++ Test Server for Exceptions, all the other things are just the same, but testInsanity is broken on both test sites.

        Is there a defined standard for Interoperability tests across all languages?

        I can not imagine that there are some error's with the c++ encoding. I did some tests with JSON and Binary with simple and HTTP Server. I verified the JSON Packets on wire for HTTP and regular TCP transport. => Everything is correct.

        The other test I've added does verify more details and uses json2 in addition to QUnit.
        see: http://www.bufferoverflow.ch/thrift/test/test_roger.html
        There, I recognized some error's with map types.

        As mentioned in THRIFT-829, Im still not sure if loading the JSON response into one object (robj) and reading parts of that object depending on the data type is a problem for decoding Thrift JSON Protocol within JavaScript.
        I think, that all Languages supported by Thrift do parse the JSON message within the Protocol Implementation without special functions like the eval function provided by JavaScript.

        Show
        Roger Meier added a comment - I did not test with the Java Server. It seems, that it returns other values as the C++ Test Server for Exceptions, all the other things are just the same, but testInsanity is broken on both test sites. Is there a defined standard for Interoperability tests across all languages? I can not imagine that there are some error's with the c++ encoding. I did some tests with JSON and Binary with simple and HTTP Server. I verified the JSON Packets on wire for HTTP and regular TCP transport. => Everything is correct. The other test I've added does verify more details and uses json2 in addition to QUnit. see: http://www.bufferoverflow.ch/thrift/test/test_roger.html There, I recognized some error's with map types. As mentioned in THRIFT-829 , Im still not sure if loading the JSON response into one object (robj) and reading parts of that object depending on the data type is a problem for decoding Thrift JSON Protocol within JavaScript. I think, that all Languages supported by Thrift do parse the JSON message within the Protocol Implementation without special functions like the eval function provided by JavaScript.
        Hide
        T Jake Luciani added a comment -

        Ah, I see, all the tests pass in Chrome (which I use) I checked in Safari and FF and test Insanity is now failing for me.

        Let me take a look and get it fixed. I'll also take a look at the c++ server. However If it passes with java server I'd consider this particular issue resolved.
        We can open a new one for C++ issues.

        Also, I don't agree with the idea of implementing the json protocol with string parsing, js engines are very good at marshaling json its going to be much more efficient doing it this way IMO.

        Show
        T Jake Luciani added a comment - Ah, I see, all the tests pass in Chrome (which I use) I checked in Safari and FF and test Insanity is now failing for me. Let me take a look and get it fixed. I'll also take a look at the c++ server. However If it passes with java server I'd consider this particular issue resolved. We can open a new one for C++ issues. Also, I don't agree with the idea of implementing the json protocol with string parsing, js engines are very good at marshaling json its going to be much more efficient doing it this way IMO.
        Hide
        T Jake Luciani added a comment -

        I've committed a fix for testInsanity for FF and Safari. Please take a look.

        Show
        T Jake Luciani added a comment - I've committed a fix for testInsanity for FF and Safari. Please take a look.
        Hide
        Jordan added a comment -

        Can you describe what the difference is for firefox?

        Show
        Jordan added a comment - Can you describe what the difference is for firefox?
        Hide
        Roger Meier added a comment -

        I have the Java Test Server up and running! => The current trunk version passes the tests!
        see: http://www.bufferoverflow.ch:8088/test/test.orig.html

        As mentioned I added a few additional Tests, see
        http://www.bufferoverflow.ch:8088/test/test.html

        The patch(THRIFT_815_TestFramework_test.html_json2.patch) contains additional tests, adds the Apache License Header to test.html again and adds json2 which is Public Domain.

        I absolutely agree, that implementing Thrift JSON Protocol based on string parsing is not that efficient as the eval approach.
        However, during my debug and bug fixing (list element order on wire) sessions I was often confused by the values of rstack and rpos inside thrift.js, especially with map and mapmap objects. That's why I'm thinking about a classic string parsing approach.

        Show
        Roger Meier added a comment - I have the Java Test Server up and running! => The current trunk version passes the tests! see: http://www.bufferoverflow.ch:8088/test/test.orig.html As mentioned I added a few additional Tests, see http://www.bufferoverflow.ch:8088/test/test.html The patch(THRIFT_815_TestFramework_test.html_json2.patch) contains additional tests, adds the Apache License Header to test.html again and adds json2 which is Public Domain. I absolutely agree, that implementing Thrift JSON Protocol based on string parsing is not that efficient as the eval approach. However, during my debug and bug fixing (list element order on wire) sessions I was often confused by the values of rstack and rpos inside thrift.js, especially with map and mapmap objects. That's why I'm thinking about a classic string parsing approach.
        Hide
        T Jake Luciani added a comment -

        Hi Roger,

        I'd like to mark this issue resolved. We can work on your extended testcase in another ticket. Please create this.

        Thanks,

        -Jake

        Show
        T Jake Luciani added a comment - Hi Roger, I'd like to mark this issue resolved. We can work on your extended testcase in another ticket. Please create this. Thanks, -Jake
        Hide
        Roger Meier added a comment -

        Hi Jake

        I fully agree, with your proposal!

        Just created THRIFT-846 to extend the Test Framework.

        Regards Roger

        Show
        Roger Meier added a comment - Hi Jake I fully agree, with your proposal! Just created THRIFT-846 to extend the Test Framework. Regards Roger

          People

          • Assignee:
            T Jake Luciani
            Reporter:
            Jordan
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development