Blazored / FluentValidation

A library for using FluentValidation with Blazor
https://blazored.github.io/FluentValidation/
MIT License
587 stars 84 forks source link

[Feature Request] Explicit validator specification #22

Closed nullpainter closed 4 years ago

nullpainter commented 4 years ago

As per #20 , I have:

The DI-registered validator also new's up a child validator containing a non-DI constructor argument, as I have non-trivial validation rules:

RuleForEach(i => i.FinalProducts).SetValidator(i => new ProductValidator(i.Nature));

Expectation:

Reality:

Is the design of your component that for each bound value, a validator is located for the associated model? This works for self-contained components, but isn't so great for a situation like mine where the models for my child components are just fields in the parent EditContext model, and where we have a single parent validator for the lot.

Given the desired behaviour appears to be a significant departure from how your component is designed, perhaps we can pass in the target validator model type as an argument for FluentValidationValidator? Or just a flag to say to use the EditContext model's validator?

chrissainty commented 4 years ago

@nullpainter Have you tried setting the validator manually using the Validator parameter?

<FluentValidationValidator Validator="myValidator" />
nullpainter commented 4 years ago

I didn't realise one could do that! Unfortunately, that results in a different error:

System.InvalidCastException: Unable to cast object of type 'xxx.ProductSelector' to type 'xxx.InteractionContext'.
   at FluentValidation.ValidationContext.ToGeneric[T]() in /home/jskinner/code/FluentValidation/src/FluentValidation/ValidationContext.cs:line 211

My parent form has the following in the code-behind:

[Inject]
protected IValidator<InteractionContext>? Validator { get; set; }

This is my single DI-registered validator which validates all fields on this form. InteractionContext is the model for the form and is used by all of its child components.

ProductSelector is a code-behind class used by one of the form's child components.

chrissainty commented 4 years ago

Can you provide all of this code as something doesn't sound right about what you're doing? But without seeing your code I can't tell if it's just how I'm interpreting things.

nullpainter commented 4 years ago

Unfortunately I'm not able to do that.

Debugging via your decompiled code, GetValidatorForModel(serviceProvider, fieldIdentifier.Model) is never being called inside ValidateField, because the parent validator is being passed via AddFluentValidation.

The underlying problem appears to be a lack of support for validation of forms where fields are distributed across multiple child components and where the same model isn't being used for all of them.

In my exception above, the field being validated was bound to a property on the component itself. However the one DI-registered validator I have is against a parent model.

The expected behaviour in my case is that the context isn't model of the field in the child component, but the model associated with the EditContext.

chrissainty commented 4 years ago

The underlying problem appears to be a lack of support for validation of forms where fields are distributed across multiple child components.

This doesn't make sense to me. Any form component is a child component of the EditForm.

If you can't provide your code, then can you provide a minimal repro project showing the issue please?

nullpainter commented 4 years ago

Sorry, I know that being able to provide code would make things significantly easier. I've just added some clarification to my comment, but will create a minimal repro project today.

chrissainty commented 4 years ago

That would be great, thank you.

nullpainter commented 4 years ago

Here's a contrived repro which demonstrates the exception above. I can create a separate one for #20 if you like - it's a variant on a theme.

chrissainty commented 4 years ago

I've had a look at your repro, and everything appears to be working as expected. You have a model with a single property and a single validation rule. If I try and submit the form with an empty first field then I see an error. If I enter a value in the field it goes away.

nullpainter commented 4 years ago

The first field isn't the problem. The issue is when you enter text into the second field:

image

chrissainty commented 4 years ago

I think I know what's going on. In your repro app, you have a second field which is bound to a property which isn't part of the model the EditContext handling. But that field gets registered with the EditContext anyway as that is what the built-in form components do. When that field is altered it triggers a validation check, at that point the FluentValidator tries to validate it but as it's not part of the model the EditContext is handling we get the error in the console.

I'm not sure this is a bug as such. Your form should only be handling one model not two, which is essentially what your repro sample is doing. If you setup your form to handle a single model then there isn't a problem.

nullpainter commented 4 years ago

Yes, that's exactly what's going on. I'd be interested to see what the recommended approach is (apart from the obvious "handle one model").

In my real code, I have a form with a child component which is repeated twice. The child component contains a handful of fields. Here's a simplistic example of the model. Here, ParentClass is the single model which is handled by the EditContext:

  public class ParentClass
    {
       public ChildClass FirstChild { get; set; } 
       public ChildClass SecondChild { get; set; }
    }

    public class ChildClass
    {
        public string FirstField { get; set; }
        public string SecondField { get; set; }
    }

