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

Options Validation: support eager validation #459

Open
HaoK opened this Issue Aug 3, 2018 · 6 comments

Comments

Projects
None yet
4 participants
@HaoK
Member

HaoK commented Aug 3, 2018

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

@HaoK HaoK added this to the 2.2.0-preview2 milestone Aug 3, 2018

@hishamco

This comment has been minimized.

Contributor

hishamco commented Aug 5, 2018

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

@andrewlock

This comment has been minimized.

andrewlock commented Aug 5, 2018

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

This comment has been minimized.

Member

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 HaoK modified the milestones: 2.2.0-preview2, 2.2.0-preview3 Aug 27, 2018

@HaoK

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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 HaoK modified the milestones: 2.2.0-preview3, 3.0.0 Sep 17, 2018

@HaoK

This comment has been minimized.

Member

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 aspnet/AspNetCore Nov 5, 2018

@Eilon Eilon added this to the 3.0.0-preview1 milestone Nov 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment