diff --git a/src/Transports/RabbitMQ/Wolverine.RabbitMQ.Tests/ConventionalRouting/Bug_2681_handler_type_naming_binds_all_exchanges.cs b/src/Transports/RabbitMQ/Wolverine.RabbitMQ.Tests/ConventionalRouting/Bug_2681_handler_type_naming_binds_all_exchanges.cs new file mode 100644 index 000000000..f9f7f573a --- /dev/null +++ b/src/Transports/RabbitMQ/Wolverine.RabbitMQ.Tests/ConventionalRouting/Bug_2681_handler_type_naming_binds_all_exchanges.cs @@ -0,0 +1,171 @@ +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Shouldly; +using Wolverine.ComplianceTests; +using Wolverine.RabbitMQ.Internal; +using Wolverine.Runtime; +using Wolverine.Transports; +using Wolverine.Util; +using Xunit; + +namespace Wolverine.RabbitMQ.Tests.ConventionalRouting; + +/// +/// Regression for https://github.com/JasperFx/wolverine/issues/2681. +/// +/// When conventional routing is configured with +/// and a single handler type handles two or more message types (typical for sagas +/// or any aggregate-style handler), the handler-named queue is supposed to be bound +/// to the exchange of every message type the handler accepts. Pre-fix, only the +/// FIRST message type's exchange got a binding because +/// RabbitMqMessageRoutingConvention.ApplyListenerRoutingDefaults short-circuits +/// on queue.HasBindings — once the first BindExchange call landed, the +/// second message type's pass through the convention saw a queue with bindings +/// already present and skipped the binding entirely. +/// +/// The reporter has a candidate fix; this test pins down the expected behaviour +/// independently so the contract is verifiable from outside the convention. +/// +public class Bug_2681_handler_type_naming_binds_all_exchanges : IDisposable +{ + private readonly IHost _host; + private readonly IWolverineRuntime _runtime; + + public Bug_2681_handler_type_naming_binds_all_exchanges() + { + _host = WolverineHost.For(opts => + { + opts.UseRabbitMq() + .AutoProvision() + .UseConventionalRouting(NamingSource.FromHandlerType); + + opts.Discovery.DisableConventionalDiscovery() + .IncludeType(typeof(MultiMessageHandler)); + }); + + _runtime = _host.Services.GetRequiredService(); + } + + [Fact] + public void handler_queue_is_bound_to_every_handled_message_exchange() + { + var queueName = typeof(MultiMessageHandler).ToMessageTypeName(); + var fooExchange = typeof(MultiMessageFoo).ToMessageTypeName(); + var barExchange = typeof(MultiMessageBar).ToMessageTypeName(); + + var transport = _runtime.Options.RabbitMqTransport(); + var queue = transport.Queues[queueName]; + + queue.HasBindings.ShouldBeTrue($"queue '{queueName}' should have bindings"); + + var boundExchanges = queue.Bindings().Select(b => b.ExchangeName).ToArray(); + + boundExchanges.ShouldContain(fooExchange, + $"queue '{queueName}' should be bound to '{fooExchange}'; bound to: {string.Join(", ", boundExchanges)}"); + + boundExchanges.ShouldContain(barExchange, + $"queue '{queueName}' should be bound to '{barExchange}'; bound to: {string.Join(", ", boundExchanges)}"); + } + + [Fact] + public void both_message_exchanges_were_created() + { + // Sanity check: the exchanges themselves are registered (so the binding + // failure is purely on the queue side, not because we never created the + // second exchange). + var fooExchange = typeof(MultiMessageFoo).ToMessageTypeName(); + var barExchange = typeof(MultiMessageBar).ToMessageTypeName(); + + var transport = _runtime.Options.RabbitMqTransport(); + + transport.Exchanges.Any(e => e.Name == fooExchange).ShouldBeTrue(); + transport.Exchanges.Any(e => e.Name == barExchange).ShouldBeTrue(); + } + + public void Dispose() => _host.Dispose(); +} + +/// +/// Companion guard for the GH-2681 fix: the original queue.HasBindings +/// short-circuit existed so a user-configured custom binding (via +/// ) +/// wouldn't get a default binding stacked on top. The fix keeps that protected +/// case intact by deduping per-exchange — a queue already bound to the +/// message-type exchange via a custom configuration must not be bound a second +/// time. +/// +public class Bug_2681_custom_bindings_are_not_double_added : IDisposable +{ + private readonly IHost _host; + private readonly IWolverineRuntime _runtime; + + public Bug_2681_custom_bindings_are_not_double_added() + { + _host = WolverineHost.For(opts => + { + opts.UseRabbitMq() + .AutoProvision() + .UseConventionalRouting(NamingSource.FromHandlerType, conventions => + { + conventions.ConfigureListeners((listener, _) => + { + // Pre-register a custom binding to the same exchange the + // default convention would otherwise add. Pre-fix and + // post-fix this MUST result in exactly one binding to + // that exchange — no stacking. + var name = typeof(SinglyHandledMessage).ToMessageTypeName(); + listener.BindToExchange(ExchangeType.Fanout, name); + }); + }); + + opts.Discovery.DisableConventionalDiscovery() + .IncludeType(typeof(SinglyHandledMessageHandler)); + }); + + _runtime = _host.Services.GetRequiredService(); + } + + [Fact] + public void user_custom_binding_is_not_double_added_by_the_convention() + { + var queueName = typeof(SinglyHandledMessageHandler).ToMessageTypeName(); + var exchangeName = typeof(SinglyHandledMessage).ToMessageTypeName(); + + var transport = _runtime.Options.RabbitMqTransport(); + var queue = transport.Queues[queueName]; + + var bindingsToTarget = queue.Bindings() + .Where(b => b.ExchangeName == exchangeName) + .ToArray(); + + bindingsToTarget.Length.ShouldBe(1, + $"queue '{queueName}' should have exactly one binding to '{exchangeName}', got {bindingsToTarget.Length}: " + + string.Join(", ", queue.Bindings().Select(b => $"{b.ExchangeName}/{b.BindingKey}"))); + } + + public void Dispose() => _host.Dispose(); +} + +public record SinglyHandledMessage; + +public class SinglyHandledMessageHandler +{ + public void Handle(SinglyHandledMessage message) + { + } +} + +public record MultiMessageFoo; + +public record MultiMessageBar; + +public class MultiMessageHandler +{ + public void Handle(MultiMessageFoo message) + { + } + + public void Handle(MultiMessageBar message) + { + } +} diff --git a/src/Transports/RabbitMQ/Wolverine.RabbitMQ/RabbitMqMessageRoutingConvention.cs b/src/Transports/RabbitMQ/Wolverine.RabbitMQ/RabbitMqMessageRoutingConvention.cs index e46a85562..fe1483988 100644 --- a/src/Transports/RabbitMQ/Wolverine.RabbitMQ/RabbitMqMessageRoutingConvention.cs +++ b/src/Transports/RabbitMQ/Wolverine.RabbitMQ/RabbitMqMessageRoutingConvention.cs @@ -23,16 +23,31 @@ protected override (RabbitMqConventionalListenerConfiguration, Endpoint) FindOrC protected override void ApplyListenerRoutingDefaults(string listenerIdentifier, RabbitMqTransport transport, Type messageType) { var queue = transport.Queues[listenerIdentifier]; - // If there's no custom bindings, bind to an exchange with the default convention - if (!queue.HasBindings) + + var identifier = _identifierForSender(messageType); + if (identifier is null) + return; + + var name = transport.MaybeCorrectName(identifier); + var exchange = transport.Exchanges[name]; + + // Per-exchange dedup. The original guard short-circuited on + // `queue.HasBindings` so user-configured custom bindings on this queue + // wouldn't get a default binding stacked on top — but under + // NamingSource.FromHandlerType a single handler queue legitimately needs + // a binding to every message-type exchange the handler accepts, and that + // pattern dispatches ApplyListenerRoutingDefaults once per (handlerType, + // messageType) pair. The earlier guard silently dropped every binding + // after the first. Narrow the suppression to "we already bind THIS queue + // to THIS exchange" so the multi-message-type case binds correctly while + // still leaving custom user bindings (and prior passes for the same + // message type) untouched. See GH-2681. + if (queue.Bindings().Any(b => b.ExchangeName == exchange.Name)) { - var identifier = _identifierForSender(messageType); - if (identifier is null) - return; - var name = transport.MaybeCorrectName(identifier); - var exchange = transport.Exchanges[name]; - queue.BindExchange(exchange.Name, exchange.Name); + return; } + + queue.BindExchange(exchange.Name, exchange.Name); } protected override (RabbitMqExchangeConfiguration, Endpoint) FindOrCreateSubscriber(string identifier,