Skip to content
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

Fix inconsistent field-level validation (Fixes #204) #205

Conversation

matthetherington
Copy link

@matthetherington matthetherington commented Dec 6, 2023

This fixes a few issues that are causing inconsistent behaviour between field-level and whole-model validation.

See #204 for more context and a demo project

Summary of changes:

  • The type of EditContext.Model is used to locate a validator for field-level validation if one is not provided
  • For field-level validation, the received FieldIdentifier is converted to a Fluent property path (eg Customer.Address[0].Line1) by searching the EditContext.Model instance, and then the validation results at that path are used to determine if the field is valid. The <FluentValidationValidator> options are supplied to Fluent, but are combined with an additional restriction so that only rules affecting the changed property are ran (as per the default MemberNameValidatorSelector behaviour)
  • The configuration of <FluentValidationValidator> that is currently used for whole-model validation is also used when validating fields, this means that RuleSets and other configuration work as expected at the field-level.

The above would just mirror the existing whole-model validation for field-level validation, so I think it's a fairly safe change. If a project is using Blazored.FluentValidation then they'll have a validator configured that works for the whole model.

Could I please get your thoughts? I'm looking at this through quite a narrow lense so there may be issues with the proposal that haven't occured to me - it does seem to work okay though.

Resolves #204
Resolves #188
Resolves #180

@matthetherington matthetherington force-pushed the bugfix/204-inconsistent-field-level-validation branch from 1eb55dc to d8d4443 Compare December 6, 2023 20:54
@matthetherington matthetherington force-pushed the bugfix/204-inconsistent-field-level-validation branch from 9b7fd1d to 2faff2f Compare December 7, 2023 10:11
@UniMichael
Copy link

UniMichael commented Dec 7, 2023

EDIT: You can ignore this comment. I messed something up when I tried the PR. It's working fine as far as I can tell.


Was asked to provide some feedback in #180, since we ended-up making our own validation handler at work to tackle something similar.

From playing around with it, it does seem to properly validate nested objects' fields, though it's worth pointing-out that it doesn't address the problem discussed in #180 (all RuleSets firing, regardless of options), but it's like I said in that discussion:

FluentValidation's API doesn't expose the necessary functions to fix that (which is why our fix at work validates the entire model on field change and only tracks the new validation errors so we can fake a field-speicific validation).


If anyone's interested in trying to fix that specific issue with this PR, here's the basic example I was using to reproduce it:

public class Person
{
    public string Name { get; set; } = string.Empty;
    public int Age { get; set; }
    public Address Address { get; set; } = new();
}

public class Address
{
    public string Street { get; set; } = string.Empty;
    public string Country { get; set; } = string.Empty;
    public string PostalCode { get; set; } = string.Empty;
}

public class PersonValidator : AbstractValidator<Person>
{
    public PersonValidator(IValidator<Address> addressValidator)
    {
        RuleFor(a => a.Address).SetValidator(addressValidator);
        
        RuleFor(p => p.Name).NotEmpty().WithMessage("Name required");
        
        RuleSet("Adult", () =>
        {
            RuleFor(p => p.Age).GreaterThan(18).WithMessage("Must be an adult");
        });
    }
}

public class AddressValidator : AbstractValidator<Address>
{
    public const string PostalCodePattern = "[a-zA-Z][0-0][a-zA-Z] [0-9][a-zA-Z][0-9]";
    public static readonly Regex PostalCodeRegex = new(PostalCodePattern);
    
    public AddressValidator()
    {
        RuleFor(a => a.Country).NotEmpty().WithMessage("Country required");
        RuleFor(a => a.Street).NotEmpty().WithMessage("Street required");
        RuleFor(a => a.PostalCode).Matches(PostalCodeRegex).WithMessage("Must be valid postal code");
        
        RuleSet("Canadian", () =>
        {
            RuleFor(a => a.Country)
                .NotEmpty().WithMessage("Country required")
                .Equal("Canada").WithMessage("Must be Canadian");
        });
    }
}
@page "/"
@using Microsoft.AspNetCore.Components.Forms
@using BlazorServerApp.Models
@using Blazored.FluentValidation

<EditForm Model="_person" OnValidSubmit="() => { }">
    @* NOTE: The options' rulesets are basically ignored, you can use whatever RuleSet you want, and it'll still run all of them *@
    <FluentValidationValidator Options="@(options => options.IncludeRuleSets("default"))"/>
    <ValidationSummary/>

    <h1>Person</h1>
    <div>
        <label>Name</label>
        <InputText @bind-Value="_person.Name"/>
    </div>
    <div>
        <label>Age</label>
        <InputNumber @bind-Value="_person.Age"/>
    </div>

    <h2>Address</h2>
    <div>
        <label>Country</label>
        <InputText @bind-Value="_person.Address.Country"/>
    </div>
    <div>
        <label>Street</label>
        <InputText @bind-Value="_person.Address.Street"/>
    </div>
    <div>
        <label>Postal Code</label>
        <InputText @bind-Value="_person.Address.PostalCode"/>
    </div>
    
    <button type="submit">
        Submit
    </button>
</EditForm>

@code {

    private readonly Person _person = new();

}

@matthetherington
Copy link
Author

matthetherington commented Dec 7, 2023

Thanks for that @UniMichael

I tried to recreate the problem you were seeing, but it seems to be working okay when using the modified Blazored.FluentValidation from this PR.

Screenshot 2023-12-07 at 15 59 59

I can enter 17 for age, and UK as the country, and both fields are valid when blurring out.

Submitting the form also does not produce a validation message for those fields, so it does seem to be using only the default rule set as specified in the <FluentValidationValidator> config and not including Adult / Canadian. Omitting the options produces the same result.

Screenshot 2023-12-07 at 16 01 10

Is that what you were expecting to happen?

@matthetherington
Copy link
Author

matthetherington commented Dec 7, 2023

And here's the result when I include the validator via:

<FluentValidationValidator Options="@(options => options.IncludeRuleSets("Adult"))"/>
Screenshot 2023-12-07 at 16 10 49

The postcode format check is not ran, so the default ruleset for address is not being ran, and the "must be an adult" message appears, so the Adult ruleset is being ran as per the options provided.

@UniMichael
Copy link

@matthetherington Ok, I think I messed something up when I tried-out the PR. I may have forgotten to actually checkout the branch after I pulled it. I'm running it now and it does seem like the rule problem was fixed.

That's what I get for testing it on a Friday morning without having coffee, I guess. 😅

I'll edit my other comment.

@matthetherington
Copy link
Author

@UniMichael ah brilliant, thanks for taking another look!

@Albiniivari
Copy link

@matthetherington Hey, so I've tried the PR and it seems to be working fine. However, I find the partial-validation a bit odd.
If you first input some non-allowed value in to the other fields, it displays a few errors. But if you then press the partial-validate, the existing errors are deleted, so that it seems that they are now OK. I would expect the partial-validate to validate only the fields included in rule-set, but leave existing errors etc to be?

@chrissainty chrissainty added the Bug Something isn't working label Feb 18, 2024
return BuildPropertyPath(currentNode, fieldIdentifier);
}

