majek / puka

Puka - the opinionated RabbitMQ client
https://github.com/majek/puka
Other
182 stars 34 forks source link

parse_amqp_url doesn't handle empty vhost #45

Closed schmir closed 11 years ago

schmir commented 11 years ago

puka.connection.parse_amqp_url violates RabbitMQ's AMQP URI Specification (http://www.rabbitmq.com/uri-spec.html)

parsing amqp://localhost/ should result in an empty vhost, but puka uses '/' as vhost.

There's a comment in parse_puka_connection that reads:

# We do not support empty vhost case. Empty vhost is treated as
# '/'. This is mostly for backwards compatibility, and the fact
# that empty vhost is not very useful.

I think it would be better if puka favoured interoperability with other implementations over backwards compatibility. (puka is at version 0.0.6)

In my case I'm trying to configure a clojure program and a python program with the same .ini based config file. the clojure library conforms to the above specification and consequently both programs 'disagree' on the connection parameters.

majek commented 11 years ago

Puka url parsing was introduced before the rabbitmq uri spec was defined. People already use Puka in production and for quite some time we've suggested relying on defaults (ie: default "/" vhost). Making puka parsing compliant would introduce potential confusion for users without any benefit. I don't think that specifying empty vhost makes any sense thus decided not to change puka's behaviour.

schmir commented 11 years ago

well, the confusion is already there when the user expects rabbitmq spec compliant parsing of rabbitmq urls.

I can understand that you don't want to change the current behaviour in order to maintain backward compatibility. However, the gratuitious fact that you don't think empty vhosts do make sense, isn't a reason to not change it. That argument is bogus. It would probably be a reason to bail out if a user specifies an empty vhost. But that doesn't happen, instead it silently chooses some other vhost.

so, can we at least introduce a way to initialize a Client object by specifying connection parameters like host, vhost, user, password, port directly instead of packing them into an amqp_url string?

majek commented 11 years ago

well, the confusion is already there when the user expects rabbitmq spec compliant parsing of rabbitmq urls.

... and tries to get empty vhost to work. Unlikely scenario.

can we at least introduce a way to initialize a Client object by specifying connection parameters like host, vhost, user, password, port directly instead of packing them into an amqp_url string?

I still don't get it. What useful functionality would it introduce to Puka that it doesn't provide already? Puka is an "opinionated" client, some things are just impossible by design. For example - non blocking publishing (publish always uses a publisher-confirm, thus if you want to fill your bandwidth with publishes you can get better performance using a different client), dealing with channels is not possible from user application, or even weirder things - did you know that you can't set say "content_type" header in puka? (you can set a header property though, properties and headers are mixed in the API: https://github.com/majek/puka/blob/master/puka/spec.py#L838 )

If I understand you correctly - you want the url scheme to be fully compatible between clients. Fair enough. But in this one corner case (empty vhost) I just don't think it's worth the work. If we wanted to do it, we'd need to do it in two steps: 1) print warning that empty vhost is going to change its meaning 2) wait few months and change the code. Again, in this case I don't think it's worth it, I don't think supporting empty vhosts makes any practical sense and I do prefer backwards compatibility here. Am I missing something?

Additionally, the uri scheme is not perfect. It doesn't say anything about SSL. I'd love to be able to set client certificate from the URI and change if the server certificate should be verified against CA or not. But that's not defined in the RabbitMQ URI scheme, so it's likely we're going to diverge from it even more.

schmir commented 11 years ago

I still don't get it. What useful functionality would it introduce to Puka that it doesn't provide already?

well, it allows to pass in parameters to a connect function without serializing them to an uri/string. that's not possible at the moment and therefor I think it's useful functionality.

Additionally, the uri scheme is not perfect. It doesn't say anything about SSL. I'd love to be able to set client certificate from the URI and change if the server certificate should be verified against CA or not. But that's not defined in the RabbitMQ URI scheme, so it's likely we're going to diverge from it even more.

I would few the above SSL use-case as a reason to implement the exact functionality that I asked for. Encoding all SSL parameters into that URI is what I would call insane.

what's wrong with

def make_client(host="localhost", vhost="/", port=..., ...) ?

You can give that to any python programmer and he knows how to work with that.

schmir commented 11 years ago

s/few/view/ in my last comment

Marek Majkowski notifications@github.com writes:

well, the confusion is already there when the user expects rabbitmq spec compliant parsing of rabbitmq urls.

... and tries to get empty vhost to work. Unlikely scenario.

if you assume that I want to get empty vhost working, you're wrong. that's really not what I'm trying to achieve. I just want to configure two different rabbitmq client libraries from the same ini file. And the fact that they disagree on the interpretation of the rabbitmq url makes it very likely that they connect with different vhost settings.

but enough of that now, I guess I'll have to live with puka and it's author being opinionated :)