Skip to content

Add virtual host support to RabbitMQ connection handling#180

Open
LucasCoderT wants to merge 2 commits into
cloudblue:masterfrom
LucasCoderT:implement_rabbitmq_virtual_host
Open

Add virtual host support to RabbitMQ connection handling#180
LucasCoderT wants to merge 2 commits into
cloudblue:masterfrom
LucasCoderT:implement_rabbitmq_virtual_host

Conversation

@LucasCoderT
Copy link
Copy Markdown

Adds support for providing RabbitMQ's virtual host

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 3, 2026

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Member

@maxipavlovic maxipavlovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

The PR adds virtual_host support to RabbitMQTransport, threading it through settings parsing, connection creation, and the dead letters command. It's a reasonable feature — pika's ConnectionParameters supports virtual_host, and many RabbitMQ deployments use non-default vhosts.

Issues

1. Fragile positional tuple unpacking

The existing code already has a fragile pattern of returning settings as positional tuples and unpacking/concatenating them. This PR makes it worse by inserting virtual_host between creds and exchange in the return tuple. Any downstream code that destructures this tuple by position (as cqrs_dead_letters.py does) must be updated in lockstep.

The PR does update cqrs_dead_letters.py correctly, so there's no bug here, but this is an existing design smell that this PR deepens. A NamedTuple or dataclass would be more robust. Not a blocker.

2. _parse_url edge case with trailing slash

For amqp://usr:pswd@rabbit:8000/test/nested, parts.path is "/test/nested", lstrip('/') gives "test/nested". RabbitMQ vhosts can contain / chars, and pika expects the unencoded vhost name. This edge case is probably fine since it matches how most AMQP URL parsers work, but there's no test for it.

3. Missing test coverage

  • No test for URL with explicit / vhost (e.g., amqp://guest:guest@localhost:5672/)
  • The _get_consumer_rmq_objects and _get_producer_rmq_objects changes are not tested — existing tests mock at a higher level and don't verify virtual_host reaches ConnectionParameters

4. No documentation update

The README shows CQRS settings examples but doesn't mention virtual_host. Would be good to add it to the settings example or at minimum note it in the PR description.

Verdict

The code is functionally correct and the change is clean. The virtual_host is properly threaded through all code paths (common settings, producer, consumer, dead letters). Tests are updated for the new tuple position.

Requests:

  1. Add a test for URL-based vhost with trailing slash (amqp://guest:guest@localhost:5672/) to verify it correctly defaults
  2. Add virtual_host to the README settings example

@maxipavlovic
Copy link
Copy Markdown
Member

Adds support for providing RabbitMQ's virtual host

@LucasCoderT thank you for the contribution! Please review the comments above.

- Added documentation
- Added more tests
- Updated README.md
@LucasCoderT LucasCoderT requested a review from maxipavlovic March 5, 2026 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants