rabbitmq / rabbitmq-stream-dotnet-client

RabbitMQ client for the stream protocol
https://rabbitmq.github.io/rabbitmq-stream-dotnet-client/stable/htmlsingle/index.html
Other
122 stars 41 forks source link

Update ReadAny to skip over lists and maps #372

Closed ngbrown closed 7 months ago

ngbrown commented 7 months ago

Addresses #371 enough to at least get the client working again with RabbitMQ 3.13.

For now, this just returns null as the value for lists and maps. The project can decide how to correctly handle this, since the x-received-from message annotation has the following structure:

{
  "x-received-from": [
    {
      "uri": "amqp://something",
      "exchange": "events",
      "redelivered": true,
      "cluster-name": "rabbit@something",
      "vhost": "./"
    }
  ]
}

The x-shoveled message annotation has the following structure:

{
  "x-shovelled": [
    {
      "shovelled-by": "rabbit@820c50d2107b",
      "shovel-type": "dynamic",
      "shovel-name": "events-from-...",
      "shovel-vhost": "./",
      "src-uri": "amqp://somewhere",
      "dest-uri": "amqp://",
      "src-exchange": "events",
      "src-exchange-key": "#",
      "dest-exchange": "events"
    }
  ]
}

But since the message annotation of AMQP 1.0 is completely flexible, this client shouldn't lock into just the current value types. Maybe return generic List<object> and Dictionary<object, object>?

I have a patch-any-2 branch toward that end, but no tests, and breaking API changes to enable use of the existing Map<>...

Gsantomaggio commented 7 months ago

According to the AMQP 1.0 specification, application properties are only flat values; annotations can be flat and list.

https://www.rabbitmq.com/docs/conversions#amqpl-amqp

The current parser handles all the maps in the same way. We'd need to distingue when we are parsing: application properties or annotations in the first case your fix is correct. In the second case, we should handle it ( like you are doing in the other branch )

and breaking API changes to enable use of the existing Map<>

I want to avoid breaking changes. I'd merge and release this fix for the moment. Then, implement the annotation list.

Thank you for the fix

Gsantomaggio commented 7 months ago

FYI for the moment : https://github.com/rabbitmq/rabbitmq-stream-dotnet-client/releases/tag/v1.8.3

Thank you for the fix