diff --git a/src/Testing/CoreTests/Bugs/Bug_3399_batched_message_separated_handler_codegen.cs b/src/Testing/CoreTests/Bugs/Bug_3399_batched_message_separated_handler_codegen.cs new file mode 100644 index 000000000..f4bd0ad74 --- /dev/null +++ b/src/Testing/CoreTests/Bugs/Bug_3399_batched_message_separated_handler_codegen.cs @@ -0,0 +1,134 @@ +using System; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using JasperFx.Core; +using Microsoft.Extensions.Hosting; +using Shouldly; +using Wolverine; +using Wolverine.Attributes; +using Wolverine.Tracking; +using Xunit; + +namespace CoreTests.Bugs; + +// GH-3399: with MultipleHandlerBehavior.Separated, a handler class that handles two message types where +// one of them is batched (BatchMessagesOf() makes T[] the handled message type) produced duplicate +// HandlerChain.TypeNames -- both sticky chains are named off the *handler type*. The duplicate +// disambiguation in HandlerGraph then rebuilt the generated class name straight off the message type, +// yielding "ItemDeleted[]1177234954_TelemetryHandlerHandler550305999", which is not a valid C# +// identifier -> "Compilation failures!" and the app dies at startup. +public class Bug_3399_batched_message_separated_handler_codegen +{ + [Fact] + public async Task can_start_up_with_separated_batched_handler() + { + using var host = await Host.CreateDefaultBuilder() + .UseWolverine(opts => + { + opts.Discovery.DisableConventionalDiscovery(); + opts.Discovery.IncludeType(); + opts.Discovery.IncludeType(); + opts.Discovery.IncludeType(); + + opts.MultipleHandlerBehavior = MultipleHandlerBehavior.Separated; + + opts.BatchMessagesOf(); + }).StartAsync(); + + var runtime = host.GetRuntime(); + + // Both the batched (array) chain and the single-message chain exist... + var chains = runtime.Handlers.Chains + .SelectMany(x => x.ByEndpoint.Any() ? x.ByEndpoint : [x]) + .Where(x => x.Handlers.Any()) + .ToArray(); + + // ...and every generated class name is a legal C# identifier. Before the fix, the chain for + // ItemDeleted3399[] was named "ItemDeleted3399[]_TelemetryHandler3399Handler". + foreach (var chain in chains) + { + chain.TypeName.ShouldNotContain("["); + chain.TypeName.ShouldNotContain("]"); + isValidIdentifier(chain.TypeName).ShouldBeTrue($"'{chain.TypeName}' is not a valid C# identifier"); + } + + // The array chain and the single chain must NOT collide after sanitizing + chains.Select(x => x.TypeName).Distinct().Count().ShouldBe(chains.Length); + } + + [Fact] + public async Task batched_handler_actually_executes() + { + TelemetryHandler3399.Batched = 0; + + using var host = await Host.CreateDefaultBuilder() + .UseWolverine(opts => + { + opts.Discovery.DisableConventionalDiscovery(); + opts.Discovery.IncludeType(); + opts.Discovery.IncludeType(); + opts.Discovery.IncludeType(); + + opts.MultipleHandlerBehavior = MultipleHandlerBehavior.Separated; + + opts.BatchMessagesOf(); + }).StartAsync(); + + await host.TrackActivity() + .Timeout(30.Seconds()) + .WaitForMessageToBeReceivedAt(host) + .SendMessageAndWaitAsync(new ItemDeleted3399(Guid.NewGuid())); + + TelemetryHandler3399.Batched.ShouldBeGreaterThan(0); + } + + private static bool isValidIdentifier(string name) + { + if (name.IsEmpty()) return false; + if (!char.IsLetter(name[0]) && name[0] != '_') return false; + + return name.All(c => char.IsLetterOrDigit(c) || c == '_'); + } +} + +public record ItemDeleted3399(Guid Id); + +public record ItemCreated3399(Guid Id); + +// The trigger: ONE handler class handling TWO message types, one of which is batched, where each of +// those message types ALSO has a second handler. The second handler is what pushes each message type's +// grouping past 1 (HandlerChain.cs:114), so under MultipleHandlerBehavior.Separated both of +// TelemetryHandler3399's calls get a sticky chain -- and sticky chains are named off the HANDLER type +// (HandlerChain.cs:85). The two sticky chains therefore share a TypeName, which drives HandlerGraph's +// duplicate disambiguation to rebuild the name off the message type: "ItemDeleted3399[]_...". +[WolverineIgnore] +public class TelemetryHandler3399 +{ + public static int Batched; + + public void Handle(ItemCreated3399 created) + { + } + + public void Handle(ItemDeleted3399[] deleted) + { + Interlocked.Add(ref Batched, deleted.Length); + } +} + +[WolverineIgnore] +public class OtherCreatedHandler3399 +{ + public void Handle(ItemCreated3399 created) + { + } +} + +[WolverineIgnore] +public class OtherDeletedHandler3399 +{ + public void Handle(ItemDeleted3399[] deleted) + { + } +} diff --git a/src/Wolverine/Persistence/Sagas/SagaChain.cs b/src/Wolverine/Persistence/Sagas/SagaChain.cs index 9c4af2415..1c441245e 100644 --- a/src/Wolverine/Persistence/Sagas/SagaChain.cs +++ b/src/Wolverine/Persistence/Sagas/SagaChain.cs @@ -163,7 +163,7 @@ internal SagaChain(HandlerCall[] sagaCalls, HandlerGraph handlerGraph, Endpoint[ AuditedMembers.Add(new AuditedMember(SagaIdMember, SagaIdMember.Name, SagaIdMember.Name)); } - TypeName = saga.HandlerType.ToSuffixedTypeName(HandlerSuffix).Replace("[]", "Array"); + TypeName = GeneratedTypeNameFor(saga.HandlerType, HandlerSuffix); } public override bool TryInferMessageIdentity(out PropertyInfo? property) diff --git a/src/Wolverine/Runtime/Handlers/HandlerChain.cs b/src/Wolverine/Runtime/Handlers/HandlerChain.cs index d387173b2..d0377b532 100644 --- a/src/Wolverine/Runtime/Handlers/HandlerChain.cs +++ b/src/Wolverine/Runtime/Handlers/HandlerChain.cs @@ -49,6 +49,21 @@ public class HandlerChain : Chain, IW protected readonly List _byEndpoint = []; + /// + /// Compose the name of a generated handler class for a type in a way that is guaranteed to be a legal + /// C# identifier. only strips the generic arity + /// (the backtick suffix), so array types like ItemDeleted[] would otherwise leak the brackets + /// straight into the generated class name and fail compilation. See GH-3399. + /// + /// + /// Uniqueness is unaffected: ToSuffixedTypeName appends a stable hash of the type's *full* name, and + /// T and T[] have different full names, so their generated names cannot collide after sanitizing. + /// + internal static string GeneratedTypeNameFor(Type type, string suffix) + { + return type.ToSuffixedTypeName(suffix).Replace("[]", "Array").Sanitize(); + } + private readonly List _endpoints = []; private readonly HandlerGraph _parent; @@ -66,7 +81,7 @@ public HandlerChain(Type messageType, HandlerGraph parent) _parent = parent; MessageType = messageType ?? throw new ArgumentNullException(nameof(messageType)); - TypeName = messageType.ToSuffixedTypeName(HandlerSuffix).Replace("[]", "Array"); + TypeName = GeneratedTypeNameFor(messageType, HandlerSuffix); Description = "Message Handler for " + MessageType.FullNameInCode(); @@ -82,7 +97,7 @@ internal HandlerChain(MethodCall call, HandlerGraph parent, Endpoint[] endpoints { foreach (var endpoint in endpoints) RegisterEndpoint(endpoint); - TypeName = call.HandlerType.ToSuffixedTypeName(HandlerSuffix).Replace("[]", "Array"); + TypeName = GeneratedTypeNameFor(call.HandlerType, HandlerSuffix); Description = $"Message Handler for {MessageType.FullNameInCode()} using {call}"; } diff --git a/src/Wolverine/Runtime/Handlers/HandlerGraph.cs b/src/Wolverine/Runtime/Handlers/HandlerGraph.cs index a472253d5..69c6323a1 100644 --- a/src/Wolverine/Runtime/Handlers/HandlerGraph.cs +++ b/src/Wolverine/Runtime/Handlers/HandlerGraph.cs @@ -386,8 +386,11 @@ IEnumerable explodeChains(HandlerChain chain) { foreach (var chain in @group) { + // Both halves have to be sanitized into legal C# identifiers -- the message type may well be + // an array (batched messages are handled as T[]), which would otherwise emit brackets into + // the generated class name and blow up compilation. See GH-3399. chain.TypeName = - $"{chain.MessageType.ToSuffixedTypeName("")}_{chain.HandlerCalls().First().HandlerType.ToSuffixedTypeName("Handler")}"; + $"{HandlerChain.GeneratedTypeNameFor(chain.MessageType, "")}_{HandlerChain.GeneratedTypeNameFor(chain.HandlerCalls().First().HandlerType, "Handler")}"; } }