Skip to content

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

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

Open
SteveSandersonMS opened this issue Sep 25, 2019 · 11 comments
Open

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

SteveSandersonMS opened this issue Sep 25, 2019 · 11 comments
Labels
affected-medium This issue impacts approximately half of our customers area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-blazor-builtin-components Features related to the built in components we ship or could ship in the future severity-minor This label is used by an internal tool
Milestone

Comments

@SteveSandersonMS
Copy link
Member

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
Copy link
Member Author

Some thoughts about this from @mrpmorris: #10526 (comment)

@mrpmorris
Copy link

mrpmorris commented Sep 25, 2019

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

@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone Sep 26, 2019
@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Sep 26, 2019
@mrpmorris
Copy link

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

@SteveSandersonMS
Copy link
Member Author

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
Copy link

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
Copy link

mrpmorris commented Nov 5, 2019

@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:
#14972 (comment)

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 #14426 (comment)

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
Copy link

boukenka commented Jan 8, 2020

@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
Copy link

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

@SteveSandersonMS SteveSandersonMS added affected-medium This issue impacts approximately half of our customers severity-minor This label is used by an internal tool labels Oct 14, 2020 — with ASP.NET Core Issue Ranking
@javiercn javiercn added the feature-blazor-builtin-components Features related to the built in components we ship or could ship in the future label Apr 19, 2021
@mkArtakMSFT
Copy link
Member

Potentially related: #31252

@mrpmorris
Copy link

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
Copy link

sipi41 commented Dec 5, 2022

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!

@dotnet dotnet deleted a comment Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-medium This issue impacts approximately half of our customers area-blazor Includes: Blazor, Razor Components enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-blazor-builtin-components Features related to the built in components we ship or could ship in the future severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

6 participants