Throw exceptions when async validator is invoked synchronously

FluentValidation · Jun 8, 2021 · f6a28bd · f6a28bd
1 parent ee05651
commit f6a28bd
Show file tree

Hide file tree

Showing 14 changed files with 148 additions and 66 deletions.
diff --git 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
@@ -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;
 
 	/// <summary>
 	/// 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
@@ -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<AsyncValidatorInvokedSynchronouslyException>(() => validator.Validate(person));
 		}
 
 		[Fact]

diff --git 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<AsyncValidatorInvokedSynchronouslyException>(() =>
+				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<AsyncValidatorInvokedSynchronouslyException>(() =>
+				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<AsyncValidatorInvokedSynchronouslyException>(() => 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<AsyncValidatorInvokedSynchronouslyException>(() =>
+				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<AsyncValidatorInvokedSynchronouslyException>(() => validator.Validate(new Person {NickNames = new string[0]}));
 		}
 
 		[Fact]

diff --git 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<AsyncValidatorInvokedSynchronouslyException>(() =>
+				validator.Validate(new Person()));
 		}
 
 		[Fact]

diff --git 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<AsyncValidatorInvokedSynchronouslyException>(() =>
+				validator.Validate(new Person {Id = 0, Surname = "foo"}));
 		}
 
 		[Fact]

diff --git 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<Person>(new Person(), new PropertyChain(), new DefaultValidatorSelector()));
+				.GreaterThanOrEqualTo(3)
+				.WhenAsync((x,c) => Task.FromResult(x.NullableInt != null));
+
+			await _validator.ValidateAsync(new ValidationContext<Person>(new Person(), new PropertyChain(), new DefaultValidatorSelector()));
 		}
 
 		[Fact]

diff --git a/src/FluentValidation.Tests/SharedConditionTests.cs b/src/FluentValidation.Tests/SharedConditionTests.cs
@@ -115,6 +115,18 @@ class BadValidatorDisablesNullCheck : AbstractValidator<string> {
 				When(x => x != null, () => {
 					RuleFor(x => x).Must(x => x != "foo");
 				});
+			}
+
+			protected override void EnsureInstanceNotNull(object instanceToValidate) {
+				//bad.
+			}
+		}
+
+		class AsyncBadValidatorDisablesNullCheck : AbstractValidator<string> {
+			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<string> {
 			}
 		}
 
+
 		[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
@@ -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<AsyncValidatorInvokedSynchronouslyException>(() =>
+				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<ValidationTestException>(() => 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<AsyncValidatorInvokedSynchronouslyException>(() => validator.ShouldNotHaveValidationErrorFor(x => x.CreditCard, testPerson));
+
+			// Executes normally when run async.
+			await Assert.ThrowsAsync<ValidationTestException>(async () => await validator.ShouldNotHaveValidationErrorForAsync(x => x.CreditCard, testPerson));
 		}
 
 		[Fact]

diff --git a/src/FluentValidation/AbstractValidator.cs b/src/FluentValidation/AbstractValidator.cs
@@ -93,17 +93,22 @@ public abstract class AbstractValidator<T> : IValidator<T>, IEnumerable<IValidat
 
 			EnsureInstanceNotNull(context.InstanceToValidate);
 
-			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;
+			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
@@ -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;
+
+	/// <summary>
+	/// This exception is thrown when an asynchronous validator is executed synchronously.
+	/// </summary>
+	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) {
+		}
+	}
+}