Skip to content
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

Go → rabbitmq type mappings #391

Closed
TheCount opened this issue Mar 5, 2019 · 10 comments · Fixed by rabbitmq/amqp091-go#26
Closed

Go → rabbitmq type mappings #391

TheCount opened this issue Mar 5, 2019 · 10 comments · Fixed by rabbitmq/amqp091-go#26

Comments

@TheCount
Copy link

TheCount commented Mar 5, 2019

This package currently maps the Go type byte (equivalent to uint8) in an amqp.Table to the RabbitMQ type b, which is a signed integer. The correct type would be B (see section 3 of http://www.rabbitmq.com/amqp-0-9-1-errata.html, third column in the table).

Furthermore, the following "obvious" type mappings from Go to RabbitMQ are missing completely:

  • int8b,
  • uint16u,
  • uint32i

For completeness, uint8 and uint could be added as well.

I could create a pull request if you like.

@michaelklishin
Copy link
Collaborator

@TheCount I'd say go for it. The b => B switch is a bug fix from where I sit. I don't think headers of that type are particularly common in any client.

@streadway @gerhard any objections?

@streadway
Copy link
Owner

streadway commented Mar 6, 2019

For an end-to-end test, or a test between Go <-> Java, how would you map the table tags back to Go types?

It may not be obvious for Go <-> Java interop that the 'B' tag is not supported. In Java, Byte is encoded as b. Java's Byte is signed when it is cast to an int.

https://github.com/rabbitmq/rabbitmq-java-client/blob/b55bd20a1a236fc2d1ea9369b579770fa0237615/src/main/java/com/rabbitmq/client/impl/ValueWriter.java#L166-L168

In .NET the b tag field value is a signed byte int8.

https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/master/projects/client/RabbitMQ.Client/src/client/impl/WireFormatting.cs#L136-L138

I'm not certain we should change b since AMQP is an integration protocol.

For type completeness for u and i, I believe there is ambiguity on the decoding side. If you were to write an exhaustive test for writing and reading all primitive types as field values, would they be equivalent with these two extra tags?

What would be valuable is better documentation for the field value encodings, especially with regards to RabbitMQ plugins, and Java/.NET expectations.

@TheCount
Copy link
Author

TheCount commented Mar 6, 2019

It may not be obvious for Go <-> Java interop that the 'B' tag is not supported. In Java, Byte is encoded as b.

https://github.com/rabbitmq/rabbitmq-java-client/blob/b55bd20a1a236fc2d1ea9369b579770fa0237615/src/main/java/com/rabbitmq/client/impl/ValueWriter.java#L166-L168

In .NET the b tag field value is a signed byte int8.

https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/master/projects/client/RabbitMQ.Client/src/client/impl/WireFormatting.cs#L136-L138

I'm not certain we should change b since AMQP is an integration protocol.

But this is the correct behaviour, isn't it? In Java, byte is a signed type (unlike in golang, where byte is unsigned), so changing byte to int8 for b in golang would exactly reproduce the behaviour for Java and .NET. Am I missing something?

For type completeness for u and i, I believe there is ambiguity on the decoding side. If you were to write an exhaustive test for writing and reading all primitive types as field values, would they be equivalent with these two extra tags?

What would be valuable is better documentation for the field value encodings, especially with regards to RabbitMQ plugins, and Java/.NET expectations.

Adding uint16, and uint32 (u, and i, respectively) would not create additional ambiguity, but uint would. This ambiguity could be resolved analogously to the current int ambiguity (i. e., encoding uint as i, and decoding i as uint32).

As for uint8, let's first discuss how to deal with the b vs. B issue (even if b maps to int8 in the future, the question still remains whether B should decode to byte or uint8).

@streadway
Copy link
Owner

Taking a step back - could you please explain the scenario that you have where the Go encoding rules and cross-client supported field values are not sufficient?

@TheCount
Copy link
Author

TheCount commented Mar 6, 2019

Taking a step back - could you please explain the scenario that you have where the Go encoding rules and cross-client supported field values are not sufficient?

We have a couple of cases where we want to receive a message M1 from third party A, process the body, and then send a new message M2 to A or another third party B. M2 is supposed to reproduce all the headers from M1 that don't get specifically defined treatment. The client A uses is not specified. So, in general, we'd need to support the whole spectrum of RabbitMQ types.

That being said, we're still in the draft phase, so adding a requirement to the effect of "no headers may be 8-bit or unsigned" should hopefully not cause trouble. So, no sweat for now.

Taking a step forward again: if I understand you correctly, there are some clients out there which do not implement the full spectrum of RabbitMQ types on the decoding side, and you don't want to break them by golang software sending them an unsupported type? Makes sense if this is a common limitation, though I think in this case those clients should be fixed in the long run. Or you could argue the plethora of types in the RabbitMQ spec is kind of a defect (it's clear that not all of them are terribly useful, cf. decimal).

