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

[FEATURE] Options do not follow .Net standard pattern #339

Open
TibbsTerry opened this issue Dec 10, 2024 · 10 comments
Open

[FEATURE] Options do not follow .Net standard pattern #339

TibbsTerry opened this issue Dec 10, 2024 · 10 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@TibbsTerry
Copy link

Problem

Configuration of options do not follow the standard .Net options pattern
Options cannot be set via configuration and have to be 'hard-coded'

services.AddFusionCache()
    .WithOptions(o =>
    {
        // Can not access other dependencies eg IConfiguration here!
        o.DefaultEntryOptions.Duration = duration;
    });
[Test]
public void Test_WithOptions()
{
    var duration = TimeSpan.FromMinutes(50);
    var services = new ServiceCollection();
    services.AddFusionCache()
        .WithOptions(o => o.DefaultEntryOptions.Duration = duration);
    var serviceProvider = services.BuildServiceProvider();
    var fusionCache = serviceProvider.GetRequiredService<IFusionCache>();
    Assert.AreEqual(duration, fusionCache.DefaultEntryOptions.Duration);  // PASS
}

[Test]
public void Test_WithConfiguration()
{
    var duration = TimeSpan.FromMinutes(50);
    var configuration = new ConfigurationBuilder()
        .AddInMemoryCollection(new Dictionary<string, string?>  { { "DefaultDurationMinutes", duration.TotalMinutes.ToString()} })
        .Build();
    var services = new ServiceCollection();
    services.AddSingleton<IConfiguration>(configuration);
    services.AddFusionCache();
    services.AddOptions<FusionCacheOptions>()
        .Configure<IConfiguration>((options, config) =>
        {
            var durationMinutes = long.Parse(config["DefaultDurationMinutes"]!);
            options.DefaultEntryOptions.Duration = TimeSpan.FromMinutes(durationMinutes);
        });
    var serviceProvider = services.BuildServiceProvider();
    var fusionCache = serviceProvider.GetRequiredService<IFusionCache>();
    Assert.AreEqual(duration, fusionCache.DefaultEntryOptions.Duration); // FAIL
}

Solution

Confguration/Options should follow standards for .NET
ie should use
Microsoft.Extensions.DependencyInjection.OptionsServiceCollectionExtensions.Configure

Microsoft.Extensions.DependencyInjection.MemoryCacheServiceCollectionExtensions.AddDistributedMemoryCache
https://github.com/dotnet/runtime/blob/a5af0ab77caa5ed7a6844fc5f4f459e5edfe23d3/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCacheServiceCollectionExtensions.cs#L93
is probably a good example of how to do it.

or (less preferable solution)
change FusionCacheBuilder.SetupOptionsAction
to
Action<IServiceProvider, FusionCacheOptions>? SetupOptionsAction { get; set; }

@TibbsTerry
Copy link
Author

Also add IValidateOptions<FusionCacheOptions>

@jodydonetti
Copy link
Collaborator

jodydonetti commented Dec 30, 2024

Hi @TibbsTerry , I tried reproducing your test locally, here it is:

[Fact]
public void _AAA()
{
var duration = TimeSpan.FromMinutes(50);
var configuration = new ConfigurationBuilder()
	.AddInMemoryCollection(new Dictionary<string, string?> { { "DefaultDurationMinutes", duration.TotalMinutes.ToString() } })
	.Build();
var services = new ServiceCollection();
services.AddSingleton<IConfiguration>(configuration);
services.AddFusionCache();
services.AddOptions<FusionCacheOptions>()
	.Configure<IConfiguration>((options, config) =>
	{
		var durationMinutes = long.Parse(config["DefaultDurationMinutes"]!);
		options.DefaultEntryOptions.Duration = TimeSpan.FromMinutes(durationMinutes);
	});
var serviceProvider = services.BuildServiceProvider();
var fusionCache = serviceProvider.GetRequiredService<IFusionCache>();
Assert.Equal(duration, fusionCache.DefaultEntryOptions.Duration); // SUCCESS
}

I did not change anything, except for adapting it to use xUnit:

  • [Fact] instead of [Test]
  • Assert.Equal() instead of Assert.AreEqual()

that's all, and here is the result:

image

Can you give me more info?

Thanks.

@TibbsTerry
Copy link
Author

@jodydonetti
it works with
Version="2.0.0-preview-4"
but not
Version="1.4.1"

@jodydonetti
Copy link
Collaborator

Ah, I think I know why that is!
It's a consequence of this, which is now solved.

Can you add more details about the options validation?

Thanks!

@jodydonetti jodydonetti self-assigned this Jan 13, 2025
@jodydonetti jodydonetti reopened this Jan 13, 2025
@jodydonetti jodydonetti added the enhancement New feature or request label Jan 13, 2025
@jodydonetti jodydonetti added this to the v2.0.0 milestone Jan 13, 2025
@jodydonetti
Copy link
Collaborator

Hi @TibbsTerry thanks for the links, I'll try to see what I can do for v2.

@jodydonetti
Copy link
Collaborator

jodydonetti commented Jan 18, 2025

Hi @TibbsTerry , getting back to this.

I read the docs, and made some tests with the v2 preview.

I created this:

public class MyOptionsValidation
  : IValidateOptions<FusionCacheOptions>
{
  public ValidateOptionsResult Validate(string name, FusionCacheOptions options)
  {
    if (name == "Foo" && options.CacheName == "/")
      return ValidateOptionsResult.Fail("Oh no!");

    return ValidateOptionsResult.Success;
  }
}

Then this:

var builder = Host.CreateDefaultBuilder();
builder.ConfigureServices(services =>
{
  services
    .AddOptions<FusionCacheOptions>("Foo")
    .Configure(options =>
    {
      options.CacheName = "/";
    })
    .ValidateOnStart();

  services.AddSingleton<IValidateOptions<FusionCacheOptions>, MyOptionsValidation>();
});

var app = builder.Build();
app.Run();

This was the result:

Image

It seems to me that v2 is aligned with all the standard patterns, so I'll close this when v2 will be released.

If you find anything else please let me know.

Thanks!

@jodydonetti
Copy link
Collaborator

Hi all, I still can't believe it but v2.0.0 is finally out 🎉
Now I can rest.

@TibbsTerry
Copy link
Author

@jodydonetti I'm not too sure that you followed what I meant. Fusion cache should have its own built in Validator (that implements IValidateOptions<FusionCacheOptions> like above). This will stop users doing stupid things (like configuring negative values for timespans or other options). If its done in the correct pattern users can write their own to add to or override the existing validation.

@jodydonetti jodydonetti reopened this Jan 21, 2025
@jodydonetti
Copy link
Collaborator

Hi @TibbsTerry , thanks for the clarification.
In general the options are already self validating, but I see your point: I'll try to come up with some cases where an explicit validator can make sense and proceed with the implementation.
Will update you asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants