dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.59k stars 10.06k forks source link

Please reconsider allowing async model validation #31905

Open JeremySkinner opened 3 years ago

JeremySkinner commented 3 years ago

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:

(#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

javiercn commented 3 years ago

@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.

ghost commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

sumtec commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

thanks!

stevendarby commented 2 years ago

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

celluj34 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

Would very much like to see this feature

campbellja commented 1 year ago

I too would love to see this added ☺️

kidchenko commented 1 year ago

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

jari-kuipers commented 1 year ago

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

jesslilly commented 1 year ago

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 commented 1 year ago

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, https://github.com/MudBlazor/MudBlazor/pull/2418

It also appears supported in https://github.com/Blazored/FluentValidation/pull/137

miguel93041 commented 1 year ago

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.

ghost commented 11 months ago

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.

m33p commented 11 months ago

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

abbottdev commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 9 months ago

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 commented 9 months ago

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

qiuzman commented 6 months ago

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 commented 6 months ago

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 commented 4 months ago

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