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.46k stars 10.03k forks source link

Ensure it's possible to validate based on the fields in the UI #14426

Open SteveSandersonMS opened 5 years ago

SteveSandersonMS commented 5 years ago

The out-of-box DataAnnotationsValidator follows Data Annotations conventions of doing model-based, not UI-based, validation.

Sometimes people may want UI-based validation instead (i.e., we validate just the fields that have a corresponding editor on the screen right now).

If we're missing APIs to enable customizations for this, add them.

SteveSandersonMS commented 5 years ago

Some thoughts about this from @mrpmorris: https://github.com/aspnet/AspNetCore/issues/10526#issuecomment-533926474

mrpmorris commented 5 years ago

Hi all

I've put together some changes for you to consider. They aren't in a format that I can submit a pull-request. What I've done is to copy the various form classes into a folder called Modified (into a namespace with the same name) - from there I have made a branch, made the changes, and created a PR into my master branch so you can see the changes, which are accompanied by comments in the PR explaining the purpose of the changes.

This code should be backward compatible, but give the option for the user to specify Mode = DataAnnotationsValidatorMode.Recursive to get a full tree walk when validating.

I've done it this way so you can download the branch, and then run it to see it in action.

Benefits are

  1. No breaking changes
  2. Recursive validation is optional
  3. Doesn't require any additional attributes on the model being edited
  4. Will validate inputs regardless of how deep the @bind-Value expression is
  5. When not doing a Recursive validation it will only validate the form inputs, making it possible to edit a large object in a wizard-style UI.
  6. The tree walking code can be shared by other custom validation libraries (such as Blazor-Validation plug)

The recursive attribute on the model doesn't sit right with my gut. It seems UI decisions are being imposed on the model being edited, but the model is imposing an approach on the UI. If an app wanted two ways of editing the same object (Expert=one form / New user=wizard UI) then a recursive attribute would prevent this by forcing one approach.

PS: Ignore any changes with the text FormX or NewInputField. Those were corrections of earlier mistakes.

https://github.com/mrpmorris/_BlazorValidationChanges_/pull/1/files

mrpmorris commented 5 years ago

Any thoughts on my suggested approach? If it has failings then I'd like to know so I can rethink.

SteveSandersonMS commented 5 years ago

It's not something we're focusing on at the moment, as it's on the backlog and we have lots of urgent 3.1 work to get through. We'll consider possible designs in the future when we have capacity. Hope that's OK!

mrpmorris commented 5 years ago

Sure, thanks for your reply.

I just wanted to make sure if it needed additional mental work that I was aware so I could do it!

mrpmorris commented 5 years ago

@SteveSandersonMS @danroth27 @pranavkm @rynowak

I see this has been added to Blazor 3.1 -> https://github.com/aspnet/AspNetCore/blob/release/3.1/src/Components/Blazor/Validation/src/ObjectGraphDataAnnotationsValidator.cs

I feel it is just introducing a different problem, where we end up with two solutions and neither of them solve all three validation scenarios. I want to ensure you've seen my comments on this: https://github.com/aspnet/AspNetCore/pull/14972#issuecomment-546251922

I did propose a solution that would deal with all three validation scenarios (the two you have now + wizard UI validating only visible components) here https://github.com/aspnet/AspNetCore/issues/14426#issuecomment-535238974

I'm not sure what you thought of my approach, but regardless of how you ultimately proceed I feel it would be better to leave this class out until a more complete solution has been identified.

boukenka commented 4 years ago

@mrpmorris Did you get any new feedback/comment for your solution or/and your suggestion to add the following code in #14972:

if (value.GetType().Assembly == typeof(object).Assembly)
  return;
mrpmorris commented 4 years ago

@mrpmorris Did you get any new feedback/comment for your solution or/and your suggestion to add the following code in #14972:

if (value.GetType().Assembly == typeof(object).Assembly)
  return;

The suggestion was wrong, because lists and arrays live there, but it should skip String though.

mkArtakMSFT commented 2 years ago

Potentially related: https://github.com/dotnet/aspnetcore/issues/31252

mrpmorris commented 2 years ago

As this was a number of years ago, it seems I've deleted my proposed changes.

The gist of it was that inputs register themselves with the EditContext by asking for its Field, and then you can have a validation strategy that only validates items that have fields in the EditContext.

Please ask if that made no sense :)

sipi41 commented 1 year ago

I think validation based on UI is very important mr @SteveSandersonMS sometimes it makes no sense to validate the whole model except what we are interested into showing... please make this possible!