search

FoundatioFx / Foundatio.AzureServiceBus

Foundatio Azure ServiceBus
Apache License 2.0
19 stars 15 forks source link

Trouble deserializing. #23

Open jamescurran opened 4 years ago

jamescurran commented 4 years ago

Everytime I tried reading a message from a queue, I got an exception when it tried to deserialize it. Debugging through the code, I found the data which it is trying to deserialize from JSON was in the following format: (6) "STRING" (196) "http://....(long uri here)" (xx) "{ (actual json here) ....

After some googling, I downloaded the v9.0.0.0 code base, and changed the code for HandleDequeueAsync from

            var message = _serializer.Deserialize<T>(brokeredMessage.Body);`

to

            var messageText = brokeredMessage?.GetBody<string>();
            var message = _serializer.Deserialize<T>(messageText);

recompiled and run with that, and everything worked fine.

This had me a bit confused how you'd allow such an obvious bug in the released (and presumably, tested) code, so I dug a bit further.

I discovered that in commit 4db08359cc813467c4870006b0233294645af1fa (committed on Dec 10, 2018; yassinebennani), that exact code was removed from the source code.

So, I'm confused --- why was that call no longer needed -- and why do I still need it? And how can we create a version which handles both use cases?

jamescurran commented 4 years ago

Researching further, this page tells why & when the two methods should be used:

https://github.com/Azure/azure-sdk-for-net/blob/4bab05144ce647cc9e704d46d3763de5f9681ee0/sdk/servicebus/Microsoft.Azure.ServiceBus/src/Extensions/MessageInterOpExtensions.cs

Bottom line, decoding the message depends on who sent it, and recommends using the User property to determine who (but doesn't explain how).

niemyjski commented 4 years ago

Do you think we could check both properties just in case? Is it just empty? We don't use this integration in production (because we use storage queues), so it isn't as battle tested as I'd like there also could be a unit test issue as we aren't perfect. If you could submit a pr for this and ensure the tests pass / any other issues that would be greatly appreciated.

niemyjski commented 4 years ago

Do you think we could check both properties just in case? Is it just empty? We don't use this integration in production (because we use storage queues), so it isn't as battle tested as I'd like there also could be a unit test issue as we aren't perfect. If you could submit a pr for this and ensure the tests pass / any other issues that would be greatly appreciated.

jamescurran commented 4 years ago

The problem I have with that is that I don't have control over the thing sending the messages, so any changes I make can only be tested against my setup, so I can't prevent them from breaking the current code.

Now,as far as I can see, the older method would always have an 0x06 as the first byte of the message (or at least a hex value of a non-printable character) (the first byte is the length of the ASCII string that follows, which is the name of the data type of the actual data, so it's unlikely to be greater than 31 characters)

So, I can check the first byte of the stream, and then choose the seemingly appropriate method, which is kind-of kludgy, but should work.