Fix IServiceProvider resolution for DataAnnotations validation on Options#125651
Fix IServiceProvider resolution for DataAnnotations validation on Options#125651
Conversation
|
@copilot, you can proceed now. |
…ons validation Pass the root IServiceProvider to ValidationContext when validating DataAnnotations attributes on Options. This enables custom validation attributes to resolve services via validationContext.GetService() or validationContext.GetRequiredService(). Changes: - Add new public constructor DataAnnotationValidateOptions(name, serviceProvider) - Change ValidateDataAnnotations() to register using a factory that captures and passes IServiceProvider to the new constructor - Pass IServiceProvider through to all ValidationContext instances, including nested ValidateObjectMembers and ValidateEnumeratedItems calls - Update ref assembly with the new constructor - Add tests verifying service resolution works and fails correctly Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/b340196e-dcb9-42de-ad3c-eb10b9d9199c Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
Done! Commit e11f8bc implements the fix:
Note: the new constructor is a new public API surface — dotnet/runtime requires an |
There was a problem hiding this comment.
Pull request overview
This PR updates Microsoft.Extensions.Options.DataAnnotations so DataAnnotations validation for options constructs ValidationContext with an IServiceProvider, enabling custom ValidationAttributes to resolve services during validation.
Changes:
- Add a new public
DataAnnotationValidateOptions(string? name, IServiceProvider? serviceProvider)constructor and flow the service provider into allValidationContextcreation paths (including recursive validation). - Change
ValidateDataAnnotations()registration to use a DI factory so the container service provider is captured and passed intoDataAnnotationValidateOptions. - Add regression tests verifying service resolution succeeds when registered and fails when missing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsBuilderTest.cs | Adds regression tests and a custom ValidationAttribute to verify service resolution via ValidationContext. |
| src/libraries/Microsoft.Extensions.Options.DataAnnotations/src/OptionsBuilderDataAnnotationsExtensions.cs | Updates ValidateDataAnnotations() to register the validator via factory to capture DI IServiceProvider. |
| src/libraries/Microsoft.Extensions.Options.DataAnnotations/src/DataAnnotationValidateOptions.cs | Introduces a new ctor storing IServiceProvider and passes it into ValidationContext for all validation calls. |
| src/libraries/Microsoft.Extensions.Options.DataAnnotations/ref/Microsoft.Extensions.Options.DataAnnotations.cs | Updates the reference assembly to include the new public constructor. |
| var sp = services.BuildServiceProvider(); | ||
|
|
||
| var error = Assert.Throws<OptionsValidationException>(() => sp.GetRequiredService<IOptions<AnnotatedOptionsWithServiceDependency>>().Value); | ||
| Assert.Contains("SomeService not resolved correctly", error.Message); |
There was a problem hiding this comment.
This test asserts on OptionsValidationException.Message, which is derived formatting over Failures and can be brittle if message formatting changes. Prefer asserting on error.Failures (or using the existing ValidateFailure<TOptions> helper in this file) to validate the exact failure(s) without depending on Message formatting.
| Assert.Contains("SomeService not resolved correctly", error.Message); | |
| Assert.Contains("SomeService not resolved correctly", error.Failures); |
| optionsBuilder.Services.AddSingleton<IValidateOptions<TOptions>>( | ||
| sp => new DataAnnotationValidateOptions<TOptions>(optionsBuilder.Name, sp)); |
There was a problem hiding this comment.
The singleton factory lambda closes over the entire optionsBuilder instance. Capture optionsBuilder.Name into a local variable first and close over the string instead, to avoid retaining the builder object longer than necessary and to make the registration behavior clearer.
| optionsBuilder.Services.AddSingleton<IValidateOptions<TOptions>>( | |
| sp => new DataAnnotationValidateOptions<TOptions>(optionsBuilder.Name, sp)); | |
| string? name = optionsBuilder.Name; | |
| optionsBuilder.Services.AddSingleton<IValidateOptions<TOptions>>( | |
| sp => new DataAnnotationValidateOptions<TOptions>(name, sp)); |
| /// <summary> | ||
| /// Initializes a new instance of <see cref="DataAnnotationValidateOptions{TOptions}"/> . | ||
| /// </summary> |
There was a problem hiding this comment.
XML doc comment has an extra space before the period after the <see cref="DataAnnotationValidateOptions{TOptions}"/> reference; this shows up in generated docs and should be removed.
| [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property, AllowMultiple = false, Inherited = true)] | ||
| public class RequiresServiceAttribute : ValidationAttribute | ||
| { | ||
| protected override ValidationResult IsValid(object value, ValidationContext validationContext) |
There was a problem hiding this comment.
ValidationAttribute.IsValid is nullability-annotated in the BCL; to avoid signature mismatches/warnings and to correctly express the contract, this override should use object? for value and return ValidationResult?.
| protected override ValidationResult IsValid(object value, ValidationContext validationContext) | |
| protected override ValidationResult? IsValid(object? value, ValidationContext validationContext) |
|
I see why the IServiceProvider isn't supported in the |
🤖 Copilot Code Review — PR #125651Note This review was generated by Copilot and reflects automated analysis. Models contributing: Claude Opus 4.6 (primary), Claude Haiku 4.5 (extensions-reviewer sub-agent). GPT-5.3-Codex timed out and did not contribute. Holistic AssessmentMotivation: The problem is real and well-documented — Approach: The approach is correct — store Summary: Detailed Findings❌ Missing API Approval — New public constructor requires
|
Description
When using
ValidateDataAnnotations()on options, theValidationContextwas created without anIServiceProvider, preventing customValidationAttributes from resolving services (e.g.IWebHostEnvironment) viavalidationContext.GetRequiredService<T>().Changes Made
DataAnnotationValidateOptions.cs: Added a new public constructorDataAnnotationValidateOptions(string? name, IServiceProvider? serviceProvider). The existing single-arg constructor now delegates to it withnull, preserving full backward compatibility. The service provider is stored in a private field and passed into everynew ValidationContext(...)call — including recursive calls for[ValidateObjectMembers]and[ValidateEnumeratedItems]properties.OptionsBuilderDataAnnotationsExtensions.cs: ChangedValidateDataAnnotations()to registerIValidateOptions<TOptions>using a factory (sp => new DataAnnotationValidateOptions<TOptions>(name, sp)) so the rootIServiceProviderfrom the DI container is captured and wired up automatically.ref/Microsoft.Extensions.Options.DataAnnotations.cs: Updated the ref assembly to include the new constructor signature.OptionsBuilderTest.cs: Added two regression tests — one confirming a service can be resolved inside a customValidationAttribute'sIsValidmethod, and one confirming validation correctly fails when the expected service is not registered.Testing
Microsoft.Extensions.Options.TestspassValidateDataAnnotations_ServiceProviderIsAvailableInValidationContext— verifies service resolution works inside a custom validation attributeValidateDataAnnotations_ServiceProviderFailsWhenServiceNotAvailableInValidationContext— verifies validation fails when the required service is not registeredOriginal prompt
📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.