Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for external class validators #67

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add support for external class validators #67

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cc66599
Add support for external class validators
ejsmith Oct 14, 2024
24d867e
Address PR feedback
ejsmith Jan 3, 2025
47b4f6b
Missed a spot for using IsService
ejsmith Jan 3, 2025
86a610b
Use linq expressions to build validator delegate
ejsmith Jan 3, 2025
f5e39c0
Minor naming
ejsmith Jan 4, 2025
471248c
Adding service provider extensions. Adding non-async IValidate.
ejsmith Jan 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/MiniValidation/IValidatable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using System.Threading.Tasks;

namespace MiniValidation;

/// <summary>
/// Provides a way to add a validator for a type outside the class.
/// </summary>
/// <typeparam name="T"></typeparam>
public interface IValidatable<in T>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sold on the name. Some alternatives to consider:

  • IValidate<TTarget>
  • IValidator<TTarget>
  • IValidatorFor<TTarget>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for IValidate<T>.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

{
/// <summary>
/// Determines whether the specified object is valid.
/// </summary>
/// <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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It throws for non-async already using the ThrowIfAsyncNotAllowed method.

}
52 changes: 48 additions & 4 deletions src/MiniValidation/MiniValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@ public static class MiniValidator
/// </remarks>
/// <param name="targetType">The <see cref="Type"/>.</param>
/// <param name="recurse"><c>true</c> to recursively check descendant types; if <c>false</c> only simple values directly on the target type are checked.</param>
/// <param name="serviceProvider">The service provider to use when checking for validators.</param>
/// <returns><c>true</c> if <paramref name="targetType"/> has anything to validate, <c>false</c> if not.</returns>
/// <exception cref="ArgumentNullException">Thrown when <paramref name="targetType"/> is <c>null</c>.</exception>
public static bool RequiresValidation(Type targetType, bool recurse = true)
public static bool RequiresValidation(Type targetType, bool recurse = true, IServiceProvider? serviceProvider = null)
{
if (targetType is null)
{
Expand All @@ -44,7 +45,8 @@ public static bool RequiresValidation(Type targetType, bool recurse = true)
return typeof(IValidatableObject).IsAssignableFrom(targetType)
|| typeof(IAsyncValidatableObject).IsAssignableFrom(targetType)
|| (recurse && typeof(IEnumerable).IsAssignableFrom(targetType))
|| _typeDetailsCache.Get(targetType).Properties.Any(p => p.HasValidationAttributes || recurse);
|| _typeDetailsCache.Get(targetType).Properties.Any(p => p.HasValidationAttributes || recurse)
|| serviceProvider?.GetService(typeof(IValidatable<>).MakeGenericType(targetType)) != null;
}

/// <summary>
Expand Down Expand Up @@ -163,7 +165,7 @@ private static bool TryValidateImpl<TTarget>(TTarget target, IServiceProvider? s
throw new ArgumentNullException(nameof(target));
}

if (!RequiresValidation(target.GetType(), recurse))
if (!RequiresValidation(target.GetType(), recurse, serviceProvider))
{
errors = _emptyErrors;

Expand Down Expand Up @@ -306,7 +308,7 @@ private static bool TryValidateImpl<TTarget>(TTarget target, IServiceProvider? s

IDictionary<string, string[]>? errors;

if (!RequiresValidation(target.GetType(), recurse))
if (!RequiresValidation(target.GetType(), recurse, serviceProvider))
{
errors = _emptyErrors;

Expand Down Expand Up @@ -415,6 +417,7 @@ private static async Task<bool> TryValidateImpl(
(property.Recurse
|| typeof(IValidatableObject).IsAssignableFrom(propertyValueType)
|| typeof(IAsyncValidatableObject).IsAssignableFrom(propertyValueType)
|| serviceProvider?.GetService(typeof(IValidatable<>).MakeGenericType(propertyValueType!)) != null
|| properties.Any(p => p.Recurse)))
{
propertiesToRecurse!.Add(property, propertyValue);
Expand Down Expand Up @@ -532,6 +535,47 @@ private static async Task<bool> TryValidateImpl(
}
}

if (isValid)
{
var validators = (IEnumerable?)serviceProvider?.GetService(typeof(IEnumerable<>).MakeGenericType(typeof(IValidatable<>).MakeGenericType(targetType)));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cached and used IServiceProviderIsService

if (validators != null)
{
foreach (var validator in validators)
{
if (!isValid)
continue;

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 });
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added as much caching as I could. I couldn't figure out how to get MethodInfo.CreateDelegate to work because we need to cast the target parameter to the target type, but at runtime, we don't know that type.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, figured out how to use expressions to compile a delegate that casts the parameter.

if (validateTask is null)
{
throw new InvalidOperationException(
$"The type {validators.GetType().Name} does not implement the required method 'Task<IEnumerable<ValidationResult>> ValidateAsync(object, ValidationContext)'.");
}

// Reset validation context
validationContext.MemberName = null;
validationContext.DisplayName = validationContext.ObjectType.Name;

ThrowIfAsyncNotAllowed(validateTask.IsCompleted, allowAsync);

var validatableResults = await validateTask.ConfigureAwait(false);
if (validatableResults is not null)
{
ProcessValidationResults(validatableResults, workingErrors, prefix);
isValid = workingErrors.Count == 0 && isValid;
}
}
}
}

// Update state of target in tracking dictionary
validatedObjects[target] = isValid;

Expand Down
41 changes: 41 additions & 0 deletions tests/MiniValidation.UnitTests/TestTypes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,47 @@ public async Task<IEnumerable<ValidationResult>> ValidateAsync(ValidationContext
}
}

class TestClassLevel
{
public int TwentyOrMore { get; set; } = 20;
}

class TestClassLevelValidator : IValidatable<TestClassLevel>
{
public async Task<IEnumerable<ValidationResult>> ValidateAsync(TestClassLevel instance, ValidationContext validationContext)
{
await Task.Yield();

List<ValidationResult>? errors = null;

if (instance.TwentyOrMore < 20)
{
errors ??= new List<ValidationResult>();
errors.Add(new ValidationResult($"The field {validationContext.DisplayName} must have a value greater than 20.", new[] { nameof(TestClassLevel.TwentyOrMore) }));
}

return errors ?? Enumerable.Empty<ValidationResult>();
}
}

class ExtraTestClassLevelValidator : IValidatable<TestClassLevel>
{
public async Task<IEnumerable<ValidationResult>> ValidateAsync(TestClassLevel instance, ValidationContext validationContext)
{
await Task.Yield();

List<ValidationResult>? errors = null;

if (instance.TwentyOrMore > 20)
{
errors ??= new List<ValidationResult>();
errors.Add(new ValidationResult($"The field {validationContext.DisplayName} must have a value less than 20.", new[] { nameof(TestClassLevel.TwentyOrMore) }));
}

return errors ?? Enumerable.Empty<ValidationResult>();
}
}

class TestClassLevelAsyncValidatableOnlyTypeWithServiceProvider : IAsyncValidatableObject
{
public async Task<IEnumerable<ValidationResult>> ValidateAsync(ValidationContext validationContext)
Expand Down
57 changes: 57 additions & 0 deletions tests/MiniValidation.UnitTests/TryValidate.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,63 @@ public async Task TryValidateAsync_With_ServiceProvider()
Assert.Equal(nameof(IServiceProvider), errors.Keys.First());
}

[Fact]
public void TryValidate_With_Validator()
{
var serviceCollection = new ServiceCollection();
serviceCollection.AddSingleton<IValidatable<TestClassLevel>, TestClassLevelValidator>();
var serviceProvider = serviceCollection.BuildServiceProvider();

var thingToValidate = new TestClassLevel
{
TwentyOrMore = 12
};

Assert.Throws<InvalidOperationException>(() =>
{
var isValid = MiniValidator.TryValidate(thingToValidate, serviceProvider, out var errors);
});
}

[Fact]
public async Task TryValidateAsync_With_Validator()
{
var serviceCollection = new ServiceCollection();
serviceCollection.AddSingleton<IValidatable<TestClassLevel>, TestClassLevelValidator>();
var serviceProvider = serviceCollection.BuildServiceProvider();

var thingToValidate = new TestClassLevel
{
TwentyOrMore = 12
};

var (isValid, errors) = await MiniValidator.TryValidateAsync(thingToValidate, serviceProvider);

Assert.False(isValid);
Assert.Single(errors);
Assert.Equal(nameof(TestValidatableType.TwentyOrMore), errors.Keys.First());
}

[Fact]
public async Task TryValidateAsync_With_Multiple_Validators()
{
var serviceCollection = new ServiceCollection();
serviceCollection.AddSingleton<IValidatable<TestClassLevel>, TestClassLevelValidator>();
serviceCollection.AddSingleton<IValidatable<TestClassLevel>, ExtraTestClassLevelValidator>();
var serviceProvider = serviceCollection.BuildServiceProvider();

var thingToValidate = new TestClassLevel
{
TwentyOrMore = 22
};

var (isValid, errors) = await MiniValidator.TryValidateAsync(thingToValidate, serviceProvider);

Assert.False(isValid);
Assert.Single(errors);
Assert.Equal(nameof(TestValidatableType.TwentyOrMore), errors.Keys.First());
}

[Fact]
public void TryValidate_Enumerable_With_ServiceProvider()
{
Expand Down