Tolerate null ValidationContext in validation resolver APIs #61854
Conversation
github-actions
bot
added
the
area-networking
There was a problem hiding this comment.
Pull Request Overview
This pull request improves validation logic by ensuring that a ValidationContext is initialized when it is not pre-populated, thereby tolerating a null context during validation.
- Initializes ValidationContext in ValidateAsync methods for parameters, properties, and types
- Removes Debug.Assert statements that assumed a non-null context
- Adds comprehensive unit tests to verify the new behavior
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Http/Http.Abstractions/test/Validation/ValidatableTypeInfoTests.cs | Adds new tests to verify error handling and successful validation with uninitialized ValidationContext |
| src/Http/Http.Abstractions/test/Validation/ValidatableParameterInfoTests.cs | Introduces tests for required parameters where ValidationContext is initialized during validation |
| src/Http/Http.Abstractions/src/Validation/ValidatableTypeInfo.cs | Removes Debug.Assert and initializes ValidationContext with a fallback display name |
| src/Http/Http.Abstractions/src/Validation/ValidatablePropertyInfo.cs | Removes redundant Debug.Assert and adds contextual initialization of ValidationContext |
| src/Http/Http.Abstractions/src/Validation/ValidatableParameterInfo.cs | Removes Debug.Assert and ensures a non-null value for ValidationContext initialization |
| await paramInfo.ValidateAsync(null, context, default); | ||
|
|
||
| // Assert – a ValidationContext should have been created and the error recorded | ||
| Assert.NotNull(context.ValidationContext); |
There was a problem hiding this comment.
Any concerns with the internally created ValidationContext "leaking" outside of the validate call?
| if (value == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| // Although classes can be annotated with [DisplayName], we only process display names when producing |
There was a problem hiding this comment.
Seems fine. Future looking, any reason we might prefer [DisplayName] here?
| // ValidationContext requires a non-null value although the invocation pattern that we use | ||
| // calls `GetValidationResult` and passes the value there. `GetValidationResult` tolerates | ||
| // null values so we only need to set a non-null value to the ValidationContext here. | ||
| context.ValidationContext ??= new ValidationContext(value ?? new object(), displayName: DisplayName, serviceProvider: null, items: null); |
There was a problem hiding this comment.
Can we avoid allocating a new value each time in the case we don't need one?
There was a problem hiding this comment.
Oh yeah, like only newing up in the getter?
| // ValidationContext requires a non-null value although the invocation pattern that we use | ||
| // calls `GetValidationResult` and passes the value there. `GetValidationResult` tolerates | ||
| // null values so we only need to set a non-null value to the ValidationContext here. | ||
| context.ValidationContext ??= new ValidationContext(value ?? new object(), displayName: DisplayName, serviceProvider: null, items: null); |
There was a problem hiding this comment.
Curious about the serviceProvider: null here. Is there any case where validation logic using this context instance would require the context to be able to resolve services?
This pull request improves the validation logic in the
Microsoft.AspNetCore.Http.Validationnamespace by ensuring that aValidationContextis initialized when it is not pre-populated. It also adds corresponding unit tests to verify this behavior.Fixes #61738
Validation Logic Enhancements:
Updated
ValidateAsyncmethods inValidatableParameterInfo,ValidatablePropertyInfo, andValidatableTypeInfoto initialize aValidationContextif it is not already set. This ensures validation can proceed even when aValidationContextis not pre-populated. (src/Http/Http.Abstractions/src/Validation/ValidatableParameterInfo.cs- [1]src/Http/Http.Abstractions/src/Validation/ValidatablePropertyInfo.cs- [2]src/Http/Http.Abstractions/src/Validation/ValidatableTypeInfo.cs- [3]Removed unnecessary
Debug.Assertstatements that previously assumed a non-nullValidationContext. (src/Http/Http.Abstractions/src/Validation/ValidatableParameterInfo.cs- [1]src/Http/Http.Abstractions/src/Validation/ValidatablePropertyInfo.cs- [2]src/Http/Http.Abstractions/src/Validation/ValidatableTypeInfo.cs- [3]Test Coverage Improvements:
Added unit tests in
ValidatableParameterInfoTeststo verify that aValidationContextis correctly initialized and validation errors are handled when the context is not pre-populated. (src/Http/Http.Abstractions/test/Validation/ValidatableParameterInfoTests.cs- src/Http/Http.Abstractions/test/Validation/ValidatableParameterInfoTests.csR288-R341)Added unit tests in
ValidatableTypeInfoTeststo confirm thatValidationContextinitialization and error handling work as expected for valid and invalid objects. (src/Http/Http.Abstractions/test/Validation/ValidatableTypeInfoTests.cs- src/Http/Http.Abstractions/test/Validation/ValidatableTypeInfoTests.csR555-R617)Code Cleanup:
using System.Diagnostics;directives from multiple files to streamline the codebase. (src/Http/Http.Abstractions/src/Validation/ValidatableParameterInfo.cs- [1]src/Http/Http.Abstractions/src/Validation/ValidatablePropertyInfo.cs- [2]src/Http/Http.Abstractions/src/Validation/ValidatableTypeInfo.cs- [3]