Add support for external class validators #67
Conversation
|
@DamianEdwards would this be something you would consider merging. We found this much easier to convert our existing fluent validation while leaving our models clean. |
|
@DamianEdwards now that .NET 9 has shipped, would this be something you would accept? |
| @@ -532,6 +535,47 @@ private static async Task<bool> TryValidateImpl( | |||
| } | |||
| } | |||
|
|
|||
| if (isValid) | |||
| { | |||
| var validators = (IEnumerable?)serviceProvider?.GetService(typeof(IEnumerable<>).MakeGenericType(typeof(IValidatable<>).MakeGenericType(targetType))); | |||
There was a problem hiding this comment.
It'd be better to cache as much of this reflection in the type cache while walking the type. We don't want to be calling MakeGenericType on every validate call if we can avoid it. The type cache could probably just do this discovery of validators from IServiceProvider up front and make those types available. Also you can check if the IServiceProvider supports IServiceProviderIsService and use that to check for validators rather than performing a lookup and checking for null (which can have side-effects), but if we move all this into the type cache that's probably not important anymore.
| var validatorMethod = validator.GetType().GetMethod(nameof(IValidatable<object>.ValidateAsync)); | ||
| if (validatorMethod is null) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"The type {validators.GetType().Name} does not implement the required method 'Task<IEnumerable<ValidationResult>> ValidateAsync(object, ValidationContext)'."); | ||
| } | ||
|
|
||
| var validateTask = (Task<IEnumerable<ValidationResult>>?)validatorMethod.Invoke(validator, | ||
| new[] { target, validationContext }); |
There was a problem hiding this comment.
Invoking this via reflection every time is not ideal. Would be good to figure out if there's a way to avoid this somehow but, e.g. generating a delegate in the type cache that can be invoked from here without the generic (the body of the generated method would just cast to the target type).
Pre-generating a delegate would mean we could avoid the checks here too on every call and just store the exception up front when the type cache is being built and generate the method to just throw.
| /// Provides a way to add a validator for a type outside the class. | ||
| /// </summary> | ||
| /// <typeparam name="T"></typeparam> | ||
| public interface IValidatable<in T> |
There was a problem hiding this comment.
Not 100% sold on the name. Some alternatives to consider:
IValidate<TTarget>IValidator<TTarget>IValidatorFor<TTarget>
| /// <param name="instance">The object instance to validate.</param> | ||
| /// <param name="validationContext">The validation context.</param> | ||
| /// <returns>A collection that holds failed-validation information.</returns> | ||
| Task<IEnumerable<ValidationResult>> ValidateAsync(T instance, ValidationContext validationContext); |
There was a problem hiding this comment.
Should we have a non-async version? Or do we think given this is a net-new addition it's fine to just make it async-only? Do the non-async validate methods throw if an IValidatable is registered on the type you're trying to validate?
There was a problem hiding this comment.
It feels like this whole library could be simplified if it was async / valuetask only, but that would obviously be a breaking change and dataannotations doesn't support async either. I really wish .NET would modernize data annotations and make them more powerful. :-)
Anyway, it would be pretty trivial to add non-async support. Up to you.
There was a problem hiding this comment.
I think for net-new stuff it's fine to say it's async only. This needs to add logic to throw if the non-async validate method is called on a type that has one of these registered.
|
@ejsmith @niemyjski thanks for this. I quite like the idea but have some suggested implementation changes and open questions. |
Adds support for defining
IValidator<T>classes to provide validation for class types outside of the model class where you are unable to alter the target model or would prefer to keep the model classes simple and not have theIValidatableObjectinterface implementation.