Fix short integer encoding

[arkq]

Messages encoded with this library are not conformant with AMQP specification - such an incorrect message might crash client (written in other language, in my case py-amqp-1.4.9) which is conformant with the AMQP 0-9-1 specification.

In short: s is for short string, not for short integer. For signed short integer is U.

Reference:
https://www.rabbitmq.com/resources/specs/amqp0-9-1.pdf (Technical Specifications, page 31)

PS.
This commit also changes signedness of the long integer. From now, every integer is encoded as a signed one.

[michaelklishin]

@arkq are you aware of the errata document? The 0-9-1 spec is known to have inconsistencies and contradictions when it comes to table encoding. At this point client library maintainers are better off by following what the Java (and .NET, and Ruby, and many other) clients use than blindly following the spec.

The prefix for short signed integers indeed seems to be an s there.

[michaelklishin]

I strongly oppose this change. What needs to change is py-amqp (or you should move to Pika if you can), not most other clients. There is no right or wrong way of encoding table values in this case: the errata document was produced because the spec has contradictions that were never clarified (the work then switched to AMQP 0.10, then 1.0).

Here's a list of popular clients that encode strings using S and s (or I, depending on what numerical types are supported or common in a particular language) for short signed integers:

• Java client
• .NET client
• Bunny
• Pika
• php-amqplib

By introducing this change you will break this client's interoperability with those client and gain interoperability with py-amqp. The loss would be much greater than the gain if you ask me.

@squaremo would you agree with me?

[arkq]

@michaelklishin thanks for the errata link. I was not aware of this document. During my battle with this strange bug (encoding incompatibilities), I've stumbled upon 0.9.1 spec and I've thought that it is the "master". If you say, that Pika is a way to go, I will see if it is possible in my case.

Many thanks for such a quick response!

I'm closing this PR as invalid.

[michaelklishin]

@arkq thank you. I completely understand that this is very confusing. The AMQP 0-9-1 spec had a rough childhood.

[squaremo]

@michaelklishin Yes I agree; the codec here is carefully crafted to be compatible with RabbitMQ and do what (most) other client libraries do.

Unfortunately, the reason that one has to take a lot of care also means that it's not at all obvious why it's the way it is!

[arkq]

After some investigation, I have learned, that pika is NOT a way to go (pika-0.10.0) if one wants interoperability with clients conformant with AMQP 0.9.1 + errata. Unfortunately, that was after, I've ported everything to pika... :)

So, for anyone who has stumbled upon similar issue, the solution is:

• haigha
  or
• amqpstorm (due to usage of pamqp)

PS.
I don't know if anyone who maintains rabbitmq developer tools website (https://www.rabbitmq.com/devtools.html) reads these comments, but if so, I would recommend to investigate, which client is conformant which which specification. This might help people like me in the future.

PS2.
Currently I'm using: amqp.node for Node, haigha for Python, amqp for Go, and spring-amqp for Java. Every library seems to cooperate with each other as expected.

[michaelklishin]

We intend to work on Pika instead of moving the tutorials for other clients. Pika is certainly fine as far as interoperability goes in most places. Attribute tables was or still is an edge case in most clients.

[arkq]

@michaelklishin that's fine. Tutorial is only an example (imho). It could have been written in a pseudocode as well, it would serve its purpose. However, I'm not complaining about the tutorial, but about the list of RabbitMQ libraries which is kinda misleading (for edge cases).

<cool-story>
For a past few years I (actually the company I'm working for) have been using kombu with py-amqp. There was no problems, because we've been using mostly Python for rabbitmq-related work. Even recently, when the variety of used languages has expanded (including amqp.node), we have encountered no problems at all. Because our RabbitMQ usage was kind of "standard". Send and receive message without any customizations. However, few days ago I've added custom headers while sending messages and... kabooom. Amqp.node has encoded integer value 1000 as a short integer "s", which has destroyed our ecosystem :D
So, for most cases these libraries are fine, but someday....
</cool-story>

Maybe the solution would be to change the amqp.node after all. E.g.: Encode integers in a compatible way with all clients - as an integer. Decoding will stay as it is now. This would be similar to Pika - which for encoding uses an intersection of types from 0.9.1 and 0.9.1-errata. In such a way there is 100% guaranteed compatibility with all clients.

[squaremo]

However, few days ago I've added custom headers while sending messages and... kabooom. Amqp.node has encoded integer value 1000 as a short integer "s", which has destroyed our ecosystem :D

Since JavaScript only has the one numerical type, double, I made a guess about what was the most useful way to encode numerical values: by prioritising precision, then size of encoding, more or less. But it's impossible to get this right for everyone. (Note for protocol designers: don't include a "generic" encoding for values.)

There's a suggestion that may turn into a PR: #358. This would let you specify that a value is specifically a long, or short, or whatever. Possibly I ought to have just required people to do this all along! (But then people using other dynamic languages would have found it less convenient, ho hum).

[arkq]

Possibly I ought to have just required people to do this all along!

For JS it would be a little bit strange. Like you've said, there is only one numerical type. However, it is possible to determine whether numerical value is an integer or not. So this approach of encoding floats and integers in a separate way is OK (IMHO).

I've seen issue #358 before. It would be possible to mitigate my problem by using integer (int32). And to be fair, we've got exactly the same problem as described in the issue #358. In fact this problem appears in Go as well.

However, right now I don't care much about the encoding. Changing Python library has resolved my issues. And the problem with dynamically selected integer type is quite easy to overcome anyway.

I will not bump this thread any more. Once again many tanks to @michaelklishin, who has pointed me in a right direction - amqp spec incompatibilities.

[michaelklishin]

@arkq that's borderline overkill and an overly smart solution. Dynamically typed languages will always struggle with the plethora of numerical types available in/to their statically typed counterparts. JavaScript is particularly limited in this area.

Unless you have to use headers exchanges (which is an exception, not the norm), I'd recommend sticking your values into message payload, and encoding the entire thing using protocol buffers.

I think the idea in #358 is quite good, given the circumstances.