AddValidatorsFromAssemblyContaining would register Validators twice

[peterwurzinger]

FluentValidation version

11.8.1

ASP.NET version

(ASP).NET Core 8

Summary

I honestly don't know if it is an issue, but it manifested as one to me when it occured.

2 (but actually any amount of) calls to AddValidatorsFromAssemblyContaining(<assembly>) would register every validator in the matching assembly 2 times. Clients, that retrieve something like IEnumerable<IValidator<TTarget>> via DI would get 2 instances of the same validator, potentially leading to executing it twice. Or thrice, or ... you get the point.

What sounds as an exotic use case in the first place, isn't too exotic for applications that make use of Vertical Slice Architecture. There the following structures are quite common:

• Feature A
  -- Validators of Feature A (registered via FeatureA.ServiceCollectionExtensions.AddFeatureAServices)
• Feature B
  -- Validators of Feature B (registered via FeatureA.ServiceCollectionExtensions.AddFeatureBServices)
• Feature C
  -- ...

It basically boils down to the registrations in

FluentValidation/src/FluentValidation.DependencyInjectionExtensions/ServiceCollectionExtensions.cs

Lines 95 to 106 in 51e365b

	services.Add(
	new ServiceDescriptor(
	serviceType: scanResult.InterfaceType,
	implementationType: scanResult.ValidatorType,
	lifetime: lifetime));
	//Register as self
	services.Add(
	new ServiceDescriptor(
	serviceType: scanResult.ValidatorType,
	implementationType: scanResult.ValidatorType,
	lifetime: lifetime));

Here the descriptors are added via the Add method.
Changing those to TryAdd, that verifies if a corresponding service descriptor is already contained in the service collection, would resolve this issue.

But as said, I can't tell if this is an inteded behavior for a use case that I currently cannot see. In that case a parameter for example could provice both options:

AddValidatorsFromAssemblyContaining(..., allowDuplicates: false) {
    if (allowDuplicates)
        services.Add(...)
    else
        services.TryAdd(...)
}

Steps to Reproduce

public class ArbitraryTarget{}

public class ArbitraryValidator : AbstractValidator<ArbitraryTarget> { ... }

//Program.cs
services.AddValidatorsFromAssemblyContaining<Program>();
services.AddValidatorsFromAssemblyContaining<Program>();

//Somewhere.cs
var provider = services.BuildServiceProvider();
var validators = provider.GetServices<IValidator<ArbitraryTarget>>();
//Inspect validators.Count();

[Tihomirov-Vasiliy]

@peterwurzinger
Hi, i think that in your case with a slice Vertical Slice Architecture you can use:
services.AddValidatorsFromAssembly(Assembly.GetExecutingAssembly());
or as you showed in steps to reproduce:
services.AddValidatorsFromAssemblyContaining<Program>();

But I also like the idea of not creating duplicates in the DI container and suggest to consider throwing the InvalidOperationException as another way to avoid situations where developer forgets about creating duplicates when adding validators into DI-container

You can check for it in AddScanResult method:

private static IServiceCollection AddScanResult(this IServiceCollection services, AssemblyScanner.AssemblyScanResult scanResult, ServiceLifetime lifetime, Func<AssemblyScanner.AssemblyScanResult, bool> filter) {
		bool shouldRegister = filter?.Invoke(scanResult) ?? true;
		if (shouldRegister) {
                        //Check if ValidatorType is already in DI container
                        if(services.Any(s => s.ServiceType == scanResult.InterfaceType || s.ServiceType == scanResult.ValidatorType)
                              throw new InvalidOperationException($"An attempt was made to add a duplicate validator to the container. InterfaceType: {typeof(scanResult.InterfaceType)}. ValidatorType: {typeof(scanResult.ValidatorType)}")
			....
		}

		return services;
	}

[peterwurzinger]

Hi, i think that in your case with a slice Vertical Slice Architecture you can use:
services.AddValidatorsFromAssembly(Assembly.GetExecutingAssembly());

I'm not sure if I understand what you mean. The problem basically boils down to hosting environments, where multiple identical calls to services.AddValidatorsFromAssembly(<theSameAssembly>) happened for some reason. The VSA use case was just an example to make clear how that could happen without somebody actively misusing the library by calling it twice just for fun.
In my case it would lead to the same behavior, since every Feature slice would register its own validators by scanning the whole assembly, given the fact, that every Feature is implemented in the same assembly.

But I also like the idea of not creating duplicates in the DI container and suggest to consider throwing the InvalidOperationException as another way to avoid situations where developer forgets about creating duplicates when adding validators into DI-container

Honestly I would disagree with that. There certainly are situations, where multiple calls to AddValidatorsFromAssembly are a result of composing some features/libraries/components on a hosting level that are implemented in the same assembly. So something like

services
  .AddPricingFeatures() //registers pricing validators by AddValidatorsFromAssembly() <-- This would not throw
  .AddSalesFeatures() //registers sales validators by AddValidatorsFromAssembly() <-- This would throw
  .AddInventoryFeatures() //registers inventory validators by AddValidatorsFromAssembly() <-- This would also throw

Imho those subsequent call to AddValidatorsFromAssembly() should simply do nothing to adhere to the idempotent DI semantics of .NET Core - hence, the TryAddXY methods to register services. E.g. you could certainly do

services.AddAuthentication().AddAuthentication().AddAuthentication()

and neither would it throw, nor register the authentication services thrice.

[Tihomirov-Vasiliy]

I've just checked which methods are used in Microsoft IServiceCollection extension methods and came to the conclusion that I was wrong about throwing exceptions

I think it's worth removing the possibility of double registration of services in DI and agree with your idea of adding TryAdd to AddValidatorsFrom....()

But what approach will be better?

1. Adding bool argument as you mention it in your first comment
2. Adding methods TryAddValidatorsFrom...() with TryAdd() inside
3. Set TryAdd() as default behaviour

@peterwurzinger What do you think about those second and third ideas?

[peterwurzinger]

In my personal opinion TryAdd or the TryAdd{Lifetime} equivalents are used thoroughly to register services in quite a lot of libraries, therefore being a that well-understood concept to call it good practice.
Yet it alters the current behavior of how validators are registered and how they are resolved, so if somebody relies on being able to resolve the same validator twice, it could even complicate things.

But that all being said, that's something for @JeremySkinner to decide, I'm only the messenger.

[JeremySkinner]

I'm very open to switching to using TryAdd internally, I don't see any problems with that. If you want to submit a PR for this I'll happily review it.

[peterwurzinger]

@JeremySkinner Alright, I opened a fix via #2184 , feel free to review at your convenience

[JeremySkinner]

I've pushed out the 11.9 release with this change