Skip to content

Fix ValidatableTypeInfo to skip IValidatableObject validation when property validation fails #62623

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

Closed
wants to merge 7 commits into from
Closed

Fix ValidatableTypeInfo to skip IValidatableObject validation when property validation fails #62623

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a3b84bb
Initial plan
Copilot Jul 9, 2025
57b678f
Initial investigation and test setup
Copilot Jul 9, 2025
7a655f4
Fix ValidatableTypeInfo to skip IValidatableObject validation when pr…
Copilot Jul 9, 2025
9708b81
Make ValidatePropertyAttributesAsync and ValidateComplexObjectsAsync …
Copilot Jul 9, 2025
0934867
Add test demonstrating System.ComponentModel.DataAnnotations.Validato…
Copilot Jul 10, 2025
3e8c7fb
Clarify ValidatePropertyAttributesAsync result handling with explicit…
Copilot Jul 10, 2025
ae00450
Fix ValidatablePropertyInfo to skip complex object validation when pr…
Copilot Jul 10, 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
81 changes: 65 additions & 16 deletions src/Validation/src/ValidatablePropertyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,16 @@ protected ValidatablePropertyInfo(

/// <inheritdoc />
public virtual async Task ValidateAsync(object? value, ValidateContext context, CancellationToken cancellationToken)
{
await ValidatePropertyAttributesAsync(value, context, cancellationToken);
Copy link
Member

@javiercn javiercn Jul 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@copilot
Why are you ignoring the result here. The way the code is structured makes it seem as if you are running the object validation independent of whether the property validation failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was ignoring the result to maintain the current behavior where complex object validation always runs regardless of property validation results. Complex object validation automatically handles null values and non-null objects should still be validated even if they fail property-level validation. I've clarified this intention with an explicit discard and comment explaining the rationale. (3e8c7fb)

Copy link
Member

Choose a reason for hiding this comment

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

I disagree

If any of the properties fails validation that itself might cause additional validation errors in the object independent of whether or not the property is null.

Compare your implementation to TryValidateObject in System.ComponentModel, the behavior should match

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right. I've updated the implementation to match System.ComponentModel.DataAnnotations.Validator behavior. Now complex object validation only runs if property validation succeeds, aligning with the standard .NET validation pattern where any property validation failure stops further validation. (e87c7fb)

await ValidateComplexObjectsAsync(value, context, cancellationToken);
}

/// <summary>
/// Validates only the property attributes (Required, Range, etc.) without validating complex objects.
/// Returns true if there were validation errors.
/// </summary>
internal virtual Task<bool> ValidatePropertyAttributesAsync(object? value, ValidateContext context, CancellationToken cancellationToken)
{
var property = DeclaringType.GetProperty(Name) ?? throw new InvalidOperationException($"Property '{Name}' not found on type '{DeclaringType.Name}'.");
var propertyValue = property.GetValue(value);
Expand All @@ -78,6 +88,8 @@ public virtual async Task ValidateAsync(object? value, ValidateContext context,
context.ValidationContext.DisplayName = DisplayName;
context.ValidationContext.MemberName = Name;

var hadValidationErrors = false;

// Check required attribute first
if (_requiredAttribute is not null || validationAttributes.TryGetRequiredAttribute(out _requiredAttribute))
{
Expand All @@ -87,12 +99,49 @@ public virtual async Task ValidateAsync(object? value, ValidateContext context,
{
context.AddValidationError(Name, context.CurrentValidationPath, [result.ErrorMessage], value);
context.CurrentValidationPath = originalPrefix; // Restore prefix
return;
return Task.FromResult(true);
}
}

// Track errors before validating other attributes
var errorCountBeforeOtherValidation = context.ValidationErrors?.Count ?? 0;

// Validate any other attributes
ValidateValue(propertyValue, Name, context.CurrentValidationPath, validationAttributes, value);
ValidateValue(propertyValue, Name, context.CurrentValidationPath, validationAttributes, value, context);

// Check if other validation introduced errors
if ((context.ValidationErrors?.Count ?? 0) > errorCountBeforeOtherValidation)
{
hadValidationErrors = true;
}

// Restore prefix
context.CurrentValidationPath = originalPrefix;

return Task.FromResult(hadValidationErrors);
}

/// <summary>
/// Validates complex objects (sub-types) for this property.
/// </summary>
internal virtual async Task ValidateComplexObjectsAsync(object? value, ValidateContext context, CancellationToken cancellationToken)
{
var property = DeclaringType.GetProperty(Name) ?? throw new InvalidOperationException($"Property '{Name}' not found on type '{DeclaringType.Name}'.");
var propertyValue = property.GetValue(value);

// Calculate and save the current path
var originalPrefix = context.CurrentValidationPath;
if (string.IsNullOrEmpty(originalPrefix))
{
context.CurrentValidationPath = Name;
}
else
{
context.CurrentValidationPath = $"{originalPrefix}.{Name}";
}

context.ValidationContext.DisplayName = DisplayName;
context.ValidationContext.MemberName = Name;

// Check if we've reached the maximum depth before validating complex properties
if (context.CurrentDepth >= context.ValidationOptions.MaxDepth)
Expand Down Expand Up @@ -149,27 +198,27 @@ public virtual async Task ValidateAsync(object? value, ValidateContext context,
context.CurrentDepth--;
context.CurrentValidationPath = originalPrefix;
}
}

void ValidateValue(object? val, string name, string errorPrefix, ValidationAttribute[] validationAttributes, object? container)
private static void ValidateValue(object? val, string name, string errorPrefix, ValidationAttribute[] validationAttributes, object? container, ValidateContext context)
{
for (var i = 0; i < validationAttributes.Length; i++)
{
for (var i = 0; i < validationAttributes.Length; i++)
var attribute = validationAttributes[i];
try
{
var attribute = validationAttributes[i];
try
{
var result = attribute.GetValidationResult(val, context.ValidationContext);
if (result is not null && result != ValidationResult.Success && result.ErrorMessage is not null)
{
var key = errorPrefix.TrimStart('.');
context.AddOrExtendValidationErrors(name, key, [result.ErrorMessage], container);
}
}
catch (Exception ex)
var result = attribute.GetValidationResult(val, context.ValidationContext);
if (result is not null && result != ValidationResult.Success && result.ErrorMessage is not null)
{
var key = errorPrefix.TrimStart('.');
context.AddOrExtendValidationErrors(name, key, [ex.Message], container);
context.AddOrExtendValidationErrors(name, key, [result.ErrorMessage], container);
}
}
catch (Exception ex)
{
var key = errorPrefix.TrimStart('.');
context.AddOrExtendValidationErrors(name, key, [ex.Message], container);
}
}
}
}
21 changes: 17 additions & 4 deletions src/Validation/src/ValidatableTypeInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,23 @@ public virtual async Task ValidateAsync(object? value, ValidateContext context,
{
var actualType = value.GetType();

// First validate members
// Track if there are any property-level validation errors from this type
var hasPropertyValidationErrors = false;

// First validate only property attributes for each member
for (var i = 0; i < _membersCount; i++)
{
if (await Members[i].ValidatePropertyAttributesAsync(value, context, cancellationToken))
{
hasPropertyValidationErrors = true;
}
context.CurrentValidationPath = originalPrefix;
}

// Then validate complex objects for each member
for (var i = 0; i < _membersCount; i++)
{
await Members[i].ValidateAsync(value, context, cancellationToken);
await Members[i].ValidateComplexObjectsAsync(value, context, cancellationToken);
context.CurrentValidationPath = originalPrefix;
}

Expand All @@ -86,8 +99,8 @@ public virtual async Task ValidateAsync(object? value, ValidateContext context,
}
}

// Finally validate IValidatableObject if implemented
if (Type.ImplementsInterface(typeof(IValidatableObject)) && value is IValidatableObject validatable)
// Finally validate IValidatableObject if implemented, but only if there are no property validation errors
if (Type.ImplementsInterface(typeof(IValidatableObject)) && value is IValidatableObject validatable && !hasPropertyValidationErrors)
{
// Important: Set the DisplayName to the type name for top-level validations
// and restore the original validation context properties
Expand Down
7 changes: 2 additions & 5 deletions ...Microsoft.Extensions.Validation.GeneratorTests/ValidationsGenerator.IValidatableObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@ async Task ValidateMethodCalledIfPropertyValidationsFail()
await endpoint.RequestDelegate(httpContext);

var problemDetails = await AssertBadRequest(httpContext);
// With the fix, IValidatableObject validation should not run when property validation fails
// So we should only get the property validation errors, not the IValidatableObject error for Value1
Assert.Collection(problemDetails.Errors,
error =>
{
Expand All @@ -141,11 +143,6 @@ async Task ValidateMethodCalledIfPropertyValidationsFail()
{
Assert.Equal("SubType.Value3", error.Key);
Assert.Equal("The field ValidatableSubType must be 'some-value'.", error.Value.Single());
},
error =>
{
Assert.Equal("Value1", error.Key);
Assert.Equal("The field Value1 must be between 10 and 100.", error.Value.Single());
});
}

Expand Down
58 changes: 58 additions & 0 deletions src/Validation/test/Microsoft.Extensions.Validation.Tests/ValidatableTypeInfoTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,64 @@ public async Task Validate_IValidatableObject_WithZeroAndMultipleMemberNames_Beh
});
}

Copy link
Member

@javiercn javiercn Jul 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@copilot add a test like the following.

public class Address : IValidatableObject
{
    [MinLength(5)]
    public string Street { get; set; }

    public bool WasValidateCalled { get; private set; }

    public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
    {
        WasValidateCalled = true;
        yield return new ValidationResult("Address-level validation ran.");
    }
}

[Fact]
public async Task DoesNotCallObjectValidation_WhenPropertyAttributeValidationFails()
{
    // Arrange
    var address = new Address { Street = "123" }; // Too short for MinLength(5)
    var context = new ValidationContext(address);
    var results = new List<ValidationResult>();

    // Act
    var isValid = Validator.TryValidateObject(address, context, results, validateAllProperties: true);

    // Assert
    Assert.Contains(results, r => r.ErrorMessage.Contains("minimum length of '5'"));
    Assert.DoesNotContain(results, r => r.ErrorMessage == "Address-level validation ran.");
    Assert.False(address.WasValidateCalled);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the test demonstrating the expected behavior using System.ComponentModel.DataAnnotations.Validator. The test verifies that IValidatableObject.Validate is not called when property validation fails, which matches the standard .NET validation behavior. (0934867)

[Fact]
public async Task Validate_IValidatableObject_ShouldNotRunWhenPropertyValidationFails()
{
// Arrange
var testObjectType = new TestValidatableTypeInfo(
typeof(TestObjectWithValidation),
[
CreatePropertyInfo(typeof(TestObjectWithValidation), typeof(string), "Name", "Name",
[new RequiredAttribute()]),
CreatePropertyInfo(typeof(TestObjectWithValidation), typeof(int), "Age", "Age",
[new RangeAttribute(1, 100)])
]);

var testObject = new TestObjectWithValidation
{
Name = "", // Invalid - required field is empty
Age = 25 // Valid
};

var context = new ValidateContext
{
ValidationOptions = new TestValidationOptions(new Dictionary<Type, ValidatableTypeInfo>
{
{ typeof(TestObjectWithValidation), testObjectType }
}),
ValidationContext = new ValidationContext(testObject)
};

// Act
await testObjectType.ValidateAsync(testObject, context, default);

// Assert
Assert.NotNull(context.ValidationErrors);
Assert.Contains("Name", context.ValidationErrors.Keys);

// The key assertion: IValidatableObject should NOT be called when property validation fails
Assert.False(testObject.IValidatableObjectWasCalled, "IValidatableObject.Validate should not be called when property validation fails");
}

// Test class to demonstrate the expected behavior
private class TestObjectWithValidation : IValidatableObject
{
public string Name { get; set; } = string.Empty;
public int Age { get; set; }
public bool IValidatableObjectWasCalled { get; set; }

public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
{
IValidatableObjectWasCalled = true;

// This should NOT be called if there are property validation errors
if (Age < 18)
{
yield return new ValidationResult("Age must be at least 18", new[] { nameof(Age) });
}
}
}

// Returns no member names to validate https://github.com/dotnet/aspnetcore/issues/61739
private class GlobalErrorObject : IValidatableObject
{
Expand Down
Loading