Blazored / FluentValidation

A library for using FluentValidation with Blazor
https://blazored.github.io/FluentValidation/
MIT License
586 stars 84 forks source link

[Bug] Validation is not correct when validating items in a nested dictionary #124

Open srollinet opened 2 years ago

srollinet commented 2 years ago

Describe the bug When validating items in a nested dictionary, the validation result is true, even when items are not valid

To Reproduce I made a repro unit test here: https://github.com/srollinet/BlazoredFluentValidation/blob/DictionaryValidationIssue/Blazored.FluentValidation.Tests/NestedDictionaryTest.cs

just clone the repo and execute the test.

Expected behavior Validation result should be same as manually performed validation (valid=false)

Sharaf-Mansour commented 2 years ago

@srollinet are you sure you are using Blazored.FluentValidation Nuget Package ? and if so please downgrade Package version to 2.0.1.

chrissainty commented 2 years ago

@srollinet There is a flaw in your test. On this line:

https://github.com/srollinet/BlazoredFluentValidation/blob/29e545c4321a76396e8b3cc21294a0133ee08623/Blazored.FluentValidation.Tests/NestedDictionaryTest.cs#L46

You're passing in null to the final argument, but that needs to be an instance of a FluentValidationValidator component.

However, with that said, there is definitely an issue with dictionaries.

srollinet commented 2 years ago

@chrissainty Yes, in fact I modified the method signature (temporarily) to make it testable this way. Normally it won't impact the result. In the original method, you are passing an instance of FluentValidationValidator, only to access its Options property. But it defaults to opt => opt.IncludeAllRuleSets() when Options is null.

https://github.com/srollinet/BlazoredFluentValidation/commit/29e545c4321a76396e8b3cc21294a0133ee08623#diff-e15a1de63ffa32103f6a5667b0df65a9585ed7ddb422abb99dbb9c8a14f350ae

Probably it would have been better to not change the signature and make it nullable by adding a null propagation operator at line 48

Also, while digging into the code, I realized that only one validator is used, whereas you can have multiple validators registered in the DI. Is this on purpose? If not, it probably deserves its own issue.

chrissainty commented 2 years ago

@srollinet I've been working on this recently, but I'm struggling to get something working. Is this scenario something you've used previously with Fluent Validation on the server?

srollinet commented 2 years ago

@chrissainty It is the first time I have this scenario with FluentValidation, And I think it is quite uncommon.

Finally, I switched to a custom implementation based on https://github.com/ryanelian/FluentValidation.Blazor, which have a correct validation result, but I think there are still some minor glitches with this scenario.

For special cases like this, maybe just ensure that the overall validation result is consistent with a manual validation. This was my main issue. I was able to send invalid data because it was considered valid in the UI. But it was rejected later by the "server-side" validation (in this case, it is Blazor server, so I put server-side in quotes)