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.36k stars 9.99k forks source link

Validation doesn't automatically traverse members #10526

Closed danroth27 closed 5 years ago

danroth27 commented 5 years ago

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

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

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

mkArtakMSFT commented 5 years ago

Summary of an in-person discussion:

mrpmorris commented 5 years ago

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

@mrpmorris do you have sample code of your suggestion?

mrpmorris commented 5 years ago

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

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

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

pranavkm commented 5 years ago

@mrpmorris for your ask, we're going to use https://github.com/aspnet/AspNetCore/issues/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 commented 5 years ago

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

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

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