Azure / go-amqp

AMQP 1.0 client library for Go.
https://github.com/Azure/go-amqp
MIT License
110 stars 58 forks source link

Incorrect TypeCode encode when the message body is nil #332

Closed Gsantomaggio closed 2 months ago

Gsantomaggio commented 2 months ago

When the body message is nil the client should encode TypeCodeNull value.

Let's go more in-depth :)!

The behaviour between .NET amqpnetlite and this codec differs.


amqpnetlite

Given a message with the null body with the amqpnetlite :

Message nullEmptyMessage = new Message(null);
var buff =nullEmptyMessage.Encode();

[0] = {byte} 0
[1] = {byte} 83 // TypeCodeSmallUlong
[2] = {byte} 119 // TypeCodeAMQPValue 
[3] = {byte} 64 // TypeCodeNull

āœ… This behaviour is correct.


go-amqp

nilGolangMessage := amqp.NewMessage(nil)
binary, _ := nilGolangMessage.MarshalBinary()

[0] = {byte} 0
1 = {uint8} 83  // TypeCodeSmallUlong
2 = {uint8} 117 // TypeCodeApplicationData
3 = {uint8} 160 // TypeCodeVbin8
4 = {uint8} 0

āŒ That, I think, is a sort of bug when the body is nil; the client should encode TypeCodeNull


šŸ’” Possible Fix

It works with this possible fix https://github.com/Gsantomaggio/go-amqp/blob/null_value/message.go#L124-L133

nilGolangMessage := amqp.NewMessageWithValue(nil)
binary, _ := nilGolangMessage.MarshalBinary()

0 = {uint8} 0
1 = {uint8} 83 // TypeCodeSmallUlong
2 = {uint8} 119 // TypeCodeAMQPValue 
3 = {uint8} 64 // TypeCodeNull

oasis documentation

The problem statement is summarised as follows: "The client should be able to send a body consisting of a single amqp-value section (https://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#type-amqp-value) which in turn consists of the AMQP null value (https://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-types-v1.0-os.html#type-null)"


Thank you

Gsantomaggio commented 2 months ago

We use this in our RabbitMQ AMQP 1.0 client, here.

Thank you

jhendrixMSFT commented 2 months ago

Thanks for the detailed bug report, it really helps. I haven't had a chance to dig into this too much yet but was wondering if the current behavior is causing issues/incorrect behavior?

Gsantomaggio commented 2 months ago

but was wondering if the current behavior is causing issues/incorrect behavior?

Yes, unfortunately :)

Let me explain: In RabbitMQ, we released native support for AMQP 1.0

There is an implementation REST-like for dealing with resource creation, such as queues, exchanges, and bindings.

To delete a resource, we send a message with a NULL (TypeCodeNull) value. See here

The other clients send TypeCodeSmallUlong, TypeCodeAMQPValue, TypeCodeNull in case of null/nil message , so it works correctly:

The Go AMQP 1.0 Client currently uses this client as a base client, and It is not possible to send TypeCodeAMQPValue with a TypeCodeNull value āŒ.

That's why I temporarily did this fix here: https://github.com/Gsantomaggio/go-amqp/blob/null_value/message.go#L124-L133. Please let me know if it makes sense to PR it. Any suggestion is welcome :)

Thank you