The first time the child component is in the form, I am passing it FirstChild to bind its properties to, and the second time I'm passing it SecondChild:

<form>
     <ChildComponent Context="FirstChild" />
     <ChildComponent Context="SecondChild" />
</form>

So I do still have a single model, but the child components are being passed different bits of it.

chrissainty commented 4 years ago

Well, the recommended approach is only handle one model. But as you said in your real code you are handling one model. The frustration here is that the repro wasn't actually a repro of your real code then? Your real code was only using one model, so it seems to have been a bit of a waste of time.

nullpainter commented 4 years ago

The repro was possibly a little too contrived, but I am pretty sure it's demonstrating the same behaviour I'm seeing (unless I have just made a catastrophic mistake). Let me update the repro later today.

nullpainter commented 4 years ago

Okay, I've updated the repro with a more representative example of the problem. Apologies that the last one was too simplistic - I was trying to demonstrate the issue using as few moving parts as possible.

Here's a screenshot of the updated code, without validation:

image

As before, I receive the following exception when either field of ChildComponent is set:

System.InvalidCastException: Unable to cast object of type 'ValidationIssue.Models.ChildContext' to type 'ValidationIssue.Models.ParentContext'.
   at FluentValidation.ValidationContext.ToGeneric[T]() in /home/jskinner/code/FluentValidation/src/FluentValidation/ValidationContext.cs:line 211
   at FluentValidation.AbstractValidator`1.FluentValidation.IValidator.ValidateAsync(ValidationContext context, CancellationToken cancellation) in /home/jskinner/code/FluentValidation/src/FluentValidation/AbstractValidator.cs:line 74
   at Blazored.FluentValidation.EditContextFluentValidationExtensions.ValidateField(EditContext editContext, ValidationMessageStore messages, FieldIdentifier fieldIdentifier, IServiceProvider serviceProvider, IValidator validator)
nullpainter commented 4 years ago

So I think we both know why it's happening -- the question is whether this is a misuse of Blazor, or if there is a better way to compose form components.

This seems a fairly common use case though -- say, for example, an address component used twice, once to capture physical address and again to capture residential address.

chrissainty commented 4 years ago

I've been looking at this again and in your latest repro code, if no validator is specified then everything appears to work as expected.

nullpainter commented 4 years ago

That's because this was a repro in response to your suggestion that I explicitly pass a validator, and confusion as to what I was seeing.

The original issue, raised as part of #20, is when child validators are explicitly new'd up. I've created a new branch with this demonstrated.

Here is the updated validator:

 public class FormValidator : AbstractValidator<ParentContext>
    {
        public FormValidator()
        {
            RuleFor(f => f.ParentField).NotEmpty();

            // Set conditional validators for child fields. 
            RuleFor(f => f.FirstChild).SetValidator(new ChildValidator(true));
            RuleFor(f => f.SecondChild).SetValidator(new ChildValidator(false));
        }
    }

    public class ChildValidator : AbstractValidator<ChildContext>
    {
        // Workaround to prevent Blazored.FluentValidation failing trying to resolve validator
        // https://github.com/Blazored/FluentValidation/issues/20
        public ChildValidator()
        {
        }

        public ChildValidator(bool validateSecondField)
        {
            RuleFor(v => v.FirstField).NotEmpty();
            RuleFor(v => v.SecondField).NotEmpty().When(_ => validateSecondField);
        }
    }

The empty constructor for ChildValidator is my workaround for #20. Without it, we get the following exception:

System.InvalidOperationException: Unable to resolve service for type 'System.Boolean' while attempting to activate 'ValidationIssue.Validators.ChildValidator'.
   at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.ConstructorMatcher.CreateInstance(IServiceProvider provider)
   at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.CreateInstance(IServiceProvider provider, Type instanceType, Object[] parameters)
   at Blazored.FluentValidation.EditContextFluentValidationExtensions.GetValidatorForModel(IServiceProvider serviceProvider, Object model)

Again, there are workarounds for these issues. The empty constructor does the trick, but I thought I should raise this anyway as it's most likely something that other people will come across.

The original issue in #20 - the exception with a third-party package missing a dependency - is related, insofar as it happens when the (wrong, in my case) child validator is being searched for. I worked around that by explicitly including the dependency, even though it's not needed for my solution.

So, three separate but related issues.