Remove DataAnnotations' ValidationContext from MEV public API#67549
Remove DataAnnotations' ValidationContext from MEV public API#67549Youssef1313 wants to merge 5 commits into
Conversation
691e402 to
f1b2170
Compare
c4802fe to
20c9328
Compare
5e84a5c to
48a92bf
Compare
48a92bf to
63912cd
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes System.ComponentModel.DataAnnotations.ValidationContext from the Microsoft.Extensions.Validation public surface by replacing it with a ServiceProvider on ValidateContext, and refactors the validation pipeline to construct ValidationContext internally where needed. It also updates Minimal APIs, Blazor Forms integration, tests, and benchmarks to use the new API shape.
Changes:
- Replace
ValidateContext.ValidationContextwithValidateContext.ServiceProviderand makeValidationOptionsinit-only. - Refactor attribute validation to pass
ValidationContext/display name explicitly through the pipeline (properties, parameters, and type-level validation). - Update integrations (Minimal API endpoint filter, Blazor
EditContextDataAnnotations) plus tests/benchmarks for the new API.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Validation/src/ValidateContext.cs | Replaces public ValidationContext with ServiceProvider; refactors attribute validation plumbing. |
| src/Validation/src/ValidateContextMutableState.cs | Removes mutable state that previously mirrored ValidationContext display/member metadata. |
| src/Validation/src/IValidationErrorReporter.cs | Updates error reporting contract to carry display name and member name explicitly. |
| src/Validation/src/ValidatableTypeInfo.cs | Refactors type-level validation to work without storing ValidationContext on ValidateContext. |
| src/Validation/src/ValidatablePropertyInfo.cs | Creates per-property ValidationContext from the containing object and passes display name explicitly. |
| src/Validation/src/ValidatableParameterInfo.cs | Creates per-parameter ValidationContext (now derived from value + ServiceProvider) and passes display name explicitly. |
| src/Validation/src/PublicAPI.Unshipped.txt | Records public API removals/additions (ValidationContext removal, ServiceProvider + init-only ValidationOptions). |
| src/Http/Routing/src/ValidationEndpointFilterFactory.cs | Updates Minimal API validation filter to use ServiceProvider instead of creating/storing ValidationContext. |
| src/Http/Http/perf/Microbenchmarks/ValidatableTypesBenchmark.cs | Updates benchmark setup to populate ServiceProvider. |
| src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs | Updates Blazor Forms integration to populate ServiceProvider instead of ValidationContext. |
| src/Validation/test/Microsoft.Extensions.Validation.Tests/ValidationLocalizationIntegrationTests.cs | Updates tests to use ServiceProvider API. |
| src/Validation/test/Microsoft.Extensions.Validation.Tests/ValidatableTypeInfoTests.cs | Updates tests to use ServiceProvider API. |
| src/Validation/test/Microsoft.Extensions.Validation.Tests/ValidatableParameterInfoTests.cs | Updates tests to use ServiceProvider API. |
| src/Validation/test/Microsoft.Extensions.Validation.Tests/RuntimeValidatableParameterInfoResolverTests.cs | Updates tests to use ServiceProvider API. |
| src/Validation/test/Microsoft.Extensions.Validation.Tests/DisplayNameInfoTests.cs | Updates tests to use ServiceProvider API. |
| src/Validation/test/Microsoft.Extensions.Validation.Tests/AsyncValidationTests.cs | Updates async validation tests to use ServiceProvider API. |
| src/Validation/Localization/test/ValidationLocalizationPipelineTests.cs | Updates localization pipeline tests to use ServiceProvider API. |
| src/Validation/test/Microsoft.Extensions.Validation.GeneratorTests/ValidationsGenerator.ValidatableType.cs | Updates generator tests to use ServiceProvider API. |
| src/Validation/test/Microsoft.Extensions.Validation.GeneratorTests/ValidationsGenerator.SkipValidation.cs | Updates generator tests to use ServiceProvider API. |
| src/Validation/test/Microsoft.Extensions.Validation.GeneratorTests/ValidationsGenerator.ClassAttributes.cs | Updates generator tests to use ServiceProvider API. |
| return attribute.GetValidationResult(value, validationContext); | ||
| } | ||
|
|
||
| if (attribute.IsValid(value)) |
There was a problem hiding this comment.
I think this call ValidationAttribute.IsValid(object? value, ValidationContext validationContext) with a null ValidationContext despite the fact that the parameter is marked non-nullable. This means the attribute has no way to access the IServiceProvider from the ValidationContext iff the value is null.
I know it's gross to pass in new object() to represent null, but I think for the attributes in particular, that's probably what we should do. We could request an API change to ValidationContext in the runtime to support null ObjectInstances, but I doubt there will be much of any appetite assuming all the code that already reads it assuming it cannot be null.
There was a problem hiding this comment.
Yup, it's passing null.
This code will be reachable only when we have a "null" value to validate, for which we previously skipped the validation completely.
I didn't like much using new object() to represent null. It feels like a workaround to me and we are constructing a malformed ValidationContext, but happy to re-consider if you think that is a better approach. In this case, it will be defeating one of the reasons for considering the API change, but I think the API shape to remove ValidationContext from ValidateContext is still a good thing to do regardless :)
There was a problem hiding this comment.
Note: I expect most realistic use cases to be able to decide the validation of a "null" without needing anything from ValidationContext.
I'm trying to respect the design of ValidationContext as much as possible.
Fixes #67033
Fixes #67004
Fixes #67550