var nonPrimitiveProperties = currentModelObject?.GetType()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had an issue with this bit of code, in a model it kept adding DateTime types on the NodeStack in an endless loop, eventually ending in out of memory. I think additional type guards are needed here for types such as DateTime. This change to check for additional simple types fixed the issue with DateTime not being a primitive.
jafin@2de01ab

@etarvt
Copy link

etarvt commented Apr 26, 2024

@matthetherington I've been using the Blazored.FluentValidation library and find it to be excellent, but I encountered issue #204 a week ago. I've tested the PR, and it seems to resolve the problem with the sub-objects for me.

Do you expect this PR to be merged anytime soon?

@Xavia1991
Copy link

Please merge this soon, I have been waiting for this since end of last year when we switched to .net8.

@psandler
Copy link

Any timeline on when this will be reviewed/merged? I am also having this problem and this PR seems to resolve it.

@ckirker
Copy link

ckirker commented Sep 13, 2024

I am in need of this fix as well--as I'm blocked using FluentValidation without it.

@DaveVW-AWE
Copy link

Any updates on merging this in? Been waiting a VERY long time for this and it's holding up our project. I like to rely on and support 3rd party packages however this has taken far too long to resolve.

@pwelter34
Copy link
Collaborator

apologies, I'll work to get this resolved and merged

@pwelter34 pwelter34 merged commit 8afbf82 into Blazored:main Oct 7, 2024
1 check passed
@dmorawetz
Copy link

dmorawetz commented Dec 6, 2024

Has anyone tested this PR with more than 5 fields and a non-trivial model?

We have hundreds of fields in our model with lists of more complex models. Because our form is user-configurable, we have effectively a tree of those complex models. The performance of PropertyPathHelper.ToFluentPropertyPath is making this package unusable for us.

We use this package since 2021 and had never issues with the default MemberNameValidatorSelector.

Concerning the original issue from #204, I think there should be a better way to select the validator of the root model, if needed. It should at least be configurable, as for our project, the previous selection strategy worked better.

It seems like a difficult problem, though, considering #235, #76, and #104.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet