[Extensions.Options] ValidateDataAnnotation doesn't work when classes have properties that also have DataAnnotations on their members

Question

[Extensions.Options] ValidateDataAnnotation doesn't work when classes have properties that also have DataAnnotations on their members

ardove opened this issue 7 years ago · 28 comments

Describe the bug

When using ValidateDataAnnotations to validate a configuration hierarchy, you can only validate top level members of the class. Any properties that are types which also include validation are NOT validated.

Perhaps this was viewed as a design decision, but I think it'd be a lot more consistent if automatic data annotation validation worked in the same fashion as mvc model validation.

To Reproduce

Design the classes which represent the nested configuration

public class AnnotatedOptions
{
    [Required]
    public string Required { get; set; }

    [StringLength(5, ErrorMessage = "Too long.")]
    public string StringLength { get; set; }

    [Range(-5, 5, ErrorMessage = "Out of range.")]
    public int IntRange { get; set; }

    public AnnotatedOptionsSubsection AnnotatedOptionSubsection { get; set; }
}

public class AnnotatedOptionsSubsection
{
    [Range(-5, 5, ErrorMessage = "Really out of range.")]
    public int IntRange2 { get; set; }
}

Register the options

var services= new ServiceCollection();
services
     .AddOptions<AnnotatedOptions>()
     .Configure(o =>
                {
                    o.StringLength = "111111";
                    o.IntRange = 10;
                    o.AnnotatedOptionSubsection = new AnnotatedOptionsSubsection
                    {
                        IntRange2 = 10000
                    };
                })
       .ValidateDataAnnotations();

Set up a controller which retrieves the options

[Route("api/[controller]")]
[ApiController]
public class ConfigController : ControllerBase
{
    private readonly AnnotatedOptions _configuration;

    public ConfigController(IOptionsMonitor<AnnotatedOptions> configuration)
    {
        try
        {
            _configuration = configuration.CurrentValue;
        }
        catch (OptionsValidationException ex)
        {
            throw new Exception(string.Join(@"\r\n", ex.Failures), ex);
        }
    }

    // GET api/config/
    [HttpGet("{configKey}")]
    public ActionResult<AnnotatedOptions> Get()
    {
        return _configuration;
    }
}

Invoke the controller method and observe the error results

System.Exception: DataAnnotation validation failed for members Required with the error 'The Required field is required.'.
DataAnnotation validation failed for members StringLength with the error 'Too long.'.
DataAnnotation validation failed for members IntRange with the error 'Out of range.'. ---> Microsoft.Extensions.Options.OptionsValidationException: Exception of type 'Microsoft.Extensions.Options.OptionsValidationException' was thrown.
   at Microsoft.Extensions.Options.OptionsFactory`1.Create(String name)
   at Microsoft.Extensions.Options.OptionsMonitor`1.<>c__DisplayClass10_0.<Get>b__0()
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
   at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
   at System.Lazy`1.CreateValue()
   at Microsoft.Extensions.Options.OptionsCache`1.GetOrAdd(String name, Func`1 createOptions)
   at Microsoft.Extensions.Options.OptionsMonitor`1.Get(String name)
   at Microsoft.Extensions.Options.OptionsMonitor`1.get_CurrentValue()
   at PF.ClinicalEndpoint.Host.Controllers.ConfigController..ctor(IOptionsMonitor`1 configuration) in C:\dev\test\ClinicalEndpoint\src\Host\Controllers\ConfigController.cs:line 19
   --- End of inner exception stack trace ---
   at PF.ClinicalEndpoint.Host.Controllers.ConfigController..ctor(IOptionsMonitor`1 configuration) in C:\dev\test\ClinicalEndpoint\src\Host\Controllers\ConfigController.cs:line 23
   at lambda_method(Closure , IServiceProvider , Object[] )
   at Microsoft.AspNetCore.Mvc.Controllers.ControllerActivatorProvider.<>c__DisplayClass4_0.<CreateActivator>b__0(ControllerContext controllerContext)
   at Microsoft.AspNetCore.Mvc.Controllers.ControllerFactoryProvider.<>c__DisplayClass5_0.<CreateControllerFactory>g__CreateController|0(ControllerContext controllerContext)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.InvokeInnerFilterAsync()
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeNextResourceFilter()
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResourceExecutedContext context)
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeFilterPipelineAsync()
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeAsync()
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.Invoke(HttpContext httpContext)
   at Microsoft.AspNetCore.Routing.EndpointRoutingMiddleware.Invoke(HttpContext httpContext)
   at PF.Core.WebApi.Middleware.ExceptionHandlingMiddleware.InvokeAsync(HttpContext httpContext)
   at PF.Core.WebApi.Middleware.RequestLoggingMiddleware.InvokeAsync(HttpContext httpContext)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
```

### Expected behavior
Another error in the list of failures for the `IntRange2` field which reads as follows
`DataAnnotation validation failed for members IntRange2 with the error 'Really out of range.'`

Answer 1 · 2019-04-18T16:32:41.000Z

@rynowak When MVC does validation, does it attempt to drill down into composed types like is being requested here? (To me, this seems quite a dangerous approach to take, but wondering if there is a commonly used pattern for it.)

Answer 2 · 2019-05-28T18:49:51.000Z

@ajcvickers Yes, but it will do it once. It get the flat metadata and then attempts to validate it. I think we can just recursively validate options, or maybe make recursive validation optional.

Answer 3 · 2020-05-08T00:05:30.000Z

As part of the migration of components from dotnet/extensions to dotnet/runtime (aspnet/Announcements#411) we will be bulk closing some of the older issues. If you are still interested in having this issue addressed, just comment and the issue will be automatically reactivated (even if you aren't the author). When you do that, I'll page the team to come take a look. If you've moved on or workaround the issue and no longer need this change, just ignore this and the issue will be closed in 7 days.

If you know that the issue affects a package that has moved to a different repo, please consider re-opening the issue in that repo. If you're unsure, that's OK, someone from the team can help!

Answer 4 · 2020-05-08T01:02:22.000Z

I'm still interested in having this issue addressed.

Answer 5 · 2020-05-08T01:02:25.000Z

Paging @dotnet/extensions-migration ! This issue has been revived from staleness. Please take a look and route to the appropriate repository.

Answer 6 · 2020-10-27T15:47:28.000Z

Somewhat related to #36391.

Answer 7 · 2020-12-13T07:23:28.000Z

I am also having this issue.

In my case it's a Dictionary of items

    public class MinecraftOptions {
        public const string SectionName = "Minecraft";

        [Required]
        public Dictionary<string, MinecraftServer> Servers { get; set; } = null!;
    }

    public class MinecraftServer {
        [Required, MinLength(1)]
        public string Host { get; set; } = null!;

        [Required, Range(1, ushort.MaxValue)]
        public ushort RconPort { get; set; }

        [Required, MinLength(1)]
        public string RconPassword { get; set; } = null!;

        [Required, Range(1, ushort.MaxValue)]
        public ushort QueryPort { get; set; }
    }

Answer 8 · 2021-01-06T18:44:37.000Z

Found a workaround for nested objects based on similar StackOveflow question and corresponding Nuget package. It works for nested objects, arrays and dictionaries but requires some preparation:

Installing the package: Install-Package DataAnnotationsValidator or copying the code from the repository
Creating custom options validator:

public class RecursiveDataAnnotationValidateOptions<TOptions>: IValidateOptions<TOptions> where TOptions: class
{
    private readonly DataAnnotationsValidator.DataAnnotationsValidator _validator = new();

    public RecursiveDataAnnotationValidateOptions(string name) => Name = name;

    public string Name { get; }

    public ValidateOptionsResult Validate(string name, TOptions options)
    {
        if (name != Name)
            return ValidateOptionsResult.Skip;

        var results = new List<ValidationResult>();
        if (_validator.TryValidateObjectRecursive(options, results))
            return ValidateOptionsResult.Success;

        var stringResult = results.Select(validationResult =>
            $"DataAnnotation validation failed for members: '{string.Join((string?)",", (IEnumerable<string?>)validationResult.MemberNames)}' with the error: '{validationResult.ErrorMessage}'."
        ).ToList();

        return ValidateOptionsResult.Fail(stringResult);
    }
}

Creating extension method for it:

public static OptionsBuilder<T> ValidateDataAnnotationsRecursively<T>(this OptionsBuilder<T> builder) where T: class
{
    builder.Services.AddSingleton<IValidateOptions<T>>(new RecursiveDataAnnotationValidateOptions<T>(builder.Name));
    return builder;
}

Usage should be simple - just replace calls of ValidateDataAnnotations with ValidateDataAnnotationsRecursively.

Answer 9 · 2021-01-06T19:47:00.000Z

I know that it's possible to fix, but this should be part of the .NET runtime. Not an extension method I need to copy to every application I ever work on.

Answer 10 · 2021-04-13T13:20:04.000Z

I know that it's possible to fix, but this should be part of the .NET runtime. Not an extension method I need to copy to every application I ever work on.

Options is part of every applications. Some may have lots of. Nesting options is obvious in this case as it helps sorting them. It is even the purpose of the Option pattern. And adding lots of options in the DI to avoid nesting them is not a good idea because you may have a service using many of them and this will result in a method declaration with lots of parameters for each options you need instead having only one option "registry" you can navigate into.

Answer 11 · 2021-07-22T20:51:13.000Z

It would be good to address this in 7.0.

Answer 12 · 2021-07-23T10:28:38.000Z

@maryamariyan It feels very weird to me to include #36391 in release 6 but not this issue. I guess it's too late now though. An idea is to at least add to the documentation for Options Validation that it (currently) only validates fields on the top Options class, not fields in nested Options classes.

Answer 13 · 2021-12-16T10:09:43.000Z

Just came across this issue and am quite flabberghasted and annoyed that recursive validation does not take place nor is there an option to set it. It has cost me a decent chunk of time trying to figure out why my tests were failing.

I echo the request to at least document this to save others frustration time.

Another question would be why this was removed from the 6.0.0 milestone. Are inclusions in these milestones not commitments but "best effort" ?

Answer 14 · 2022-01-19T09:27:54.000Z

For Options validation, you can use this NuGet package (source and docs (GitHub)) until the release of .NET 7 (or 8, 9...).

It is compatible with .NET Standard 2.0, .NET Core 3.1, .NET 5 and .NET 6.

It provides two OptionBuilder extension methods:

ValidateDataAnnotationsRecursively: based on Microsoft.Extensions.Options.DataAnnotations, but it can even validate nested properties
ValidateOnStart: call Microsoft.Extensions.Hosting method in version 6, but custom code in previous .NET version

And a ServiceCollection extension method:

services.ConfigureAndValidate<TOptions>(configureOptions)

which is syntactic sugar for

services
    .AddOptions<TOptions>()
        .Configure(configureOptions) // Microsoft
        .ValidateDataAnnotationsRecursively() // based on Microsoft's ValidateDataAnnotations, but supports nested properties
        .ValidateOnStart() // Microsoft (.NET 6) or ValidateEagerly() in previous versions
        .Services

Answer 15 · 2022-01-19T14:42:22.000Z

I also came across this issue and would like to have seen at least a large disclaimer that this does not work as one would reasonably expect from a released (finished?) product. I agree with #36093 (comment)

I came up with my own workaround, where the only requirement is that all option types are defined in the same namespace.
It's tuned to my specific needs, but maybe it is useful to someone. 🙂

(The namespace requirement is just one way to prioritize specificity over sensitivity - matching e.g. a string is evidently wrong and will even result in a runtime exception. Finer tuned approaches could check for the presence of annotations on the members, for example.)

public static class ServiceConfiguration
{
  private static readonly MethodInfo HelperMethod =
    typeof(ServiceConfiguration).GetMethod(nameof(AddValidatedOptions), BindingFlags.Static | BindingFlags.NonPublic)
    ?? throw new InvalidOperationException("famously, this should not happen");

  public static IServiceCollection AddValidatedOptions<TOptions>(this IServiceCollection services, string sectionPath)
    where TOptions : class
  {
    var @namespace = typeof(TOptions).Namespace
                     ?? throw new ArgumentException("could not get namespace of type", nameof(TOptions));
    AddValidatedOptions<TOptions>(services, sectionPath, @namespace);
    return services;
  }

  private static void AddValidatedOptions<TOptions>(IServiceCollection services, string sectionPath, string @namespace)
    where TOptions : class
  {
    services
      .AddOptions<TOptions>()
      .BindConfiguration(sectionPath)
      .ValidateDataAnnotations()
      .ValidateOnStart();

    foreach (var pi in typeof(TOptions).GetProperties(BindingFlags.Instance | BindingFlags.Public))
    {
      bool skip = !pi.PropertyType.IsClass
                  || string.CompareOrdinal(pi.PropertyType.Namespace, @namespace) != 0;
      if (skip)
      {
        continue;
      }

      string currentPath = string.Join(':', sectionPath, pi.Name);
      HelperMethod.MakeGenericMethod(pi.PropertyType)
        .Invoke(null, new object[] { services, currentPath, @namespace });
    }
  }
}

Use as follows:

services.AddValidatedOptions<ComplexOption>("path:to:complex:option:that:is:probably:way:shorter:than:this:example");

Lastly, this is a recursive implementation, so don't go and use this with configuration files that nest ... a lot of objects. (What kind of monster would do that?)

Answer 16 · 2022-02-18T19:16:40.000Z

I could be mistaken, but I think the ValidateDataAnnotations behavior is in sync with how System.ComponentModel.DataAnnotations.Validator itself works. I'm not arguing the behavior is good, but I think creating inconsistency might be surprising.

Example...

	public class MyModel
	{
		[Required]
		public string? Id { get; set; }

		public MySubmodel? PrimarySubmodel { get; set; }

		public IEnumerable<MySubmodel>? ExtraSubmodels { get; set; }
	}

	public class MySubmodel
	{
		[Required]
		public string? Name { get; set; }
	}

	MyModel invalidModel = new()
	{
		PrimarySubmodel = new(),
		ExtraSubmodels = new[] { new MySubmodel(), new MySubmodel() }
	};

	List<ValidationResult> results = new();

	bool isValid = Validator.TryValidateObject(invalidModel, new ValidationContext(invalidModel), results);

	// isValid is false (expected) but results only contains a single error, that Id was missing.

Maybe a better solution is to add something in System.ComponentModel.DataAnnotations which could be used universally?

	public class MyModel
	{
		[Required]
		public string? Id { get; set; }

		[ValidateComplexObject] // System.ComponentModel.DataAnnotations.ValidateComplexObjectAttribute
		public MySubmodel? PrimarySubmodel { get; set; }

		[ValidateEnumerable] // System.ComponentModel.DataAnnotations.ValidateEnumerableAttribute
		public IEnumerable<MySubmodel>? ExtraSubmodels { get; set; }
	}

Answer 17 · 2022-04-01T05:33:39.000Z

@maryamariyan I'm happy to look at this one

Answer 18 · 2022-04-06T20:07:27.000Z

Tagging subscribers to this area: @ajcvickers, @bricelam, @roji
See info in area-owners.md if you want to be subscribed.

Issue Details

Describe the bug

When using ValidateDataAnnotations to validate a configuration hierarchy, you can only validate top level members of the class. Any properties that are types which also include validation are NOT validated.

Perhaps this was viewed as a design decision, but I think it'd be a lot more consistent if automatic data annotation validation worked in the same fashion as mvc model validation.

To Reproduce

Design the classes which represent the nested configuration

public class AnnotatedOptions
{
    [Required]
    public string Required { get; set; }

    [StringLength(5, ErrorMessage = "Too long.")]
    public string StringLength { get; set; }

    [Range(-5, 5, ErrorMessage = "Out of range.")]
    public int IntRange { get; set; }

    public AnnotatedOptionsSubsection AnnotatedOptionSubsection { get; set; }
}

public class AnnotatedOptionsSubsection
{
    [Range(-5, 5, ErrorMessage = "Really out of range.")]
    public int IntRange2 { get; set; }
}

Register the options

var services= new ServiceCollection();
services
     .AddOptions<AnnotatedOptions>()
     .Configure(o =>
                {
                    o.StringLength = "111111";
                    o.IntRange = 10;
                    o.AnnotatedOptionSubsection = new AnnotatedOptionsSubsection
                    {
                        IntRange2 = 10000
                    };
                })
       .ValidateDataAnnotations();

Set up a controller which retrieves the options

[Route("api/[controller]")]
[ApiController]
public class ConfigController : ControllerBase
{
    private readonly AnnotatedOptions _configuration;

    public ConfigController(IOptionsMonitor<AnnotatedOptions> configuration)
    {
        try
        {
            _configuration = configuration.CurrentValue;
        }
        catch (OptionsValidationException ex)
        {
            throw new Exception(string.Join(@"\r\n", ex.Failures), ex);
        }
    }

    // GET api/config/
    [HttpGet("{configKey}")]
    public ActionResult<AnnotatedOptions> Get()
    {
        return _configuration;
    }
}

Invoke the controller method and observe the error results

System.Exception: DataAnnotation validation failed for members Required with the error 'The Required field is required.'.
DataAnnotation validation failed for members StringLength with the error 'Too long.'.
DataAnnotation validation failed for members IntRange with the error 'Out of range.'. ---> Microsoft.Extensions.Options.OptionsValidationException: Exception of type 'Microsoft.Extensions.Options.OptionsValidationException' was thrown.
   at Microsoft.Extensions.Options.OptionsFactory`1.Create(String name)
   at Microsoft.Extensions.Options.OptionsMonitor`1.<>c__DisplayClass10_0.<Get>b__0()
   at System.Lazy`1.ViaFactory(LazyThreadSafetyMode mode)
   at System.Lazy`1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
   at System.Lazy`1.CreateValue()
   at Microsoft.Extensions.Options.OptionsCache`1.GetOrAdd(String name, Func`1 createOptions)
   at Microsoft.Extensions.Options.OptionsMonitor`1.Get(String name)
   at Microsoft.Extensions.Options.OptionsMonitor`1.get_CurrentValue()
   at PF.ClinicalEndpoint.Host.Controllers.ConfigController..ctor(IOptionsMonitor`1 configuration) in C:\dev\test\ClinicalEndpoint\src\Host\Controllers\ConfigController.cs:line 19
   --- End of inner exception stack trace ---
   at PF.ClinicalEndpoint.Host.Controllers.ConfigController..ctor(IOptionsMonitor`1 configuration) in C:\dev\test\ClinicalEndpoint\src\Host\Controllers\ConfigController.cs:line 23
   at lambda_method(Closure , IServiceProvider , Object[] )
   at Microsoft.AspNetCore.Mvc.Controllers.ControllerActivatorProvider.<>c__DisplayClass4_0.<CreateActivator>b__0(ControllerContext controllerContext)
   at Microsoft.AspNetCore.Mvc.Controllers.ControllerFactoryProvider.<>c__DisplayClass5_0.<CreateControllerFactory>g__CreateController|0(ControllerContext controllerContext)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Internal.ControllerActionInvoker.InvokeInnerFilterAsync()
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeNextResourceFilter()
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResourceExecutedContext context)
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeFilterPipelineAsync()
   at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeAsync()
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.Invoke(HttpContext httpContext)
   at Microsoft.AspNetCore.Routing.EndpointRoutingMiddleware.Invoke(HttpContext httpContext)
   at PF.Core.WebApi.Middleware.ExceptionHandlingMiddleware.InvokeAsync(HttpContext httpContext)
   at PF.Core.WebApi.Middleware.RequestLoggingMiddleware.InvokeAsync(HttpContext httpContext)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
```

### Expected behavior
Another error in the list of failures for the `IntRange2` field which reads as follows
`DataAnnotation validation failed for members IntRange2 with the error 'Really out of range.'`


<table>
  <tr>
    <th align="left">Author:</th>
    <td>ardove</td>
  </tr>
  <tr>
    <th align="left">Assignees:</th>
    <td>maryamariyan</td>
  </tr>
  <tr>
    <th align="left">Labels:</th>
    <td>

`area-System.ComponentModel.DataAnnotations`, `untriaged`, `feature-request`

</td>
  </tr>
  <tr>
    <th align="left">Milestone:</th>
    <td>7.0.0</td>
  </tr>
</table>
</details>

Answer 19 · 2022-04-06T20:08:36.000Z

To support this scenario a fix needs to be made lower down in System.ComponentModel.DataAnnotations. relabeling issue to reflect that. For more information refer to the linked closed PR: #67548

Answer 20 · 2022-05-16T04:39:46.000Z

The linked PR addresses this issue but has been dormant for a while. It also seems that fixing this might be a 'breaking change'...

Answer 21 · 2022-12-06T14:28:50.000Z

Just hit this!!

Answer 22 · 2022-12-07T22:23:51.000Z

Just hit this!!

@jchannon - it'd be nice to mark a start on whatever this one turns out to be

Answer 23 · 2023-02-25T11:49:26.000Z

Hey, any news with solution for that issue?

Answer 24 · 2023-02-26T07:39:51.000Z

I'm still keen to work on this. I don't know if a decision has yet been made on which form this validation should take, and where it should live.

Answer 25 · 2023-05-02T18:21:46.000Z

#85475 is adding a ValidateObjectMembersAttribute which would handle this case. There is a question about whether that attribute should live in System.ComponentModel.DataAnnotations or in Microsoft.Extensions.Options.

cc @jeffhandley @DamianEdwards @tarekgh

Answer 26 · 2023-08-01T08:06:11.000Z

I agree, it would be even more confusing in .NET 8 if ValidateObjectMembersAttribute exists but only works for the new source generator way of validation.

Answer 27 · 2023-09-06T19:46:22.000Z

It seems like this will be fixed in .NET 8 with the new attribute ValidateObjectMembersAttribute, see #90275

Answer 28 · 2023-09-13T19:08:02.000Z

Closing this as completed by #90275. Thanks @cremor for noticing that.