Register validators once per type #2184
Conversation
…ly once for each validator type and as IValidator<Target>
| if (shouldRegister) { | ||
| //Register as interface | ||
| services.Add( | ||
| services.TryAddEnumerable( |
There was a problem hiding this comment.
Why did you use TryAddEnumerable and not TryAdd here?
There was a problem hiding this comment.
The desired behavior is to register every validator both
- as itself (serviceType == implementationType) to be able to resolve as itself
- as
IValidator<Target>(serviceType =IValidator<Target>, implementationType = itself) to be able to injectIEnumerable<IValidator<Target>>
The first use case is covered via TryAdd, since it ensures that only one instance per serviceType is being registered. Multiple calls to AddValidatorsFromAssembly will therefore not lead to registering validators twice.
The second use case needs TryAddEnumerable, since that method matches the combination of serviceType and implementationType. As the name suggests, you can register multiple implementation types (e.g. SomePartialModelValidator, SomeOtherPartialModelValidator) as the same serviceType (e.g. IValidator<Model>).
TryAdd(interfaceType, validatorType) would lead to only the first validator for a validation target being registered and every other being ignored.
There was a problem hiding this comment.
I think we need to be careful about using TryAddEnumerable here, typically you only want 1 validator to be registered for a specific type, so the behaviour of only having the first validator being registered and ignoring the rest might be preferable. What happens if you register multiple validators for the same type and then try and resolve just 1, does it return the first or throw an exception? If the former, then this is fine but if it throws then we'll need to use TryAdd instead
There was a problem hiding this comment.
What happens if you register multiple validators for the same type and then try and resolve just 1, does it return the first or throw an exception?
Well, it would not throw, but inject the last serviceType that was registered - been there, done that, spent hours debugging that case...
But as far as I can tell that behavior is very similar if not identical to using services.Add() or services.Add{Lifetime}() instead (see https://dotnetfiddle.net/FIKgHi and https://dotnetfiddle.net/ZDchVy). So since this actually was the core of the problem, i thought it was intentional to have multiple IValidator<SameModel> right away.
There was a problem hiding this comment.
ok perfect, that should be fine then
|
Merged, thanks |
|
Spend 3 hours to understand, that IList is not supported... #2182 |
Fixes #2182
A few thoughts on that:
FluentValidation.DependencyInjectionExtensions.csprojfrom the test project to not create a whole new project for one test class. But feel free to move it around as you like.ServiceCollections to test the behavior required a new package reference onMicrosoft.Extensions.DependencyInjection. I referenced the newest version of that package (8.0.0), as it is compatible with netstandard2.0. This does not reflect the runtime environments of projects targeting .NET 6 or .NET 7, as corresponding versions of that package would be used by an enclosing framework version. But since it likely doesn't make a difference and to maintain readability, I didn't include the conditional TFM-madness in the project file. If that is desired, just ping me and I will submit it later.