Open niemyjski opened 8 months ago
@niemyjski any reason you can't skip recursion for the property using [SkipRecursion]
? I can't take a dependency on JSON.NET and seems odd to exclude it specifically based on string match of type name. Would be good to know if the same scenario throws in MVC/Blazor as they both have custom recursive validation too, and if it doesn't throw, why?
I think I just ran into something similar. Here is a simple repro:
using System.Text.Json;
public class TestOptions
{
public JsonSerializerOptions JsonSerializerOptions { get; set; } = new();
}
When calling MiniValidator.TryValidate(options, out var validationErrors)
, it throws an InvalidOperationException
:
System.InvalidOperationException
HResult=0x80131509
Message=Method may only be called on a Type for which Type.IsGenericParameter is true.
Source=System.Private.CoreLib
StackTrace:
at System.RuntimeType.get_DeclaringMethod() in /_/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs:line 3262
at MiniValidation.PropertyHelper.CallNullSafePropertyGetter[TDeclaringType,TValue](Func`2 getter, Object target) in /_/src/MiniValidation/PropertyHelper.cs:line 129
at MiniValidation.PropertyDetails.GetValue(Object target) in /_/src/MiniValidation/TypeDetailsCache.cs:line 268
at MiniValidation.MiniValidator.<TryValidateImpl>d__20.MoveNext() in /_/src/MiniValidation/MiniValidator.cs:line 383
at System.Runtime.CompilerServices.ConfiguredValueTaskAwaitable`1.ConfiguredValueTaskAwaiter.GetResult() in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConfiguredValueTaskAwaitable.cs:line 154
at MiniValidation.MiniValidator.<TryValidateImpl>d__20.MoveNext() in /_/src/MiniValidation/MiniValidator.cs:line 446
at System.Runtime.CompilerServices.ConfiguredValueTaskAwaitable`1.ConfiguredValueTaskAwaiter.GetResult() in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConfiguredValueTaskAwaitable.cs:line 154
at MiniValidation.MiniValidator.<TryValidateEnumerable>d__25.MoveNext() in /_/src/MiniValidation/MiniValidator.cs:line 572
at System.Runtime.CompilerServices.ValueTaskAwaiter`1.GetResult() in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ValueTaskAwaiter.cs:line 126
at MiniValidation.MiniValidator.<TryValidateEnumerable>d__24.MoveNext() in /_/src/MiniValidation/MiniValidator.cs:line 538
at System.Runtime.CompilerServices.ConfiguredValueTaskAwaitable`1.ConfiguredValueTaskAwaiter.GetResult() in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConfiguredValueTaskAwaitable.cs:line 154
at MiniValidation.MiniValidator.<TryValidateImpl>d__20.MoveNext() in /_/src/MiniValidation/MiniValidator.cs:line 439
at System.Runtime.CompilerServices.ConfiguredValueTaskAwaitable`1.ConfiguredValueTaskAwaiter.GetResult() in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConfiguredValueTaskAwaitable.cs:line 154
at MiniValidation.MiniValidator.<TryValidateImpl>d__20.MoveNext() in /_/src/MiniValidation/MiniValidator.cs:line 446
at System.Runtime.CompilerServices.ValueTaskAwaiter`1.GetResult() in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ValueTaskAwaiter.cs:line 126
at MiniValidation.MiniValidator.TryValidateImpl[TTarget](TTarget target, IServiceProvider serviceProvider, Boolean recurse, Boolean allowAsync, IDictionary`2& errors) in /_/src/MiniValidation/MiniValidator.cs:line 200
at MiniValidation.MiniValidator.TryValidate[TTarget](TTarget target, IDictionary`2& errors) in /_/src/MiniValidation/MiniValidator.cs:line 59
Note that this worked fine on Net 6, but started breaking after upgrading to Net 8.
For us, the problem with using [SkipRecursion]
is that the options class is in a separate shared nuget package, and we'd prefer not to add a dependency in that package on MiniValidation
.
I could add support for System.Runtime.Serialization.IgnoreDataMemberAttribute
for skipping members too, so you wouldn't need to take a dependency on MiniValidation
to annotate members to skip.
Can either of you determine if MVC has issues when accepting these types as input that gets validated? I'm curious what its behavior is.
Using ValidateDataAnnotations
seems to have no problem with the same options class:
services.AddOptions<TestOptions>()
.BindConfiguration("TestOptions")
.ValidateDataAnnotations()
.ValidateOnStart();
I don't think ValidateDataAnnotations
does recursive validation though. The behavior we're seeing in MiniValidation is because it traverses complex object graphs by default and supports poly-morphism of runtime types. That's why it tries to recurse into these types that throw on their getters. MVC validation is similar so if it behaves differently we can dig deeper to understand exactly what it's doing differently. If it behaves the same way then I'll have to consider introducing some behavior (maybe behind a flag) that prevents the exceptions during recursion.
Assuming I'm not missing something, the model validation seems to work normally:
using System.Text.Json;
public class TestOptions
{
public JsonSerializerOptions JsonSerializerOptions { get; set; } = new();
}
...
[HttpGet]
[Route("test")]
public IActionResult Test(TestOptions options)
{
if (!ModelState.IsValid) // this returns true
{
throw new InvalidOperationException();
}
}
I also tested again with DataAnnotations and the new ValidateObjectMembers
attribute (which enables recursive validation on a member; https://github.com/dotnet/runtime/pull/90275), and that also seems to work.
OK thanks. The new ValidateObjectMembers
and ValidateEnumeratedItems
attributes only work for options validation I believe so their existence on a model member has no effect in MVC or MiniValidator.
I'll have to dig in to what MVC is doing that results in the getter of the JsonSerialerOptions
either not being accessed or being accessed and the exception being dealt with.
If you have dynamic payloads like JsonPatch / Delta or content that may be a JToken or other Json Type MiniValidator throws quickly. Can we add some type guards or safety to continue on when an error occurs?