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

[Bug] All RuleSets firing, even though only one specific in component options #180

Closed
Jaffacakes82 opened this issue Feb 8, 2023 · 6 comments · Fixed by #205
Closed
Labels
Bug Something isn't working

Comments

@Jaffacakes82
Copy link

Describe the bug
I am specifying an explicit RuleSet to include when using the FluentValidationValidator component in my component, e.g.
<FluentValidationValidator Options="@(options => options.IncludeRuleSets(CreateUserModelValidator.ModifyUserRuleSet))" @ref="fluentValidationValidator" />. However, all RuleSets are firing which is printing duplicate validation messages on the screen.

NOTE: I am not explicitly calling Validate(), Fluent Validation seems to be checking for valid fields when those fields are updated.

Expected behavior
Only the specified RuleSet is executed when updating any fields.

Screenshots
Duplicate validation messages:
image

Redacated validator class with RuleSets:

RuleSet(CreateUserRuleSet, () =>
{
    ...
    When(u => u.LoginEnabled && RequiresNPI(u), () =>
    {
        RuleFor(x => x.NPI)
            ...
            .Matches(new Regex("\\d{10}"))
                .WithMessage("NPIs must be 10 digit numbers");
    });
});

RuleSet(ModifyUserRuleSet, () =>
{
  When(u => u.LoginEnabled && RequiresNPI(u), () =>
  {
      RuleFor(x => x.NPI)
          ...
          .Matches(new Regex("\\d{10}"))
              .WithMessage("NPIs must be 10 digit numbers")
  });
});

RuleSet(ManualValidateNPIRuleSet, () =>
{
    RuleFor(p => p.NPI)
        ...
        .Matches(new Regex("\\d{10}"))
            .WithMessage("NPIs must be 10 digit numbers");
});

Hosting Model (is this issue happening with a certain hosting model?):

  • Blazor Server
@Jaffacakes82 Jaffacakes82 added Bug Something isn't working Triage Issue needs to be triaged labels Feb 8, 2023
@UniMichael
Copy link

We're seeing the same thing here. Updating to the newest version didn't fix the problem. It's as if all rulesets were firing whenever we updated an input's contents.

@UniMichael
Copy link

Update on this (because it's a pretty big problem on our end):

I think I may have found the issue.

ValidateField and ValidateModel are implemented differently. ValidateField seems to ignore options altogether, and neither pass a PropertyChain to their contexts along with options, so the validation doesn't work fully.

@pit1188
Copy link

pit1188 commented Apr 12, 2023

I have the same issue.
I saw internal implementation of FluentValidation.ValidationStrategy and they do create IValidatorSelector differently:


private IValidatorSelector GetSelector() {
		IValidatorSelector selector = null;

		if (_properties != null || _ruleSets != null || _customSelector != null) {
			var selectors = new List<IValidatorSelector>(3);

			if (_customSelector != null) {
				selectors.Add(_customSelector);
			}

			if (_properties != null) {
				selectors.Add(ValidatorOptions.Global.ValidatorSelectors.MemberNameValidatorSelectorFactory(_properties.ToArray()));
			}

			if (_ruleSets != null) {
				selectors.Add(ValidatorOptions.Global.ValidatorSelectors.RulesetValidatorSelectorFactory(_ruleSets.ToArray()));
			}

			selector = selectors.Count == 1 ? selectors[0] : ValidatorOptions.Global.ValidatorSelectors.CompositeValidatorSelectorFactory(selectors);
		}
		else {
			selector = ValidatorOptions.Global.ValidatorSelectors.DefaultValidatorSelectorFactory();
		}

		return selector;
	}

is it possible to implement your ValidateField method something like this?

@matthetherington
Copy link

matthetherington commented Dec 5, 2023

Hi

I just ran into this same issue and managed to get it working by tweaking the Blazored.FluentValidation field-level validation implementation. I'll PR it and hopefully get it into the main branch, but in the meantime, if anyone else has the same issue - you can try these.

See #205 #204

@UniMichael
Copy link

Back in March, when I looked into this, the issue was that FluentValidation didn't expose enough information for ValidateField to be able to work as intended.

Admittedly, this may have changed since then (I don't know, we fixed it and moved on), but taking a look at their release page, they don't seem to have added this functionality since.

Unfortunately, our solution was to drop this library and write our own validation component that:

  • Stores the current validation errors
  • Validates the entire model when a field changes
  • Filters-out all the new errors that don't match the target field (so existing errors were untouched)

We found a way to map the FluentValidation paths (which are string paths) to what Blazor's EditContext expects (FieldIdentifiers) by using a modified version of Steven Sanderson's converter. I say modified because it used to be on this blog post according to our Git history, but I can't seem to find it there anymore. We modified it by simplifying some of the code that was in the blog post and fixing a handful of warnings about potential null reference exceptions.

There a few downsides with this, of course. We're now maintaining our own validation library (though this one doesn't seem to have had a release in a while, so that may end-up being good for us). By validating the whole model, we run into the risk of having slow validation code trigger when an unrelated field changes, though this should only happen if we did something silly like have some long-running async code in our validation, which we don't do (we've forbidden async validation altogether through code reviews).

@matthetherington
Copy link

matthetherington commented Dec 6, 2023

Hi @UniMichael

I've put together a PR which sounds like it's doing something similar to what you described (though the reverse) - would you mind weighing in over at #205 ?

I'm curious if it also helps with your use case

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
Development

Successfully merging a pull request may close this issue.

5 participants