Skip to content

Validation doesn't automatically traverse members #10526

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

Closed
danroth27 opened this issue May 24, 2019 · 12 comments
Closed

Validation doesn't automatically traverse members #10526

danroth27 opened this issue May 24, 2019 · 12 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@danroth27
Copy link
Member

Validation doesn't automatically traverse the members of something so you have to manually add validation support for any satellite objects you want validated.

Example: https://github.com/rynowak/ComponentFunTime/blob/master/Components/Shared/EmployeeEditor.razor#L85

@mrpmorris
Copy link

mrpmorris commented Aug 23, 2019

@danroth27

Possible solution:
In InputBase<T>.set_FieldIdentifier execute EditContext.GetFieldState(value, true)

This will add the field state to the EditContext and ensure it is validated.

@pranavkm pranavkm modified the milestones: Backlog, 3.1.0-preview1 Aug 26, 2019
@pranavkm
Copy link
Contributor

Moving this out of the backlog. We should consider doing something about this in 3.1 since the experience is fairly poor right now.

@pranavkm pranavkm removed this from the 3.1.0-preview1 milestone Aug 26, 2019
@mkArtakMSFT mkArtakMSFT added this to the 3.1.0-preview1 milestone Aug 27, 2019
@mkArtakMSFT
Copy link
Member

Summary of an in-person discussion:

  • Create an experimental package with a solution for 3.1
  • Document this and require the usage of the new package for this to work
  • Revisit in 5.0, based on whether corefx add a native support for this or not

@mrpmorris
Copy link

All you need to do is have InputBase ensure it has an entry in the EditContext when its EditContext changes. That will ensure only the items being edited are validated. This should be a minimum requirement.

If you also want to provide the ability to validate a whole object regardless of which properties have associated edits, please put a parameter on the validator that allows the user to disable it.

Also, please make the traversal code part of EditContext and not DataAnnoticationsValidator - so other validation frameworks can use it too.

@mgolois
Copy link

mgolois commented Sep 6, 2019

@mrpmorris do you have sample code of your suggestion?

@mrpmorris
Copy link

mrpmorris commented Sep 22, 2019

The following code adds a hacky _AccessGetFieldState to EditContext. It's just a way to access EditContext.GetFieldState.

The "fixed" InputText (should be done in InputBase) simply calls EditContext.GetFieldState when its EditContext changes, this ensures EditContext._fieldStates has an entry for this input.

If we add an IEnumerable<FieldState> to ValidationRequestedEventArgs then the validation implementation can decide whether to validate only those inputs, or use some new Blazor method in EditContext to traverse the object tree and get a full list of FieldIndentifiers - a simple validation system like DataAnnotations could just LINQ Select Distinct identifier.Model to get a list of objects to validate.

This suggestion is non breaking and gives us

1: Deep validation into complex types.
2: In custom validators like FluentValidation, the ability to only validate parts of the object we are letting the user modify. This is useful for wizard UIs where the user builds up the data over multiple pages.
3: DataAnnotationsValidator can have a Parameter specifying if we want to do deep validation (all objects in tree) or partial (only what we have FieldState for).

	public class InputTextFixed : InputText
	{
		public async override Task SetParametersAsync(ParameterView parameters)
		{
			EditContext previousEditContext = EditContext;
			await base.SetParametersAsync(parameters);
			if (EditContext != previousEditContext)
				EditContext._AccessGetFieldState(FieldIdentifier);
		}
	}

	public static class HackGetFieldState
	{
		public static void _AccessGetFieldState(this EditContext editContext, FieldIdentifier fieldIdentifier)
		{
			var getFieldStateInfo = typeof(EditContext).GetMethod("GetFieldState", BindingFlags.NonPublic | BindingFlags.Instance);
			if (getFieldStateInfo == null)
				throw new NullReferenceException("GetFieldState not found");
			System.Diagnostics.Debug.WriteLine("Updating FieldState");
			getFieldStateInfo.Invoke(editContext, new object[] { fieldIdentifier, true });
		}
	}

