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

Could AddFluentValidationAutoValidation() take a filter parameter so we can opt in/out of automatic validtion? #2091

Closed
pgauledev opened this issue Mar 21, 2023 · 7 comments

Comments

@pgauledev
Copy link

pgauledev commented Mar 21, 2023

Is your feature request related to a problem? Please describe.

We are aware that "automatic validation" is no longer recommended.

We want to get ahead of the problem and move away from automatic validation. However, we don't want to convert all validators at once, as we have 60+ legacy validators that are making use of automatic validation. Due to the size of our app, changing over all of the validators at once is risky. We would like to continue using automatic validation for the existing (legacy) validators, but enforce the use of manual validators for any new validators that get written. Over time, we would convert the legacy validators and remove automatic validation altogether.

Describe the solution you'd like

Adding a configuration filter to the AddFluentValidationAutoValidation() method would be ideal (similar to the filter option in AddValidatorsFromAssemblyContaining()

Something like this would allow us to opt-in with a list of legacy types:

var automaticValidators = new List<Type>
{
    typeof(WeatherForecastCreateModelValidator),
    typeof(WeatherForecastUpdateModelValidator)
};
services.AddFluentValidationAutoValidation(config =>
{
    config.Filter = filter => automaticValidators.Contains(filter.ValidatorType)
});

Describe alternatives you've considered

I have been able to achieve opt-in functionality with a custom attribute, IValidatorInterceptor and IValidatorSelector. Automatic Validation will only run if the EnableFluentAutoValidationAttribute is present on the Validator. If not present, the interceptor will skip validation.

    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct)]
    public class EnableFluentAutoValidationAttribute : Attribute
    {
    }

    public class CustomValidatorInterceptor : IValidatorInterceptor
    {
        private readonly IServiceProvider _serviceProvider;
        public CustomValidatorInterceptor(IServiceProvider serviceProvider)
        {
            _serviceProvider = serviceProvider;
        }
        public ValidationResult AfterAspNetValidation(ActionContext actionContext, IValidationContext validationContext, ValidationResult result)
        {
            return result;
        }

        public IValidationContext BeforeAspNetValidation(ActionContext actionContext, IValidationContext commonContext)
        {
            var IValidatorType = typeof(IValidator<>).MakeGenericType(commonContext.InstanceToValidate.GetType());
            var validatorType = _serviceProvider.GetService(IValidatorType).GetType();
            var enableFluentAutoValidation = Attribute.GetCustomAttribute(validatorType, typeof(EnableFluentAutoValidationAttribute)) != null;
            if(enableFluentAutoValidation)
            {
                //if validator has EnableFluentAutoValidationAttribute, run the validation as normal by returning commentContext
                 return commonContext;
            }
            else
            {
                //if validator does not have this attribute, don't validate the rule
                return new ValidationContext<object>(commonContext.InstanceToValidate, commonContext.PropertyChain, new NoOpValidatorSelector());
            }

        }
    }

    public class NoOpValidatorSelector : IValidatorSelector 
    {
        public bool CanExecute(IValidationRule rule, string propertyPath, IValidationContext context)
        {
            return false;
        }
    }

In Program.cs (or Startup.cs):

services.AddFluentValidationAutoValidation();
services.AddValidatorsFromAssemblyContaining<WeatherForecastCreateModel>();
services.AddTransient<IValidatorInterceptor, CustomValidatorInterceptor>();

Attribute usage:

    [EnableFluentAutoValidation]
    public class WeatherForecastUpdateModelValidator : AbstractValidator<WeatherForecastUpdateModel>
    {
        public WeatherForecastUpdateModelValidator()
        {
            RuleFor(x => x.Temperature).NotEmpty();
            RuleFor(x => x.Details).NotEmpty();
        }
    }

Additional Context

I realize it's possible to "opt-out" of automatic validation on individual validators through CustomizeValidator(Skip=true):

