search

bakkerthehacker / bramqp

A radical, raw, robust, remarkable, rapid AMQP library for node.js
MIT License
162 stars 20 forks source link

Breaking changes introduced by assertions from the specification #55

Open aartiles opened 7 years ago

aartiles commented 7 years ago

The assertions from the specification in this commit https://github.com/bakkerthehacker/bramqp/commit/dbf94bdd25fe64c292036b9e7bb3719bf8f1d40c introduced breaking changes. In our case we were using whitespaces in the queue names as our Rabbitmq server doesn't validate it and this assertion is failing: https://github.com/bakkerthehacker/bramqp/blob/master/specification/rabbitmq/full/amqp0-9-1.stripped.extended.xml#L87 I understand that strictly following the specification is the right way but any workaround without change the queue names would be very helpful.

bakkerthehacker commented 7 years ago

Hi! Thanks for bringing up your usage and problems with bramqp. I'm guessing you have been using bramqp before and updated to find things broken. I would love to help find a fix for this.

introduced breaking changes

First I just wanted to point out that bramqp and most other packages follow semver. bramqp is still using major version 0. This means that every change can be a breaking change. To work around this and provide some consistency, usually the minor version number is bumped when breaking changes are made and the patch version is bumped every other time.

In version 0.5.0 of bramqp, I intentionally added breaking changes. I have done this in the past with verion 0.2.0 which changed the setup api. A big result of the 0.5.0 release that was breaking was the close methods requiring a non-zero number (such as 200) as the code. Just wanted to make sure you were aware.

Rabbitmq server doesn't validate it

This is a bug, in my opinion. The RabbitMQ Reference and the xml spec both agree that space is not a valid character in the queue name.

The RabbitMQ Compatibility and Conformance lists this issue in the relevant section:

The queue name can be empty, or a sequence of these characters: letters, digits, hyphen, underscore, period, or colon. Notes: The lexicon is not enforced by the server.

Just wanted to make sure we are both on the same page, you probably understand all of this already.

Now as to what to do to resolve this issue. I see you have a fork with the regexs modified in the spec. Personally, I don't like this solution. It also glosses over the other invalid characters which RabbitMQ might blindly accept.

A better solution might be to disable optionally certain specific assertions, or to optionally disable assertions altogether.

An interim solution would be to just pin version 0.4.0. Not much has changed since then aside from the assertions and qpid types.

aartiles commented 7 years ago

Hi, thanks so much for the quick response. Yes, I'm aware that it's RabbitMQ's fault and also ours of course, for not complying with the specifications. Probably due to this The lexicon is not enforced by the server..

As we have been using RabbitMQ for 4 years we have more that 20 queues and exchanges using whitespaces so renaming them in production is not that easy. That's why as a first step, to be up to date with your package we have forked and patched it while we are able to rename all the queues.

Using v0.4.0 seems a good option too, now that I know that we won't take advantage of any of its changes, but anyway we will need to rename the queues at some point.

Fortunately, this is the only assertion affecting us but it could be worse so having an option to disable the assertions is not a bad idea for future affected people that need to upgrade.