Fix invalid generated class name for batched (array) message types (GH-3399)#3405
Closed
jeremydmiller wants to merge 1 commit into
Closed
Fix invalid generated class name for batched (array) message types (GH-3399)#3405jeremydmiller wants to merge 1 commit into
jeremydmiller wants to merge 1 commit into
Conversation
A header-identified saga cannot work over a gRPC hop: the saga-id envelope header is not propagated across the call, so no id can be resolved. That is unchanged and is not what this fixes. What it fixes is the diagnostic. The caller used to get StatusCode.Internal with "Could not determine a valid saga state id for Envelope ..." -- true, but it names no cause and no remedy, and the remedy is not guessable, because the reason the id is missing is a property of the transport rather than of the message. It now maps to StatusCode.InvalidArgument with a detail that says the identity must be on the message body, why (the header does not cross the hop), how to supply it ([SagaIdentity] or a matching property), and where to read more. InvalidArgument rather than Internal is deliberate per AIP-193: the request cannot succeed as sent and retrying will not change that -- it is a caller-side contract problem, not a transient server fault. The mapping lives in WolverineGrpcExceptionMapper, which only runs at the gRPC boundary, so the core exception is untouched and non-gRPC users see no change. Message-identified sagas over gRPC already work and are unaffected (covered by #3386). Per @erikshafer's analysis on the issue, full three-point saga-id propagation is deliberately NOT taken: HTTP does not exercise the header path either, so "works like HTTP" is already satisfied, and the complexity is not warranted until someone shows a concrete need. Flips the #3386 characterization test, and updates the gRPC + Sagas docs page. Full Wolverine.Grpc.Tests 273/273. Full wolverine.slnx Release build clean. Closes #3385. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
Superseded — this branch got the wrong commit pushed to it from a shared working tree (my error, not the fix's). The actual GH-3399 commit is unchanged; reopening it on a clean branch off main. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3399. Reported by @outofrange-consulting — thank you for the repro sample, it made this quick to pin down.
The bug: an illegal C# identifier, at startup
HandlerGraph's duplicate-TypeNamedisambiguation composed the generated handler class name straight off the message type:ToSuffixedTypeNameonly strips the generic arity (the backtick suffix) — it does nothing about arrays.BatchMessagesOf<T>()makesT[]the handled message type, so this produced:which is not a valid C# identifier. Codegen failed with
CS1514: { expected/CS1513: } expected/CS1001: Identifier expected→Compilation failures!→ the app dies at startup. This is startup-fatal, and any batched message type can reach it.Not a 6.17.3 regression — the disambiguation path is old.
The trigger, and why existing batch tests missed it
Three things have to line up, and it was the third that our coverage lacked:
MultipleHandlerBehavior.Separated, and(3) is what pushes each message type's grouping past 1 (
HandlerChain.cs:114), which is the gate for creating sticky per-endpoint chains. Sticky chains are named off the handler type (HandlerChain.cs:85), so the shared handler class ends up with two sticky chains carrying the sameTypeName— and only then does the duplicate-disambiguation path run and rebuild the name off the message type.We already had
Separated+ batching coverage (separated_batch_routing.cs,batching_with_separated_handlers.cs), but every handler class there handles exactly one message type, so the stickyTypeNames never collided and the disambiguation branch was never exercised. Batching alone was never the missing ingredient — theSeparatedsplit with a shared handler class was.The fix
Centralized the codegen-safe name composition that
HandlerChainwas already doing ad hoc (.Replace("[]", "Array"), in three places) into one helper, and used it on the disambiguation path that was missing it:Sanitize()is the existingJasperFx.Core.Reflectionhelper, added as a belt-and-braces pass for any other punctuation rather than hand-rolling a new sanitizer.Collisions remain impossible.
ToSuffixedTypeNameappends a stable hash of the type's full name, andTandT[]have different full names — soItemDeletedandItemDeleted[]carry different hashes and cannot collide after sanitizing. The new test asserts distinctness explicitly, not just legality.Applied the helper to
SagaChaintoo, which carried the same idiom.Red → green
New test
src/Testing/CoreTests/Bugs/Bug_3399_batched_message_separated_handler_codegen.csreproduces the reporter's shape and asserts every generated class name is a legal C# identifier and that names stay distinct.Before the fix (the reporter's exact errors):
After:
CoreTests(net9.0): 1923 passed, 0 failed (2 pre-existing skips)dotnet build wolverine.slnx -c Release: 0 Warning(s), 0 Error(s)🤖 Generated with Claude Code