Add virtual host support to RabbitMQ connection handling#180
Add virtual host support to RabbitMQ connection handling#180LucasCoderT wants to merge 2 commits into
Conversation
maxipavlovic
left a comment
There was a problem hiding this comment.
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_objectsand_get_producer_rmq_objectschanges are not tested — existing tests mock at a higher level and don't verifyvirtual_hostreachesConnectionParameters
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:
- Add a test for URL-based vhost with trailing slash (
amqp://guest:guest@localhost:5672/) to verify it correctly defaults - Add
virtual_hostto the README settings example
@LucasCoderT thank you for the contribution! Please review the comments above. |
- Added documentation - Added more tests - Updated README.md
Adds support for providing RabbitMQ's
virtual host