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

add support for registering internal validator types through FluentValidationMvcConfiguration #1748

Merged

Conversation

RyanMarcotte
Copy link
Contributor

@RyanMarcotte RyanMarcotte commented May 30, 2021

The documentation mentions being able to register internal validator types using the following syntax:

services
    .AddMvc()
    .AddFluentValidation(fv => fv.RegisterValidatorsFromAssemblyContaining<PersonValidator>(includeInternalTypes: true))

... but this is incorrect. The pull request introducing the opt-in behavior modified the IServiceCollection extension methods and not any of the RegisterValidators... methods defined by FluentValidationMvcConfiguration. This PR adds a new IncludeInternalValidatorTypes flag to FluentValidationMvcConfiguration and that value will be passed to the IServiceCollection extension method call made later (since FluentValidationMvcConfiguration caches a collection of assemblies and all validators from all assemblies are registered at once).

I also added a useful WithPropertyName method to PropertyChain to allow for fluent declaration of property chains: validationContext.PropertyChain.WithPropertyName(validationContext.PropertyName).WithPropertyName("OtherProperty")

…lidationMvcConfiguration; add useful WithPropertyName method to PropertyChain
@dnfadmin
Copy link
Collaborator

dnfadmin commented May 30, 2021

CLA assistant check
All CLA requirements met.

@JeremySkinner
Copy link
Member

Hi, thanks for the suggestions. Please could you amend the PR to make the following changes:

  • includeInternalValidatorTypes should be a parameter of the Register... methods, not a property of FluentValidationMvcConfiguration. Please could you change this to be an optional parameter, to mirror the service collection methods.
  • Undo the documentation changes to match
  • Remove the WithPropertyName change from here (let's keep the PR focused on a single issue please!)

If you could make those changes I'll get this merged in

Thanks!

@RyanMarcotte
Copy link
Contributor Author

RyanMarcotte commented May 30, 2021

includeInternalValidatorTypes should be a parameter of the Register... methods, not a property of FluentValidationMvcConfiguration. Please could you change this to be an optional parameter, to mirror the service collection methods.

Just to confirm, you are suggesting that FluentValidationMvcConfiguration be modified such that AssembliesToRegister caches a set of (Assembly, bool) tuples (or some internal class containing that information) because each individual assembly needs to respect the setting that was chosen for that specific Register... call?

@JeremySkinner
Copy link
Member

JeremySkinner commented May 30, 2021

Yes that seems like a reasonable approach. The existing AssembliesToRegister property could be changed from a List<Assembly> to a List<(Assembly Assembly, bool InclueInternal)>

@RyanMarcotte
Copy link
Contributor Author

RyanMarcotte commented May 30, 2021

That seems inconsistent with both ServiceLifetime and TypeFilter, properties already defined by FluentValidationMvcConfiguration.

Based on the documentation, it looks like the intent is for consumers of the library to call one of the Register... methods, not several.

@JeremySkinner
Copy link
Member

Hm yes you're right, let's keep it consistent.

Stick with using a single IncludeInternalValidatorTypes on the class, but make it internal and have it set via the an includeInternalValidatorTypes optional parameter on the method in the same way as the TypeFilter and ServiceLifetime. (Typically only one of the Register methods is called in startup)

@RyanMarcotte
Copy link
Contributor Author

Done!

@JeremySkinner
Copy link
Member

Thanks! I'll try and get a patch release done later today that includes this.

@JeremySkinner JeremySkinner added this to the 10.2.1 milestone May 30, 2021
@JeremySkinner JeremySkinner merged commit 17620dc into FluentValidation:main May 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants