CJWorkbench / carehare

Async RabbitMQ client that handles all the edge cases
MIT License
4 stars 0 forks source link

Leading slash in VirtualHost should be stripped #2

Closed Zagrebelin closed 3 years ago

Zagrebelin commented 3 years ago

Accoring RabbitMQ specificatiosn vhost segment does not include leading slash:

amqp_URI = "amqp://" amqp_authority [ "/" vhost ] [ "?" query ] The vhost component of the URI does not include the leading "/" character from the path. This makes it possible to refer to any vhost, not only those that begin with a "/" character.

But carehare uses an 'urlparse' python function (https://github.com/CJWorkbench/carehare/blob/main/carehare/_connection.py#L35 and https://github.com/CJWorkbench/carehare/blob/main/carehare/_connection.py#L60) which does not strip leading slash.

>>> urllib.parse.urlparse(carehare.connect('amqp://guest:guest@172.17.0.1/sample_vhost')._url).path
'/sample_vhost'
adamhooper commented 3 years ago

Thank you for the report!

The slash is there because I misinterpreted the default -- / -- as meaning, a leading slash actually belongs. I guess I was wrong? The screenshots at https://www.cloudamqp.com/blog/what-is-a-rabbitmq-vhost.html suggest RabbitMQ's default is the equivalent of amqp://authority/%2F.

@Zagrebelin Is this causing problems for you? This will require a major-version bump, I expect, so I'd like to gauge how important it is.

adamhooper commented 3 years ago

Fixed in v1. Thank you, @Zagrebelin!

For my future self and other perplexed individuals, here's my interpretation of the RabbitMQ spec -- rewritten to make me feel like I'm not stupid:

I read all this and was still confused -- why is the default "/" instead of, say, "default"? I couldn't find an answer. It certainly appears broken, and it creates problems with security-minded reverse proxies.

But I did find more confusion, and I'll explicitly write it here for other people:

The server cannot advertise any default vhost. The RabbitMQ server-side default_vhost is a completely different beast from the client-side default value. It's invisible to clients. It might have been called vhost_to_create_on_database_init to allay confusion.

All that to say: vhost "/" is not a standard: it's a convention. And it's darned confusing convention.

From v1 onwards, carehare rewrites URL path "" as vhost "/". That's the convention users expect and it's permitted by the spec.

From v1 onwards, if carehare sees URL path "/" it will warn with a DeprecationWarning and still select vhost "/". This is against the spec because the spec forces vhost "". But the value "" is itself invalid! I'm all about following specs, but this "" in RabbitMQ's URI spec seems to me to deliver no value at all. Pika violates the spec in this same manner; rabbitmq-java-client maintainers opt instead tell users to RTFM.