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

fix: add unsigned short support in table deserialisation #1270

Merged
merged 3 commits into from
Oct 18, 2022

Conversation

gytsen
Copy link
Contributor

@gytsen gytsen commented Oct 17, 2022

While it seems that this wasn't needed in the lifetime of this library so far, it seems weird not to support the full protocol as stated on the RabbitMQ website (including errata).

While I've been unable to reproduce it locally with a local install of RabbitMQ server, I have encountered at least one instance where a server sent a numeric header value that fit into an unsigned short over the wire. My extremely unscientific hypothesis is that a strict client pushed this message onto the queue which the server passed on directly, but I could be wrong here.

Bottom line is: I have a crashing RabbitMQ.Client that says it doesn't understand the field type 'u' while the docs clearly say it's part of the protocol.

This commit adds parsing support for unsigned shorts, which should ensure the library conforms fully to the protocol in those few edge cases where it is still needed.

I added tests in the TestFieldTableFormatting and TestFieldTableFormattingGeneric files since that seemed the most logical place to put them, but I could have missed a more appropriate test.

Inspired by: #662

Proposed Changes

As described in #662, RabbitMQ.Client does not support unsigned short reading out of the box. Since this is a trivial addition with relatively low-impact by my estimates, I'd figure I'd add it to ensure the library is not at fault when facing stricter or older servers.

As for the nature of the change, it would be and observable change in existing systems in that the library doesn't crash anymore when RMQ servers send unsigned shorts over the wire. I'm not sure if this is considered breaking or not.

I had to change the writer for the roundtrip tests as well, but that poses a conundrum: where previously unsigned shorts were
explicitly rejected, it is now accepted and serialized. That is a pretty big change to make, so I'm pretty sure that's a breaking change. Especially since it seems that some libraries (I checked node-amqp real quick) just do not support these types. Then again, there's a pull request already open to fix that.

Types of Changes

  • Bug fix (non-breaking change which fixes issue (un)signed shorts in table serialisation #662)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

While it seems that this wasn't needed in the lifetime of this library
so far, it seems weird not to support the full protocol as stated on the
RabbitMQ website (including errata).

This commit adds parsing support for the 'u' case, which should ensure
the library conforms fully to the protocol in those few edge cases where
it is still needed.

Inspired by: rabbitmq#662
@michaelklishin
Copy link
Member

Most clients do not support unsigned types because they are either absent or extremely rarely used in their respective programming languages/communities.

This should not be a breaking change for systems that still do not use unsigned shorts.

@michaelklishin michaelklishin merged commit f9d5952 into rabbitmq:main Oct 18, 2022
@michaelklishin
Copy link
Member

Thank you!

@michaelklishin
Copy link
Member

I think this is safe to backport.

@gytsen
Copy link
Contributor Author

gytsen commented Oct 18, 2022

I could take a stab at a backport, which versions would need it?

@michaelklishin
Copy link
Member

Just cherry-picking this to 6.x would be enough. Thank you.

@gytsen
Copy link
Contributor Author

gytsen commented Oct 18, 2022

Alright, I'll try to get a PR ready for it later this evening for you

gytsen added a commit to gytsen/rabbitmq-dotnet-client that referenced this pull request Oct 18, 2022
It appears the library did not support reading or writing unsigned
shorts in the wire protocol, even though RabbitMQ's AMQP 0.9.1 flavor
supports it.

This commit adds both reading and writing of unsigned short values, and
the relevant tests for this feature.

backport of PR rabbitmq#1270
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants