Add flexibility to AddStackExchangeRedisCache by removing setupAction

[tebeco]

Background and Motivation

If you're declaring your own Configuration section like this:

{
  "MyOwnRedis": {
    "DefaultLease": "00:10:00",
    "KeyPrefix": "my-app"
    "ConnectionString": "this is the actual connection string for RedisCacheOptions"
  }
}

and thus doing:

public class MyOwnRedisOptions
{
  public const string SectionName = "MyOwnRedis";

  public TimeSpan DefaultLease { get; set; }
  public string KeyPrefix { get; set; } = default!;
  public string ConnectionString { get; set; } = default!;
}
services
  .AddOptions<MyOwnRedisOptions>
  .BindConfiguration(MyOwnRedisOptions.SectionName)
  .Validate(options => !string.IsNullOrWhiteSpace(options.ConnectionString), $"The configuration key for {MyOwnRedisOptions.SectionName}:{nameof(MyOwnRedisOptions.ConnectionString)} cannot be null or  whitespace.")
  .Validate(options => options...............);
  //...

the moment you're trying to call AddStackExchangeRedisCache you're ending up in a weird situation:

services.AddStackExchangeRedisCache(options =>
{
  option.Connection = builders.Configuration.Get<string>($"{MyOwnRedisOptions.SectionName}:{nameof(MyOwnRedisOptions.ConnectionString)})
};

or
// this has the benefits or still running your Options Validation entiely

services.AddStackExchangeRedisCache(_ => {}) // this is super weird
services
  .AddOptions<RedisCacheOptions>()
  .Configure<IOptions<MyOwnRedisOptions>>((redisCacheOptions, myOwnRedisOptions) =>
  {
    redisCacheOptions.Configuration = myOwnRedisOptions.Value.ConnectionString;
  });
};

Proposed API

• Add an overload where the setupAction is not required
• Avoid nullable on setupAction

Please provide the specific public API signature diff that you are proposing. For example:

namespace Microsoft.Extensions.DependencyInjection;

public static class StackExchangeRedisCacheServiceCollectionExtensions
{
+    public static IServiceCollection AddStackExchangeRedisCache(this IServiceCollection services)
+    public static IServiceCollection AddStackExchangeRedisOutputCache(this IServiceCollection services)
}

Usage Examples

services
  .AddOptions<MyOwnRedisOptions>
  .BindConfiguration(MyOwnRedisOptions.SectionName)
  .Validate(options => !string.IsNullOrWhiteSpace(options.ConnectionString), "configuration for redis ConnectionString cannot be null or ...... ")
  .Validate(options => options...............);
  //...

services
  .AddOptions<RedisCacheOptions>()
  .Configure<IOptions<MyOwnRedisOptions>>((redisCacheOptions, myOwnRedisOptions) =>
  {
    redisCacheOptions.Configuration = myOwnRedisOptions.Value.ConnectionString;
  });
};

services.AddStackExchangeRedisCache()

Alternative Designs

Not exactly a fan of using null as optional overload in general. It opens the code to more nullability check or bug tracking NRE like "well I was sure I gave it a value".

PS: Only writing it for AddStackExchangeRedisCache but it would be the the same for the OutputCache as well

namespace Microsoft.Extensions.DependencyInjection;

public static class StackExchangeRedisCacheServiceCollectionExtensions
{
+    public static IServiceCollection AddStackExchangeRedisCache(this IServiceCollection services, Action<RedisCacheOptions> setupAction = null)
{
        ArgumentNullThrowHelper.ThrowIfNull(services);
-       ArgumentNullThrowHelper.ThrowIfNull(setupAction);

        services.AddOptions();
-       services.Configure(setupAction);
+       if(setupAction != null)
+       {
+           services.Configure(setupAction);
+       }

        services.Add(ServiceDescriptor.Singleton<IDistributedCache, RedisCacheImpl>());

        return services;
}

or

namespace Microsoft.Extensions.DependencyInjection;

public static class StackExchangeRedisCacheServiceCollectionExtensions
{
+    public static IServiceCollection AddStackExchangeRedisCache(this IServiceCollection services, Action<RedisCacheOptions> setupAction = null)
{
        ArgumentNullThrowHelper.ThrowIfNull(services);
-       ArgumentNullThrowHelper.ThrowIfNull(setupAction);

        services.AddOptions();
-       services.Configure(setupAction);
+       services.Configure(setupAction ?? _ => {});

        services.Add(ServiceDescriptor.Singleton<IDistributedCache, RedisCacheImpl>());

        return services;
}

Risks

The risk of bad setup/configuration is still owned by inner implementation such as RedisCache / RedisCacheImpl etc ...

[gerdelma]

What do you think about adding an overload for Action<IServiceProvider, RedisCacheOptions> setup?

This way .GetRequiredService<T>() could be used to retrieve the required configuration, which has been added via DI.

[tebeco]

yesn't, it could be an addition to the proposal here.
working with IOptions<T> should just work 🤷

I'm looking for simplicity here
I mostly think services.AddStackExchangeRedisCache() as/is should works regardless of how IOptions<RedisCacheOptions> has been setup in DI

because adding Action<IServiceProvider, RedisCacheOptions> will just call a .Configure(... and passing the options inside, just more boilerplate for everyone at every api layer

[Shinyshark1]

What do you think about adding an overload for Action<IServiceProvider, RedisCacheOptions> setup?

This way .GetRequiredService<T>() could be used to retrieve the required configuration, which has been added via DI.

This is what I need currently. I'd like to configure my cache by retrieving the configuration with the IServiceProvider.

At least having the option as an overload would be a welcome addition.

[SapiensAnatis]

What do you think about adding an overload for Action<IServiceProvider, RedisCacheOptions> setup?

This way .GetRequiredService<T>() could be used to retrieve the required configuration, which has been added via DI.

I really want this as well. It would also be useful for OTEL tracing, where it looks like I need to have one IConnectionMultiplexer used by the entire app, so I have to do this 'hack'

IConnectionMultiplexer multiplexer = ConnectionMultiplexer.Connect(
    builder.Configuration.GetConnectionString("redis")
        ?? throw new InvalidOperationException("Missing redis connection string!")
);
builder.Services.AddSingleton(multiplexer);
builder.Services.AddStackExchangeRedisCache(options =>
{
    options.ConnectionMultiplexerFactory = () => Task.FromResult(multiplexer);
    options.InstanceName = "RedisInstance";
});

[mgravell]

I've ummed and ahhed on this for an absurd amount of time. I apologise for that: sorry

The main thing that has been tormenting me is the contrast between the scenarios where this would be useful (custom sections, etc), vs the scenarios where it would add confusion. The normal and expected usage uses the options callback. Not requiring this is fairly niche, but the existing workaround to not use this options is ... fairly harmless:

// existing workaround today
services.AddStackExchangeRedisCache(_ => { });

I have verified that this works alongside your suggested custom section / binding code: it appears to.

It feels like adding this option would make it easy for people to shoot themselves in the foot and get into a situation where the cache is incorrectly configured, only discoverable at runtime - i.e. calling the more trivial zero-arg overload. I will grant: it is entirely possible to use the existing overload today and not do the right things, but we've at least reminded them to try (by exploring what the options arg gives them).

This contrasts with several other scenarios where we do not demand this, and the default instance is at least useful.

For that reason, I've been a little reluctant to go beyond "just use the do-nothing callback".

Genuine question: help me understand - it is entirely possible I have gaps in my understanding

Is there anything beyond "it is 5 characters tidier" that makes this significant:

- services.AddStackExchangeRedisCache(_ => { });
+ services.AddStackExchangeRedisCache();

If you can help me see what I'm missing, I'm happy to try to push this over the line. The change itself seems fairly harmless; I just want to understand whether it will hurt vs help.

I note that the PR also covers AddStackExchangeRedisOutputCache - most of the same points exist there too.