public ActionResult Save([CustomizeValidator(Skip=true)] Person person) 
{
  // ...
}

However, I'm not aware of a way to make automatic validation happen only on an "opt-in" basis. The "opt-in" approach would be preferable for us because we have a large team and I want to make it obvious that automatic validation should not be used going forward. If we keep automatic validation enabled globally, we have to rely on individual contributors knowing that they should avoid using automatic validation.

I love your library by the way (been using it for 8+ years). Thanks!

@JeremySkinner
Copy link
Member

JeremySkinner commented Mar 24, 2023

Thanks for the suggestion - I'm not really keen on adding new features to auto-validation as it's essentially in an unmaintained state now, but I can see that this would be valuable for migrating away from auto validation. I'm not personally interested on working on anything related to auto validation anymore for the reasons outlined here, but if you've got the time to work on this and are able to put together a PR which adds the feature and also adds appropriate integration tests then I'd be happy to review it and merge it in.

Examples of how the integration tests work can be found in the FluentValidation.Tests.AspNetCore project (eg in MvcIntegrationTests although i'd probably create a separate test file for this). The logic for handling the filtering itself can be done as part of ShouldSkip in FluentValidationModelValidator( https://github.com/FluentValidation/FluentValidation.AspNetCore/blob/main/src/FluentValidation.AspNetCore/FluentValidationModelValidatorProvider.cs#L142)

@pgauledev
Copy link
Author

Thanks @JeremySkinner . One issue with the ShouldSkip() method seems to be "implicit required errors".

You can see this behavior if you simply hard-code ShouldSkip() to always return true:

	protected bool ShouldSkip(ModelValidationContext mvContext) {
		return true;
	}

Given the TestModel4 class:

public class TestModel4 {
	public string Surname { get; set; }
	public string Forename { get; set; }
	public string Email { get; set; }
	public DateTime DateOfBirth { get; set; }
	public string Address1 { get; set; }
}

Add the following test in MvcIntegrationTests - it will fail because DateOfBirth in TestModel4 is a non-nullable value type (DateTime), so it is returning an error for the DateOfBirth property, even if we hard-code ShouldSkip to always return true.

	[Fact]
	public async Task Should_Not_have_Errors() {
		var form = new FormData {
			{"Email", "foo"},
			{"Surname", "foo"},
			{"Forename", "foo"},
			{"DateOfBirth", null},
			{"Address1", null}
		};

		var result = await _client.GetErrors("Test4", form);

		result.Count.ShouldEqual(0);
	}

@JeremySkinner
Copy link
Member

JeremySkinner commented Mar 24, 2023

Implicit required errors are nothing to do with FluentValidation. These are generated by asp.net’s model binding process. They are generated before FluentValidation even gets a chance to run and we have no way to turn them off that I am aware of.

Model binding will always generate an error when you supply a null value for a Non-nullable property. You must make the property nullable if you want the validation layer to have an opportunity to run against them.

We do some hackery to intercept them and replace them with FV’s own messages (if an equivalent NotNull rule is defined in the validator), otherwise asp.net’s default messages will end up in modelstate instead. See the FluentValidationVisitor class which does this. (Maintaining these hacks to work abound asp.net’s behaviour is a big part of why im not interested in maintaining the asp.net integration anymore).

If you’re wanting to completely skip FluentValidation’s auto validation integration then you should allow the default asp.net messages to bubble up and handle them as required in your application rather than in FluentValidation.AspNetCore (either remove them from modelstate or make the properties nullable)

@pgauledev
Copy link
Author

Model binding will always generate an error when you supply a null value for a Non-nullable property. You must make the property nullable if you want the validation layer to have an opportunity to run against them.

That makes sense. I completely forgot about that fact. I will try to submit a PR soon.

@pgauledev
Copy link
Author

@JeremySkinner
Copy link
Member

Thanks! I’ll try and get round to reviewing it later this week

@JeremySkinner
Copy link
Member

Published FluentValidation.AspNetCore 11.3.0 with this change. Thanks for contributing!

renovate bot added a commit to orso-co/Orso.Arpa.Api that referenced this issue Apr 1, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [FluentValidation.AspNetCore](https://fluentvalidation.net/)
([source](https://togithub.com/FluentValidation/FluentValidation.AspNetCore))
| nuget | minor | `11.2.2` -> `11.3.0` |

---

### Release Notes

<details>
<summary>FluentValidation/FluentValidation.AspNetCore</summary>

###
[`v11.3.0`](https://togithub.com/FluentValidation/FluentValidation.AspNetCore/releases/tag/11.3.0)

[Compare
Source](https://togithub.com/FluentValidation/FluentValidation.AspNetCore/compare/11.2.2...11.3.0)

### Release notes

[Please read the upgrade guide if you are moving from 10.x to
11.x](https://docs.fluentvalidation.net/en/latest/upgrading-to-11.html)

#### Changes in 11.3.0

- Added `Filter` property to auto-validation configuration to optionally
include/exclude certain types from auto-validation
([FluentValidation/FluentValidation#2091)

#### Changes in 11.2.2

- Fix issue where implicit child validation could still be executed for
Record types even when disabled
([#&#8203;7](https://togithub.com/FluentValidation/FluentValidation.AspNetCore/issues/7))
- Add additional overload of `AddToModelState` that doesn't require a
prefix
([#&#8203;5](https://togithub.com/FluentValidation/FluentValidation.AspNetCore/issues/5))

#### Changes in 11.2.1

- Add deprecation warnings on various registration methods within the
call to `AddFluentValidation` (see
[FluentValidation/FluentValidation#1963)
- The dependency on the FluentValidation.DependencyInjectionExtensions
package now uses a floating version.

#### Changes in 11.2.0

- The dependency on the core FluentValidation library now uses a
floating version.

#### Changes in 11.1.3

- Deprecate the `AddFluentValidation` registration methods. These have
been replaced with `AddFluentValidationAutoValidation` and
`AddFluentValidationClientsideAdapters` (see
[FluentValidation/FluentValidation#1965)

#### Changes in 11.1.2

- Fixed `NullReferenceException` being thrown in
`AddFluentValidationClientsideAdapters` caused by a missing registration
for `IValidatorFactory`

#### Changes in 11.1.1

- Fixed `NullReferenceException` being thrown in
`AddFluentValidationClientsideAdapters`
([FluentValidation/FluentValidation#1969)

#### Changes in 11.1.0

- Added a `ToDictionary` method to `ValidationResult` (particularly
useful when working with Minimal APIs)-
- MVC auto validation: Deprecated Implicit validation of child
properties
([FluentValidation/FluentValidation#1960)
- MVC auto validation: Deprecated Implicit validation of root collection
elements
([FluentValidation/FluentValidation#1960)
- Introduce `services.AddFluentValidationAutoValidation()` and
`services.AddFluentValidationClientsideAdapters()` as replacements for
`services.AddFluentValidation()`
([FluentValidation/FluentValidation#1965)

#### Changes in 11.0

- Throw exceptions when async validator is invoked synchronously
([#&#8203;1705](https://togithub.com/FluentValidation/FluentValidation.AspNetCore/issues/1705))
- Remove deprecated
`RunDefaultMvcValidationAfterFluentValidationExecutes` option from
ASP.NET integration.

### Downloads

Binaries can be downloaded from nuget:

-
[FluentValidation.AspNetCore](http://nuget.org/packages/fluentvalidation.aspnetcore)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 10pm every weekday,every
weekend,before 5am every weekday" in timezone Europe/Berlin, Automerge -
At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/orso-co/Orso.Arpa.Api).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4yMi4xIiwidXBkYXRlZEluVmVyIjoiMzUuMjIuMSJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants