benmccallum / fairybread

Input validation for HotChocolate
MIT License
116 stars 13 forks source link

Same validation error repeated in response #60

Closed drewbj21 closed 3 years ago

drewbj21 commented 3 years ago

I'm not sure what triggered this behavior as it wasn't happening when I first set up FairyBread/FluentValidation... When a validation error is thrown for a single property or multiple, it is returned in the response 20+ times. We are using custom validators for some of the properties so I am to step through some of the validation and can see that same validator is being hit multiple times for the same property.

Here's our set up for this validation:

Startup (ConfigureServices):

services.AddValidatorsFromAssemblyContaining();

services .AddGraphQLServer() .AddFairyBread() ...

Validation rule within CreateFormInputValidator:

RuleFor(e => e.Description).SetValidator(new ValidText(100)).When(e => e.Description != null);

This issue is also happening for properties that are using FluentValidation's validators like NotEmpty. I'm not sure if this is an issue with FluentValidation, the communication between FluentValidation and FairyBread, or how we have validation set up. Let me know if you need more information. Thanks!

Versions:

benmccallum commented 3 years ago

Hey @drewbj21, thanks for the detailed report, sorry it's taken me so long to get back to you, I've just come back from holidays.

I can't see this behaviour with those versions in place, but I have a feeling you might be registering your validators multiple times.

Can you show me how you register them?

Don't forget that AddValidatorsFromAssemblyContaining<TValidator>(); only needs to be called once for all validators in the assembly of TValidator. e.g. don't do the following if 1 2 and 3 are all in the same assembly.

services.AddValidatorsFromAssemblyContaining<CustomValidator1>();
services.AddValidatorsFromAssemblyContaining<CustomValidator2>();
services.AddValidatorsFromAssemblyContaining<CustomValidator3>();
drewbj21 commented 3 years ago

No worries, thanks for looking into this.

It looks like how we are registering validators might be the issue... For most of our calls we have an input object and a validator associated with the input object (see below):

public class CreateTagInput { [GraphQLDescription("50 character limit")] public string TagName { get; set; } }

`public class CreateTagInputValidator : AbstractValidator<CreateTagInput>
{
    public CreateTagInputValidator()
    {
        RuleFor(e => e.TagName).NotEmpty().WithErrorCode("6000").SetValidator(new ValidText(50));
    }
}`

Then in our startup class we register each validator:

services.AddValidatorsFromAssemblyContaining<CreateTagInputValidator>();

We have about 70 different validator classes set up this way. Is this incorrect? If so, how should we be implementing validators? We've had the same set up since HC v10 and FairyBread v3 which worked perfectly then. I'm assuming something has changed...

benmccallum commented 3 years ago

Yea the problem will definitely be that you're doing this for every validator: services.AddValidatorsFromAssemblyContaining<CreateTagInputValidator>();

You should just call that once per assembly you have that contains validators using a type in that assembly.

For instance, if you only had one project, your web app, you could just do: services.AddValidatorsFromAssemblyContaining<Program>();

This would register all validators in the project in one go.

If you wanted to add them individually, you'd call AddTransient per validator as per FluentValidation's docs here. Update: You actually need to add it as the interface impl and as itself, as the helper method does for FV here.

We've had the same set up since HC v10 and FairyBread v3 which worked perfectly then. I'm assuming something has changed...

That does sound a bit weird, maybe somehow it wasn't an issue in earlier versions, but I can't see why not. Maybe it became an issue in FluentValidation v10 even 🤷🏻‍♂️

I could potentially have an option that you could configure that checks for dual registrations of the same validator, this isn't the first time someone has had this problem, but it'd also be an issue for FV users, so not sure :P

Going to close this, shout out if you're still having issues after applying the suggestion above.

drewbj21 commented 3 years ago

Thanks for looking into this. I forgot to let you and anybody else that looks at this that the suggested fix did work. That was a misunderstanding on my part. Thanks again for the info!