You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by "Jimmy John (JIRA)" <qp...@incubator.apache.org> on 2007/05/24 18:03:16 UTC
[jira] Created: (QPID-498) python code.py handling of data types
beyond acceptable range.
python code.py handling of data types beyond acceptable range.
--------------------------------------------------------------
Key: QPID-498
URL: https://issues.apache.org/jira/browse/QPID-498
Project: Qpid
Issue Type: Bug
Components: Python Client
Reporter: Jimmy John
Priority: Minor
Perform range checking on all data types such as octet[0,255], short[0,65535], long etc. [Refer QPID-497 for unit test that detects these bugs]
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (QPID-498) python code.py handling of data types
beyond acceptable range.
Posted by "Jimmy John (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-498?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12505624 ]
Jimmy John commented on QPID-498:
---------------------------------
Need to modify the code so that codec.py picks up the error descriptions/constants etc from the spec rather than defining it again in constants.py.
> python code.py handling of data types beyond acceptable range.
> --------------------------------------------------------------
>
> Key: QPID-498
> URL: https://issues.apache.org/jira/browse/QPID-498
> Project: Qpid
> Issue Type: Bug
> Components: Python Client
> Reporter: Jimmy John
> Assignee: Rafael H. Schloming
> Priority: Minor
> Attachments: codec.py, constants.py, exception.py
>
>
> Perform range checking on all data types such as octet[0,255], short[0,65535], long etc. [Refer QPID-497 for unit test that detects these bugs]
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Assigned: (QPID-498) python code.py handling of data types
beyond acceptable range.
Posted by "Rafael H. Schloming (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-498?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Rafael H. Schloming reassigned QPID-498:
----------------------------------------
Assignee: Rafael H. Schloming
> python code.py handling of data types beyond acceptable range.
> --------------------------------------------------------------
>
> Key: QPID-498
> URL: https://issues.apache.org/jira/browse/QPID-498
> Project: Qpid
> Issue Type: Bug
> Components: Python Client
> Reporter: Jimmy John
> Assigned To: Rafael H. Schloming
> Priority: Minor
> Attachments: codec.py, constants.py, exception.py
>
>
> Perform range checking on all data types such as octet[0,255], short[0,65535], long etc. [Refer QPID-497 for unit test that detects these bugs]
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (QPID-498) python code.py handling of data types
beyond acceptable range.
Posted by "Rafael H. Schloming (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-498?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12499215 ]
Rafael H. Schloming commented on QPID-498:
------------------------------------------
Thanks, I'll take a look early next week. I have your test suite read to go in my local checkout. I'll probably submit the whole thing at once so I don't create any interim failures.
> python code.py handling of data types beyond acceptable range.
> --------------------------------------------------------------
>
> Key: QPID-498
> URL: https://issues.apache.org/jira/browse/QPID-498
> Project: Qpid
> Issue Type: Bug
> Components: Python Client
> Reporter: Jimmy John
> Assigned To: Rafael H. Schloming
> Priority: Minor
> Attachments: codec.py, constants.py, exception.py
>
>
> Perform range checking on all data types such as octet[0,255], short[0,65535], long etc. [Refer QPID-497 for unit test that detects these bugs]
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (QPID-498) python code.py handling of data types
beyond acceptable range.
Posted by "Jimmy John (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-498?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jimmy John updated QPID-498:
----------------------------
Attachment: exception.py
constants.py
codec.py
Attached are the files that resolve this issue.
All files to be placed in python/qpid directory
code.py
------------
1. added doc strings for all functions
2. added appropriate range checks
exception.py
---------------------
new exception module that describes the channel and connection exceptions. I think that all exceptions should be subclasses from these. Utility methods are provided to extract the error code, error descriptions etc.
constants.py
-------------------
module containing the global error codes and error descriptions. Global data can be placed here.
The unit test tests_0-9/codec.py now passes successfully.
Let me know if you have any questions....
Jimmy
> python code.py handling of data types beyond acceptable range.
> --------------------------------------------------------------
>
> Key: QPID-498
> URL: https://issues.apache.org/jira/browse/QPID-498
> Project: Qpid
> Issue Type: Bug
> Components: Python Client
> Reporter: Jimmy John
> Priority: Minor
> Attachments: codec.py, constants.py, exception.py
>
>
> Perform range checking on all data types such as octet[0,255], short[0,65535], long etc. [Refer QPID-497 for unit test that detects these bugs]
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (QPID-498) python code.py handling of data types
beyond acceptable range.
Posted by "Jimmy John (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-498?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12507632 ]
Jimmy John commented on QPID-498:
---------------------------------
Thanks for the comments...
The intention of adding the exceptions like SyntaxErrorException, FrameErrorException etc into the codec.py script was that they would be caught some place upstream and display the appropriate error message, close connection etc. e.g. the connection.write function calls the encode functions. The function should be within a try/except block so that in case codec.py threw an exception, it could take the appropriate action. This handler function can be common so that it does the required action whenever it catches an exception.
The issue is that one can never tell if a bad value has been read off the wire or not...e.g
>>> struct.pack("!H", 65538)
'\x00\x02'
65538 is an invalid value for a short. When we try and unpack it:
>>> struct.unpack("!H", '\x00\x02')
(2,)
We get a 2 i.e. we can never know if the client was trying to send us a valid 2 or an invalid 65538. So one would never know that a value read of the pipe is invalid or not, unless we catch it at encoding time and raise an exception.
Buy yes, if you look at codec.py standalone, then raising a SyntaxErrorException dosen't make much sense. But in the context of the client-server interactions, if codec.py throws an exception, then there is some value in it. If we define our own exceptions, then when it is caught upstream and displayed to the user, it will not be matching the amqp specification.
Regarding you coment about not including version specific code into the codebase, i think we can still use all the exceptions defined and still staisfy this criteria. exceptions.py would probably need to be re-written so it reads data of spec.py rather than reading hard coded values from constants.py. So in future even if the spec changes, exceptions.py will have the correct text. [need to get rid of constants.py. I didn't know what spec.py was doing when i coded these changes :)]
thots...?
> python code.py handling of data types beyond acceptable range.
> --------------------------------------------------------------
>
> Key: QPID-498
> URL: https://issues.apache.org/jira/browse/QPID-498
> Project: Qpid
> Issue Type: Bug
> Components: Python Client
> Reporter: Jimmy John
> Assignee: Rafael H. Schloming
> Priority: Minor
> Attachments: codec.py, constants.py, exception.py
>
>
> Perform range checking on all data types such as octet[0,255], short[0,65535], long etc. [Refer QPID-497 for unit test that detects these bugs]
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (QPID-498) python code.py handling of data types
beyond acceptable range.
Posted by "Aidan Skinner (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-498?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aidan Skinner resolved QPID-498.
--------------------------------
Resolution: Fixed
Fixed, or somethign
> python code.py handling of data types beyond acceptable range.
> --------------------------------------------------------------
>
> Key: QPID-498
> URL: https://issues.apache.org/jira/browse/QPID-498
> Project: Qpid
> Issue Type: Bug
> Components: Python Client
> Reporter: Jimmy John
> Assignee: Rafael H. Schloming
> Priority: Minor
> Attachments: codec.py, constants.py, exception.py
>
>
> Perform range checking on all data types such as octet[0,255], short[0,65535], long etc. [Refer QPID-497 for unit test that detects these bugs]
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (QPID-498) python code.py handling of data types
beyond acceptable range.
Posted by "Rafael H. Schloming (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-498?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12507460 ]
Rafael H. Schloming commented on QPID-498:
------------------------------------------
I finally got around to looking at this patch and I have a few comments. I agree with your comment above that data from constants.py should be loaded from the spec file.
My other comment on codec.py is that it doesn't seem quite right for codec.py to be throwing the kinds of exceptions you have in exceptions.py. The exceptions you have in exceptions.py correspond to protocol errors used to indicate error conditions encountered by the other peer in an AMQP interaction. This is not the case the way you have used them in codec.py. For example you've used SyntaxErrorException in codec.py to indicate an attempt to locally encode a bad value. The error condition is actually defined by the protocol as meaning that a bad value was read off of the wire, and as such the SyntaxErrorException documentation doesn't acurrately reflect what has occured where you use it in codec.py. It also seems to violate the exception hierarchy you have created since SyntaxErrorException is a ConnectionException which should indicate that the peer closed the connection, but would not in this case. I've always viewed codec.py as purely an encoding/decoding utility that may not even know it is talking over a socket, so I would probably expand the codec.EOF exception into a small exception hierarchy that has the necessary granularity to indicate the various overflow conditions you have added.
Additionally I'm not sure I would include all the exceptions you have in exceptions.py. The details of each error condition depend on the precise version of the spec in question, and in general I've been avoiding putting version dependant code into this codebase as it needs to function as a version agnostic testing tool. The approach I would take is to stick with only channel and connection exceptions, and have them simply include the error code and description as fields. I believe this keeps the code version independent and permits users of the API to check the details of the particular error case if they so wish.
> python code.py handling of data types beyond acceptable range.
> --------------------------------------------------------------
>
> Key: QPID-498
> URL: https://issues.apache.org/jira/browse/QPID-498
> Project: Qpid
> Issue Type: Bug
> Components: Python Client
> Reporter: Jimmy John
> Assignee: Rafael H. Schloming
> Priority: Minor
> Attachments: codec.py, constants.py, exception.py
>
>
> Perform range checking on all data types such as octet[0,255], short[0,65535], long etc. [Refer QPID-497 for unit test that detects these bugs]
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (QPID-498) python code.py handling of data types
beyond acceptable range.
Posted by "Rafael H. Schloming (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-498?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12507880 ]
Rafael H. Schloming commented on QPID-498:
------------------------------------------
I agree we should always throw an exception if someone tries to encode a bad value. My point is simply that we shouldn't be throwing a protocol error since a protocol error specifically indicates that the other end of the connection/channel encountered an error *and closed the connection/channel*, whereas in this case the other end of the connection will never encounter an error because we abort the writing attempt before ever sending any bad data. This will lead to bugs since the expected action taken by the user of the python API is different in each case.
In case one (struct.pack("!H", 65538)) the user is trying to encode a bad value. This is always guaranteed to be a programming error on the part of the user or python library operating at the sending end of the connection. When this situation is encountered the appropriate exception would be a ValueError, or perhaps a codec specific exception akin to a ValueError. The user or library would then catch this exception in some catchall piece of code somewhere and then send a protocol-error ("internal-error", 541) indicating that there was an internal problem at this end and this end of the connection has been shutdown, and the other end should proceed to shutdown it's end of the connection/channel.
In case two when a channel or connection exception is spontaneously received from the peer, this could be caused by a whole host of errors, e.g. incompatible versions, or a bug at the *other* end of the connection. In all cases though the action taken by the user/library at this end is different because the user/library expects the other end to have already shutdown the connection and so should not attempt to send any sort of error code to the peer.
Regarding exception specifics in the codebase, I agree it would be beneficial to read the error codes and exception texts from the spec file, however that would still leave the list and python name for all the exceptions being hand written for each version of the spec. This makes our API overly brittle when new error codes are introduced by new versions of the spec, or when vendor specific error codes are used. At the moment I don't see why anyone would care to catch the specific subclasses of channel or connection exception since in each case the channel or connection is closed, and there is no recourse other than gracefully shutting down and accurately reporting the problem to the user. This seems to be best acomplished with two exceptions that each contain an error code and text description to accurately identify what went wrong. This seems to satisfy the common case, but also permits programatic interception of errors using the error code if that is needed.
To be clear I'm not completely opposed to adding such a hierarchy if needed, but I would probably want it added it differently. I believe the core of the library should simply report the error codes as submitted by the peer since that permits this library to be used to proxy error codes without understanding them. I would then add in a specific exception hierarchy as part of an API layer on top of the core library, much the way the client delegate adds in-memory message queues on top of the core listener pattern. This keeps the bulk of the code version and vendor independent, and isolates version/vendor specific code to its own API layer. That said, I would still not be eager to add that API layer at this point unless it is definitely needed since it will almost surely need to be rewritten in a matter of months for 0-10.
> python code.py handling of data types beyond acceptable range.
> --------------------------------------------------------------
>
> Key: QPID-498
> URL: https://issues.apache.org/jira/browse/QPID-498
> Project: Qpid
> Issue Type: Bug
> Components: Python Client
> Reporter: Jimmy John
> Assignee: Rafael H. Schloming
> Priority: Minor
> Attachments: codec.py, constants.py, exception.py
>
>
> Perform range checking on all data types such as octet[0,255], short[0,65535], long etc. [Refer QPID-497 for unit test that detects these bugs]
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.