diff --git a/Changelog.txt b/Changelog.txt index 90d629e39..fbceb77a0 100644 --- a/Changelog.txt +++ b/Changelog.txt @@ -1,5 +1,6 @@ 11.0.0 - Ensure property covariance is properly handled throughout the internal model (#1713) +Throw exceptions when async validator is invoked synchronously (#1705) 10.2.3 - 3 Jun 2021 Resolve issue with rulesets not cascading correctly to Inheritance Validators (#1754) diff --git a/src/FluentValidation.AspNetCore/FluentValidationModelValidatorProvider.cs b/src/FluentValidation.AspNetCore/FluentValidationModelValidatorProvider.cs index 5aa212305..af394b152 100644 --- a/src/FluentValidation.AspNetCore/FluentValidationModelValidatorProvider.cs +++ b/src/FluentValidation.AspNetCore/FluentValidationModelValidatorProvider.cs @@ -28,6 +28,7 @@ namespace FluentValidation.AspNetCore { using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Microsoft.Extensions.DependencyInjection; + using Results; /// /// ModelValidatorProvider implementation only used for child properties. @@ -110,7 +111,15 @@ public FluentValidationModelValidator(bool implicitValidationEnabled) context = interceptor.BeforeAspNetValidation(mvContext.ActionContext, context) ?? context; } - var result = validator.Validate(context); + + ValidationResult result; + + try { + result = validator.Validate(context); + } + catch (AsyncValidatorInvokedSynchronouslyException ex) { + throw new AsyncValidatorInvokedSynchronouslyException($"The validator \"{ex.ValidatorType.Name}\" can't be used with ASP.NET automatic validation as it contains asynchronous rules."); + } if (interceptor != null) { // allow the user to provide a custom collection of failures, which could be empty. diff --git a/src/FluentValidation.Tests/ComplexValidationTester.cs b/src/FluentValidation.Tests/ComplexValidationTester.cs index 1f3ecdd67..1cdccf47b 100644 --- a/src/FluentValidation.Tests/ComplexValidationTester.cs +++ b/src/FluentValidation.Tests/ComplexValidationTester.cs @@ -134,13 +134,12 @@ public class ComplexValidationTester { } [Fact] - public void Async_condition_should_work_with_complex_property_when_validator_invoked_synchronously() { + public void Async_condition_throws_when_validator_invoked_synchronously() { var validator = new TestValidator() { v => v.RuleFor(x => x.Address).SetValidator(new AddressValidator()).WhenAsync(async (x, c) => x.Address.Line1 == "foo") }; - var result = validator.Validate(person); - result.IsValid.ShouldBeTrue(); + Assert.Throws(() => validator.Validate(person)); } [Fact] diff --git a/src/FluentValidation.Tests/ConditionTests.cs b/src/FluentValidation.Tests/ConditionTests.cs index d67d38867..265fb9470 100644 --- a/src/FluentValidation.Tests/ConditionTests.cs +++ b/src/FluentValidation.Tests/ConditionTests.cs @@ -106,8 +106,8 @@ public class ConditionTests { v => v.RuleFor(x => x.Surname).NotNull().NotEqual("foo").WhenAsync(async (x,c) => x.Id > 0) }; - var result = validator.Validate(new Person()); - result.Errors.Count.ShouldEqual(0); + Assert.Throws(() => + validator.Validate(new Person())); } [Fact] @@ -144,41 +144,42 @@ public class ConditionTests { } [Fact] - public void Async_condition_executed_synchronosuly_with_synchronous_role() { + public void Async_condition_throws_when_executed_synchronosuly_with_synchronous_role() { var validator = new TestValidator(); validator.RuleFor(x => x.Surname).NotNull() .WhenAsync((x, token) => Task.FromResult(false)); - var result = validator.Validate(new Person()); - result.IsValid.ShouldBeTrue(); + + Assert.Throws(() => + validator.Validate(new Person())); } [Fact] - public void Async_condition_executed_synchronosuly_with_asynchronous_rule() { + public void Async_condition_throws_when_executed_synchronosuly_with_asynchronous_rule() { var validator = new TestValidator(); validator.RuleFor(x => x.Surname) .MustAsync((surname, c) => Task.FromResult(surname != null)) .WhenAsync((x, token) => Task.FromResult(false)); - var result = validator.Validate(new Person()); - result.IsValid.ShouldBeTrue(); + + Assert.Throws(() => validator.Validate(new Person())); } [Fact] - public void Async_condition_executed_synchronosuly_with_synchronous_collection_role() { + public void Async_condition_throws_when_executed_synchronosuly_with_synchronous_collection_role() { var validator = new TestValidator(); validator.RuleForEach(x => x.NickNames).NotNull() .WhenAsync((x, token) => Task.FromResult(false)); - var result = validator.Validate(new Person { NickNames = new string[0] }); - result.IsValid.ShouldBeTrue(); + Assert.Throws(() => + validator.Validate(new Person {NickNames = new string[0]})); } [Fact] - public void Async_condition_executed_synchronosuly_with_asynchronous_collection_rule() { + public void Async_condition_throws_when_invoked_synchronosuly_with_asynchronous_collection_rule() { var validator = new TestValidator(); validator.RuleForEach(x => x.NickNames) .MustAsync((n, c) => Task.FromResult(n != null)) .WhenAsync((x, token) => Task.FromResult(false)); - var result = validator.Validate(new Person { NickNames = new string[0]}); - result.IsValid.ShouldBeTrue(); + + Assert.Throws(() => validator.Validate(new Person {NickNames = new string[0]})); } [Fact] diff --git a/src/FluentValidation.Tests/CustomValidatorTester.cs b/src/FluentValidation.Tests/CustomValidatorTester.cs index 6d5d382d7..4ec0738fe 100644 --- a/src/FluentValidation.Tests/CustomValidatorTester.cs +++ b/src/FluentValidation.Tests/CustomValidatorTester.cs @@ -144,13 +144,13 @@ public class CustomValidatorTester { } [Fact] - public void Runs_async_rule_synchronously_when_validator_invoked_synchronously() { + public void Throws_when_async_rule_invoked_synchronously() { validator.RuleFor(x => x.Forename).CustomAsync((x, context, cancel) => { context.AddFailure("foo"); return Task.CompletedTask; }); - var result = validator.Validate(new Person()); - result.Errors.Count.ShouldEqual(1); + Assert.Throws(() => + validator.Validate(new Person())); } [Fact] diff --git a/src/FluentValidation.Tests/OnFailureTests.cs b/src/FluentValidation.Tests/OnFailureTests.cs index d8364fbd1..d5bad7504 100644 --- a/src/FluentValidation.Tests/OnFailureTests.cs +++ b/src/FluentValidation.Tests/OnFailureTests.cs @@ -125,17 +125,15 @@ public class OnFailureTests { } [Fact] - public void WhenAsyncWithOnFailure_should_invoke_condition_on_inner_validator_invoked_synchronously() { - bool shouldNotBeTrue = false; + public void WhenAsyncWithOnFailure_throws_when_async_condition_on_inner_validator_invoked_synchronously() { var validator = new TestValidator(); validator.RuleFor(x => x.Surname) .NotEqual("foo") .WhenAsync((x, token) => Task.FromResult(x.Id > 0)) - .OnFailure(x => shouldNotBeTrue = true); + .OnFailure(x => {}); - var result = validator.Validate(new Person {Id = 0, Surname = "foo"}); - result.Errors.Count.ShouldEqual(0); - shouldNotBeTrue.ShouldBeFalse(); + Assert.Throws(() => + validator.Validate(new Person {Id = 0, Surname = "foo"})); } [Fact] diff --git a/src/FluentValidation.Tests/RuleBuilderTests.cs b/src/FluentValidation.Tests/RuleBuilderTests.cs index 122d6c45f..9c0890d0a 100644 --- a/src/FluentValidation.Tests/RuleBuilderTests.cs +++ b/src/FluentValidation.Tests/RuleBuilderTests.cs @@ -142,10 +142,12 @@ public class RuleBuilderTests { } [Fact] - public void Nullable_object_with_async_condition_should_not_throw() { + public async Task Nullable_object_with_async_condition_should_not_throw() { _validator.RuleFor(x => x.NullableInt.Value) - .GreaterThanOrEqualTo(3).WhenAsync((x,c) => Task.FromResult(x.NullableInt != null)); - _validator.Validate(new ValidationContext(new Person(), new PropertyChain(), new DefaultValidatorSelector())); + .GreaterThanOrEqualTo(3) + .WhenAsync((x,c) => Task.FromResult(x.NullableInt != null)); + + await _validator.ValidateAsync(new ValidationContext(new Person(), new PropertyChain(), new DefaultValidatorSelector())); } [Fact] diff --git a/src/FluentValidation.Tests/SharedConditionTests.cs b/src/FluentValidation.Tests/SharedConditionTests.cs index ebb764c5b..916c2750f 100644 --- a/src/FluentValidation.Tests/SharedConditionTests.cs +++ b/src/FluentValidation.Tests/SharedConditionTests.cs @@ -115,6 +115,18 @@ class BadValidatorDisablesNullCheck : AbstractValidator { When(x => x != null, () => { RuleFor(x => x).Must(x => x != "foo"); }); + } + + protected override void EnsureInstanceNotNull(object instanceToValidate) { + //bad. + } + } + + class AsyncBadValidatorDisablesNullCheck : AbstractValidator { + public AsyncBadValidatorDisablesNullCheck() { + When(x => x != null, () => { + RuleFor(x => x).Must(x => x != "foo"); + }); WhenAsync(async (x, ct) => x != null, () => { RuleFor(x => x).Must(x => x != "foo"); @@ -126,6 +138,7 @@ class BadValidatorDisablesNullCheck : AbstractValidator { } } + [Fact] public void Shared_When_is_not_applied_to_grouped_rules_when_initial_predicate_is_false() { var validator = new SharedConditionValidator(); @@ -416,14 +429,14 @@ public void Does_not_execute_customasync_Rule_when_condition_false() } [Fact] - public void Executes_custom_rule_when_async_condition_true() { + public async Task Executes_custom_rule_when_async_condition_true() { var validator = new TestValidator(); validator.WhenAsync(async (x,c) =>(true), () => { validator.RuleFor(x=>x).Custom((x,ctx) => ctx.AddFailure(new ValidationFailure("foo", "bar"))); }); - var result = validator.Validate(new Person()); + var result = await validator.ValidateAsync(new Person()); result.IsValid.ShouldBeFalse(); } @@ -459,14 +472,14 @@ public void Does_not_execute_customasync_Rule_when_condition_false() } [Fact] - public void Nested_async_conditions_with_Custom_rule() { + public async Task Nested_async_conditions_with_Custom_rule() { var validator = new TestValidator(); validator.When(x => true, () => { validator.WhenAsync(async (x,c) =>(false), () => { validator.RuleFor(x=>x).Custom((x,ctx) => ctx.AddFailure(new ValidationFailure("Custom", "The validation failed"))); }); }); - var result = validator.Validate(new Person()); + var result = await validator.ValidateAsync(new Person()); result.IsValid.ShouldBeTrue(); } @@ -667,7 +680,7 @@ public void Does_not_execute_customasync_Rule_when_condition_false() [Fact] public async Task Doesnt_throw_NullReferenceException_when_instance_not_null_async() { - var v = new BadValidatorDisablesNullCheck(); + var v = new AsyncBadValidatorDisablesNullCheck(); var result = await v.ValidateAsync((string) null); result.IsValid.ShouldBeTrue(); } diff --git a/src/FluentValidation.Tests/ValidatorTesterTester.cs b/src/FluentValidation.Tests/ValidatorTesterTester.cs index a93322e33..121f32639 100644 --- a/src/FluentValidation.Tests/ValidatorTesterTester.cs +++ b/src/FluentValidation.Tests/ValidatorTesterTester.cs @@ -32,7 +32,6 @@ public class ValidatorTesterTester { public ValidatorTesterTester() { validator = new TestValidator(); - validator.RuleFor(x => x.CreditCard).Must(creditCard => !string.IsNullOrEmpty(creditCard)).WhenAsync((x, cancel) => Task.Run(() => { return x.Age >= 18; })); validator.RuleFor(x => x.Forename).NotNull(); validator.RuleForEach(person => person.NickNames).MinimumLength(5); CultureScope.SetDefaultCulture(); @@ -551,13 +550,22 @@ public class ValidatorTesterTester { [Theory] [InlineData(42, null)] [InlineData(42, "")] - public void ShouldHaveValidationError_should_not_throw_when_there_are_validation_errors__WhenAsyn_is_used(int age, string cardNumber) { + public async Task ShouldHaveValidationError_should_not_throw_when_there_are_validation_errors__WhenAsyn_is_used(int age, string cardNumber) { Person testPerson = new Person() { CreditCard = cardNumber, Age = age }; - validator.ShouldHaveValidationErrorFor(x => x.CreditCard, testPerson); + validator.RuleFor(x => x.CreditCard) + .Must(creditCard => !string.IsNullOrEmpty(creditCard)) + .WhenAsync((x, cancel) => Task.FromResult(x.Age >= 18)); + + // Throws when called sync. + Assert.Throws(() => + validator.ShouldHaveValidationErrorFor(x => x.CreditCard, testPerson)); + + // Executes normally when called async + await validator.ShouldHaveValidationErrorForAsync(x => x.CreditCard, testPerson); } [Theory] @@ -591,13 +599,21 @@ public class ValidatorTesterTester { [Theory] [InlineData(42, null)] [InlineData(42, "")] - public void ShouldNotHaveValidationError_should_throw_when_there_are_validation_errors__WhenAsyn_is_used(int age, string cardNumber) { + public async Task ShouldNotHaveValidationError_should_throw_when_there_are_validation_errors__WhenAsyn_is_used(int age, string cardNumber) { Person testPerson = new Person() { CreditCard = cardNumber, Age = age }; - Assert.Throws(() => validator.ShouldNotHaveValidationErrorFor(x => x.CreditCard, testPerson)); + validator.RuleFor(x => x.CreditCard) + .Must(creditCard => !string.IsNullOrEmpty(creditCard)) + .WhenAsync((x, cancel) => Task.FromResult(x.Age >= 18)); + + // Throws async exception when invoked synchronously + Assert.Throws(() => validator.ShouldNotHaveValidationErrorFor(x => x.CreditCard, testPerson)); + + // Executes normally when run async. + await Assert.ThrowsAsync(async () => await validator.ShouldNotHaveValidationErrorForAsync(x => x.CreditCard, testPerson)); } [Fact] diff --git a/src/FluentValidation/AbstractValidator.cs b/src/FluentValidation/AbstractValidator.cs index bc3bf0a09..3c9d70b32 100644 --- a/src/FluentValidation/AbstractValidator.cs +++ b/src/FluentValidation/AbstractValidator.cs @@ -93,17 +93,22 @@ public abstract class AbstractValidator : IValidator, IEnumerable 0) { - // Bail out if we're "failing-fast". - // Check for > 0 rather than == 1 because a rule chain may have overridden the Stop behaviour to Continue - // meaning that although the first rule failed, it actually generated 2 failures if there were 2 validators - // in the chain. - break; + try { + foreach (var rule in Rules) { + rule.Validate(context); + + if (CascadeMode == CascadeMode.Stop && result.Errors.Count > 0) { + // Bail out if we're "failing-fast". + // Check for > 0 rather than == 1 because a rule chain may have overridden the Stop behaviour to Continue + // meaning that although the first rule failed, it actually generated 2 failures if there were 2 validators + // in the chain. + break; + } } } + catch (AsyncValidatorInvokedSynchronouslyException) { + throw new AsyncValidatorInvokedSynchronouslyException(GetType()); + } SetExecutedRulesets(result, context); diff --git a/src/FluentValidation/AsyncValidatorInvokedSynchronouslyException.cs b/src/FluentValidation/AsyncValidatorInvokedSynchronouslyException.cs new file mode 100644 index 000000000..7f5d92e27 --- /dev/null +++ b/src/FluentValidation/AsyncValidatorInvokedSynchronouslyException.cs @@ -0,0 +1,39 @@ +#region License +// Copyright (c) .NET Foundation and contributors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// The latest version of this file can be found at https://github.com/FluentValidation/FluentValidation +#endregion + +namespace FluentValidation { + using System; + + /// + /// This exception is thrown when an asynchronous validator is executed synchronously. + /// + public class AsyncValidatorInvokedSynchronouslyException : InvalidOperationException { + public Type ValidatorType { get; } + + internal AsyncValidatorInvokedSynchronouslyException() { + } + + internal AsyncValidatorInvokedSynchronouslyException(Type validatorType) + : base($"Validator \"{validatorType.Name}\" contains asynchronous rules but was invoked synchronously. Please call ValidateAsync rather than Validate.") { + ValidatorType = validatorType; + } + + internal AsyncValidatorInvokedSynchronouslyException(string message) : base(message) { + } + } +} diff --git a/src/FluentValidation/Internal/CollectionPropertyRule.cs b/src/FluentValidation/Internal/CollectionPropertyRule.cs index 88722d634..ac95b2735 100644 --- a/src/FluentValidation/Internal/CollectionPropertyRule.cs +++ b/src/FluentValidation/Internal/CollectionPropertyRule.cs @@ -115,9 +115,7 @@ public CollectionPropertyRule(MemberInfo member, Func> } if (AsyncCondition != null) { - if (! AsyncCondition(context, default).GetAwaiter().GetResult()) { - return; - } + throw new AsyncValidatorInvokedSynchronouslyException(); } var filteredValidators = GetValidatorsToExecute(context); @@ -165,7 +163,7 @@ public CollectionPropertyRule(MemberInfo member, Func> foreach (var validator in filteredValidators) { context.MessageFormatter.Reset(); if (validator.ShouldValidateAsynchronously(context)) { - InvokePropertyValidatorAsync(context, valueToValidate, propertyNameToValidate, validator, index, default).GetAwaiter().GetResult(); + throw new AsyncValidatorInvokedSynchronouslyException(); } else { InvokePropertyValidator(context, valueToValidate, propertyNameToValidate, validator, index); @@ -229,9 +227,7 @@ public CollectionPropertyRule(MemberInfo member, Func> } if (AsyncCondition != null) { - if (! AsyncCondition(context, default).GetAwaiter().GetResult()) { - return; - } + throw new AsyncValidatorInvokedSynchronouslyException(); } var filteredValidators = await GetValidatorsToExecuteAsync(context, cancellation); @@ -335,9 +331,7 @@ public CollectionPropertyRule(MemberInfo member, Func> } if (component.HasAsyncCondition) { - if (!component.InvokeAsyncCondition(context, default).GetAwaiter().GetResult()) { - validators.Remove(component); - } + throw new AsyncValidatorInvokedSynchronouslyException(); } } diff --git a/src/FluentValidation/Internal/PropertyRule.cs b/src/FluentValidation/Internal/PropertyRule.cs index dc2ebfdda..d591a463b 100644 --- a/src/FluentValidation/Internal/PropertyRule.cs +++ b/src/FluentValidation/Internal/PropertyRule.cs @@ -99,9 +99,7 @@ TProperty PropertyFunc(T instance) } if (AsyncCondition != null) { - if (!AsyncCondition(context, default).GetAwaiter().GetResult()) { - return; - } + throw new AsyncValidatorInvokedSynchronouslyException(); } var cascade = CascadeMode; @@ -117,12 +115,12 @@ TProperty PropertyFunc(T instance) continue; } - if (step.HasAsyncCondition && !step.InvokeAsyncCondition(context, CancellationToken.None).GetAwaiter().GetResult()) { - continue; + if (step.HasAsyncCondition) { + throw new AsyncValidatorInvokedSynchronouslyException(); } if (step.ShouldValidateAsynchronously(context)) { - InvokePropertyValidatorAsync(context, accessor, propertyName, step, default).GetAwaiter().GetResult(); + throw new AsyncValidatorInvokedSynchronouslyException(); } else { InvokePropertyValidator(context, accessor, propertyName, step); diff --git a/src/FluentValidation/TestHelper/ValidatorTestExtensions.cs b/src/FluentValidation/TestHelper/ValidatorTestExtensions.cs index 455940a93..35e6efbcb 100644 --- a/src/FluentValidation/TestHelper/ValidatorTestExtensions.cs +++ b/src/FluentValidation/TestHelper/ValidatorTestExtensions.cs @@ -175,7 +175,14 @@ public static class ValidationTestExtension { /// public static TestValidationResult TestValidate(this IValidator validator, T objectToTest, Action> options = null) where T : class { options ??= _ => { }; - var validationResult = validator.Validate(objectToTest, options); + ValidationResult validationResult; + try { + validationResult = validator.Validate(objectToTest, options); + } + catch (AsyncValidatorInvokedSynchronouslyException ex) { + throw new AsyncValidatorInvokedSynchronouslyException(ex.ValidatorType.Name + " contains asynchronous rules - please use the asynchronous test methods instead."); + } + return new TestValidationResult(validationResult); }