@pranavkm
Copy link
Contributor

All you need to do is have InputBase ensure it has an entry in the EditContext when its EditContext changes. That will ensure only the items being edited are validated.

@mrpmorris I'm not sure how that works. Would each of these fields be the models passed in to data annotations? That would mean validation attributes decorated on properties wouldn't be discovered. In addition, the EditForm has already established the model to operate on. It would be surprising if validation did not apply to all the properties on the model the user declared was being edited.

@mrpmorris
Copy link

Sorry, can you elaborate? I'm not quite sure I am following what your concerns are.

@pranavkm
Copy link
Contributor

@mrpmorris for your ask, we're going to use #14426 to track adding the API to the application. We'll use this issue to continue tracking the original goal of the issue which is to produce an experimental package based on the sample: https://github.com/aspnet/samples/tree/master/samples/aspnetcore/blazor/Validation/Validation

@mrpmorris
Copy link

I work for the UK government Department for Education. A common approach is to allow the user to edit a large object over multiple steps, the experimental approach wouldn't work and would require us to create multiple classes that identically represent parts of the object being edited and map the properties across.

It's more work than it needs to be. If InputBase would just instruct EditContext to create FieldState for its FieldIndentifier whenever its EditContext changes then Blazor would ensure all input controls are validated.

@SteveSandersonMS
Copy link
Member

Yes, I think hopefully the issue #14426 should cover your UI-based validation requirement (which is not exactly the same as this issue, i.e., traversal into child objects).

If you have any further comments about UI-based validation requirements, could you post them at #14426? Thanks!

@mkArtakMSFT
Copy link
Member

@pranavkm please make sure this happens as the last thing in 3.1, as there is no time pressure.

pranavkm added a commit that referenced this issue Oct 11, 2019
* Ensure validation result that are not associated with a member are recorded. Fixes #10643
* Add support for showing model-specific errors to ValidationSummary
* Add support for nested validation and a more suitable CompareAttribute. Fixes #10526
pranavkm added a commit that referenced this issue Oct 14, 2019
* Ensure validation result that are not associated with a member are recorded. Fixes #10643
* Add support for showing model-specific errors to ValidationSummary
* Add support for nested validation and a more suitable CompareAttribute. Fixes #10526
pranavkm added a commit that referenced this issue Oct 16, 2019
* Ensure validation result that are not associated with a member are recorded. Fixes #10643
* Add support for showing model-specific errors to ValidationSummary
* Add support for nested validation and a more suitable CompareAttribute. Fixes #10526
pranavkm added a commit that referenced this issue Oct 17, 2019
* Ensure validation result that are not associated with a member are recorded. Fixes #10643
* Add support for showing model-specific errors to ValidationSummary
* Add support for nested validation and a more suitable CompareAttribute. Fixes #10526
pranavkm added a commit that referenced this issue Oct 18, 2019
* Validation fixes for Blazor

* Ensure validation result that are not associated with a member are recorded. Fixes #10643
* Add support for showing model-specific errors to ValidationSummary
* Add support for nested validation and a more suitable CompareAttribute. Fixes #10526
@pranavkm pranavkm added Done This issue has been fixed and removed Working labels Oct 18, 2019
@mkArtakMSFT mkArtakMSFT modified the milestones: 3.1.0, 3.1.0-preview2 Oct 19, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
3dots pushed a commit to 3dots/aspnetcore-Web.JS that referenced this issue Feb 19, 2024
* Validation fixes for Blazor

* Ensure validation result that are not associated with a member are recorded. Fixes dotnet/aspnetcore#10643
* Add support for showing model-specific errors to ValidationSummary
* Add support for nested validation and a more suitable CompareAttribute. Fixes dotnet/aspnetcore#10526
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

6 participants