Connection Parameters Handling

[Tensho]

I'd like to return back to the conversation regarding connection parameters defaults, because I was much confused how the current bunny and amp-protocol code handle session (and transport) initialization. We have already discussed that briefly during my work on amp-protocol basic URI query parameters adoption in #69, but I'd like to put your attention to the next circumstance.

AMQ::Settings determines the defaults for the URI string missing components. But the matter is bunny can accept connection parameters in two possible ways simultaneously – map of attributes and uri string. This would not be a problem, if we had unified attribute (option) key names everywhere and both ways accept the whole set of the possible values. But we don't. Here are a couple of issues I met during URI query parameters adoption in bunny:

Example 1. bunny has :automatically_recover option, which is not supported in URI query parameter according to RabbitMQ official documentation. It's not a big deal, but I still convinced automatically_recover parameter in the URI string may confuse users and require at least extra documentation.

Example 2. Imagine user passes cacertfile option in the URI string and tls_ca_certificates in the parameters hash. Which one has precedence? I understand bunny tries to support many variants of the same option (I guess due to backwards compatibility reasons and the lack of deprecation harnesses in the gem). But in such a case it's very hard to handle them all properly for both ways (map of attributes and URI string) without the option names unification.

Example 3. bunny uses AMQ::Settings.parse_amqp_url method, but it doesn't return merged defaults with the parsed parameters. However AMQ::Settings.configure method does.

One more note, I found that bunny already has the concept I wanted to introduce – extensions – exactly to enrich basic amq-protocol behaviour.

AMQP URI Parsing

@michaelklishin, Despite of the fact you don't see much profit from this extraction, I developed uri-amqp gem that conforms URI public API and feels much less painful in the maintenance, than current AMQ::URI and AMQ::Settings tandem. I'm still think it will be a good move to adopt URI generic behaviour and shift URI parsing to a separate gem. I've tried to put their almost maximum test coverage to be sure URI.parse processes all possible cases as expected. Particularly I've addressed everything described in "Appendix A: Examples" of "RabbitMQ URI Specification". Please, take a look there and don't hesitate to say your opinion. I'm ready for all kind of critics ^_^

Proposals

• Specify connection parameter defaults on the client (bunny and amqp gems) side, merge defaults with passed options on the client side and abolish AMQ::Settings in amq-protocol
• Shift URI parsing responsibility to uri-amqp gem. I don't mind to put it under ruby-amqp organisation umbrella if it's necessary.

Expecting Benefits

• URI parsing will become an isolated unit, that is easy to test and maintain
• Connection parameter defaults will be much easier to merge with map of attributes AND URI string together
• No backward compatibility problems will arise

Alternative plan

I assume I could miss something important, so if you still feel all the described above unneeded, consider the next possible variant:

• Normalize all kind of possible options in the beginning of bunny session initialization
• Duplicate defaults across clients and amq-protocol to be able to work with map of attributes AND URI string in the same time

I'd happy to contribute according with the described plan and hear any suggestions 🙇

[michaelklishin]

You are trying to unify things that were never meant to be unified. Client libraries do not have to support parameters in URIs. RabbitMQ plugins such as Shovel and Federation do because there is no other obvious way, and on managed platforms such as Heroku, Cloud Foundry or CloudAMQP there may be no other way whatsoever (all a developer is provided is a URI).

Client libraries don't have to support the same set of keys. Even if that would be nice, historically they use different names and it is too late to change that.

We don't have a problem of a lot of issues popping up in URI parsing. It's an area with decent test coverage, fixed scope and except for typos, not a lot of opportunity to mess up.

tls_ca_certificates and cacertfile passed at the same is not a common occurrence. One of the options is a list of paths, another is a single path. The only reasonable approach is to wrap a single value into a list. However, this won't work for other options. What value should take precedence with two string values? I don't know, presumably what Bunny has been using all along because any other alternative would make no sense to a Bunny user who decides to upgrade.

Maps and URIs are not combined all that often and when they are, it is usually because there is no way to specify certain things in a URI and the users don't care* because there is a way to specify a map.

I'm sorry but you are putting a lot of thought and effort into a problem that largely doesn't exist.

[michaelklishin]

This library ended up being a set of utilities Bunny and amqp gem share on top of a protocol serializer. It was never meant to support URIs used with Shovel and Federation. It never was never meant to support and unify map arguments of amqp gem and Bunny because they have had developed independently for several years before this library had been started.

I'm not sure what problem other than "improved test coverage" (which is a sensible goal if you have an area that gets a lot of bugs reported and is an endless time sink otherwise) we would solve by trying to unify all of the above.

I'm also not sure I have the time and energy to participate in decision making around all the intricate cases and all the subtle and silly bugs that may pop up, as with any moderately sized refactoring in a dynamically typed language.

[michaelklishin]

As I mentioned in ruby-amqp/bunny#526, I don't think Bunny or even amq-protocol has a major code quality problem. It's OK. I am not very interested in PRs that polish, unify, and do other things that do not really reduce the amount of time it takes for me to maintain the Ruby clients.

If you want to help out, consider looking into issues such as ruby-amqp/bunny#522, ruby-amqp/bunny#533 or ruby-amqp/bunny#528. They would make a difference for every Bunny user out there, not just code quality cruisaders.

[Tensho]

Thank you for the open answer 🙇 I accept "not a common occurrence" as a fact and will try to find the way to bring URI parameters to bunny and amqp without much justifying.

Last things you didn't cover and I'd like to sort out – Example 3 and extensions. Please could you give me more insight on AMQ::Settings.configure method and intention of bunny extensions?

[michaelklishin]

Note that I also don't see anything wrong with adding features just to Bunny. amqp gem is on life support, its README explicitly recommends Bunny and March Hare is a pretty different animal (no pun intended) that has a bunch of Java client-specific bits that make no sense for Bunny.

In other words, any further usability improvements around URIs can probably go into Bunny first and if they end up being so useful we want to backport them here, we can always do that.

Thank you for your time.

[michaelklishin]

AMQ::Settings#configure was moved from a now defunct library called "amq-client" (which was a largely failed experiment in unifying amqp gem and Bunny well beyond this library). I don't remember what was the thinking around it, it was probably just moved as is.

If your question is "can we switch Bunny to use that method" I don't see why not, as long as we don't break anything or at least provide enough of a reason for a minor version bump and for the users to adapt their code at some point.