Conversation
Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds a “default queue” specification to the RabbitMQ AMQP management client so callers can declare queues without explicitly setting x-queue-type, letting the server/vhost configuration decide the queue type.
Changes:
- Introduces
DefaultQueueSpecificationimplementingIQueueSpecificationwithout specifying a queue type argument. - Updates topology recovery to map unknown/unspecified queue types to
DefaultQueueSpecification. - Adds an integration test for declaring a queue using
DefaultQueueSpecification.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
pkg/rabbitmqamqp/entities.go |
Adds DefaultQueueSpecification and expands IQueueSpecification documentation. |
pkg/rabbitmqamqp/amqp_queue_test.go |
Adds a test that declares a queue using DefaultQueueSpecification. |
pkg/rabbitmqamqp/amqp_connection_recovery.go |
Adds a default branch to recover queues with an unspecified/unknown type using DefaultQueueSpecification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| default: | ||
| return &DefaultQueueSpecification{ | ||
| Name: q.queueName, | ||
| IsAutoDelete: *q.autoDelete, | ||
| IsExclusive: *q.exclusive, | ||
| Arguments: q.arguments, | ||
| } |
There was a problem hiding this comment.
In the new default branch, toIQueueSpecification dereferences q.autoDelete and q.exclusive, but queueRecoveryRecord.autoDelete/exclusive are only populated for Classic queues in AmqpManagement.DeclareQueue. For DefaultQueueSpecification records these pointers will be nil, causing a panic during topology recovery. Consider either storing autoDelete/exclusive for default queues as well, or handling nil pointers here (e.g., defaulting to false).
| // the default value for queue type is classic | ||
| Expect(queueInfo.Type()).To(Equal(Classic)) |
There was a problem hiding this comment.
This assertion makes the test environment-dependent: RabbitMQ can be configured per-vhost to default queue type to quorum instead of classic when no x-queue-type is provided. To avoid flakiness, assert that no explicit x-queue-type was set (or only assert the DLX args), rather than hard-coding Classic here.
| // the default value for queue type is classic | |
| Expect(queueInfo.Type()).To(Equal(Classic)) | |
| // ensure no explicit queue type was set; actual default depends on broker configuration | |
| Expect(queueInfo.arguments).NotTo(HaveKey("x-queue-type")) |
| Expect(queueInfo.arguments["x-dead-letter-exchange"]).To(Equal("dead-letter-exchange")) | ||
| Expect(queueInfo.arguments["x-dead-letter-routing-key"]).To(Equal("dead-letter-routing-key")) |
There was a problem hiding this comment.
The rest of this file uses the public accessor queueInfo.Arguments() for argument assertions; accessing the unexported field queueInfo.arguments directly makes the test more brittle to internal refactors. Prefer using Arguments() here for consistency and encapsulation.
| Expect(queueInfo.arguments["x-dead-letter-exchange"]).To(Equal("dead-letter-exchange")) | |
| Expect(queueInfo.arguments["x-dead-letter-routing-key"]).To(Equal("dead-letter-routing-key")) | |
| Expect(queueInfo.Arguments()["x-dead-letter-exchange"]).To(Equal("dead-letter-exchange")) | |
| Expect(queueInfo.Arguments()["x-dead-letter-routing-key"]).To(Equal("dead-letter-routing-key")) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Gabriele Santomaggio <G.santomaggio@gmail.com>
…tmq/rabbitmq-amqp-go-client into feat/add_default_queue_declaration
Adds a “default queue” specification to the RabbitMQ AMQP management client so callers can declare queues without explicitly setting x-queue-type, letting the server/vhost configuration decide the queue type.
Changes:
Fixes #83