@streadway
Copy link
Owner

We have a couple of cases where we want to receive a message M1 from third party A, process the body, and then send a new message M2 to A or another third party B. M2 is supposed to reproduce all the headers from M1 that don't get specifically defined treatment. The client A uses is not specified. So, in general, we'd need to support the whole spectrum of RabbitMQ types.

This is very helpful! In this case, introducing something closer to a RawField, like the json.RawMessage that contains the slice of the uninterpreted wire protocol would be a more general approach for transparent forwarding.

There were two use cases in mind for designing the table fields: declaring custom exchange properties for RabbitMQ plugins and pub/sub communication between clients. This is where the practical set of supported types came from.

In both of those cases, it only made sense to encode what can be decoded, and visa versa. That, combined with ambiguity in the spec, I chose to go after a few reference implementations for the supported field tags, and pick reasonable corresponding Go primitive types.

As you work through your application's needs, if you have a need for new end-to-end field types, lets take a look at supporting the "relay" use case as well as the "pub/sub".

@michaelklishin
Copy link
Collaborator

In the "relay" case, if a client uses a certain rare type it probably can decode it in a reasonable way or otherwise it wouldn't use said type.

Usually fine grained numerical types are problematic for dynamically typed languages since they often have features such as autopromotion of numbers. They often don't distinguish between signed and unsigned types as well. The most sensible thing to do for such clients is to implement the decoding part. Bunny does that for the byte type even though there is no way to provide a "byte value" (as opposed to a Fixnum which is small enough to fit) in Ruby. I am not aware of cases where there's no reasonable decoding strategy for such languages.

I personally think this client should support more types if it makes sense. Most popular clients can be updated as needed.

Also note that when in doubt, most developers won't use header tables or types that may be hard to express in many languages. We can explicitly call out the risks in the docs but FWIW I recall only a couple of such issues in the last few years.

@TheCount
Copy link
Author

For our application, we have decided to restrict ourselves to the s, I, and l integer types (corresponding to int16, int32, and int64 in golang, respectively) for the time being. Thus, we don't need a code change right now, though a documentation update would be nice.

Regarding the long run, I would also say that failure to decode a valid RabbitMQ header is a client bug, while restrictions on encoding are merely missing features (which golang at least could easily provide, though certain other languages, not so easily). But yes, I understand it's dangerous sending around all kinds of fancy new headers when other clients don't understand them. The correct approach might be to make a survey of clients to see what decoding capabilities they have and possibly file bug reports against them. For example, the link provided by @michaelklishin shows that Bunny raises an exception for all the unsigned integer types.

I might be convinced to perform such a survey at some point, but as I said, for now I'm happy with the status quo.

@streadway
Copy link
Owner

It sounds like you are meeting your needs with the current implementation. Feel free to reopen if we should consider different needs.

@BratSinot
Copy link

So Go, AMQP library still not work with unsigned (despite that AMQP protocol support it)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants