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

Developers can get immediate feedback on validation problems #36391

Closed
Tracked by #44314
HaoK opened this issue Aug 3, 2018 · 27 comments · Fixed by #47821
Closed
Tracked by #44314

Developers can get immediate feedback on validation problems #36391

HaoK opened this issue Aug 3, 2018 · 27 comments · Fixed by #47821
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Options Bottom Up Work Not part of a theme, epic, or user story Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@HaoK
Copy link
Member

HaoK commented Aug 3, 2018

Updated by @maryamariyan:

Goal

When an application starts, we want to get immediate feedback on validation problems. e.g. we would like to get exceptions thrown on app startup rather than later.

Benefits of eager validation:

  • Enabling this feature forces the program to crash fast when an invalid configuration is passed, as opposed to the default lazy validation only when IOptions<T> is requested

API Proposal

ValidateOnStart feature is implemented as extension to OptionsBuilder<TOptions>
To allow for eager validation, an API suggestion is to add an extension method on OptionsBuilder, (not IHost).

Usage:

services.AddOptions<MyOptions>()
    .ValidateDataAnnotations()
    .ValidateOnStart();                 // Support eager validation

APIs:

According to usage, we'd need the APIs below:

namespace Microsoft.Extensions.DependencyInjection
{
    public static class OptionsBuilderExtensions
    {
        public static OptionsBuilder<TOptions> ValidateOnStart<TOptions>(this OptionsBuilder<TOptions> optionsBuilder) where TOptions : class;
    }
}

Focus of this issue:

The focus here is on eager validation at application startup, and these APIs don't trigger for IOptionsSnapshot and IOptionsMonitor, where values may get recomputed on every request or upon configuration reload after the startup has completed.

  • It would support named options too.
IOptions<TOptions>:
  • Reads configuration data only after the app has started.
  • Does not support named options
IOptionsSnapshot<TOptions>:

May be recomputed on every request, and supports named options

IOptionsMonitor<TOptions>:

Is registered as a singleton, supports named options, change notifications, configuration reloads, and can be injected to any service lifetime.

Original Description (click to view)

AB#1244419
From exp review with @ajcvickers @DamianEdwards @Eilon @davidfowl

We should support some mechanism for eager (fail fast on startup) validation of options.

Needs to also work for generic host as well as webhost, must be configurable on a per options instance, since this will never work for request based options.

@HaoK HaoK self-assigned this Aug 3, 2018
@hishamco
Copy link
Member

hishamco commented Aug 5, 2018

I think @andrewlock did something similar for Configuration. Andrew could you please share some of your thoughts here

@andrewlock
Copy link
Contributor

I wrote a post about my approach here: https://andrewlock.net/adding-validation-to-strongly-typed-configuration-objects-in-asp-net-core/, and I have NuGet package to provide the funcitonality: https://github.com/andrewlock/NetEscapades.Configuration/blob/master/src/NetEscapades.Configuration.Validation

Essentially, I implement a simple interface in my options objects (IValidatable), and register it with the DI container. You can perform the validation any way you like (DataAnnotations, Fluent validation etc).

Then I have a StartupFilter that just calls Validate on all the settings:

public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
{
    foreach (var validatableObject in _validatableObjects)
    {
        validatableObject.Validate();
    }

    //don't alter the configuration
    return next;
}

Any exceptions will happen on app startup. There's a few caveats to this approach:

  • IStartupFilter is HTTP/middleware specific AFAIK, not generic host
  • My implementation assumes singleton settings objects that don't change. That's the way I almost invariably use them, but would require a different approach for the IOptionsMonitor approach.
  • Named options would require each named option being registered separately I guess (I don't use them)

@HaoK
Copy link
Member Author

HaoK commented Aug 5, 2018

Thanks @andrewlock I'm guessing we will end up with something similar just in a more generic host friendly way, that probably uses options itself to configure what option instances to validate at startup.

@HaoK
Copy link
Member Author

HaoK commented Aug 27, 2018

Out of time to get this in this week for preview2, will start a PR this week targeting preview3

@HaoK
Copy link
Member Author

HaoK commented Aug 30, 2018

Discussed with @ajcvickers current thinking will be to add the add a way to register startup actions which happen to do options validation.

   public class StartupOptions {
       public IEnumerable<Action> StartupActions { get; set; } = new List<Action>();
   }

   // The Validation overloads that request eager validation would register an action something like this
   services.Configure<StartupOptions, IOptionsMonitor<TOptions>>(
      (o,mon) => o.StartupActions.Add(() => mon.Get(optionsName))

   // Hosts would just need to invoke the startup actions

Thoughts @davidfowl ?

@HaoK
Copy link
Member Author

HaoK commented Sep 17, 2018

Per review with @DamianEdwards @davidfowl @ajcvickers @Eilon we are going to ship 2.2 without eager validation

The existing eager validation code in the PR will be moved into the tests and referenced/explained in the 2.2 samples/documentation so we can gather feedback before adding official support

@Eilon Eilon transferred this issue from dotnet/aspnetcore Nov 5, 2018
@JechoJekov
Copy link

I have created a small class (based on the idea by @andrewlock of using an IStartupFilter) that "extends" the OptionsConfigurationServiceCollectionExtensions.Configure method by adding validation and also supports eager validation at startup.

Usage:

public void ConfigureServices(IServiceCollection services)
{
    services.ConfigureWithDataAnnotationsValidation<MyOptions>(
        Configuration.GetSection("MyOptionsPath"), 
        validateAtStartup: true
        );
}

Class:

/// <summary>
/// Extension methods for adding configuration related options services to the DI container.
/// </summary>
public static class OptionsConfigurationServiceCollectionExtensions
{
    /// <summary>
    /// Registers a configuration instance which <typeparamref name="TOptions"/> will bind against and validates the instance
    /// on by using <see cref="DataAnnotationValidateOptions{TOptions}"/>.
    /// </summary>
    /// <typeparam name="TOptions">The type of options being configured.</typeparam>
    /// <param name="services">The <see cref="IServiceCollection"/> to add the services to.</param>
    /// <param name="config">The configuration being bound.</param>
    /// <param name="validateAtStartup">Indicates if the options should be validated during application startup.</param>
    /// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns>
    public static IServiceCollection ConfigureWithDataAnnotationsValidation<TOptions>(this IServiceCollection services, IConfiguration config, bool validateAtStartup = false) where TOptions : class
    {
        services.Configure<TOptions>(config);
        services.AddSingleton<IValidateOptions<TOptions>>(new DataAnnotationValidateOptions<TOptions>(Microsoft.Extensions.Options.Options.DefaultName));

        if (validateAtStartup)
        {
            ValidateAtStartup(services, typeof(TOptions));
        }

        return services;
    }

    /// <summary>
    /// Registers a type of options to be validated during application startup.
    /// </summary>
    /// <param name="services">The <see cref="IServiceCollection"/> to add the services to.</param>
    /// <param name="type">The type of options to validate.</param>
    static void ValidateAtStartup(IServiceCollection services, Type type)
    {
        var existingService = services.Select(x => x.ImplementationInstance).OfType<StartupOptionsValidation>().FirstOrDefault();
        if (existingService == null)
        {
            existingService = new StartupOptionsValidation();
            services.AddSingleton<IStartupFilter>(existingService);
        }

        existingService.OptionsTypes.Add(type);
    }

    /// <summary>
    /// A startup filter that validates option instances during application startup.
    /// </summary>
    class StartupOptionsValidation : IStartupFilter
    {
        IList<Type> _optionsTypes;

        /// <summary>
        /// The type of options to validate.
        /// </summary>
        public IList<Type> OptionsTypes => _optionsTypes ?? (_optionsTypes = new List<Type>());

        /// <inheritdoc/>
        public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
        {
            return builder =>
            {
                if (_optionsTypes != null)
                {
                    foreach (var optionsType in _optionsTypes)
                    {
                        var options = builder.ApplicationServices.GetService(typeof(IOptions<>).MakeGenericType(optionsType));
                        if (options != null)
                        {
                            // Retrieve the value to trigger validation
                            var optionsValue = ((IOptions<object>)options).Value;
                        }
                    }
                }

                next(builder);
            };
        }
    }
}

@JechoJekov
Copy link

The empty error message of OptionsValidationException is utterly unhelpful - at least an error message containing the number of errors and the first error would be much more helpful. As the current implementation stands unless the exception is specifically caught and the containing errors manually logged there is no way to know what the error is except that it is somewhere in the configuration. Even having the first error message will be extremely helpful as even if there are more than one errors they can be addressed one by one.

A possible implementation:

/// <summary>
/// Thrown when options validation fails.
/// </summary>
public class OptionsValidationException : Exception
{
    string _message;

    /// <inheritdoc/>
    public override string Message => _message ?? (_message = CreateExceptionMessage());

    /// <summary>
    /// Constructor.
    /// </summary>
    /// <param name="optionsName">The name of the options instance that failed.</param>
    /// <param name="optionsType">The options type that failed.</param>
    /// <param name="failureMessages">The validation failure messages.</param>
    public OptionsValidationException(string optionsName, Type optionsType, IReadOnlyCollection<string> failureMessages)
    {
        Failures = failureMessages ?? Array.Empty<string>();
        OptionsType = optionsType ?? throw new ArgumentNullException(nameof(optionsType));
        OptionsName = optionsName ?? throw new ArgumentNullException(nameof(optionsName));
    }

    /// <summary>
    /// The name of the options instance that failed.
    /// </summary>
    public string OptionsName { get; }

    /// <summary>
    /// The type of the options that failed.
    /// </summary>
    public Type OptionsType { get; }

    /// <summary>
    /// The validation failures.
    /// </summary>
    public IReadOnlyCollection<string> Failures { get; }

    /// <summary>
    /// Returns an error message.
    /// </summary>
    /// <returns></returns>
    string CreateExceptionMessage()
    {
        if (Failures.Count > 0)
        {
            return $"{Failures.Count} validation error(s) occurred while validating options of type '{OptionsType.FullName}'. The first error is: {Failures.First()}";
        }
        else
        {
            return $"One or more validation errors occurred while validating options of type '{OptionsType.FullName}'.";
        }
    }
}

@HaoK
Copy link
Member Author

HaoK commented Apr 17, 2019

No plans to add eager validation yet

@HaoK HaoK removed their assignment Sep 12, 2019
@michaelgregson
Copy link

Thank you for all the great ideas in this thread. I wanted to retain the Validate and ValidateDataAnnotations provided by OptionsBuilder but enable eager validation, the patterns above led me to:

Usage:

services.AddOptions<LoggingSettings>()
     .ValidateDataAnnotations()
     .ValidateEagerly();

Classes:

public class StartupOptionsValidation<T> : IStartupFilter
    {
        public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
        {
            return builder =>
            {
                var options = builder.ApplicationServices.GetService(typeof(IOptions<>).MakeGenericType(typeof(T)));
                if (options != null)
                {
                    // Retrieve the value to trigger validation
                    var optionsValue = ((IOptions<object>)options).Value;
                }

                next(builder);
            };
        }
    }
public static class OptionsBuilderValidationExtensions
    {
        public static OptionsBuilder<TOptions> ValidateEagerly<TOptions>(this OptionsBuilder<TOptions> optionsBuilder) where TOptions : class
        {
            optionsBuilder.Services.AddTransient<IStartupFilter, StartupOptionsValidation<TOptions>>();
            return optionsBuilder;
        }
    }

@wdolek
Copy link

wdolek commented Oct 3, 2019

Just be aware that using IStartupFilter won't work for non-host apps, consider command line application, Azure Function or AWS Lambda. (I would expect something more handy than manually resolving options from service provider)

Another thing to consider: how should be "reloadOnChange" handled? Last thing I want is to throw at runtime. With current instrumentation of IOptionsMonitor<T>.OnChange I get last valid options (good), in other hand, I'm not getting notification about changed invalid options (not so good).

@ArgTang
Copy link

ArgTang commented Oct 4, 2019

For container loads, that genericHost is perfect for, changing of config at runtime would not be a common usecase.

@connorads
Copy link

@ArgTang that is true. But there are people who are using generic host without containers (like I am at work) and it might be a common use case to change config at runtime for these people.

@analogrelay analogrelay transferred this issue from dotnet/extensions May 13, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label May 13, 2020
@analogrelay analogrelay added this to the Future milestone May 13, 2020
@ngbrown
Copy link

ngbrown commented May 19, 2020

For a background worker (console and/or service), I am already using ConfigureAppConfiguration and would like to eagerly validate options from command-line, environment settings, and appsettings.json.

Adapting the above, I did the following:

public class Program
{
    public static int Main(string[] args)
    {
        try
        {
            CreateHostBuilder(args).Build()
                .ValidateOptions<LoggingOptions>()       // new line per option to validate.
                .ValidateOptions<CredentialsOptions>()
                .Run();
            return 0;
        }
        catch (Exception ex)
        {
            Log.Fatal(ex, "Host terminated unexpectedly");
            return 1;
        }
    }

    // etc.
}
public static class OptionsBuilderValidationExtensions
{
    public static IHost ValidateOptions<T>(this IHost host)
    {
        object options = host.Services.GetService(typeof(IOptions<>).MakeGenericType(typeof(T)));
        if (options != null)
        {
            // Retrieve the value to trigger validation
            var optionsValue = ((IOptions<object>)options).Value;
        }
        return host;
    }
}

@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Jul 1, 2020
@scottbarnesbass
Copy link

Has anyone written any tests for these solutions? @michaelgregson I've implemented somehting very similar to your solution...

@maryamariyan maryamariyan added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 5, 2020
@maryamariyan maryamariyan modified the milestones: Future, 6.0.0 Oct 5, 2020
@maryamariyan
Copy link
Member

Added api-suggesion label to this issue. The best next step on this would be to prepare the API proposal, usage, etc. in the issue page here (following https://github.com/dotnet/runtime/blob/master/docs/project/api-review-process.md).

@davidfowl
Copy link
Member

I'm not a fan of this proposal (I don't think there should be an extension method on the host). Also that implementation is more complex than it needs to be.

@maryamariyan maryamariyan added the Bottom Up Work Not part of a theme, epic, or user story label Dec 4, 2020
@danmoseley
Copy link
Member

@maryamariyan can you please ensure User stories have a title in the form "XXX can YY" indicating what user experience changes. Take a look at other titles: https://themesof.net/?q=is:open%20kinds:ub%20team:Libraries Same for #44517.

@maryamariyan maryamariyan changed the title Options Validation: support eager validation Developers can get immediate feedback on validation problems Jan 21, 2021
@maryamariyan maryamariyan added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 26, 2021
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 28, 2021
@terrajobst
Copy link
Member

terrajobst commented Jan 28, 2021

Video

  • Looks good as proposed
  • ValidateAnnotationsEagerly should be ValidateDataAnnotationsEagerly to be a suffix of the existing APIs
  • Why do we need both ValidateEagerly and ValidateDataAnnotationsEagerly?
    • We concluded we only need ValidateEagerly for now
  • Is Eagerly the right term here? I think we'd prefer OnStartup Any opinions @davidfowl?
namespace Microsoft.Extensions.DependencyInjection
{
    public static class OptionsBuilderDataAnnotationsExtensions
    {
    //  Already exists:
    //  public static OptionsBuilder<TOptions> ValidateDataAnnotations<TOptions>(this OptionsBuilder<TOptions> optionsBuilder) where TOptions : class;
        public static OptionsBuilder<TOptions> ValidateOnStartup<TOptions>(this OptionsBuilder<TOptions> optionsBuilder) where TOptions : class;
    }
}

@davidfowl
Copy link
Member

Is Eagerly the right term here? I think we'd prefer OnStartup Any opinions @davidfowl?

I prefer ValidateOnStartup, though people may think it has something to do with the Startup class. What about ValidateOnStart?

@maryamariyan
Copy link
Member

maryamariyan commented Jan 28, 2021

Why do we need both ValidateEagerly and ValidateDataAnnotationsEagerly

The ValidateDataAnnotationsOnStartup would validate data annotations,

services.AddOptions<MyOptions>()
   .Configure(o => o.Boolean = false)
   .ValidateDataAnnotationsOnStartup(); // or ValidateDataAnnotationsOnStart()

would be same as calling (shorthand for):

services.AddOptions<MyOptions>()
   .Configure(o => o.Boolean = false)
   .ValidateDataAnnotations()
   .ValidateOnStartup(); // or ValidateOnStart();

but it is also possible to validate options that are specified using a delegate without data annotations, e.g.

services.AddOptions<MyOptions>()
   .Configure(o => o.Boolean = false)
   .Validate(o => o.Boolean)
   .ValidateOnStartup(); // or ValidateOnStart();

Doesn't seem like we'd need the shorthand for DataAnnotations, so the plan is to just add ValidateOnStartup (or ValidateOnStart) for now.

@HaoK
Copy link
Member Author

HaoK commented Jan 28, 2021

I like ValidateOnStart too, eagerly isn't clear exactly what that means

@terrajobst terrajobst added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-approved API was approved in API review, it can be implemented labels Feb 2, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 3, 2021
@terrajobst
Copy link
Member

terrajobst commented Feb 4, 2021

Video

  • Let's drop the new() constraint? The existing OptionsBuilderDataAnnotationsExtensions.ValidateDataAnnotations() method doesn't have it.
  • We considered existing types in that namespace but we don't think there is an appropriate home for this.
namespace Microsoft.Extensions.DependencyInjection
{
    public static class OptionsBuilderValidationExtensions
    {
        public static OptionsBuilder<TOptions> ValidateOnStart<TOptions>(this OptionsBuilder<TOptions> optionsBuilder) where TOptions : class;
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 4, 2021
@davidfowl
Copy link
Member

Let's drop the new() constraint? The existing OptionsBuilderDataAnnotationsExtensions.ValidateDataAnnotations() method doesn't have it.

👍🏾

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 17, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Options Bottom Up Work Not part of a theme, epic, or user story Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Projects
None yet
Development

Successfully merging a pull request may close this issue.