-
-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Add keyed service support for decorator registration (Phase 2 of #3) #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…#3) This commit completes Phase 2 of issue #3 by adding full support for keyed service registrations with decorators. ## New Features - **Keyed service support** - Decorators now work with keyed service registrations: - `AddKeyedScoped<TService, TImplementation>(serviceKey)` - Keyed parameterless registration - `AddKeyedScoped<TService, TImplementation>(serviceKey, factory)` - Keyed with factory delegate (two-parameter) - `AddKeyedScoped<TService>(serviceKey, factory)` - Keyed with factory delegate (single-parameter) - All lifetimes supported: `AddKeyedScoped`, `AddKeyedTransient`, `AddKeyedSingleton` - **Multiple keys per service** - Register the same service type with different keys independently - **All key types supported** - Works with `string`, `int`, `enum`, and custom object keys - **Nested key strategy** - Prevents circular resolution while preserving user's original key ## Implementation Changes - Extended `RegistrationKind` enum with three keyed service variants - Added `ServiceKeyParameterName` field to `ClosedGenericRegistration` model - Updated `ClosedGenericRegistrationProvider` to detect keyed service signatures (2 or 3 parameters) - Updated `InterceptorEmitter` to generate keyed service interceptors with nested key strategy - Added `ForKeyed` helper method to `DecoratorKeys` class for nested key generation ## Technical Details - Nested key format: `"{userKey}|{ServiceAQN}|{ImplAQN}"` prevents conflicts between keys - User's original key preserved for resolution via `GetRequiredKeyedService` - Each key gets independent decorator chain - no sharing between keys - 7 new test cases (039-045) covering keyed service scenarios - Updated sample project with keyed service examples (string and integer keys, multiple keys) - All existing functionality remains unchanged - this is purely additive ## Documentation - Updated changelog.md with 1.0.3-beta release notes - Updated usage documentation with comprehensive keyed service section - Updated README.md and CLAUDE.md with keyed service feature - Updated releasenotes.props with 1.0.3-beta condition and content - Version bumped to 1.0.3-beta in Directory.Build.props 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/LayeredCraft.DecoWeaver.Generators/Emit/InterceptorEmitter.cs
Outdated
Show resolved
Hide resolved
- Update Polyfill from 8.9.0 to 9.0.2 across Attributes and Generators projects - Update Microsoft.NET.Test.Sdk from 18.0.0 to 18.0.1 - Update Verify.XunitV3 from 31.3.0 to 31.7.1 - Update xunit.v3 from 3.1.0 to 3.2.0 - Move Microsoft.Extensions.DependencyInjection to framework-specific ItemGroups: - net8.0: 8.0.1 - net9.0: 9.0.11 - net10.0: 10.0.0 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ider Refactored the Transformer method to improve maintainability and reduce complexity: ## Changes - **Extracted validation methods**: Split 186-line Transformer into focused methods - `ValidateNonKeyedRegistration()` - ~44 lines, handles non-keyed patterns - `ValidateKeyedRegistration()` - ~52 lines, handles keyed patterns - `ValidateFactoryDelegate()` - ~14 lines, validates Func<IServiceProvider, T> - `ValidateKeyedFactoryDelegate()` - ~18 lines, validates Func<IServiceProvider, object?, T> - `ExtractServiceAndImplementationTypes()` - ~13 lines, type extraction logic - `GetInterceptsData()` - ~7 lines, interceptable location retrieval - **Added RegistrationValidationResult**: Record struct to encapsulate validation results - Kind, FactoryParameterName, ServiceKeyParameterName - **Main Transformer method**: Now ~53 lines with clear pipeline structure - Guard clauses - Validate registration pattern - Extract types - Get intercepts data - Build result ## Benefits - **Reduced cyclomatic complexity**: From ~18-20 to ~5 per method - **Single level of abstraction**: Each method has one clear purpose - **Improved testability**: Methods can be tested independently - **Better maintainability**: Adding new patterns easier with extraction pattern - **Eliminated duplication**: Factory delegate validation shared between keyed/non-keyed ## Verification - ✅ All 45 tests pass on net8.0, net9.0, net10.0 - ✅ Sample project runs correctly - ✅ No behavioral changes - pure refactoring 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/LayeredCraft.DecoWeaver.Generators/Emit/InterceptorEmitter.cs
Outdated
Show resolved
Hide resolved
Fixed critical bug where distinct custom object keys collapsed to the same nested key, breaking isolation between keyed service registrations. ## Problem The original implementation converted the user key to string using ToString(): ```csharp var keyStr = userKey?.ToString() ?? "null"; return string.Concat(keyStr, "|", s, "|", i); ``` For custom object keys, ToString() returns the same value for different instances: - `new object().ToString()` → "System.Object" - Another `new object().ToString()` → "System.Object" This caused both registrations to share the same nested key, breaking the isolation guarantee and causing decorator chains to be shared. ## Solution Changed ForKeyed to return a tuple that preserves the actual key object: ```csharp return (userKey, serviceType, implementationType); ``` The tuple preserves object reference identity, ensuring distinct keys create distinct nested keys. .NET's keyed services use object.Equals() for comparison, so two different object() instances maintain separate identities. ## Impact - ✅ String keys: Still work correctly - ✅ Integer keys: Still work correctly - ✅ Enum keys: Still work correctly - ✅ Custom object keys: Now work correctly (previously broken) ## Testing - Updated all 38 snapshot files to reflect corrected generated code - All 45 tests pass on net8.0, net9.0, net10.0 Reported by code review feedback on PR #12 (lines 414-419 of InterceptorEmitter.cs) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary
This PR completes Phase 2 of issue #3 by adding full support for keyed service registrations with decorators. Users can now register services with keys and DecoWeaver will automatically apply decorators while preserving the keyed service semantics.
Closes #3
New Features
Keyed Service Registration Support
AddKeyedScoped<TService, TImplementation>(serviceKey)AddKeyedScoped<TService, TImplementation>(serviceKey, factory)AddKeyedScoped<TService>(serviceKey, factory)AddKeyedScoped,AddKeyedTransient,AddKeyedSingletonstring,int,enum, custom objectsNested Key Strategy
To prevent circular resolution while preserving the user's original key:
GetRequiredKeyedService(userKey)"{userKey}|{ServiceAQN}|{ImplAQN}"Example Usage
Implementation Changes
Core Generator Changes
Extended
RegistrationKindenum with three keyed service variants:KeyedParameterlessKeyedFactoryTwoTypeParamsKeyedFactorySingleTypeParamUpdated
ClosedGenericRegistrationmodel:ServiceKeyParameterNamefield to capture the key parameter nameEnhanced
ClosedGenericRegistrationProvider:object?type for service key parameterFunc<IServiceProvider, object?, T>for keyed factory delegatesUpdated
InterceptorEmitter:DecoratorKeys.ForKeyed()helperTest Coverage
Added 7 comprehensive test cases (039-045):
All existing tests pass with updated snapshots.
Sample Project
Updated sample project with working examples:
Documentation
Comprehensive documentation updates:
Technical Details
Nested Key Format
Generated Interceptor Pattern
Breaking Changes
None. This is a purely additive feature that does not affect any existing functionality.
Test Plan
Related Issues
Closes #3 (Phase 2: Keyed service support)
🤖 Generated with Claude Code