Runtime: services.ConfigureOptions<T> does not register validations

[loic-sharma]

Description

The OptionsServiceCollectionExtensions.ConfigureOptions method is used to automatically register I[Post]ConfigureOptions. It would be convenient if it also registered IValidateOptions.

For example, say I have this class:

public class ConfigureAppOptions : IConfigureOptions<AppOptions>, IValidateOptions<AppOptions>
{
  public void Configure(AppOptions options)
  {
    options.Foo = "Bar";
  }

  public ValidateOptionsResult Validate(string name, AppOptions options)
  {
    if (options.Foo != "Bar")
    {
      return ValidateOptionsResult.Fail("I find your lack of foo bar disturbing");
    }

    return ValidateOptions.Success;
  }
}

Now I register it in my dependency injection container:

services.ConfigureOptions<ConfigureAppOptions>();

When I create AppOptions, the Configure method will run as expected... however, my Validate method won't! 😢

Workaround

To fix this, I need to register my validation manually:

services.ConfigureOptions<ConfigureAppOptions>();
services.AddTransient<IValidateOptions<AppOptions>, ConfigureAppOptions>());

Real world example

See:

• https://github.com/loic-sharma/BaGet/blob/872fd2e92aaf055eb144975a8572b5337cc45f27/src/BaGet/Startup.cs#L30-L41
• https://github.com/loic-sharma/BaGet/blob/872fd2e92aaf055eb144975a8572b5337cc45f27/src/BaGet/ConfigureBaGetOptions.cs#L17-L22

Fix

The FindIConfigureOptions method will likely need to be updated:

private static IEnumerable<Type> FindIConfigureOptions(Type type)
{
    IEnumerable<Type> serviceTypes = type.GetTypeInfo().ImplementedInterfaces
        .Where(t => t.GetTypeInfo().IsGenericType &&
        (t.GetGenericTypeDefinition() == typeof(IConfigureOptions<>)
        || t.GetGenericTypeDefinition() == typeof(IPostConfigureOptions<>)
+       || t.GetGenericTypeDefinition() == typeof(IValidateOptions<>)));
    if (!serviceTypes.Any())
    {
        throw new InvalidOperationException(
            IsAction(type)
            ? SR.Error_NoIConfigureOptionsAndAction
            : SR.Error_NoIConfigureOptions);
    }
    return serviceTypes;
}

Likewise, the error strings will likely need to be updated (link).

I'd be happy to contribute this fix if y'all like it.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[loic-sharma]

/cc @HaoK who added options validations in https://github.com/aspnet/Options/pull/266.

[HaoK]

Yeah for sure, that seems like a great idea to me, my team doesn't own this feature area anymore, but your proposed fix is definitely what the behavior should be

[HaoK]

The only small issue I have is naming of the method is a ConfigureOptions, but since this one is doing reflection magic and currently takes care of IPostConfigure anyways, it still seems reasonable to have it also do IValidateOptions

[loic-sharma]

The only small issue I have is naming of the method is a ConfigureOptions, but since this one is doing reflection magic and currently takes care of IPostConfigure anyways, it still seems reasonable to have it also do IValidateOptions

Agreed. Here are some options:

1. Update ConfigureOptions to register IValidateOptions. This may break customer's apps that already call ConfigureOptions on a type that also implements IValidateOptions<>. It's not particularly clear from the name whether this will register validations, but, I think updating ConfigureOptions's comment comment could help alleviate this.
2. Don't register validations, but, update the comment on ConfigureOptions to specify that validations will not be registered.
3. Add an overload to ConfigureOptions to specify whether we should register validations. Something like ConfigureServices<T>(addValidations: true);. This may confuse users that assume that ConfigureOptions will register validations by default.
4. Add a new method like ConfigureAndValidateOptions or Configure. This may confuse users that assumeConfigureOptions` will register validations by default.
5. Rename ConfigureOptions. I'm guessing this is not an option.

Personally I prefer option 1, followed by option 2. Do you agree? Do you have any other suggestions? Should we review this proposal with the new stakeholders, and if so, who are they? Thanks for your help 😊

[HaoK]

Right, I think option 1 is probably okay, to just update the comments and explicitly state validations will be registered by ConfigureOptions, this is 5.0 so not the worst time to introduce a small breaking change. I find it hard to think of many situations where someone would implement IValidateOptions on this class and not want them registered, especially since specific named instances can targeted

[HaoK]

I think its best to just wait for the normal triage process for the next steps, but never hurts to see if @davidfowl agrees with our thinking

[HaoK]

BTW just as an FYI, you can also do your registration via OptionsBuilder without the class

  services.AddOptions<AppOptions>
     .Configure(options => options.Foo = "Bar")
     .Validate(name, options => {
        if (options.Foo != "Bar")
        {
          return false;
        }
        return true
     }, "I find your lack of foo bar disturbing");

[loic-sharma]

I sent out a pull request with the current proposal: https://github.com/dotnet/runtime/pull/38513. Please let me know if you have any feedback!

[msftbot[bot]]

Tagging subscribers to this area: @maryamariyan
Notify danmosemsft if you want to be subscribed.

[maryamariyan]

@HaoK thanks for the feedback thread.

[loic-sharma]

@maryamariyan is the up-for-grabs label correct? The PR for this issue has been merged already: https://github.com/dotnet/runtime/pull/38513

[eriawan]

@maryamariyan
the PR that fixes this issue has been merged: #38513

Could we close this issue?

[maryamariyan]

the PR that fixes this issue has been merged: #38513. Could we close this issue?

Sure. Closing as it's already fixed.