Support base type validators, e.g. IEnumerable<MyInput>

[benmccallum]

~In HotChocolate v12, array arguments changed from being typed as T[] (v11 and prior) to List<T>.~ Update: Turns out there's a HC v12 bug reporting the arg def incorrectly.

In lieu of someone using the input object type pattern at the top level, e.g. never exposing a scalar arg like this, the question is whether there's a more generic way to handle this.

Option one would be to solve #56 and then people could just turn that feature on.

Option two would be to support lookup of validators by base types, e.g. you'd create a AbstractValidator<IEnumerable<MyInput>> and a List of Array would work.

The interesting thing is FluentValidation's MVC integration would need to tackle the same problems and it seems like they're just dodging it entirely.

Automatic Registration for validators will only work for AbstractValidators implementing a concrete type like List or Array. Implementations with interface types like IEnumerable or IList may be used, but the validator will need to be specifically registered as a scoped service in your app’s Startup class. This is due to the ASP.NET’s Model-Binding of collection types where interfaces like IEnumerable will be converted to a List implementation and a List is the type MVC passes to FluentValidation.

Source: https://docs.fluentvalidation.net/en/latest/aspnet.html#automatic-registration

So I imagine that @jeremyskinner's ~cache of validators for T must crawl up the base types and add them to a lookup somewhere, though I'll need to do some digging.~

I'm kind of leaning towards doing option 2 and really ~following what FV.Mvc does for consistency~.

Edit: FV.Mvc relies on MVC infra (see Jeremy's comment below).

[benmccallum]

https://github.com/FluentValidation/FluentValidation/blob/fb1e9392dc0b97ed2b60bcaa3cef425a413c1a6a/src/FluentValidation.AspNetCore/FluentValidationModelValidatorProvider.cs

ASP.NET Core validator cache: https://github.com/FluentValidation/FluentValidation/blob/main/src/FluentValidation.AspNetCore/ValidatorDescriptorCache.cs

IValidatorFactory GetValidator implemented in ValidatorFactoryBase: https://github.com/FluentValidation/FluentValidation/blob/2671e7fb09af9e1ef2a938a8b4364724ef9e47bd/src/FluentValidation/ValidatorFactoryBase.cs

[JeremySkinner]

So I imagine that @JeremySkinner's cache of validators for T must crawl up the base types and add them to a lookup somewhere, though I'll need to do some digging.

No we don't actually do any caching for this, or any crawling. We rely entirely on the type information passed to us by the ASP.NET validation infrastructure. For example, if a user takes an IEnumerable<T> as a controller action parameter then ASP.NET will internally instantiate a List<T>, and this is the type that's then passed to FluentValidation via ASP.NET's metadata (so we'll then attempt to resolve an IValidator<List<T>> from the container). This behaviour can be somewhat confusing, as users would expect if their controller parameters are IEnumerable<T> then their validators should be AbstractValidator<IEnumerable<T>> but this isn't the case.

Because of this we actually recommend that users are explicit with their controller action parameters and use concrete types rather than interfaces, because the rules on what types are instantiated internally are handled silently by MVC itself, so being explicit with concrete types is much clearer.

But yeah we don't do any crawling/caching, just pass on the type that ASP.NET tells us directly to the container for resolution.

Edit: see https://github.com/FluentValidation/FluentValidation/blob/fb1e9392dc0b97ed2b60bcaa3cef425a413c1a6a/src/FluentValidation.AspNetCore/FluentValidationModelValidatorProvider.cs#L76-L82

ASP.NET Core validator cache:

This cache isn't actually related; this is used by clientside validation to ensure only one descriptor is created for each type per request (because the jquery clientside validation stuff will attempt to instantiate a validator for every property it encounters). This is somewhat legacy from when validators were registered as Transient. Less of an issue now they're Scoped per-request.

[benmccallum]

Thanks @JeremySkinner, appreciate your insights as always! I was just coming to the same conclusions as I made my way through the code and into the FluentValidationModelValidatorProvider, so great to get that confirmed.

I'm going to have to think about this and how it applies to FairyBread. Essentially we have a similar problem, substituting MVC with HotChocolate. It's even more of a misdirection with HC as the GraphQL only has arrays, so in the SDL you type it as an array but get a C# List, and you can't really control that unlike a model param in an MVC action which you could make concrete like you mentioned.

I'm now doubting whether supporting #56 is even the right way to go.

I may just need to make this very obvious in documentation and perhaps put in some integration testing to make sure I notice a change in HotChocolate's runtime handling of GraphQL array-typed arguments so I can spread the word. Recommend that as you've done yourself.

Thanks again!

[JeremySkinner]

Personally I'm not at all a fan of implicit validation, it's a pain to maintain, or of validating IEnumerables. Model binding to interfaces doesn't really make sense, as at the end of the day you have to bind to something that can be instantiated. With IEnumerable you can make a best guess as to which concrete type to use (usually List), but this is a special case, for all other interface types you have no real way of knowing what concrete type should be. IMO it's much better to be explicit: only bind to concrete types, and explicitly set up validators for those types. Much easier to follow the flow of code.

In the case of Hot Chocolate 12, I'd say that refactoring and explicitly changing the validator types from AbstractValidator<T[]> to AbstractValidator<List<T>> is the correct approach, and that users should be encouraged to be explicit with their collection types during model binding for clarity, rather than relying on IEnumerable. In my opinion trying to handle this in a generic way at a library level is more trouble than it's worth.

[benmccallum]

Thanks for the advice @JeremySkinner, it turns out the original issue is a HC v12 bug as it's telling me the arg is gonna be an array but it's actually a List.

It's being tracked here: https://github.com/ChilliCream/hotchocolate/issues/4281

So I'm going to close this issue as I agree trying to do any kind of smarts around finding applicable base type validators is just opening a can of worms.