New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for type "x" #1011

Merged
merged 6 commits into from Apr 27, 2018

Conversation

Projects
None yet
4 participants
@masell

masell commented Apr 10, 2018

Sender is java spring application, and on python side i get this exception:

Traceback (most recent call last):
  File "/usr/lib/python3.6/asyncio/events.py", line 127, in _run
    self._callback(*self._args)
  File ".venv/lib/python3.6/site-packages/pika/adapters/base_connection.py", line 395, in _handle_events
    self._handle_read()
  File ".venv/lib/python3.6/site-packages/pika/adapters/base_connection.py", line 449, in _handle_read
    self._on_data_available(data)
  File ".venv/lib/python3.6/site-packages/pika/connection.py", line 1934, in _on_data_available
    consumed_count, frame_value = self._read_frame()
  File ".venv/lib/python3.6/site-packages/pika/connection.py", line 2080, in _read_frame
    return frame.decode_frame(self._frame_buffer)
  File ".venv/lib/python3.6/site-packages/pika/frame.py", line 250, in decode_frame
    out = properties.decode(frame_data[12:])
  File ".venv/lib/python3.6/site-packages/pika/spec.py", line 2112, in decode
    (self.headers, offset) = data.decode_table(encoded, offset)
  File ".venv/lib/python3.6/site-packages/pika/data.py", line 186, in decode_table
    value, offset = decode_value(encoded, offset)
  File ".venv/lib/python3.6/site-packages/pika/data.py", line 308, in decode_value
    raise exceptions.InvalidFieldTypeException(kind)
pika.exceptions.InvalidFieldTypeException: b'x'

Docs here:
https://www.rabbitmq.com/releases/rabbitmq-dotnet-client/v3.6.0/rabbitmq-dotnet-client-3.6.0-client-htmldoc/html/type-RabbitMQ.Client.BinaryTableValue.html

Martin Åsell

@lukebakken lukebakken self-assigned this Apr 10, 2018

@lukebakken lukebakken added this to the 0.12 milestone Apr 10, 2018

Martin Åsell
@@ -15,7 +15,7 @@
class DataTests(unittest.TestCase):

FIELD_TBL_ENCODED = (
b'\x00\x00\x00\xcb'
b'\x00\x00\x00\xd9'

This comment has been minimized.

@masell

masell Apr 11, 2018

I dont know what this is...

@masell

This comment has been minimized.

masell commented Apr 11, 2018

@lukebakken Added test case, and encode also (Needed for test.. :) )

Martin Åsell added some commits Apr 11, 2018

Martin Åsell
Martin Åsell
@masell

This comment has been minimized.

masell commented Apr 11, 2018

@lukebakken The coverage job is failing due to some credentials stuff... I can't fix that i assume?

@lukebakken

This comment has been minimized.

Contributor

lukebakken commented Apr 11, 2018

@masell - yep, don't worry about that! I'll review this PR within a week. Thanks again.

