fix(messaging): align QualifiedName(Class) naming with annotation defaults for nested classes#4630
fix(messaging): align QualifiedName(Class) naming with annotation defaults for nested classes#4630MateuszNaKodach wants to merge 2 commits into
Conversation
…aults for nested classes The framework derived two different qualified names for the same nested class, depending on the path: - QualifiedName(Class) - and thus MessageType(Class) - used Class.getName(), producing "pkg.Outer$Inner" for nested classes. - Annotation-based resolution (@Command/@event defaults via AnnotationMessageTypeResolver) uses getPackageName() + getSimpleName(), producing "pkg.Inner". - ClassBasedMessageTypeResolver documented getPackageName() + getSimpleName(), but implemented Class.getName(), contradicting its own javadoc for nested classes. As a result, a MessageType built from a nested payload class could never match messages dispatched through annotation-based naming - a silent mismatch, e.g. a QualifiedName-keyed interceptor or handler lookup that simply never matches. Nested payload classes are idiomatic in Kotlin (sealed command/event hierarchies), making this especially easy to hit there. Changes: - QualifiedName(Class) now combines getPackageName() and getSimpleName(), aligning with the annotation defaults. Primitive wrapper resolution is preserved. Classes without a simple name (e.g. anonymous classes) fall back to Class.getName(). Javadoc updated accordingly. - ClassBasedMessageTypeResolver now routes through the Class-based MessageType constructor, making the implementation match its documented behavior. - MessageTypeTest gains nested-class tests (written first, TDD) pinning that MessageType(Class) equals the package + simple name spelling for nested and top-level classes. - AsyncMessageHandlerTest's declarative registrations used the raw Class.getName() String and are updated to the Class-based QualifiedName constructor. Verified: messaging (3530 tests), eventsourcing (489), modelling (370), and conversion (174) suites pass. Notes for review: - Behavioral change: nested, un-annotated payload classes resolved through ClassBasedMessageTypeResolver change their name from "pkg.Outer$Inner" to "pkg.Inner". Events already stored under the old spelling will no longer match handlers/criteria without a mapping - needs a release note or migration guidance. - Nested classes with the same simple name in the same package now collide, mirroring the trade-off the annotation defaults already made; explicit @Command/@event name/namespace attributes remain the escape hatch. - String-based constructors (QualifiedName(String), MessageType(String, ...)) remain verbatim by design. Kotlin users should pass ::class.java rather than KClass.qualifiedName, which renders nested classes with a dotted spelling that does not match either.
…nnotationMessageTypeResolver The annotation-based resolver spelled its class-derived defaults (getPackageName() / getSimpleName()) itself, duplicating the naming policy that QualifiedName(Class) owns. Derive the defaults from QualifiedName(Class) instead, so the policy lives in exactly one place and partial annotation overrides (namespace without name, and vice versa) overlay the same class-derived name as every other path. Behavior is unchanged for regular classes, pinned by the new default-name tests in AnnotationMessageTypeResolverTest. For the exotic edge of an annotated type without a simple name, the local name now falls back to the binary-name-derived local name instead of failing on an empty local name.
hjohn
left a comment
There was a problem hiding this comment.
Are we sure is the right trade-off?
two nested classes with the same simple name in the same package now map to the same name
I think if we're going to qualify the class with the entire package structure, we should also qualify it with the outer type, like how Java would do it, and not create some "new" not-Java scheme that allows collisions.
I made the behaviors consistent, but you're right - we should discuss what to do in this case. |
Problem
The framework derives a message's qualified name from a
Classthrough three paths — and they disagreed for nested classes:pkg.Outer.Innerbecamenew QualifiedName(Class)/new MessageType(Class)Class.getName()pkg.Outer$Inner@Command/@Eventannotation defaults (AnnotationMessageTypeResolver)getPackageName()+getSimpleName()pkg.InnerClassBasedMessageTypeResolverClass.getName()— contradicting its own javadoc, which documentsgetPackageName()+getSimpleName()pkg.Outer$InnerFor top-level classes all three coincide, so the inconsistency stays invisible — until a payload class is nested.
Worth highlighting:
ClassBasedMessageTypeResolver's own usage was different from the constructor sitting right next to it. It did not callnew MessageType(payloadType, version)— it spelled the name itself through theString-based constructor:which is exactly how its implementation drifted from its javadoc: the documented policy (
getPackageName()+getSimpleName()) lived in theClass-based constructor's intent, while the resolver bypassed it with a hand-spelledgetName()string.How it manifested
Any
QualifiedName-keyed comparison built withnew MessageType(MyNested.class).qualifiedName()could never match a message dispatched through annotation-based naming. There is no error — the comparison is just silently false.Concrete case that surfaced this: a
MessageHandlerInterceptormatching commands by qualified name,against a dispatched
@Command-annotated nested record (pkg.MyCommand). The interceptor silently no-opped: commands that should have been rejected succeeded. The identical resolver pattern worked for a top-level command class purely by luck of class placement — which is exactly what makes this such a sharp edge.This is particularly easy to hit from Kotlin, where nesting commands/events in sealed hierarchies is idiomatic (
sealed interface DwellingCommand { data class Build(..) : DwellingCommand }— every member is a nested class).Even this repository's own test suite contained the footgun:
AsyncMessageHandlerTest's declarative registrations subscribed handlers vianew QualifiedName(CheckIfPrime.class.getName())while dispatching through a resolver.Workaround needed before this fix
Application code had to avoid the
Class-based constructors for nested classes and hand-build the annotation-compatible spelling:— provided the developer realised the mismatch existed at all, since nothing fails loudly.
The workaround was in fact already visible in this repository's own tests, where authors sidestepped the
Class-based constructor rather than relying on it:DefaultCommandGatewayTestdefines an ad-hoc resolver instead of theClass-derived name:AsyncMessageHandlerTest's declarative registrations subscribed with the raw binary-name string to match the dispatch spelling of the day:Both are symptoms of the same root cause: the
Class-based constructor couldn't be trusted to produce the name a message would actually carry.Fix
Single-source the naming policy and make every
Class-derived path agree with the annotation defaults:QualifiedName(Class)now combinesgetPackageName()andgetSimpleName()(delegating to the existingcombineNames), aligning with the@Command/@Eventdefaults. Primitive-wrapper resolution is preserved. Classes without a simple name (e.g. anonymous classes) fall back toClass.getName(). Javadoc updated.ClassBasedMessageTypeResolvernow routes through theClass-basedMessageTypeconstructor — a one-line change that makes the implementation finally match its documented behavior.AsyncMessageHandlerTestdeclarative registrations updated to theClass-based constructor.AnnotationMessageTypeResolver(second commit) no longer spells its class-derived defaults (getPackageName()/getSimpleName()) itself — it derives them fromQualifiedName(Class), so the naming policy now lives in exactly one place. Partial annotation overrides (namespacewithoutname, and vice versa) overlay the same class-derived name as every other path. Behavior is unchanged for regular classes (pinned by new default-name tests); the exotic edge of an annotated type without a simple name now falls back to the binary-name-derived local name instead of failing on an empty local name.After the change,
new MessageType(Class),ClassBasedMessageTypeResolver, and the annotation defaults produce the same name for the same class — nested or not — and all of them derive it from the single policy inQualifiedName(Class). TheString-based constructors remain verbatim by design.Tests
Written TDD — they fail on the previous implementation with:
See
MessageTypeTest.WithNestedClasses:classConstructorUsesPackageAndSimpleNameForNestedClasses— pins the concrete namespace/localName/name for a nested payload.classConstructorMatchesAnnotationBasedNamingForNestedClasses— pins the contract:MessageType(Class)equals the annotation-default spelling.classConstructorMatchesAnnotationBasedNamingForTopLevelClasses— documents that top-level behavior is unchanged.And
AnnotationMessageTypeResolverTest.CommandMessageResolution:classAnnotatedWithCommandWithoutNameAndNamespaceDefaultsToQualifiedNameOfClass— pins that the annotation defaults equalQualifiedName(Class).classAnnotatedWithCommandWithNamespaceOnlyDefaultsNameToLocalNameOfClass— pins the partial-override case (namespace overridden, name defaulted).Verification
PooledStreamingEventProcessorTesttiming flakes pass in isolation)AsyncMessageHandlerTestregistration mentioned above — itself a demonstration of the footgun.Notes for review
ClassBasedMessageTypeResolverchange their name frompkg.Outer$Innertopkg.Inner. Events already persisted under the old spelling will no longer match handlers/criteria without a mapping — needs a release note and/or migration guidance. The alternative direction (making the annotation defaults include the enclosing class) was considered, but would change the documented, intentional@Command/@Eventbehavior instead.@Command/@Eventname/namespaceattributes remain the escape hatch.KClass.qualifiedNamerenders nested classes with dots (pkg.Outer.Inner) and matches neither spelling — Kotlin users should pass::class.java. A small Kotlin-extension helper (KClass.toQualifiedName()) could be a follow-up.Follow-up candidate: single-source— done in this PR (second commit); the consolidation is complete.AnnotationMessageTypeResolver's defaults