Some fixes and additional features

[adamsussman]

Hi, I have added in decode support for all AMQP field types and added a few more encodings for types that were missing. I was getting crashes that could not clear bad messages from a queue because of unrecognized field types in message headers. If you prefer not to add these, then can we at least make unrecognized types a non-fatal issue?

Also, I have added the ability to set connection properties.

Please review and let me know if you have any problems.

thanks,

-adam

[majek]

1) In future, please send separate pull requests for every feature. It's okay to have too many pull requests, it's not okay to have two completely different things done in a single one.

2) You extend decode_value by adding support for s, f, d, V and few integer types while encode_value is only extended by V and d. Why?

For the reference, here's the table of types we follow: http://www.rabbitmq.com/amqp-0-9-1-errata.html#section_3

3) You say:

  # RFC 1832 XDR (struct module only does IEE 754 doubles, not the same)

Why? Why struct's d type is wrong?

4) As you see, there are doc tests for the encode and decode functions, please extend them to cover new code.

5) client_properties look okay, but why this is useful?

6) client_properties also require some basic tests. You may extend test_basic.TestBasic class.

[majek]

7) In python long type has unlimited precision. Currently we just encode it as l (unsigned 64 bit), but use struct.pack(q), which is signed. This is a bug. Also, we should use signed/unsigned depending on a value.

[adamsussman]

Thanks for the quick feedback:

1) In future, please send separate pull requests for every feature. It's okay to have too many pull requests, it's not okay to have two completely different things done in a single one.

Will do.

2) You extend decode_value by adding support for s, f, d, V and few integer types while encode_value is only extended by V and d. Why?

My original need to make a change was due to the decode breaking on unsupported types that were coming in from other AMQP libraries (in particular, the Java reference library from Rabbit). So I tried to cover every possible type there.

For the sake of completeness, I tried to do the same thing on the encode side, but I ran into problems with Rabbit itself.

Most every other integer type I added to the encode was rejected by my RabbitMQ test servers. Its not yet clear why, but for now I decided to go with the least breaking changes I could make. In the encode, there was no support at all for real number or the 'no-field' type and I was able to successfully add those. Every other missing type was either a smaller footprint version of an already supported type, or a signed vs unsigned integer type (which I could not get to work with Rabbit).

For the reference, here's the table of types we follow: http://www.rabbitmq.com/amqp-0-9-1-errata.html#section_3

3) You say:

     # RFC 1832 XDR (struct module only does IEE 754 doubles, not the same)

Why? Why struct's d type is wrong?

The 'official' 0-9-1 spec goes out of its way to specify f as IEE 754 and d as RFC 1832 XDR. I see however that XDR references the IEE spec. I will look deeper and if they are the same, I will change it to the struct.

4) As you see, there are doc tests for the encode and decode functions, please extend them to cover new code.

Will do. Do you know of an easy way to unit test these encodings against a real Rabbit server? That's where I had the most problems.

5) client_properties look okay, but why this is useful?

The values of client properties are viewable in the Rabbit admin tools and management plugin functions. Having this information there is very useful operationally when there are lots of connections to Rabbit from the same client machine with different applications. It allows you to put in some distinguishing information about what application is connected where and which application is doing what. It is especially useful to distinguish applications that use anonymous named queues.

6) client_properties also require some basic tests. You may extend test_basic.TestBasic class.

Will do.

-adam

[adamsussman]

7) In python long type has unlimited precision. Currently we just encode it as l (unsigned 64 bit), but use struct.pack(q), which is signed. This is a bug. Also, we should use signed/unsigned depending on a value

Since I had so many problems with the Integer types in the encode, I decided to leave them alone. Its unclear to me what should be done for an integer larger than 64 bits. Convert to real? String?

[majek]

For the reference, here's the table of types we follow: http://www.rabbitmq.com/amqp-0-9-1-errata.html#section_3

3) You say:

     # RFC 1832 XDR (struct module only does IEE 754 doubles, not the same)

Why? Why struct's d type is wrong?

The 'official' 0-9-1 spec goes out of its way to specify f as IEE 754 and d as RFC 1832 XDR.  I see however that XDR references the IEE spec.  I will look deeper and if they are the same, I will change it to the struct.

I'm not saying it's wrong. I'm just curious.

4) As you see, there are doc tests for the encode and decode functions, please extend them to cover new code.

Will do.  Do you know of an easy way to unit test these encodings against a real Rabbit server?  That's where I had the most problems.

Sure. Here's one test that just sends properties expecting to get the same data back: https://github.com/majek/puka/blob/master/tests/test_basic.py#L282

5) client_properties look okay, but why this is useful?

The values of client properties are viewable in the Rabbit admin tools and management plugin functions.  Having this information there is very useful operationally when there are lots of connections to Rabbit from the same client machine with different applications.  It allows you to put in some distinguishing information about what application is connected where and which application is doing what.  It is especially useful to distinguish applications that use anonymous named queues.

Makes sense!

[adamsussman]

Sure. Here's one test that just sends properties expecting to get the same data back: https://github.com/majek/puka/blob/master/tests/test_basic.py#L282

Thanks, I will put some tests here. By the way, test_basic currently fails for me on a fresh clone with:

ERROR: test_basic_reject_dead_letter_exchange (main.TestBasic)

Traceback (most recent call last): File "tests/test_basic.py", line 271, in test_basic_reject_dead_letter_exchange self.assertEqual(r['body'], 'a') KeyError: 'body'

[adamsussman]

On Thu, Jun 14, 2012 at 4:07 AM, Marek Majkowski reply@reply.github.com wrote:

1) In future, please send separate pull requests for every feature. It's okay to have too many pull requests, it's not okay to have two completely different things done in a single one. 2) You extend decode_value by adding support for s, f, d, V and few integer types while encode_value is only extended by V and d. Why?

For the reference, here's the table of types we follow: http://www.rabbitmq.com/amqp-0-9-1-errata.html#section_3

I see now. Rabbit doesn't actually follow the 0-9-1 standard. I will implement based on Rabbit's internals.

-adam