search

Antti / rust-amqp

AMQP client in pure rust. Corresponds to rabbitmq spec.
MIT License
249 stars 45 forks source link

Refactor default vhost implementation #68

Closed whitfin closed 6 years ago

whitfin commented 6 years ago

This is intended to fix #33 as well as stay compatible with the fix for #8.

The AMQP URI specification states that a missing vhost (i.e. no path on the URI) should be treated as not provided, whereas a trailing / should be treated as an empty vhost.

This PR will adjust this behaviour by using the default vhost if not provided, and trimming the / if it is provided (thus making it empty). Previously this was broken as url.path() will always return / (even if it was not provided in the input); in the case it wasn't provided, the vhost was being set to "" due to the previous clean_vhost implementation. This was coincidentally the same as the default, but this shouldn't be relied upon (read on...).

In order to resolve #33, I took the liberty of setting the default vhost to "/" as it's the default for RabbitMQ, which is widely regarded as the most popular AMQP implementation. Having "" as the default is unintuitive (see #33) for RabbitMQ users as you need to supply amqp://host// to connect successfully.

Tests still pass after accounting for the change to the default, so it shouldn't really break anything; anyone still using amqp://host// will still function correctly, as well as anyone specifying the vhost. The only behaviour change is the default vhost, which can technically be seen as a bug anyway :p

Antti commented 6 years ago

I think this looks alright. This vhost story is really confusing, hopefully this should clear up some confusion. Thanks for the PR!