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

Nested RuleForEach and ChildRules don't work in RuleSets #2097

Closed
UniMichael opened this issue Mar 30, 2023 · 2 comments · Fixed by #2098
Closed

Nested RuleForEach and ChildRules don't work in RuleSets #2097

UniMichael opened this issue Mar 30, 2023 · 2 comments · Fixed by #2098
Assignees

Comments

@UniMichael
Copy link

UniMichael commented Mar 30, 2023

FluentValidation version

11.5.1

ASP.NET version

No response

Summary

We've been working on migrating a project that was previously using FluentValidation 10.3.0 to the newest version. In this project, we have a lot of rulesets that run rules on deeply-nested complex objects. After migrating to 11.5.1, some of our validation has stopped working. We've narrowed it down to an issue with nested complex objects in a nested RuleForEach and ChildRules chain.

Looking at the changelogs, we haven't been able to find any mention of whether this is intentional or not, and most web searches lead to issues with SetValidator(), which isn't something we're using in our project. Porting the current rules over to individual validators would be very time consuming, so we'd like to avoid it for now, if possible.

Our problem is as follows:

Given classes with lists of complex objects that have their own lists of complex objects (and so on), nested usage of RuleForEach and ChildRules breaks when inside a RuleSet, but works otherwise.

Additionally, adding the "default" ruleset rule to the validator seems to run these child rules, even if they are only declared in a ruleset. Note that this doesn't fix our issue, because it would run all rules that are in the "default" ruleset as well, which is something we don't want.

I expect that this behavior should either work in rulesets, or not work outside of them. It shouldn't work in one case and not in the other.

I am unsure if it supposed to work in the first place, and that we've been relying on a bug to get our current validation working.

Steps to Reproduce

Code sample

Below is a trivial code example that showcases the problem:

public static class Program
{
    public class Foo
    {
        public List<string> Names { get; set; } = new();
    }

    public class Bar
    {
        public List<Foo> Foos { get; set; } = new();
    }

    public class Baz
    {
        public List<Bar> Bars { get; set; } = new();
    }

    public class WorkingValidator : AbstractValidator<Baz>
    {
        public WorkingValidator()
        {
            RuleForEach(baz => baz.Bars)
                .ChildRules(barRule => barRule.RuleForEach(bar => bar.Foos)
                    .ChildRules(fooRule => fooRule.RuleForEach(foo => foo.Names)
                        .ChildRules(name => name.RuleFor(n => n)
                            .NotEmpty()
                            .WithMessage("Name is required"))));
        }
    }

    public class BrokenValidator : AbstractValidator<Baz>
    {
        public const string RuleSetName = "Broken-Ruleset-Name";
        
        public BrokenValidator()
        {
            RuleSet(RuleSetName, () =>
            {
                RuleForEach(baz => baz.Bars)
                    .ChildRules(barRule => barRule.RuleForEach(bar => bar.Foos)
                        .ChildRules(fooRule => fooRule.RuleForEach(foo => foo.Names)
                            .ChildRules(name => name.RuleFor(n => n)
                                .NotEmpty()
                                .WithMessage("Name is required"))));
            });
        }
    }

    public static void Main()
    {
        var foos = new List<Foo>()
        {
            new() { Names = { "Bob" }},
            new() { Names = { string.Empty }},
        };

        var bars = new List<Bar>()
        {
            new(),
            new() { Foos = foos }
        };

        var baz = new Baz()
        {
            Bars = bars
        };

        var workingValidator = new WorkingValidator();
        var workingResult = workingValidator.Validate(baz);
        
        if (workingResult.IsValid)
        {
            Console.WriteLine("WorkingValidator - Valid");
        }
        else
        {
            Console.WriteLine("WorkingValidator - Invalid");
            foreach (var error in workingResult.Errors)
            {
                Console.WriteLine(error.ErrorMessage);
            }
        }
        
        var brokenValidator = new BrokenValidator();
        var brokenResult = brokenValidator.Validate(baz, options => options.IncludeRuleSets(BrokenValidator.RuleSetName));
        
        if (brokenResult.IsValid)
        {
            Console.WriteLine("BrokenValidator - Valid");
        }
        else
        {
            Console.WriteLine("BrokenValidator - Invalid");
            foreach (var error in brokenResult.Errors)
            {
                Console.WriteLine(error.ErrorMessage);
            }
        }
    }
}

Expected output (FluentValidation 11.5.1)

I would expect the following output:

WorkingValidator - Invalid
Name is required
BrokenValidator - Invalid
Name is required

Or, in the events that this behavior isn't supported:

WorkingValidator - Valid
BrokenValidator - Valid

Actual output (FluentValidation 11.5.1)

WorkingValidator - Invalid
Name is required
BrokenValidator - Valid

Original output (FluentValidation 10.3.0)

WorkingValidator - Invalid
Name is required
BrokenValidator - Invalid
Name is required
@JeremySkinner
Copy link
Member

JeremySkinner commented Apr 3, 2023

I am unsure if it supposed to work in the first place, and that we've been relying on a bug to get our current validation working.

I believe this is the case - it looks like you were relying on the broken behaviour which was fixed in #1981 as part of the 11.1.1 release. The fix for that issue was to ensure that the parent ruleset was copied to the child rules (see this code), however this only happens at the top level. Multiple nested levels of child rules isn't a use case that was considered, which is why you're running into this issue. I believe the fix is that we need to recursively find all calls to ChildRules and apply the same change there. I'll look at getting that changed, although I'm not sure what the best approach is yet.

That being said, I wouldn't recommend structuring validators like this - multiple levels of child rules like this is incredibly hard to read. Using SetValidator with a dedicated child validator is the recommended approach for handling complex validation logic for nested types.

I don't have time to work on this properly at the moment, but I'll let you know when I have a further update.

Edit: I have a prototype of a fix in #2098 but will need to take some time to work out if this is the best approach

@JeremySkinner
Copy link
Member

Fixed in 11.5.2

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants