Add proposal for async validation#363
Conversation
| ### `IAsyncValidatableObject` Returns `IAsyncEnumerable` | ||
| Allows streaming results and is aligned with the pattern where validation may involve multiple async checks. If deemed too complex for V1, `Task<IEnumerable<ValidationResult>>` is an acceptable simplification. |
There was a problem hiding this comment.
It's not directly related to this, but I think the way async validation will execute deserves more discussion. This has a big impact on the UX for UI frameworks, and can have very unpredictable outcomes in API contexts too.
Is the expectation that ValidateAsync will walk through the validations as it does today and await each async validation before triggering the next one?
| - Clean failure: the sync `IsValid` override throws `NotSupportedException` in sync paths | ||
| - No silent skipping: since it derives from `ValidationAttribute`, sync `Validator` still discovers it | ||
| - No sync-over-async: sync callers get a clear error telling them to use async APIs | ||
| - TODO: Add opt-in to sync-over-async (via ValidationContext and/or AppContext switch?) |
There was a problem hiding this comment.
This can easily lead to issues like threadpool starvation right? Do we have a scenario/motivation to do this?
| { | ||
| protected override ValidationResult? IsValid(object? value, ValidationContext validationContext) | ||
| { | ||
| throw new NotSupportedException( |
There was a problem hiding this comment.
If I would read the API documentation (before async validation is shipped), I would assume that IsValid can only throw NotImplementedException and InvalidOperationException, and exactly for the reasons documented (and to be clear, I'm not following with the explanation of the InvalidOperationException in this case). So, isn't this technically unexpected break if existing code encounters the new attribute? i.e, consider an already-built library that scans some user code for attributes and the user tries to use AsyncValidationAttribute.
| ### Option A: Virtual `IsValidAsync` Only on `ValidationAttribute` (No Separate Subclass) | ||
| Same as above but without the `AsyncValidationAttribute` intermediate class. Users override `IsValidAsync` directly on `ValidationAttribute` and the sync `IsValid` base uses reflection to detect async-only attributes. | ||
|
|
||
| **Not chosen because:** Reflection-based override detection is fragile. Option C's override is cleaner and more explicit. |
There was a problem hiding this comment.
I'm not completely following with the downsides of this approach. I would personally hope to keep everything under ValidationAttribute (obviously if possible and we think it's good). So trying to understand more the problems with this approach.
Also for the case of calling the sync IsValid on an attribute that only can provide async implementation, I wonder if we should have a special ValidationResult for it, instead of throwing an exception. This suggestion/question applies to both Option A and Option C.
ploeh
left a comment
ploeh
left a comment
There was a problem hiding this comment.
This is not a hill I'm going to die on, but as I've already outlined in dotnet/runtime#121536 (comment), I think this is a bad idea in general.
|
|
||
| Per review feedback, the chosen approach is a new `AsyncValidationAttribute` class that **derives from** `ValidationAttribute`. This provides: | ||
| - Clear separation: async-only attributes are a distinct type | ||
| - Clean failure: the sync `IsValid` override throws `NotSupportedException` in sync paths |
There was a problem hiding this comment.
Throwing NotSupportedException is a code smell that the design violates the Liskov Substitution Principle. This should strongly indicate consideration of an alternative design.
If the class isn't going to honour the behaviour of the base class, then why even derive from it?
4 participants
I'm just opening this temporarily for people to leave feedback. I plan to turn this into API Proposals for the runtime and aspnetcore repos. After that, I plan to close this without merging. Sorry if that's a misuse of this repo.