Use Type FullName when calling deposit

[lillo42]

Fix: #3944

Use the type full name as cache

[claude]

PR Review: Use Type FullName when calling deposit

Summary

This PR addresses issue #3944 where generic events fail to post due to cache key collisions. The fix changes the cache key from Type.Name to Type.FullName in four method reflection caching dictionaries.

---

Code Quality - APPROVED

Positive Points:

• The fix is minimal, focused, and directly addresses the root cause
• Changes are consistent across all four affected cache dictionaries (sync/async, single/bulk)
• The solution aligns with the codebase approach to type identification

Observations:
The change is structurally sound. Using FullName instead of Name ensures that generic types with different type parameters are treated as distinct cache entries, preventing the type confusion that caused the reported exception.

---

Bug Fix Analysis - APPROVED

Root Cause:
The issue occurred because Type.Name for generic types returns the same value for different type parameters, causing cache collisions. When the second generic variant tried to use the cached MethodInfo from the first, it resulted in a type mismatch exception.

Solution:
Using Type.FullName includes the namespace and properly distinguishes between different generic type instantiations, ensuring each gets its own cached reflection method binding.

---

Performance Considerations - APPROVED

Performance Impact: Neutral to slightly positive

• FullName is negligibly more expensive than Name (both are properties on Type)
• The caching strategy remains the same (lookup once, reuse many times)
• Prevents runtime exceptions and reflection rebinding on cache misses
• The static concurrent dictionaries provide thread-safe caching with minimal overhead

---

Security Concerns - APPROVED

No security concerns identified. This is an internal caching mechanism using type metadata for reflection optimization.

---

Test Coverage - NEEDS ATTENTION

Concern: Missing Test Coverage

While the fix is correct, there is no accompanying test in this PR to prevent regression. Based on the repository TDD guidelines in docs/agent_instructions/testing.md, all bug fixes should include appropriate test coverage.

Recommendation:
Add a test case that exercises the bug scenario:

1. Define two generic events with different type parameters
2. Call DepositPostAsync for both types in sequence
3. Assert that both succeed without type conversion exceptions

The reproduction repository mentioned in issue #3944 could be adapted for this test.

---

Additional Observations

1. Consistency Check: All four cache sites have been updated correctly in CommandProcessor.cs

2. Breaking Change: This is NOT a breaking change. The cache is static and internal; changing the key format only affects internal caching behavior, not the public API.

3. Cache Invalidation: The existing cache clearing logic in Dispose remains appropriate.

---

Recommendation

Approve with condition: Add test coverage for the generic event scenario before merging.

The fix is technically sound and solves the reported issue. However, following the project TDD guidelines and preventing regression requires appropriate test coverage.

[codescene-delta-analysis]

Gates Passed
4 Quality Gates Passed

See analysis details in CodeScene

Quality Gate Profile: Clean Code Collective
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.

[claude]

Pull Request Review

Overview

This PR fixes issue #3944 where generic events with different type parameters were incorrectly cached and reused, causing runtime type conversion exceptions. The fix changes the cache key from Type.Name to Type.FullName to properly distinguish between generic types with different type arguments.

Code Quality: Good

The fix correctly addresses the root cause. Using Type.FullName instead of Type.Name properly distinguishes GenericEvent with different type arguments. Consistent application across all four affected methods with defensive null checks.

Minor style: if(string.IsNullOrEmpty should be if (string.IsNullOrEmpty per C# conventions (space after if).

Potential Issues: Minor Concerns

1. Type.FullName can theoretically return null for compiler-generated types, though extremely unlikely for IRequest implementations. Defensive check is good.
2. Old cache entries keyed by Name will become orphaned after upgrade, but harmless and cleared on Dispose().

Performance: Neutral to Positive

Using FullName vs Name has negligible performance impact. Fix prevents incorrect cache hits and expensive reflection calls.

Security: No Concerns

No security implications. Added null checks improve robustness.

Test Coverage: Missing

Critical Gap: This PR lacks test coverage for the generic event scenario.

Recommendations:

1. Add test reproducing issue Generic events fails to post #3944 with two generic events with different type arguments
2. Test naming: When_posting_generic_events_with_different_type_arguments_should_cache_separately.cs
3. Follows projects TDD philosophy per docs/agent_instructions/testing.md

Verdict: Approve with Recommendations

The fix is correct and solves the reported issue. However, I strongly recommend adding test coverage before merging to prevent regression and follow TDD practices.

Great work identifying and fixing this subtle generic type caching issue!