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

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

Closed
ardove opened this issue Mar 22, 2019 · 28 comments

Comments

@ardove
Copy link

ardove commented Mar 22, 2019

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

  1. 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; }
}
  1. 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();
  1. 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;
    }
}
  1. 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.'`
@ardove ardove changed the title ValidateDataAnnotation doesn't work with nested types ValidateDataAnnotation doesn't work with properties that also have DataAnnotations on their members Mar 22, 2019
@ardove ardove changed the title ValidateDataAnnotation doesn't work with properties that also have DataAnnotations on their members ValidateDataAnnotation doesn't work when classes have properties that also have DataAnnotations on their members Mar 22, 2019
@HaoK HaoK self-assigned this Mar 27, 2019
@ajcvickers
Copy link
Member

@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.)

@vanbukin
Copy link
Contributor

vanbukin commented May 28, 2019

@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.

@ghost
Copy link

ghost commented May 8, 2020

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!

@ardove
Copy link
Author

ardove commented May 8, 2020

I'm still interested in having this issue addressed.

@ghost
Copy link

ghost commented May 8, 2020

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

@analogrelay analogrelay transferred this issue from dotnet/extensions May 8, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.ComponentModel.DataAnnotations untriaged New issue has not been triaged by the area owner labels May 8, 2020
@analogrelay analogrelay added this to the Future milestone May 8, 2020
@analogrelay analogrelay changed the title ValidateDataAnnotation doesn't work when classes have properties that also have DataAnnotations on their members [Extensions.Options] ValidateDataAnnotation doesn't work when classes have properties that also have DataAnnotations on their members May 8, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Jul 1, 2020
@maryamariyan maryamariyan added this to Uncommitted in ML, Extensions, Globalization, etc, POD. via automation Oct 5, 2020
@maryamariyan maryamariyan modified the milestones: Future, 6.0.0 Oct 5, 2020
@ghost
Copy link

ghost commented Oct 27, 2020

Somewhat related to #36391.

@jcotton42
Copy link

jcotton42 commented Dec 13, 2020

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; }
    }

@alexb5dh
Copy link

alexb5dh commented Jan 6, 2021

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.

@ardove
Copy link
Author

ardove commented Jan 6, 2021

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.

@bgillet
Copy link

bgillet commented Apr 13, 2021

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.

@maryamariyan
Copy link
Member

It would be good to address this in 7.0.

@maryamariyan maryamariyan removed this from the 6.0.0 milestone Jul 22, 2021
@CodeBlanch
Copy link
Contributor

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; }
	}

@SteveDunn
Copy link
Contributor

@maryamariyan I'm happy to look at this one

@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Apr 4, 2022
@maryamariyan maryamariyan added area-System.ComponentModel.DataAnnotations untriaged New issue has not been triaged by the area owner and removed area-Extensions-Options labels Apr 6, 2022
@ghost
Copy link

ghost commented Apr 6, 2022

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

  1. 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; }
}
  1. 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();
  1. 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;
    }
}
  1. 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>

@maryamariyan maryamariyan removed this from the 7.0.0 milestone Apr 6, 2022
@maryamariyan
Copy link
Member

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

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 9, 2022
@SteveDunn
Copy link
Contributor

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

@ajcvickers ajcvickers removed the untriaged New issue has not been triaged by the area owner label Jul 6, 2022
@ajcvickers ajcvickers added this to the Future milestone Jul 6, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 23, 2022
@jchannon
Copy link

jchannon commented Dec 6, 2022

Just hit this!!

@SteveDunn
Copy link
Contributor

Just hit this!!

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

@maryamariyan maryamariyan removed their assignment Jan 9, 2023
@tzachihakmon
Copy link

Hey, any news with solution for that issue?

@SteveDunn
Copy link
Contributor

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.

@eerhardt
Copy link
Member

eerhardt commented May 2, 2023

#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

@cremor
Copy link

cremor commented Aug 1, 2023

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.

@cremor
Copy link

cremor commented Sep 6, 2023

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

@tarekgh
Copy link
Member

tarekgh commented Sep 13, 2023

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

@tarekgh tarekgh closed this as completed Sep 13, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Oct 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.