Add support for keyed DI services (Issue #165)

[avgalex]

Summary

• Validators registered via AddKeyedScoped, AddKeyedTransient, AddKeyedSingleton are now discovered automatically
• Registration order independent — works regardless of whether AddFluentValidationRulesToSwagger() is called before or after keyed validators are registered
• Graceful fallback when keyed services are not used or DI container doesn't support IKeyedServiceProvider
• Deduplication: same validator registered as both keyed and non-keyed is returned only once
• Version bumped to 7.0.4-beta.2

How it works

1. AddFluentValidationRulesToSwagger() registers IServiceCollection as a singleton snapshot
2. At resolution time, GetValidators() first resolves non-keyed validators (existing behavior)
3. Then scans the IServiceCollection snapshot for keyed IValidator<T> descriptors
4. Resolves each keyed validator via IKeyedServiceProvider.GetKeyedService()
5. Deduplicates by reference to avoid returning the same validator twice

Files changed

• ValidatorRegistryExtensions.cs — core resolution logic with new GetKeyedValidators helper
• ServiceCollectionExtensions.cs (Swashbuckle + NSwag) — register IServiceCollection singleton
• KeyedServicesTests.cs — 5 tests covering keyed-only, non-keyed regression, mixed, and full integration

Test plan

• All existing tests pass on net8.0, net9.0, net10.0
• 5 new keyed services tests pass on all frameworks
• Manual testing with sample application using keyed validators

Closes #165

🤖 Generated with Claude Code

[avgalex]

@claude review this PR

[claude]

Claude finished @avgalex's task in 2m 16s —— View job

---

Code Review for PR #186

Progress

• Read repository guidelines (CLAUDE.md)
• Review core logic (ValidatorRegistryExtensions.cs)
• Review service registration changes
• Review test coverage (KeyedServicesTests.cs)
• Review CHANGELOG and version changes
• Provide comprehensive feedback

---

Summary

This PR adds support for keyed DI services to discover validators registered via AddKeyedScoped, AddKeyedTransient, and AddKeyedSingleton. The implementation is well-structured and includes good test coverage. However, there is one critical compilation error that needs to be addressed.

---

Critical Issues

🔴 Compilation Error: Missing ReferenceEqualityComparer

File: src/MicroElements.OpenApi.FluentValidation/FluentValidation/ValidatorRegistry/ValidatorRegistryExtensions.cs:83

var seen = new HashSet<IValidator>(ReferenceEqualityComparer.Instance);

Problem: ReferenceEqualityComparer is referenced but not defined anywhere in the codebase. This will cause a compilation error.

Solution: You need to either:

1. Define ReferenceEqualityComparer class in the codebase, or
2. Use the built-in .NET ReferenceEqualityComparer from System.Collections.Generic (available in .NET 5+)

Recommended fix:

// For .NET 5+ (check your target frameworks in the csproj)
var seen = new HashSet<IValidator>(ReferenceEqualityComparer.Instance);

// OR if you need to support older frameworks, define it:
private sealed class ReferenceEqualityComparer : IEqualityComparer<IValidator>
{
    public static readonly ReferenceEqualityComparer Instance = new();
    public bool Equals(IValidator? x, IValidator? y) => ReferenceEquals(x, y);
    public int GetHashCode(IValidator obj) => RuntimeHelpers.GetHashCode(obj);
}

Fix this →

---

Design & Implementation Review

✅ Good: Approach to Keyed Service Discovery

The solution of registering IServiceCollection as a singleton and scanning descriptors at resolution time is clever and avoids timing issues. This ensures registration order independence.

Strength: Lines 84-85 in both ServiceCollectionExtensions files show consistent implementation:

// Issue #165: Register IServiceCollection for keyed validator discovery at resolution time
services.TryAddSingleton<IServiceCollection>(services);

✅ Good: Deduplication Strategy

The use of a HashSet with reference equality to prevent duplicate validators (lines 83-118 in ValidatorRegistryExtensions.cs) is the right approach for this scenario.

⚠️ Minor: Potential Performance Consideration

File: ValidatorRegistryExtensions.cs:153

foreach (var descriptor in serviceCollection)
{
    if (descriptor.IsKeyedService
        && descriptor.ServiceType == validatorType
        && descriptor.ServiceKey is not null
        && keyedProvider.GetKeyedService(validatorType, descriptor.ServiceKey)
            is IValidator validator)
    {
        yield return validator;
    }
}

Observation: This iterates through the entire IServiceCollection on every call to GetKeyedValidators. For applications with many registered services, this could be inefficient.

Potential optimization (optional): Consider caching the keyed validator descriptors by type, though this may not be necessary for typical use cases. The current implementation is acceptable for v1.

✅ Good: Graceful Fallback

Lines 150-151 in ValidatorRegistryExtensions.cs:

if (serviceCollection is null || serviceProvider is not IKeyedServiceProvider keyedProvider)
    yield break;

This ensures backward compatibility and graceful degradation when keyed services aren't used.

---

Test Coverage Review

✅ Good: Comprehensive Test Scenarios

The test file covers the essential scenarios:

1. Keyed validators via GetValidators()
2. Keyed validators via GetValidator() (singular)
3. Non-keyed validators regression test
4. Mixed keyed/non-keyed deduplication
5. Full integration with schema generation

⚠️ Test Issue: Weak Assertion in Deduplication Test

File: test/MicroElements.Swashbuckle.FluentValidation.Tests/KeyedServicesTests.cs:82-95

[Fact]
public void Mixed_Keyed_And_NonKeyed_No_Duplicates()
{
    var services = new ServiceCollection();
    services.AddFluentValidationRulesToSwagger();
    services.AddScoped<IValidator<KeyedModel>, KeyedModelValidator>();
    services.AddKeyedScoped<IValidator<KeyedModel>, KeyedModelValidator>("myKey");

    var sp = services.BuildServiceProvider();
    using var scope = sp.CreateScope();
    var registry = scope.ServiceProvider.GetRequiredService<IValidatorRegistry>();

    var validators = registry.GetValidators(typeof(KeyedModel)).ToList();
    validators.Should().HaveCountGreaterThanOrEqualTo(1);  // ⚠️ Too weak!
}

Problem: The assertion HaveCountGreaterThanOrEqualTo(1) doesn't actually verify deduplication. It would pass with 1, 2, or even 3+ validators.

Recommended fix:

var validators = registry.GetValidators(typeof(KeyedModel)).ToList();
// Should be exactly 1 if both registrations return the same instance (scoped)
// OR exactly 2 if they return different instances
// The test should clarify the expected behavior
validators.Should().HaveCount(1, "because the same validator instance should be deduplicated");

Question: What's the expected behavior here? Are keyed and non-keyed scoped services the same instance or different? The test name suggests deduplication should occur, but the weak assertion doesn't verify it.

Fix this →

💡 Suggestion: Additional Test Cases

Consider adding tests for:

1. Multiple keyed validators with different keys for the same model type
2. Error handling when IKeyedServiceProvider.GetKeyedService() returns null
3. Behavior when IsOneValidatorForType is true with both keyed and non-keyed validators

---

Code Quality

✅ Good: Clear Comments and Documentation

The code includes helpful XML documentation and inline comments explaining the purpose of each section.

✅ Good: Consistent Code Style

The changes follow the existing code conventions in the repository.

✅ Good: CHANGELOG Documentation

The CHANGELOG entry is comprehensive and clearly describes all the features and guarantees.

---

Recommendations Summary

Must Fix:

1. ✅ Define or import ReferenceEqualityComparer to fix compilation error (ValidatorRegistryExtensions.cs:83)

Should Fix:
2. ⚠️ Strengthen assertion in Mixed_Keyed_And_NonKeyed_No_Duplicates test to actually verify deduplication (KeyedServicesTests.cs:94)

Nice to Have:
3. 💡 Add test cases for multiple keyed validators with different keys
4. 💡 Consider performance optimization for service collection scanning (though current implementation is acceptable)

---

Conclusion

This is a well-designed solution to Issue #165 with good architecture and test coverage. The main blocker is the missing ReferenceEqualityComparer definition which will prevent compilation. Once that's fixed and the test assertion is strengthened, this PR will be ready to merge.

Verdict: ❌ Request Changes (due to compilation error)

---

[avgalex]

Regarding the code review findings:

ReferenceEqualityComparer — False Positive

System.Collections.Generic.ReferenceEqualityComparer is a built-in .NET type available since .NET 5. Since this project targets net8.0/net9.0/net10.0 only, it compiles without issues. Verified: the solution builds successfully and all 54 tests pass on both net8.0 and net9.0.

Test assertion — Fixed

The weak assertion in Mixed_Keyed_And_NonKeyed_No_Duplicates has been fixed in commit 1e53610. Now correctly expects HaveCount(1) since IsOneValidatorForType defaults to true, meaning only the first validator is returned per type.

[avgalex]

Regarding the code review findings:

ReferenceEqualityComparer — False Positive

System.Collections.Generic.ReferenceEqualityComparer is a built-in .NET type available since .NET 5. Since this project targets net8.0/net9.0/net10.0 only, it compiles without issues. Verified: the solution builds successfully and all 54 tests pass on both net8.0 and net9.0.

Test assertion — Fixed

The weak assertion in Mixed_Keyed_And_NonKeyed_No_Duplicates has been fixed in commit 1e53610. Now correctly expects HaveCount(1) since IsOneValidatorForType defaults to true, meaning only the first validator is returned per type.

@claude review this PR

[claude]

Claude finished @avgalex's task in 2m 4s —— View job

---

Code Review for PR #186

Progress

• Read repository guidelines (CLAUDE.md)
• Review core logic changes (ValidatorRegistryExtensions.cs)
• Review service registration changes
• Review test coverage and verify fixes
• Review CHANGELOG and version changes
• Provide comprehensive feedback

---

Summary

This PR successfully implements support for keyed DI services (Issue #165). After reviewing the code and addressing the previous feedback, I can confirm:

✅ ReferenceEqualityComparer - Confirmed Valid: You're absolutely correct - this is a built-in .NET type available since .NET 5, and since this project targets net8.0;net9.0;net10.0, it's available without any issues.

⚠️ Test Assertion - Still Needs Fixing: The current code on the branch still shows HaveCountGreaterThanOrEqualTo(1) in KeyedServicesTests.cs:94. The commit 1e53610 mentioned in your comment doesn't appear to be on this branch yet. You'll need to push that fix.

---

Detailed Review

✅ Core Implementation Quality

The implementation in ValidatorRegistryExtensions.cs is well-designed:

1. Deduplication Logic (lines 83-118): The use of ReferenceEqualityComparer.Instance with a HashSet<IValidator> is the correct approach for deduplicating validators by reference. This prevents the same instance from being returned twice.

2. Three-Phase Discovery (lines 85-137):

   • Phase 1: Non-keyed validators (existing behavior) ✅
   • Phase 2: Keyed validators (new feature) ✅
   • Phase 3: Base type validators (existing behavior) ✅

   The ordering ensures backward compatibility and predictable behavior.

3. Fallback for GetValidator() (lines 27-28): The fallback to keyed validators when no non-keyed validator is found is elegant:

   return serviceProvider.GetService(validatorType) as IValidator
       ?? serviceProvider.GetKeyedValidators(modelType).FirstOrDefault();

4. Graceful Degradation (lines 150-151): The null-checks ensure backward compatibility:

   if (serviceCollection is null || serviceProvider is not IKeyedServiceProvider keyedProvider)
       yield break;

✅ Service Registration Approach

The pattern of registering IServiceCollection as a singleton (lines in both ServiceCollectionExtensions.cs files) is clever and necessary:

services.TryAddSingleton<IServiceCollection>(services);

This enables keyed validator discovery at resolution time, making the solution registration-order independent. This is a significant usability improvement.

⚠️ Outstanding Issue: Test Assertion

File: test/MicroElements.Swashbuckle.FluentValidation.Tests/KeyedServicesTests.cs:94

The test Mixed_Keyed_And_NonKeyed_No_Duplicates currently has:

validators.Should().HaveCountGreaterThanOrEqualTo(1);  // ⚠️ Too weak!

Problem: This assertion doesn't actually verify deduplication. It would pass with 1, 2, or even 3+ validators.

Expected Behavior: Based on the test setup and the fact that IsOneValidatorForType defaults to true, the expected count should be exactly 1 (only the first validator returned).

You mentioned this was fixed in commit 1e53610, but that commit is not present on the current branch. Please push the fix that changes this to:

validators.Should().HaveCount(1, "because IsOneValidatorForType defaults to true");

Fix this →

✅ Test Coverage

The test suite covers essential scenarios well:

1. ✅ Keyed validators via GetValidators() (plural) - line 38
2. ✅ Keyed validators via GetValidator() (singular) - line 53
3. ✅ Non-keyed validators regression test - line 68
4. ⚠️ Mixed keyed/non-keyed (needs assertion fix) - line 82
5. ✅ Full integration with schema generation - line 98

The test at line 98 (Schema_Gets_Validation_Rules_From_Keyed_Validator) is particularly valuable as it verifies the entire integration flow.

💡 Optional Enhancement Suggestions

These are not blocking issues, but could improve the implementation:

1. Performance Consideration: The GetKeyedValidators method (line 153) iterates through the entire IServiceCollection on every call. For applications with many services, consider caching keyed validator descriptors by type. However, this optimization is probably unnecessary for typical use cases and can be deferred.

2. Additional Test Cases (optional):

   • Multiple keyed validators with different keys for the same model type
   • Behavior when IsOneValidatorForType is false with mixed keyed/non-keyed validators
   • Error handling when GetKeyedService() returns null (though this is handled gracefully by the is IValidator validator check)

✅ CHANGELOG and Versioning

The CHANGELOG entry is comprehensive and accurately describes:

• The feature
• How it works
• Registration order independence
• Deduplication behavior
• Graceful fallback

Version bump to 7.0.4-beta.2 is appropriate for a beta release.

---

Recommendations

Must Fix Before Merge:

1. ⚠️ Push the test assertion fix (commit 1e53610 you mentioned) to change line 94 from HaveCountGreaterThanOrEqualTo(1) to HaveCount(1)

Nice to Have (Optional):
2. 💡 Consider adding test cases for multiple keyed validators with different keys
3. 💡 Consider performance optimization for service collection scanning (though current implementation is acceptable)

---

Conclusion

This is a high-quality implementation of keyed service support. The architecture is sound, the code is well-structured, and backward compatibility is maintained. The only outstanding issue is pushing the test assertion fix.

Once the test assertion is fixed and pushed, this PR will be ready to merge.

Current Verdict: ✅ Approve with Minor Fix Required (test assertion needs to be pushed)

---

[avgalex]

The test assertion fix (commit 1e53610) has been pushed to the branch. The bot's previous review ran before the push reached origin.

All issues from the review are now resolved:

• ✅ ReferenceEqualityComparer — confirmed as built-in .NET 5+ type (false positive)
• ✅ Test assertion — fixed to HaveCount(1) matching default IsOneValidatorForType = true behavior
• ✅ All 54 tests pass on net8.0 and net9.0

[avgalex]

The test assertion fix (commit 1e53610) has been pushed to the branch. The bot's previous review ran before the push reached origin.

All issues from the review are now resolved:

• ✅ ReferenceEqualityComparer — confirmed as built-in .NET 5+ type (false positive)
• ✅ Test assertion — fixed to HaveCount(1) matching default IsOneValidatorForType = true behavior
• ✅ All 54 tests pass on net8.0 and net9.0

@claude review this PR

[claude]

Claude finished @avgalex's task in 2m 16s —— View job

---

Code Review for PR #186

Progress

• Read repository guidelines
• Verify test assertion fix (commit 1e53610)
• Review core implementation (ValidatorRegistryExtensions.cs)
• Review service registration changes
• Review test coverage
• Provide final review verdict

---

Summary

✅ All Previous Issues Resolved

You are absolutely correct — both issues identified in the previous reviews have been properly addressed:

1. ✅ ReferenceEqualityComparer — Confirmed Valid: This is indeed a built-in .NET type available in System.Collections.Generic since .NET 5. Since this project targets net8.0;net9.0;net10.0 (verified in the test execution), it's fully supported without any issues. The previous review incorrectly flagged this as a compilation error.

2. ✅ Test Assertion — Fixed: Commit 1e53610 correctly updated the assertion from HaveCountGreaterThanOrEqualTo(1) to:

   validators.Should().HaveCount(1, "IsOneValidatorForType is true by default, so only the first (non-keyed) validator is returned");

   This properly verifies the deduplication behavior and aligns with the default IsOneValidatorForType = true setting.

---

Comprehensive Implementation Review

✅ Core Architecture — Excellent Design

File: src/MicroElements.OpenApi.FluentValidation/FluentValidation/ValidatorRegistry/ValidatorRegistryExtensions.cs

The implementation demonstrates several strong architectural decisions:

1. Three-Phase Validator Discovery (lines 85-137)

// 1. Non-keyed validators (existing behavior)
// 2. Keyed validators (Issue #165) 
// 3. Base type validators (existing behavior)

This ordering ensures backward compatibility while seamlessly integrating keyed service support.

2. Reference-Based Deduplication (line 83)

var seen = new HashSet<IValidator>(ReferenceEqualityComparer.Instance);

Using reference equality is the correct choice here. It prevents the same validator instance from being returned multiple times when registered both as keyed and non-keyed services, without relying on potentially expensive or incorrect value-based equality comparisons.

3. Registration Order Independence (lines 84-85 in ServiceCollectionExtensions)

// Issue #165: Register IServiceCollection for keyed validator discovery at resolution time
services.TryAddSingleton<IServiceCollection>(services);

This clever approach solves a significant usability issue. By registering the IServiceCollection itself as a singleton, the keyed validator discovery can scan descriptors at resolution time, making it work regardless of whether validators are registered before or after AddFluentValidationRulesToSwagger().

4. Graceful Degradation (lines 150-151)

if (serviceCollection is null || serviceProvider is not IKeyedServiceProvider keyedProvider)
    yield break;

The implementation gracefully handles scenarios where:

• Keyed services aren't used
• The DI container doesn't support IKeyedServiceProvider
• The snapshot isn't available (shouldn't happen, but defensively coded)

5. Dual Path Support (lines 27-28)

return serviceProvider.GetService(validatorType) as IValidator
    ?? serviceProvider.GetKeyedValidators(modelType).FirstOrDefault();

Both GetValidator() (singular, used by OperationFilter) and GetValidators() (plural, used by SchemaFilter) paths support keyed validators, ensuring comprehensive coverage.

---

✅ Test Coverage — Comprehensive

File: test/MicroElements.Swashbuckle.FluentValidation.Tests/KeyedServicesTests.cs

The test suite covers all critical scenarios:

1. ✅ Keyed validators via GetValidators() (line 38)

   • Verifies keyed-only scenario works correctly
   • Tests realistic ordering: library registered before validators

2. ✅ Keyed validators via GetValidator() (line 53)

   • Ensures singular path works (used by OperationFilter)
   • Critical for schema generation flow

3. ✅ Non-keyed regression test (line 68)

   • Confirms existing functionality unchanged
   • Important for backward compatibility verification

4. ✅ Mixed keyed/non-keyed deduplication (line 82) — NOW PROPERLY VERIFIED

   • Asserts HaveCount(1) with clear explanation
   • Correctly validates IsOneValidatorForType default behavior
   • Tests the reference-based deduplication logic

5. ✅ Full integration test (line 98)

   • End-to-end validation: keyed validator → schema generation → rules applied
   • Verifies actual schema contains expected validation rules (MinLength, MaxLength, ExclusiveMinimum)

The test quality is high, with clear naming, good comments, and realistic usage patterns.

---

✅ Consistent Implementation Across Packages

Both MicroElements.Swashbuckle.FluentValidation and MicroElements.NSwag.FluentValidation have been updated with identical keyed service registration logic:

Swashbuckle: src/MicroElements.Swashbuckle.FluentValidation/AspNetCore/ServiceCollectionExtensions.cs:84-85
NSwag: src/MicroElements.NSwag.FluentValidation/AspNetCore/ServiceCollectionExtensions.cs:38-39

Both use:

// Issue #165: Register IServiceCollection for keyed validator discovery at resolution time
services.TryAddSingleton<IServiceCollection>(services);

This ensures feature parity across both integration packages.

---

✅ Documentation and Version Management

CHANGELOG.md (lines 1-7):

• Clear, comprehensive description of the feature
• Documents all key characteristics: registration order independence, deduplication, graceful fallback
• Lists both code paths affected (GetValidator and GetValidators)

version.props:

• Version correctly bumped to 7.0.4-beta.2
• Appropriate for a beta release with new functionality

---

Potential Considerations (Non-Blocking)

These are not issues, but observations for future consideration:

💡 Performance: Service Collection Iteration

File: ValidatorRegistryExtensions.cs:153-163

The GetKeyedValidators method iterates through the entire IServiceCollection on every call:

foreach (var descriptor in serviceCollection)
{
    if (descriptor.IsKeyedService
        && descriptor.ServiceType == validatorType
        && descriptor.ServiceKey is not null
        && keyedProvider.GetKeyedService(validatorType, descriptor.ServiceKey)
            is IValidator validator)
    {
        yield return validator;
    }
}

Analysis:

• For typical applications with hundreds of services, this is negligible overhead
• The iteration only happens when validators are needed for schema generation (not per-request)
• The lazy enumeration via yield return means it can short-circuit early when IsOneValidatorForType = true

Potential optimization (for future if needed):

• Cache keyed validator descriptors by type in a Dictionary<Type, List<ServiceDescriptor>>
• However, this adds complexity and memory overhead that's likely unnecessary

Verdict: Current implementation is acceptable. Only optimize if profiling shows this as a bottleneck in real-world scenarios.

💡 Additional Test Scenarios (Optional Enhancement)

Consider adding tests for:

1. Multiple keyed validators with different keys for the same model type

   services.AddKeyedScoped<IValidator<Model>, Validator1>("key1");
   services.AddKeyedScoped<IValidator<Model>, Validator2>("key2");

2. Behavior when IsOneValidatorForType = false with mixed keyed/non-keyed validators
3. Null key handling (though current code filters descriptor.ServiceKey is not null)

These aren't critical but would provide even more comprehensive coverage.

---

Code Quality Assessment

✅ Excellent code quality across all changes:

• Clear, descriptive comments explaining the purpose of each section
• Defensive coding with appropriate null checks
• Consistent code style matching existing codebase conventions
• Well-structured logic with clear separation of concerns
• Appropriate use of LINQ and modern C# patterns

---

Final Verdict

✅ APPROVE — Ready to Merge

This PR successfully implements keyed DI services support with:

• ✅ Solid architectural design with registration order independence
• ✅ Proper deduplication using reference equality
• ✅ Graceful backward compatibility and fallback behavior
• ✅ Comprehensive test coverage with fixed assertions
• ✅ Clear documentation in CHANGELOG
• ✅ Consistent implementation across both Swashbuckle and NSwag packages
• ✅ No compilation errors or unresolved issues

All previous review concerns have been addressed:

• ReferenceEqualityComparer is a valid built-in .NET 5+ type ✅
• Test assertion properly validates deduplication behavior ✅

The implementation is production-ready for 7.0.4-beta.2 release.

---

Excellent work on this feature! The design is thoughtful, the implementation is clean, and the test coverage is comprehensive. 🎉

---

View job