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

Eager options validation only validates the last named option #51171

Closed
aelij opened this issue Apr 13, 2021 · 4 comments · Fixed by #55922
Closed

Eager options validation only validates the last named option #51171

aelij opened this issue Apr 13, 2021 · 4 comments · Fixed by #55922

Comments

@aelij
Copy link
Contributor

aelij commented Apr 13, 2021

Description

Repro:

using System;
using System.Threading.Tasks;
using Microsoft.Extensions.Options;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.DependencyInjection;
using System.ComponentModel.DataAnnotations;

namespace Test
{
    class Program
    {
        static Task Main() => Host.CreateDefaultBuilder().ConfigureServices(s => 
            s.AddOptions<M>().ValidateOnStart().Validate(m => m.A < 0, "A should be negative").Services
            .AddOptions<M>("y").ValidateOnStart().Validate(m => m.A > 0, "A should be positive")).RunConsoleAsync();
    }

    class M
    {
        public int A { get; set; }
        [Required]
        public string S { get; set; }
    }
}

Expected:
AggregateException with both "A should be negative" and "A should be positive" exceptions

Actual:
Only the last validation runs

Unhandled exception. Microsoft.Extensions.Options.OptionsValidationException: A should be positive
   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 System.Lazy`1.get_Value()
   at Microsoft.Extensions.Options.OptionsCache`1.GetOrAdd(String name, Func`1 createOptions)
   at Microsoft.Extensions.Options.OptionsMonitor`1.Get(String name)
   at Microsoft.Extensions.DependencyInjection.OptionsBuilderExtensions.<>c__DisplayClass0_1`1.<ValidateOnStart>b__1()
   at Microsoft.Extensions.DependencyInjection.ValidationHostedService.StartAsync(CancellationToken cancellationToken)

I think the dictionary that stores the validation actions should have a tuple key:

vo.Validators[typeof(TOptions)] = () => options.Get(optionsBuilder.Name);

Should be something like:

vo.Validators[(typeof(TOptions), optionsBuilder.Name)] = () => options.Get(optionsBuilder.Name);

Configuration

6.0.100-preview.3.21202.5 / Linux (WSL)

@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-Options untriaged New issue has not been triaged by the area owner labels Apr 13, 2021
@ghost
Copy link

ghost commented Apr 13, 2021

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Repro:

using System;
using System.Threading.Tasks;
using Microsoft.Extensions.Options;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.DependencyInjection;
using System.ComponentModel.DataAnnotations;

namespace Test
{
    class Program
    {
        static Task Main() => Host.CreateDefaultBuilder().ConfigureServices(s => 
            s.AddOptions<M>().ValidateOnStart().Validate(m => m.A < 0, "A should be negative").Services
            .AddOptions<M>("y").ValidateOnStart().Validate(m => m.A > 0, "A should be positive")).RunConsoleAsync();
    }

    class M
    {
        public int A { get; set; }
        [Required]
        public string S { get; set; }
    }
}

Expected:
AggregateException with both "A should be negative" and "A should be positive" exceptions

Actual:
Only the last validation runs

Unhandled exception. Microsoft.Extensions.Options.OptionsValidationException: A should be positive
   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 System.Lazy`1.get_Value()
   at Microsoft.Extensions.Options.OptionsCache`1.GetOrAdd(String name, Func`1 createOptions)
   at Microsoft.Extensions.Options.OptionsMonitor`1.Get(String name)
   at Microsoft.Extensions.DependencyInjection.OptionsBuilderExtensions.<>c__DisplayClass0_1`1.<ValidateOnStart>b__1()
   at Microsoft.Extensions.DependencyInjection.ValidationHostedService.StartAsync(CancellationToken cancellationToken)

I think the dictionary that stores the validation actions should have a tuple key:

vo.Validators[typeof(TOptions)] = () => options.Get(optionsBuilder.Name);

Should be something like:

vo.Validators[(typeof(TOptions), optionsBuilder.Name)] = () => options.Get(optionsBuilder.Name);

Configuration

6.0.100-preview.3.21202.5 / Linux (WSL)

Author: aelij
Assignees: -
Labels:

area-Extensions-Options, untriaged

Milestone: -

@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Apr 13, 2021
@maryamariyan maryamariyan added this to the 6.0.0 milestone Apr 13, 2021
@davidfowl
Copy link
Member

Yes that looks right.

@xakep139
Copy link
Contributor

xakep139 commented Jul 19, 2021

@davidfowl, could you please clarify? Current behavior or proposed one looks right?

@davidfowl
Copy link
Member

Proposed

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 19, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants