Skip to content

Commit b03f15a

Browse files
captainsafiacaptainsafia
committed
Address feedback
1 parent 120c6ed commit b03f15a

22 files changed

+828
-1761
lines changed

‎src/Http/Http.Abstractions/src/Validation/IValidatableInfo.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ namespace Microsoft.AspNetCore.Http.Validation;
99
public interface IValidatableInfo
1010
{
1111
/// <summary>
12-
/// Validates the specified value.
12+
/// Validates the specified <paramref name="value"/>.
1313
/// </summary>
1414
/// <param name="value">The value to validate.</param>
1515
/// <param name="context">The validation context.</param>

‎src/Http/Http.Abstractions/src/Validation/RuntimeValidatableParameterInfoResolver.cs‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,15 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System.ComponentModel.DataAnnotations;
5-
using System.Diagnostics;
65
using System.Diagnostics.CodeAnalysis;
76
using System.Linq;
87
using System.Reflection;
98

109
namespace Microsoft.AspNetCore.Http.Validation;
1110

12-
internal class RuntimeValidatableParameterInfoResolver : IValidatableInfoResolver
11+
internal sealed class RuntimeValidatableParameterInfoResolver : IValidatableInfoResolver
1312
{
13+
// TODO: the implementation currently relies on static discovery of types.
1414
public bool TryGetValidatableTypeInfo(Type type, [NotNullWhen(true)] out IValidatableInfo? validatableInfo)
1515
{
1616
validatableInfo = null;
@@ -19,7 +19,11 @@ public bool TryGetValidatableTypeInfo(Type type, [NotNullWhen(true)] out IValida
1919

2020
public bool TryGetValidatableParameterInfo(ParameterInfo parameterInfo, [NotNullWhen(true)] out IValidatableInfo? validatableInfo)
2121
{
22-
Debug.Assert(parameterInfo.Name != null, "Parameter must have name");
22+
if (parameterInfo.Name == null)
23+
{
24+
throw new InvalidOperationException($"Encountered a parameter of type '{parameterInfo.ParameterType}' without a name. Parameters must have a name.");
25+
}
26+
2327
var validationAttributes = parameterInfo
2428
.GetCustomAttributes<ValidationAttribute>()
2529
.ToArray();
@@ -43,7 +47,7 @@ private static string GetDisplayName(ParameterInfo parameterInfo)
4347
return parameterInfo.Name!;
4448
}
4549

46-
private class RuntimeValidatableParameterInfo(
50+
private sealed class RuntimeValidatableParameterInfo(
4751
Type parameterType,
4852
string name,
4953
string displayName,

‎src/Http/Http.Abstractions/src/Validation/TypeExtensions.cs‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,23 +89,27 @@ public static bool TryGetRequiredAttribute(this ValidationAttribute[] attributes
8989
/// </summary>
9090
/// <param name="type">The type to analyze.</param>
9191
/// <returns>A collection containing all implemented interfaces and all base types of the given type.</returns>
92-
public static IEnumerable<Type> GetAllImplementedTypes([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)] this Type type)
92+
public static Type[] GetAllImplementedTypes([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)] this Type type)
9393
{
9494
ArgumentNullException.ThrowIfNull(type);
9595

96+
var implementedTypes = new List<Type>();
97+
9698
// Yield all interfaces directly and indirectly implemented by this type
9799
foreach (var interfaceType in type.GetInterfaces())
98100
{
99-
yield return interfaceType;
101+
implementedTypes.Add(interfaceType);
100102
}
101103

102104
// Finally, walk up the inheritance chain
103105
var baseType = type.BaseType;
104106
while (baseType != null && baseType != typeof(object))
105107
{
106-
yield return baseType;
108+
implementedTypes.Add(baseType);
107109
baseType = baseType.BaseType;
108110
}
111+
112+
return [.. implementedTypes];
109113
}
110114

111115
/// <summary>

‎src/Http/Http.Abstractions/src/Validation/ValidatableParameterInfo.cs‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ public virtual async Task ValidateAsync(object? value, ValidateContext context,
7171

7272
var validationAttributes = GetValidationAttributes();
7373

74-
if (_requiredAttribute is not null && validationAttributes.TryGetRequiredAttribute(out _requiredAttribute))
74+
if (_requiredAttribute is not null || validationAttributes.TryGetRequiredAttribute(out _requiredAttribute))
7575
{
7676
var result = _requiredAttribute.GetValidationResult(value, context.ValidationContext);
7777

@@ -107,13 +107,15 @@ public virtual async Task ValidateAsync(object? value, ValidateContext context,
107107
if (ParameterType.IsEnumerable() && value is IEnumerable enumerable)
108108
{
109109
var index = 0;
110+
var currentPrefix = context.CurrentValidationPath;
111+
110112
foreach (var item in enumerable)
111113
{
112114
if (item != null)
113115
{
114-
var itemPrefix = string.IsNullOrEmpty(context.CurrentValidationPath)
116+
context.CurrentValidationPath = string.IsNullOrEmpty(currentPrefix)
115117
? $"{Name}[{index}]"
116-
: $"{context.CurrentValidationPath}.{Name}[{index}]";
118+
: $"{currentPrefix}.{Name}[{index}]";
117119

118120
if (context.ValidationOptions.TryGetValidatableTypeInfo(item.GetType(), out var validatableType))
119121
{
@@ -122,6 +124,8 @@ public virtual async Task ValidateAsync(object? value, ValidateContext context,
122124
}
123125
index++;
124126
}
127+
128+
context.CurrentValidationPath = currentPrefix;
125129
}
126130
// If not enumerable, validate the single value
127131
else if (value != null)

‎src/Http/Http.Abstractions/src/Validation/ValidatablePropertyInfo.cs‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,11 @@ protected ValidatablePropertyInfo(
5858
protected abstract ValidationAttribute[] GetValidationAttributes();
5959

6060
/// <inheritdoc />
61-
/// <returns>A task representing the asynchronous operation.</returns>
6261
public virtual async Task ValidateAsync(object? value, ValidateContext context, CancellationToken cancellationToken)
6362
{
6463
Debug.Assert(context.ValidationContext is not null);
6564

66-
var property = DeclaringType.GetProperty(Name)!;
65+
var property = DeclaringType.GetProperty(Name) ?? throw new InvalidOperationException($"Property '{Name}' not found on type '{DeclaringType.Name}'.");
6766
var propertyValue = property.GetValue(value);
6867
var validationAttributes = GetValidationAttributes();
6968

@@ -101,7 +100,7 @@ public virtual async Task ValidateAsync(object? value, ValidateContext context,
101100
if (context.CurrentDepth >= context.ValidationOptions.MaxDepth)
102101
{
103102
throw new InvalidOperationException(
104-
$"Maximum validation depth of {context.ValidationOptions.MaxDepth} exceeded at '{context.CurrentValidationPath}' in '{DeclaringType.Name}.{PropertyType.Name} {Name}'. " +
103+
$"Maximum validation depth of {context.ValidationOptions.MaxDepth} exceeded at '{context.CurrentValidationPath}' in '{DeclaringType.Name}.{Name}'. " +
105104
"This is likely caused by a circular reference in the object graph. " +
106105
"Consider increasing the MaxDepth in ValidationOptions if deeper validation is required.");
107106
}

‎src/Http/Http.Abstractions/src/Validation/ValidatableTypeInfo.cs‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ namespace Microsoft.AspNetCore.Http.Validation;
1414
public abstract class ValidatableTypeInfo : IValidatableInfo
1515
{
1616
private readonly int _membersCount;
17-
private readonly IEnumerable<Type> _subTypes;
17+
private readonly Type[] _subTypes;
1818

1919
/// <summary>
2020
/// Creates a new instance of <see cref="ValidatableTypeInfo"/>.
@@ -59,10 +59,13 @@ public virtual async Task ValidateAsync(object? value, ValidateContext context,
5959
"Consider increasing the MaxDepth in ValidationOptions if deeper validation is required.");
6060
}
6161

62+
// Increment depth counter since we're coming from
63+
// a parameter or property reference
64+
var originalPrefix = context.CurrentValidationPath;
65+
6266
try
6367
{
6468
var actualType = value.GetType();
65-
var originalPrefix = context.CurrentValidationPath;
6669

6770
// First validate members
6871
for (var i = 0; i < _membersCount; i++)
@@ -116,14 +119,10 @@ public virtual async Task ValidateAsync(object? value, ValidateContext context,
116119
context.ValidationContext.DisplayName = originalDisplayName;
117120
context.ValidationContext.MemberName = originalMemberName;
118121
}
119-
120-
// Always restore original prefix
121-
context.CurrentValidationPath = originalPrefix;
122122
}
123123
finally
124124
{
125-
// Decrement depth when validation completes
126-
context.CurrentDepth--;
125+
context.CurrentValidationPath = originalPrefix;
127126
}
128127
}
129128
}

‎src/Http/Http.Abstractions/src/Validation/ValidateContext.cs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public sealed class ValidateContext
3131
/// <summary>
3232
/// Gets or sets the dictionary of validation errors collected during validation.
3333
/// Keys are property names or paths, and values are arrays of error messages.
34-
/// This dictionary is lazily initialized when the first validation error is added.
34+
/// In the default implementation, this dictionary is initialized when the first error is added.
3535
/// </summary>
3636
public Dictionary<string, string[]>? ValidationErrors { get; set; }
3737

@@ -54,9 +54,10 @@ internal void AddOrExtendValidationErrors(string key, string[] errors)
5454

5555
if (ValidationErrors.TryGetValue(key, out var existingErrors))
5656
{
57-
ValidationErrors[key] = new string[existingErrors.Length + errors.Length];
58-
existingErrors.CopyTo(ValidationErrors[key], 0);
59-
errors.CopyTo(ValidationErrors[key], existingErrors.Length);
57+
var newErrors = new string[existingErrors.Length + errors.Length];
58+
existingErrors.CopyTo(newErrors, 0);
59+
errors.CopyTo(newErrors, existingErrors.Length);
60+
ValidationErrors[key] = newErrors;
6061
}
6162
else
6263
{

‎src/Http/Http.Abstractions/test/Validation/ValidatableParameterInfoTests.cs‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,32 @@ public async Task Validate_RequiredParameter_AddsErrorWhenNull()
3030
Assert.NotNull(errors);
3131
var error = Assert.Single(errors);
3232
Assert.Equal("testParam", error.Key);
33-
Assert.Equal("The Test Parameter field is required.", error.Value.First());
33+
Assert.Equal("The Test Parameter field is required.", error.Value.Single());
34+
}
35+
36+
[Fact]
37+
public async Task Validate_RequiredParameter_ShortCircuitsOtherValidations()
38+
{
39+
// Arrange
40+
var paramInfo = CreateTestParameterInfo(
41+
parameterType: typeof(string),
42+
name: "testParam",
43+
displayName: "Test Parameter",
44+
// Most ValidationAttributes skip validation if the value is null
45+
// so we use a custom one that always fails to assert on the behavior here
46+
validationAttributes: [new RequiredAttribute(), new CustomTestValidationAttribute()]);
47+
48+
var context = CreateValidatableContext();
49+
50+
// Act
51+
await paramInfo.ValidateAsync(null, context, default);
52+
53+
// Assert
54+
var errors = context.ValidationErrors;
55+
Assert.NotNull(errors);
56+
var error = Assert.Single(errors);
57+
Assert.Equal("testParam", error.Key);
58+
Assert.Equal("The Test Parameter field is required.", error.Value.Single());
3459
}
3560

3661
[Fact]
@@ -180,7 +205,7 @@ [new RequiredAttribute()])
180205
var errors = context.ValidationErrors;
181206
Assert.NotNull(errors);
182207
var error = Assert.Single(errors);
183-
Assert.Equal("Name", error.Key);
208+
Assert.Equal("people[1].Name", error.Key);
184209
Assert.Equal("The Name field is required.", error.Value[0]);
185210
}
186211

‎src/Http/Http.Abstractions/test/Validation/ValidatableTypeInfoTests.cs‎

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,8 @@ [new RequiredAttribute()]),
333333
async () => await nodeType.ValidateAsync(rootNode, context, default));
334334

335335
Assert.NotNull(exception);
336-
Assert.Contains("Maximum validation depth of 3 exceeded at 'Children[0].Parent.Children[0]' in 'TreeNode'. This is likely caused by a circular reference in the object graph. Consider increasing the MaxDepth in ValidationOptions if deeper validation is required.", exception.Message);
336+
Assert.Equal("Maximum validation depth of 3 exceeded at 'Children[0].Parent.Children[0]' in 'TreeNode'. This is likely caused by a circular reference in the object graph. Consider increasing the MaxDepth in ValidationOptions if deeper validation is required.", exception.Message);
337+
Assert.Equal(0, context.CurrentDepth);
337338
}
338339

339340
[Fact]
@@ -461,6 +462,39 @@ public async Task Validate_HandlesMultiLevelInheritance()
461462
});
462463
}
463464

465+
[Fact]
466+
public async Task Validate_RequiredOnPropertyShortCircuitsOtherValidations()
467+
{
468+
// Arrange
469+
var userType = new TestValidatableTypeInfo(
470+
typeof(User),
471+
[
472+
CreatePropertyInfo(typeof(User), typeof(string), "Password", "Password",
473+
[new RequiredAttribute(), new PasswordComplexityAttribute()])
474+
]);
475+
476+
var context = new ValidateContext
477+
{
478+
ValidationOptions = new TestValidationOptions(new Dictionary<Type, ValidatableTypeInfo>
479+
{
480+
{ typeof(User), userType }
481+
})
482+
};
483+
484+
var user = new User { Password = null }; // Invalid: required
485+
context.ValidationContext = new ValidationContext(user);
486+
487+
// Act
488+
await userType.ValidateAsync(user, context, default);
489+
490+
// Assert
491+
Assert.NotNull(context.ValidationErrors);
492+
Assert.Single(context.ValidationErrors.Keys);
493+
var error = Assert.Single(context.ValidationErrors);
494+
Assert.Equal("Password", error.Key);
495+
Assert.Equal("The Password field is required.", error.Value.Single());
496+
}
497+
464498
private ValidatablePropertyInfo CreatePropertyInfo(
465499
Type containingType,
466500
Type propertyType,
@@ -542,7 +576,7 @@ private class Product
542576

543577
private class User
544578
{
545-
public string Password { get; set; } = string.Empty;
579+
public string? Password { get; set; } = string.Empty;
546580
}
547581

548582
private class BaseEntity

0 commit comments

Comments
 (0)