-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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.
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
Added IServiceProvider to DataAnnotationsValidator #39445
Added IServiceProvider to DataAnnotationsValidator #39445
Conversation
Although not a question on the original issue, I could trivially add the |
Thanks @MariovanZeist for the PR. As you pointed out in the original issue:
may lead to us not being able to take this into the framework. cc/ @pranavkm thoughts on service DI for validations? |
Actually this is fine. MVC configures this today and it's very likely this was a straight up miss. If you're doing async validation, and/or trying to block by doing sync-over-async in the validator, that's on the user. But no reason the DI container couldn't be present. |
src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/test/testassets/BasicTestApp/FormsTest/ValidationComponentDI.razor
Outdated
Show resolved
Hide resolved
src/Components/test/testassets/BasicTestApp/FormsTest/ValidationComponentDI.razor
Outdated
Show resolved
Hide resolved
src/Components/test/testassets/BasicTestApp/FormsTest/ValidationComponentDI.razor
Show resolved
Hide resolved
src/Components/test/testassets/BasicTestApp/FormsTest/ValidationComponentDI.razor
Outdated
Show resolved
Hide resolved
src/Components/test/testassets/BasicTestApp/FormsTest/ValidationComponentDI.razor
Outdated
Show resolved
Hide resolved
…onComponentDI.razor
Other than the thread about obsoleting the older API, this all looks great to me! |
src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs
Outdated
Show resolved
Hide resolved
src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs
Outdated
Show resolved
Hide resolved
@@ -13,6 +13,8 @@ public class DataAnnotationsValidator : ComponentBase, IDisposable | |||
|
|||
[CascadingParameter] EditContext? CurrentEditContext { get; set; } | |||
|
|||
[Inject] private IServiceProvider ServiceProvider { get; set; } = default!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in two minds about the nullability annotation here. I know the only way you can make it null
is to use an obsolete API, but existing code may do that, and then newer code will be getting told a lie by the annotation.
Should we keep it as IServiceProvider?
until the obsoleted API is actually removed? Is there prior art for how we handle this scenario elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see that @pranavkm just suggested marking it as non-nullable, so perhaps that is an established pattern about obsoletion already. Just waiting for a comment from him to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is a private property and our implementation always expects injected properties to be available. So annotating it as non-nullable seems correct. Does that sound right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're quite right. Sorry for the confusion!
…y adding a dummy IServiceProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, @MariovanZeist!
I'm going to leave this for @pranavkm to merge if he's also happy. My only remaining question for Pranav is about the nullability annotation but I'll leave it with him to decide whether to merge as-is or to tweak that.
…onfiguration in tests.
I found a bug in the implementation... Take for example my code that inject the database... when the code runs the first time it creates the object, why is executed again, not sure, but the second time, it returns null throwing an error when checking the db variable... code example: `public class ConstraintsV2_Require_Resolution : ValidationAttribute
|
Hi @sipi41. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
Added DI to ValidationContext
This PR adds the ability to use Dependency Injection in a
ValidationAttribute
when using the default<DataAnnotationsValidator>
Implemented by adding an overload to the extension methods to remain binary and source compatible.
This method can also be used by other custom validators
closes: #39382