b'\x06strvalS\x00\x00\x00\x04Test'
b'\x0ctimestampvalT\x00\x00\x00\x00Ec)\x92'
b'\x07unicodeS\x00\x00\x00\x08utf8=\xe2\x9c\x93')
FIELD_TBL_ENCODED = b"".join((

This comment has been minimized.

@vitaly-krugl

vitaly-krugl Apr 14, 2018

Member

Changing all the existing lines makes it difficult for a reviewer to check what changed and what didn't.

@vitaly-krugl

In FIELD_TBL_ENCODED, changing all the existing lines makes it difficult for a reviewer to check what changed and what didn't. Please revert to the old format and only change what's affected by your fix.

Martin Åsell
@masell

This comment has been minimized.

masell commented Apr 20, 2018

@vitaly-krugl reverted the tbl_encoded.

@masell

This comment has been minimized.

masell commented Apr 21, 2018

Please note that in python 2.X, a byte (type X) is a str as they do not distinguish the types. while in 3.X its as bytes b"". This the reason for checking if its PY3 in the test.

Python 2.7.14 (default, Sep 23 2017, 22:06:14) 
[GCC 7.2.0] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> b"a" == u"a"
True
Python 3.6.3 (default, Oct  3 2017, 21:45:48) 
[GCC 7.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> b"a" == u"a"
False
@hairyhum

This comment has been minimized.

Contributor

hairyhum commented Apr 23, 2018

@masell it looks like there is no test to check the x type parsing with 2.7. Can you add one?

@masell

This comment has been minimized.

masell commented Apr 23, 2018

@hairyhum Same test, different fixture for 2.7

https://github.com/pika/pika/pull/1011/files#diff-6d92c2a692fc77fd16dcd1bc9b922392R35

Means,that in 2.7 i do not change behavior, and type X will not do anything, as the type "S" already do it.

@hairyhum

This comment has been minimized.

Contributor

hairyhum commented Apr 23, 2018

@masell yes, but it uses type S, not x. Or do I miss something?

Martin Åsell
@masell

This comment has been minimized.

masell commented Apr 23, 2018

@hairyhum
Added new test for decoding.
fca2ec9

@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Apr 23, 2018

@masell, the AMQP 0.9.1 specification defines the following types, but there is no X/x. Could you please attach a URL to the applicable AMQP specification that defines x?

field-value = 't' boolean
 / 'b' short-short-int
 / 'B' short-short-uint
 / 'U' short-int
 / 'u' short-uint
 / 'I' long-int
 / 'i' long-uint
 / 'L' long-long-int
 / 'l' long-long-uint
 / 'f' float
 / 'd' double
 / 'D' decimal-value
 / 's' short-string
Advanced Message Queuing Protocol Specification v0-9-1 Page 31 of 39
 Copyright (c) 2006-2008. All rights reserved. See Notice and License. Technical Specifications
 / 'S' long-string
 / 'A' field-array
 / 'T' timestamp
 / 'F' field-table
 / 'V' ; no field

@lukebakken lukebakken removed this from the 0.12 milestone Apr 23, 2018

@lukebakken

This comment has been minimized.

Contributor

lukebakken commented Apr 23, 2018

@vitaly-krugl - good point. @masell links to this document which mentions this non-standard extension:

The sole reason for the existence of this class is to permit encoding of byte[] as 'x' in AMQP field tables, an extension to the specification that is part of the tentative JMS mapping implemented by QPid

@masell also says that the sending application he is using is a "java spring application". It looks as though the RabbitMQ Java client supports the x encoding as well: link

Type x is also supported in the Ruby amq-protocol project: link

It seems valid to add it to Pika as well, IMHO.

@lukebakken lukebakken modified the milestones: 0.12, 1.0.0 Apr 23, 2018

@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Apr 24, 2018

@lukebakken, so what's the semantic difference between byte[] as represented by 'x' and raw byte[] as represented by 'S' ?

Is it lack of implicit conversion to Unicode?

@masell

This comment has been minimized.

masell commented Apr 24, 2018

@vitaly-krugl
"S" is string.
"x" is byte[].

"S" is byte[] on the wire, that is unicode encoded. Decoded would be str() in python3/2, System.String in java.
"x" is byte[] with no encoding, a data blob. Can even be a JPEG if you wish. Decoded would be a bytes() in python3, a byte[] in java. Python2 lacks a byte representation AFAIK, so its the same as str in python2.

@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Apr 26, 2018

@lukebakken, would you mind completing this review? Thx!

@lukebakken

This comment has been minimized.

Contributor

lukebakken commented Apr 26, 2018

@vitaly-krugl I'll get it today. I've had a cold this week so things are going slowly.

@lukebakken

This comment has been minimized.

Contributor

lukebakken commented Apr 27, 2018

@vitaly-krugl 👍 from me, could you update your review?

@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Apr 27, 2018

@lukebakken - done!

@lukebakken lukebakken merged commit bafbf2a into pika:master Apr 27, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

lukebakken added a commit that referenced this pull request May 16, 2018

lukebakken added a commit that referenced this pull request May 16, 2018

Merge pull request #1011 from masell/master
Add support for type "x"

(cherry picked from commit bafbf2a)

lukebakken added a commit that referenced this pull request May 16, 2018

lukebakken added a commit that referenced this pull request May 16, 2018

Merge pull request #1011 from masell/master
Add support for type "x"

(cherry picked from commit bafbf2a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment