You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Jordan (JIRA)" <ji...@apache.org> on 2010/07/06 03:58:49 UTC
[jira] Created: (THRIFT-815) Deserialization of lists is critically
broken.
Deserialization of lists is critically broken.
----------------------------------------------
Key: THRIFT-815
URL: https://issues.apache.org/jira/browse/THRIFT-815
Project: Thrift
Issue Type: Bug
Components: Compiler (JavaScript)
Affects Versions: 0.4
Reporter: Jordan
Priority: Critical
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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Roger Meier (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12897375#action_12897375 ]
Roger Meier commented on THRIFT-815:
------------------------------------
Hi Jake
I fully agree, with your proposal!
Just created THRIFT-846 to extend the Test Framework.
Regards Roger
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Blocker
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch, THRIFT_815_TestFramework_test.html_json2.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12886124#action_12886124 ]
Jordan commented on THRIFT-815:
-------------------------------
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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Priority: Critical
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "T Jake Luciani (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12895887#action_12895887 ]
T Jake Luciani commented on THRIFT-815:
---------------------------------------
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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Blocker
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Reopened: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jordan reopened THRIFT-815:
---------------------------
Reopening ticket, as I believe returning lists of *objects* is still broken.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "T Jake Luciani (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12886160#action_12886160 ]
T Jake Luciani commented on THRIFT-815:
---------------------------------------
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
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Priority: Critical
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887138#action_12887138 ]
Jordan commented on THRIFT-815:
-------------------------------
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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (THRIFT-815) Deserialization of lists
is critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887055#action_12887055 ]
Jordan edited comment on THRIFT-815 at 7/10/10 12:56 PM:
---------------------------------------------------------
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).
was (Author: jordo):
Thanks Jake, but I don't think that this works when the things in the list are 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).
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887006#action_12887006 ]
Jordan commented on THRIFT-815:
-------------------------------
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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Priority: Critical
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (THRIFT-815) Deserialization of lists
is critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887686#action_12887686 ]
Jordan edited comment on THRIFT-815 at 7/13/10 4:03 AM:
--------------------------------------------------------
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?
<javascript>
//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]
}
}
</code>
was (Author: jordo):
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?
<code>
//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 // it thinks that rstack[this.rstack.length-1] is an object with native fields
ftype = Thrift.Protocol.RType[f]
this.rstack[this.rstack.length-1] = this.rstack[this.rstack.length-1][f]
}
}
</code>
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12895939#action_12895939 ]
Jordan commented on THRIFT-815:
-------------------------------
Can you describe what the difference is for firefox?
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Blocker
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "T Jake Luciani (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12893400#action_12893400 ]
T Jake Luciani commented on THRIFT-815:
---------------------------------------
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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "T Jake Luciani (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887096#action_12887096 ]
T Jake Luciani commented on THRIFT-815:
---------------------------------------
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
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887055#action_12887055 ]
Jordan commented on THRIFT-815:
-------------------------------
Thanks Jake, but I don't think that this works when the things in the list are 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).
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (THRIFT-815) Deserialization of lists
is critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887686#action_12887686 ]
Jordan edited comment on THRIFT-815 at 7/13/10 4:05 AM:
--------------------------------------------------------
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?
<javascript>
//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];
}
}
</code>
Aghgggh. How do I do code snippets on jira?
was (Author: jordo):
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?
<javascript>
//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]
}
}
</code>
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-815) Deserialization of lists is critically
broken.
Posted by "Roger Meier (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Roger Meier updated THRIFT-815:
-------------------------------
Attachment: THRIFT_815_TestFramework_test.html_json2.patch
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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Blocker
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch, THRIFT_815_TestFramework_test.html_json2.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (THRIFT-815) Deserialization of lists
is critically broken.
Posted by "Roger Meier (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887094#action_12887094 ]
Roger Meier edited comment on THRIFT-815 at 7/10/10 5:35 PM:
-------------------------------------------------------------
This patch fixes wrong order on wire:
detected with the Unit Test provided above and Firebug
{code:JavaScript}
var list = new Array(1,2,3)
test("List", function() {
ok(client.testList(list), list);
});
{code}
Wire:
{code}
out: [1,"testList",1,0,{"1":{"lst":["i32",3,3,2,1]}}]
in: [1,"testList",2,0,{"0":{"lst":["i32",3,3,2,1]}}]
{code}
With that patch:
{code}
out: [1,"testList",1,0,{"1":{"lst":["i32",3,1,2,3]}}]
in: [1,"testList",2,0,{"0":{"lst":["i32",3,1,2,3]}}]
{code}
was (Author: roger.meier):
This patch fixes wrong order on wire:
detected with the Unit Test provided above and Firebug
{code:JavaScript}
var list = new Array(1,2,3)
test("List", function() {
ok(client.testList(list), list);
});
{code}
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]}}]
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "T Jake Luciani (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12895928#action_12895928 ]
T Jake Luciani commented on THRIFT-815:
---------------------------------------
I've committed a fix for testInsanity for FF and Safari. Please take a look.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Blocker
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "T Jake Luciani (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887059#action_12887059 ]
T Jake Luciani commented on THRIFT-815:
---------------------------------------
hmm, could you provide a testcase?
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12893551#action_12893551 ]
Jordan commented on THRIFT-815:
-------------------------------
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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "T Jake Luciani (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12897087#action_12897087 ]
T Jake Luciani commented on THRIFT-815:
---------------------------------------
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
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Blocker
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch, THRIFT_815_TestFramework_test.html_json2.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Roger Meier (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12895385#action_12895385 ]
Roger Meier commented on THRIFT-815:
------------------------------------
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 | http://www.JSON.org/json2.js] 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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "T Jake Luciani (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
T Jake Luciani resolved THRIFT-815.
-----------------------------------
Assignee: T Jake Luciani
Fix Version/s: 0.4
Resolution: Fixed
Fixed, thanks for finding this.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12889475#action_12889475 ]
Jordan commented on THRIFT-815:
-------------------------------
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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12895107#action_12895107 ]
Jordan commented on THRIFT-815:
-------------------------------
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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887671#action_12887671 ]
Jordan commented on THRIFT-815:
-------------------------------
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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887137#action_12887137 ]
Jordan commented on THRIFT-815:
-------------------------------
If you're adding test cases, don't forget the null being translated into dummy objects:
https://issues.apache.org/jira/browse/THRIFT-816
:)
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (THRIFT-815) Deserialization of lists
is critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887686#action_12887686 ]
Jordan edited comment on THRIFT-815 at 7/13/10 4:58 PM:
--------------------------------------------------------
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?
{code:title=Bar.java|borderStyle=solid}
//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];
}
}
{code}
was (Author: jordo):
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?
<javascript>
//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];
}
}
</code>
Aghgggh. How do I do code snippets on jira?
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Roger Meier (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12888026#action_12888026 ]
Roger Meier commented on THRIFT-815:
------------------------------------
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*
{code}
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
{code}
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!
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Roger Meier (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12893803#action_12893803 ]
Roger Meier commented on THRIFT-815:
------------------------------------
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
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12888004#action_12888004 ]
Jordan commented on THRIFT-815:
-------------------------------
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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-815) Deserialization of lists is critically
broken.
Posted by "T Jake Luciani (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
T Jake Luciani updated THRIFT-815:
----------------------------------
Attachment: 813-1.patch
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Priority: Critical
> Attachments: 813-1.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "T Jake Luciani (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887066#action_12887066 ]
T Jake Luciani commented on THRIFT-815:
---------------------------------------
yeah, or you can just attach a tar of the test dir. thanks
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12886132#action_12886132 ]
Bryan Duxbury commented on THRIFT-815:
--------------------------------------
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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Priority: Critical
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (THRIFT-815) Deserialization of lists
is critically broken.
Posted by "Roger Meier (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887094#action_12887094 ]
Roger Meier edited comment on THRIFT-815 at 7/12/10 3:17 PM:
-------------------------------------------------------------
The patch [THRIFT-815_thrift.js.patch | https://issues.apache.org/jira/secure/attachment/12449170/THRIFT-815_thrift.js.patch] fixes wrong order on wire:
detected with the Unit Test provided above and Firebug
{code:JavaScript}
var list = new Array(1,2,3)
test("List", function() {
ok(client.testList(list), list);
});
{code}
Wire:
{code}
out: [1,"testList",1,0,{"1":{"lst":["i32",3,3,2,1]}}]
in: [1,"testList",2,0,{"0":{"lst":["i32",3,3,2,1]}}]
{code}
With that patch:
{code}
out: [1,"testList",1,0,{"1":{"lst":["i32",3,1,2,3]}}]
in: [1,"testList",2,0,{"0":{"lst":["i32",3,1,2,3]}}]
{code}
was (Author: roger.meier):
This patch fixes wrong order on wire:
detected with the Unit Test provided above and Firebug
{code:JavaScript}
var list = new Array(1,2,3)
test("List", function() {
ok(client.testList(list), list);
});
{code}
Wire:
{code}
out: [1,"testList",1,0,{"1":{"lst":["i32",3,3,2,1]}}]
in: [1,"testList",2,0,{"0":{"lst":["i32",3,3,2,1]}}]
{code}
With that patch:
{code}
out: [1,"testList",1,0,{"1":{"lst":["i32",3,1,2,3]}}]
in: [1,"testList",2,0,{"0":{"lst":["i32",3,1,2,3]}}]
{code}
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-815) Deserialization of lists is critically
broken.
Posted by "Roger Meier (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Roger Meier updated THRIFT-815:
-------------------------------
Attachment: test.html
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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "T Jake Luciani (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
T Jake Luciani resolved THRIFT-815.
-----------------------------------
Resolution: Fixed
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Blocker
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch, THRIFT_815_TestFramework_test.html_json2.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Reopened: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jordan reopened THRIFT-815:
---------------------------
See my last comment. (I think the provided fix won't fix the case when lists contain objects. )
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "T Jake Luciani (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12895094#action_12895094 ]
T Jake Luciani commented on THRIFT-815:
---------------------------------------
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...
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887686#action_12887686 ]
Jordan commented on THRIFT-815:
-------------------------------
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?
<code>
//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 // it thinks that rstack[this.rstack.length-1] is an object with native fields
ftype = Thrift.Protocol.RType[f]
this.rstack[this.rstack.length-1] = this.rstack[this.rstack.length-1][f]
}
}
</code>
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887064#action_12887064 ]
Jordan commented on THRIFT-815:
-------------------------------
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?
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Roger Meier (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12887946#action_12887946 ]
Roger Meier commented on THRIFT-815:
------------------------------------
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 | https://issues.apache.org/jira/secure/attachment/12449170/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 |https://svn.apache.org/repos/asf/incubator/thrift/trunk/test/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.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: Coding Guidelines & Enumerations
Posted by Roger Meier <ro...@bufferoverflow.ch>.
Thanks Bryan!
I like the implicit set of guidelines!
reducing doc stuff and focusing on code.
I've added the Enumeration Topics as a Improvement via JIRA:
add Enumeration for protocol, transport and server types
https://issues.apache.org/jira/browse/THRIFT-819
Roger
Am 13.07.2010 18:22, schrieb Bryan Duxbury:
> Roger -
>
> We don't have any "official" coding style guidelines, though I'd say pretty
> much every library (and the compiler) have an implicit set of guidelines
> they follow.
>
> As far as having an enumeration for protocol and transport types: Do you
> mean some code construct that identifies them all, or just some wiki page?
> In either case, I'd suggest you open a ticket in JIRA for us to create what
> it is you're looking for.
>
> -Bryan
>
> On Thu, Jul 8, 2010 at 5:07 PM, Roger Meier<ro...@bufferoverflow.ch> wrote:
>
>> Hi all
>>
>> is there a coding style manual for Thrift contributers?
>>
>> I was looking for a CodeStyle inside Source, Wiki, etc. and did not found
>> anything.
>> e.g.
>> C++ => boost style, facebook?
>> Java: http://java.sun.com/docs/codeconv/html/CodeConvTOC.doc.html
>> JavaScript: http://javascript.crockford.com/code.html
>>
>> Another question is...
>> Why are there no enumaration for protocol and transport types?
>>
>> Thanks
>>
>> Roger
>>
>>
>
Re: Coding Guidelines & Enumerations
Posted by Bryan Duxbury <br...@rapleaf.com>.
Roger -
We don't have any "official" coding style guidelines, though I'd say pretty
much every library (and the compiler) have an implicit set of guidelines
they follow.
As far as having an enumeration for protocol and transport types: Do you
mean some code construct that identifies them all, or just some wiki page?
In either case, I'd suggest you open a ticket in JIRA for us to create what
it is you're looking for.
-Bryan
On Thu, Jul 8, 2010 at 5:07 PM, Roger Meier <ro...@bufferoverflow.ch> wrote:
> Hi all
>
> is there a coding style manual for Thrift contributers?
>
> I was looking for a CodeStyle inside Source, Wiki, etc. and did not found
> anything.
> e.g.
> C++ => boost style, facebook?
> Java: http://java.sun.com/docs/codeconv/html/CodeConvTOC.doc.html
> JavaScript: http://javascript.crockford.com/code.html
>
> Another question is...
> Why are there no enumaration for protocol and transport types?
>
> Thanks
>
> Roger
>
>
Coding Guidelines & Enumerations
Posted by Roger Meier <ro...@bufferoverflow.ch>.
Hi all
is there a coding style manual for Thrift contributers?
I was looking for a CodeStyle inside Source, Wiki, etc. and did not
found anything.
e.g.
C++ => boost style, facebook?
Java: http://java.sun.com/docs/codeconv/html/CodeConvTOC.doc.html
JavaScript: http://javascript.crockford.com/code.html
Another question is...
Why are there no enumaration for protocol and transport types?
Thanks
Roger
[jira] Commented: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "Jordan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12886262#action_12886262 ]
Jordan commented on THRIFT-815:
-------------------------------
Jake,
I'll try to track down where this is occurring exactly to provide enough info to pinpoint where the problem is coming from.
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Priority: Critical
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (THRIFT-815) Deserialization of lists is
critically broken.
Posted by "T Jake Luciani (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
T Jake Luciani resolved THRIFT-815.
-----------------------------------
Resolution: Fixed
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-815) Deserialization of lists is critically
broken.
Posted by "Bryan Duxbury (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Bryan Duxbury updated THRIFT-815:
---------------------------------
Priority: Blocker (was: Critical)
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Blocker
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (THRIFT-815) Deserialization of lists is critically
broken.
Posted by "Roger Meier (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-815?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Roger Meier updated THRIFT-815:
-------------------------------
Attachment: THRIFT-815_thrift.js.patch
This patch fixes wrong order on wire:
detected with the Unit Test provided above and Firebug
{code:JavaScript}
var list = new Array(1,2,3)
test("List", function() {
ok(client.testList(list), list);
});
{code}
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]}}]
> Deserialization of lists is critically broken.
> ----------------------------------------------
>
> Key: THRIFT-815
> URL: https://issues.apache.org/jira/browse/THRIFT-815
> Project: Thrift
> Issue Type: Bug
> Components: Compiler (JavaScript)
> Affects Versions: 0.4
> Reporter: Jordan
> Assignee: T Jake Luciani
> Priority: Critical
> Fix For: 0.4
>
> Attachments: 813-1.patch, test.html, THRIFT-815_thrift.js.patch
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.