You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by "Kevin Radloff (JIRA)" <ji...@apache.org> on 2012/09/20 16:20:07 UTC
[jira] [Created] (THRIFT-1699) Native Union#read has extra
read_field_end call
Kevin Radloff created THRIFT-1699:
-------------------------------------
Summary: Native Union#read has extra read_field_end call
Key: THRIFT-1699
URL: https://issues.apache.org/jira/browse/THRIFT-1699
Project: Thrift
Issue Type: Bug
Components: Ruby - Library
Affects Versions: 0.8
Reporter: Kevin Radloff
The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1699) Native Union#read has extra
read_field_end call
Posted by "Nathan Beyer (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13505997#comment-13505997 ]
Nathan Beyer commented on THRIFT-1699:
--------------------------------------
{quote}Kevin Radloff added a comment - 28/Nov/12 15:39
If you are counting the instances of read_field_begin and read_field_end, then yes, they match up right now, but this is a union and not a standard struct, and the behavior is unique here.{quote}
I'm intimately familiar with the uniqueness of unions, can you point me at some doc or another language's library that's doing it correctly?
{quote}Kevin Radloff added a comment - 28/Nov/12 15:39
And unfortunately, I have not had much luck running the thrift ruby test suite. It appears as though this has gone undiscovered because there are no tests that use unions. And no users probably use unions, either. {quote}
This was going to be my next comment. Assuming the code needs to change, the patch needs to include some test cases.
> Native Union#read has extra read_field_end call
> -----------------------------------------------
>
> Key: THRIFT-1699
> URL: https://issues.apache.org/jira/browse/THRIFT-1699
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.8
> Reporter: Kevin Radloff
> Attachments: struct.diff
>
>
> The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (THRIFT-1699) Native Union#read has extra
read_field_end call
Posted by "Nathan Beyer (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1699?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Nathan Beyer updated THRIFT-1699:
---------------------------------
Attachment: THRIFT-1699-remove-extra-read-field-end.patch
I applied the patch and tested it on Mac OS X Lion with Ruby 1.9.3-p327. All of the Ruby tests passed fine.
The original patch wasn't a unified patch and a bit painful to apply via 'patch', so I regenerated it via 'svn diff' and uploaded the file as [^THRIFT-1699-remove-extra-read-field-end.patch].
> Native Union#read has extra read_field_end call
> -----------------------------------------------
>
> Key: THRIFT-1699
> URL: https://issues.apache.org/jira/browse/THRIFT-1699
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.8
> Reporter: Kevin Radloff
> Attachments: rb-struct.diff, THRIFT-1699-remove-extra-read-field-end.patch
>
>
> The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Comment Edited] (THRIFT-1699) Native Union#read has extra
read_field_end call
Posted by "Jens Geyer (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13506009#comment-13506009 ]
Jens Geyer edited comment on THRIFT-1699 at 11/29/12 12:29 AM:
---------------------------------------------------------------
After reading THRIFT-409 and having a look at the Java reference implementation, I think you are right and the call is indeed wrong there,since the matching call to begin_read_field() is expected to return T_STOP (thus no read_end required), otherwise it raises an exception anyway.
Could you please provide a patch? And could someone who actually has some kind of Ruby running somewhere please verify this?
Thanks. :-)
was (Author: jensg):
After reading THRIFT-409 and having a look at the Java "reference" implementation, I think you are right and the call is indeed wrong there,since the matching call to begin_read_field() is expected to return T_STOP (thus no read_end required), otherwise it raises an exception anyway.
Could you please provide a patch? And could someone who actually has some kind of Ruby running somewhere please verify this?
Thanks. :-)
> Native Union#read has extra read_field_end call
> -----------------------------------------------
>
> Key: THRIFT-1699
> URL: https://issues.apache.org/jira/browse/THRIFT-1699
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.8
> Reporter: Kevin Radloff
> Attachments: rb-struct.diff
>
>
> The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (THRIFT-1699) Native Union#read has extra
read_field_end call
Posted by "Kevin Radloff (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1699?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Kevin Radloff updated THRIFT-1699:
----------------------------------
Attachment: rb-struct.diff
> Native Union#read has extra read_field_end call
> -----------------------------------------------
>
> Key: THRIFT-1699
> URL: https://issues.apache.org/jira/browse/THRIFT-1699
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.8
> Reporter: Kevin Radloff
> Attachments: rb-struct.diff
>
>
> The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1699) Native Union#read has extra
read_field_end call
Posted by "Jens Geyer (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13506009#comment-13506009 ]
Jens Geyer commented on THRIFT-1699:
------------------------------------
After reading THRIFT-409 and having a look at the Java "reference" implementation, I think you are right and the call is indeed wrong there,since the matching call to begin_read_field() is expected to return T_STOP (thus no read_end required), otherwise it raises an exception anyway.
Could you please provide a patch? And could someone who actually has some kind of Ruby running somewhere please verify this?
Thanks. :-)
> Native Union#read has extra read_field_end call
> -----------------------------------------------
>
> Key: THRIFT-1699
> URL: https://issues.apache.org/jira/browse/THRIFT-1699
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.8
> Reporter: Kevin Radloff
> Attachments: struct.diff
>
>
> The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (THRIFT-1699) Native Union#read has extra
read_field_end call
Posted by "Kevin Radloff (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1699?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Kevin Radloff updated THRIFT-1699:
----------------------------------
Attachment: (was: struct.diff)
> Native Union#read has extra read_field_end call
> -----------------------------------------------
>
> Key: THRIFT-1699
> URL: https://issues.apache.org/jira/browse/THRIFT-1699
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.8
> Reporter: Kevin Radloff
> Attachments: rb-struct.diff
>
>
> The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1699) Native Union#read has extra
read_field_end call
Posted by "Kevin Radloff (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13505491#comment-13505491 ]
Kevin Radloff commented on THRIFT-1699:
---------------------------------------
Anybody out there? It would be nice to get this merged before 1.0.
> Native Union#read has extra read_field_end call
> -----------------------------------------------
>
> Key: THRIFT-1699
> URL: https://issues.apache.org/jira/browse/THRIFT-1699
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.8
> Reporter: Kevin Radloff
> Attachments: struct.diff
>
>
> The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1699) Native Union#read has extra
read_field_end call
Posted by "Kevin Radloff (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13506025#comment-13506025 ]
Kevin Radloff commented on THRIFT-1699:
---------------------------------------
Thanks for looking. I'll try to work on specs for it, but like I said I haven't had a whole lot of luck getting them to run even using rspec 1.3 and an older ruby.
> Native Union#read has extra read_field_end call
> -----------------------------------------------
>
> Key: THRIFT-1699
> URL: https://issues.apache.org/jira/browse/THRIFT-1699
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.8
> Reporter: Kevin Radloff
> Attachments: struct.diff
>
>
> The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1699) Native Union#read has extra
read_field_end call
Posted by "Kevin Radloff (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13505921#comment-13505921 ]
Kevin Radloff commented on THRIFT-1699:
---------------------------------------
{code}
// read field end
default_read_field_end(protocol);
field_header = default_read_field_begin(protocol);
field_type_value = rb_ary_entry(field_header, 1);
field_type = FIX2INT(field_type_value);
if (field_type != TTYPE_STOP) {
rb_raise(rb_eRuntimeError, "too many fields in union!");
}
// read field end
default_read_field_end(protocol);
{code}
The code is calling default_read_field_end to look for the end of the first (which should also be the only field) in the union. It then calls default_read_field_begin to verify that there is not the start of another field. It then errantly calls default_read_field_end despite the fact the prior default_read_field_begin should not have consumed any further characters (or done anything else besides returned TTYPE_STOP). If you are counting the instances of read_field_begin and read_field_end, then yes, they match up right now, but this is a union and not a standard struct, and the behavior is unique here.
And unfortunately, I have not had much luck running the thrift ruby test suite. It appears as though this has gone undiscovered because there are no tests that use unions. And no users probably use unions, either. ;)
> Native Union#read has extra read_field_end call
> -----------------------------------------------
>
> Key: THRIFT-1699
> URL: https://issues.apache.org/jira/browse/THRIFT-1699
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.8
> Reporter: Kevin Radloff
> Attachments: struct.diff
>
>
> The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1699) Native Union#read has extra
read_field_end call
Posted by "Kevin Radloff (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13506027#comment-13506027 ]
Kevin Radloff commented on THRIFT-1699:
---------------------------------------
Ohh wait, didn't realize someone had updated the test suite. Never mind. :)
> Native Union#read has extra read_field_end call
> -----------------------------------------------
>
> Key: THRIFT-1699
> URL: https://issues.apache.org/jira/browse/THRIFT-1699
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.8
> Reporter: Kevin Radloff
> Attachments: struct.diff
>
>
> The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (THRIFT-1699) Native Union#read has extra
read_field_end call
Posted by "Kevin Radloff (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1699?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Kevin Radloff updated THRIFT-1699:
----------------------------------
Attachment: struct.diff
> Native Union#read has extra read_field_end call
> -----------------------------------------------
>
> Key: THRIFT-1699
> URL: https://issues.apache.org/jira/browse/THRIFT-1699
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.8
> Reporter: Kevin Radloff
> Attachments: struct.diff
>
>
> The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1699) Native Union#read has extra
read_field_end call
Posted by "Kevin Radloff (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13506039#comment-13506039 ]
Kevin Radloff commented on THRIFT-1699:
---------------------------------------
Updated patch attached. Confirmed that the spec fails without my change.
> Native Union#read has extra read_field_end call
> -----------------------------------------------
>
> Key: THRIFT-1699
> URL: https://issues.apache.org/jira/browse/THRIFT-1699
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.8
> Reporter: Kevin Radloff
> Attachments: rb-struct.diff
>
>
> The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1699) Native Union#read has extra
read_field_end call
Posted by "Jens Geyer (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13505913#comment-13505913 ]
Jens Geyer commented on THRIFT-1699:
------------------------------------
Kevin,
you write, you are using an own impl. of the JSON protocol.
The ruby code in the repo does also do something in this function:
{code}
def read_field_end
read_json_object_end
end
{code}
Question: Does the JSON implementation in the repo work or not?
=> If yes, the issue is most probably within your code.
=> If not, there's a big chance that you are right.
Without having done deep dive into the Ruby code, just taken a quick look at it. So I may overlook something important.
> Native Union#read has extra read_field_end call
> -----------------------------------------------
>
> Key: THRIFT-1699
> URL: https://issues.apache.org/jira/browse/THRIFT-1699
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.8
> Reporter: Kevin Radloff
> Attachments: struct.diff
>
>
> The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (THRIFT-1699) Native Union#read has extra
read_field_end call
Posted by "Nathan Beyer (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/THRIFT-1699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13505902#comment-13505902 ]
Nathan Beyer commented on THRIFT-1699:
--------------------------------------
Why do you believe that the read_field_end is extraneous? In looking at the code prior to the read_field_end, there is a read_field_begin that seemingly needs this call to read_field_end.
> Native Union#read has extra read_field_end call
> -----------------------------------------------
>
> Key: THRIFT-1699
> URL: https://issues.apache.org/jira/browse/THRIFT-1699
> Project: Thrift
> Issue Type: Bug
> Components: Ruby - Library
> Affects Versions: 0.8
> Reporter: Kevin Radloff
> Attachments: struct.diff
>
>
> The native implementation of the Union#read method has an extraneous call to read_field_end. This makes no difference to most protocols because read_field_end is a no-op, but it breaks JSON (in this case my own class, not the one in trunk), which does do something in this method.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira