Skip to content
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

Please reconsider allowing async model validation #31905

Open
JeremySkinner opened this issue Apr 17, 2021 · 30 comments
Open

Please reconsider allowing async model validation #31905

JeremySkinner opened this issue Apr 17, 2021 · 30 comments
Labels
area-ui-rendering Includes: MVC Views/Pages, Razor Views/Pages enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-model-binding Pillar: Dev Experience

Comments

@JeremySkinner
Copy link
Contributor

JeremySkinner commented Apr 17, 2021

Suggestion

Hi, it would be great if the validation APIs could allow async model validation. This was previously suggested and closed in #7573, but it'd be greatly appreciated if you could reconsider.

Background information & context

I'm the author of the FluentValidaiton library, and we provide integration with ASP.NET's model validation API by implementing a custom ObjectModelValidator and ModelValidatorProvider. This works well for the most part, and is used extensively by the .NET community, however one sticking point is handling async validation.

FluentValidation's validators allow either sync or async execution, but because ASP.NET's validation APIs are only synchronous, we are unable to offer the library's full feature-set to ASP.NET users, and warn them that if their validator contains async code then it'll end up be run synchronously. If they need this to be async, they have to stop using auto validation and instead manually invoke the validator inside an async controller action. Neither of these are ideal.

Additionally, from an API design perspective we have to maintain 2 code paths inside FluentValidation for both the sync/async APIs which leads to a lot of duplication. This is mainly done for the sake of ASP.NET - if ASP.NET's validation pipeline was async, we could deprecate (and eventually drop) our synchronous implementation, but for as long as ASP.NET mandates synchronous validators we have to continue maintaining both implementations.

I believe the following types/methods would need to become async in order to support this:

  • ObjectModelValidator.Validate overloads
  • ValidationVisitor.Validate and the various methods called by this that recurse through the object graph
  • IModelValidator.Validate and its implementors

(#7573 specifically asked for the data annotation attributes themselves to add async validation methods too, but personally I think this is less important. If the infrastructure were in place with the above types, then the community can provide async implementations of IModelValidator as necessary, whether that's using FluentValidation, or custom attributes or something else)

If you could reconsider adding support for this, it would be a very welcome addition for the users of our library. I'd be happy to help implement this, if that'd be valuable.

Please let me know if you need any more info.

Thanks for your consideration.

//cc @pranavkm

@pranavkm pranavkm added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Apr 17, 2021
@javiercn
Copy link
Member

@JeremySkinner thanks for contacting us.

We'll reconsider this and update the issue accordingly, however I can imagine already that one of the reasons we avoided this in the past is due to the perf implications involved in supporting async here.

@javiercn javiercn added this to the Next sprint planning milestone Apr 18, 2021
@ghost
Copy link

ghost commented Apr 18, 2021

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@stevendarby
Copy link

Even if you decide again that you don’t plan to do this in the near future (the closing comment on the previous issue), would it be worth leaving the issue open so it can collect feedback from the community which might help you re-evaluate in future?

I remember coming across the closed issue soon after it was closed and have never been sure how long was appropriate to leave it before requesting it again.

@ghost
Copy link

ghost commented Aug 12, 2021

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@mkArtakMSFT mkArtakMSFT added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Oct 14, 2021
@sumtec
Copy link

sumtec commented Mar 3, 2022

Maybe the async validation will have a performance impact but it should be a choice for the end-user instead of forcing the user to write their own code to solve the problem with the async libraries.

Here is one of the scenarios:
The validation actually needs to check the value from the other underlying services which only provides an async method. This means we still have to either write the code to block the thread which is not quite sure if it will cause a deadlock in some cases. Or, we might need to write some codes to run that in another thread or similar which in the end will hurt the performance. This is not the worst. The worst is that it wastes a lot of time to check what is the right design when one side said that I have only async pattern and the other side said "No, I don't like it" while there are so many articles out there said that it will have a deadlock if you turn it into synchronous pattern incorrectly.

@jesslilly
Copy link

jesslilly commented May 17, 2022

We use async code almost entirely. I have async code that I would like to use in one place and use again in a validator. But these limitations prevent me from doing this (cleanly).

This is not a bug in the framework, but it IS a big limitation.

Also @javiercn can you elaborate on the performance implications of async model validation? Couldn't this feature actually improve performance?

@JeremySkinner
Copy link
Contributor Author

Hi @javiercn @pranavkm @mkArtakMSFT it's coming up to a year since this was moved to the backlog, and I was hoping that this could be re-evaluated and re-prioritised? This is still a major issue and blocker for users of the FluentValidation library - it causes no end of problems for our end users and also prevents us from being able to move the library forward.

I'd really urge you to reconsider bringing this forward into a new release, especially now that we have ValueTask within the framework, it's straightforward to support both sync and async apis while still optimising for the common synchronous use case. Adding support for this would add a real tangible benefit for users - it is one of our most common feature requests, but my hands are tied.

As mentioned before, I would be very happy to help implement this.

@javiercn javiercn modified the milestones: Backlog, .NET 8 Planning Jun 15, 2022
@javiercn
Copy link
Member

@JeremySkinner Thanks for bringing this up to our attention.

I have moved this up to our .NET 8 planning milestone to discuss it within the team.

@JeremySkinner
Copy link
Contributor Author

thanks!

@stevendarby
Copy link

Please also consider async validation specifically in validation attributes along side this.

@celluj34
Copy link

celluj34 commented Oct 6, 2022

I would definitely like to see this added. Myriad people have run in to this problem and the only workable solution is to split validations between the validation pipeline and controller logic (or stuff everything into controller logic and repeat .Validate calls and model generation everywhere).

@IvanJosipovic
Copy link

IvanJosipovic commented Oct 6, 2022

I also agree on this issue, ASP.NET and Blazor can't natively do async validation. We shouldn't have to DIY something this crucial.

@danielgreen
Copy link

Would very much like to see this feature

@campbellja
Copy link

I too would love to see this added ☺️

@kidchenko
Copy link

Running at exactly same problem here, would be lovely to have on ASP NET Core

@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 20, 2023
@jari-kuipers
Copy link

Is this now available in .NET 8 or am I reading the milestone wrong?

@jesslilly
Copy link

I have given up on waiting for this feature.

We are implementing manual validation in our controllers instead. This is what is recommended in the FV docs and is less magic code anyway.

@IvanJosipovic
Copy link

IvanJosipovic commented Nov 16, 2023

I was able to get Fluent Validation async validation to work with custom code in MudBlazor.

See example, https://www.mudblazor.com/components/form#using-fluent-validation

PR, MudBlazor/MudBlazor#2418

It also appears supported in Blazored/FluentValidation#137

@miguel93041
Copy link

Any update on this issue? It's indeed a MUST otherwise we need to depend on third parties and break the workflow of the ASP.NET model validation pipeline for example.

@mkArtakMSFT mkArtakMSFT added enhancement This issue represents an ask for new feature or an enhancement to an existing one Pillar: Dev Experience labels Dec 28, 2023
@ghost
Copy link

ghost commented Dec 28, 2023

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@mkArtakMSFT mkArtakMSFT added area-ui-rendering Includes: MVC Views/Pages, Razor Views/Pages and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Dec 28, 2023
@m33p
Copy link

m33p commented Dec 29, 2023

I would also like to see this feature implemented in .NET 9.

@abbottdev
Copy link

Adding another use case here:

If I have a "Name" property on an entity, I would like to have a custom validator called ValidateUniqueName which ensures that the Name property on the model hasn't been used before. This requires a database call to check, which is inherently async code.

We appreciate that it may not be "optimal" for performance, but we are performing this call on every request regardless just with manual code.

@EnricoMassone
Copy link

EnricoMassone commented Jan 28, 2024

As a workaround to this limitation, you can consider using an action filter as a plug point to implement async validation logic on the action method arguments.

Basically, you put the validation logic inside the IAsyncActionFilter implementation and in case of validation failure either you short-circuit the action method execution, or you add errors to the model state.

This is not optimal, since you need to know both the action parameter name and its type in order to query the ActionExecutingContext.ActionArguments dictionary, but apart for these shortcomings it works fine.

Maybe in the original design of the ASP.NET core framework this was the envisioned way to implement async model validation (just trying to guess here). Honestly, something more semantic and more strongly typed could be a nice feature to have in the framework.

@jesslilly
Copy link

We are replacing all of the magical auto-validation with injected validators and manually calling Validate. It is a lot of work and testing, but it is actually a really good process IMHO. We have found actions that do not have validators and should and also check for ModelState.IsValid with no validators at all. So we have been able to remove some code. The result is much cleaner and easier for new developers to understand how validation is working.

@nfsp-ta
Copy link

nfsp-ta commented Jan 29, 2024

This is causing an issue for my team because we're trying to do things "The Microsoft Way" instead of cobbling together 'roll-your-own' solutions, but there is a direct conflict between the .NET model validation and feature flags.
For model validation, the sanctioned path is implementing IValidatableObject, which is only synchronous.
For feature toggling, the sanctioned path is to use IFeatureManager, which will only check feature toggles asynchronously using the IsEnabledAsync() method.
So how exactly are we supposed to feature-toggle a validation rule?

@R0boC0p
Copy link

R0boC0p commented Feb 21, 2024

3 years later and nothing really happened... my gosh. I am currently working on a proof of concept for a home-brewed async-validation extending the current mechanism. So far my use-cases seem to work.
If anyone is interested, let me know and I can see to make it publicly available.

@Bogdan-Shklanka2002
Copy link

It would be useful to see yours use-cases. Could you make it publicly available?

@qiuzman
Copy link

qiuzman commented May 18, 2024

Is there any update on this? I am trying to stick to using Data Annotations rather than FluentValidation but I typically write in async and not being able to here is stopping progress.

@R0boC0p
Copy link

R0boC0p commented May 24, 2024

First of all, sorry folks for the late reply 💔 ...
I had a first attempt on making the NativeWaves.AsyncValidation to use the current validation run asynchronously. It mainly takes the existing asp-net-core logic making it async-proof. Nothing more. You can give it a try here. I see this as a prototype and don't know atm where we are going with it from here. It's used by us in production (for our purposes) and seems to run stable.

Cheers

@AnyThing-Well
Copy link

I would also like to see this feature implemented in .NET 9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-ui-rendering Includes: MVC Views/Pages, Razor Views/Pages enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-model-binding Pillar: Dev Experience
Projects
No open projects
Status: No status
Development

No branches or pull requests