Azure / Microsoft.Extensions.Caching.Cosmos

Distributed cache implemented with Azure Cosmos DB
MIT License
51 stars 14 forks source link

Very limited DI registration options, creates a paradox between eager, and post-build services. #78

Open ApacheTech opened 7 months ago

ApacheTech commented 7 months ago

There seems to be no way to use the Options pattern, and default azure credentials.

I have a section in appSettings.json as such:

  "CosmosCache": {
    "AccountEndpoint": "https://azure.url:443",
    "ContainerName": "containerName",
    "DatabaseName": "databaseName"
  }

I'm registering these within the DI, using CosmosCacheConfiguration as a POCO:

services.Configure<CosmosCacheConfiguration>(configuration.GetRequiredSection("CosmosCache"));

We also use default azure tokens:

services.AddSingleton<TokenCredential, DefaultAzureCredential>();

However, all of this requires me to late-bind the cosmos cache, but your package can only be eagerly bound, meaning I cannot get to any of the values required to add cosmos, with what's available out of the box:

services.AddCosmosCache(o =>
{
    o.ClientBuilder = new CosmosClientBuilder(CannotGetTo.AccountEndpoint, CannotGetTo.TokenCredential);
    o.ContainerName = CannotGetTo.ContainerName;
    o.DatabaseName = CannotGetTo.DatabaseName;
});

If I try to create my own registrar, then have no way of creating an IOptions<CosmosCacheOptions>, or IOptionsMonitor<CosmosCacheOptions>, which are the only two available constructors for CosmosCache.

public static IServiceCollection AddCosmosCache(this IServiceCollection services, IConfiguration configuration)
{
    services.Configure<CosmosCacheConfiguration>(configuration.GetRequiredSection("CosmosCache"));
    services.AddSingleton<IDistributedCache, CosmosCache>(sp =>
    {
        var config = sp.GetRequiredService<IOptions<CosmosCacheConfiguration>>().Value;
        var tokenCredential = sp.GetRequiredService<TokenCredential>();

        // ERROR: System.InvalidOperationException: 'The service collection cannot be modified because it is read-only.'
        services.Configure<CosmosCacheOptions>(CosmosCacheOptionsFactory);

        // ERROR: CosmosCacheOptions is not configured.
        var service = sp.GetRequiredService<IOptionsMonitor<CosmosCacheOptions>>();

        return new CosmosCache(service);

        void CosmosCacheOptionsFactory(CosmosCacheOptions o)
        {
        o.ClientBuilder = new CosmosClientBuilder(config.AccountEndpoint, tokenCredential);
        o.ContainerName = config.ContainerName;
        o.DatabaseName = config.DatabaseName;
        }
    });
}

So we have a paradox in which we can't eager-load the service because we have no access to any of the properties that the service needs; and we can't load the service via an implementation factory, because we can't add to the service collection a posterori.

Has this been resolved any other way? Or, is it possible to get a more robust registration procedure? There may be a more obscure way of resolving this with the package as it stands, but I can't see the path to making it work, with what I currently understand about M.E.DI.

ealsur commented 7 months ago

@ApacheTech thanks for the detailed Issue.

This sounds like something related to the DI framework itself, not particular to this library. Basically the scenario is, how do you register components that require each other. AddCosmosCache is mainly a shortcut for simple cases, and it follows the AddXXXXX extension pattern used for any IServiceCollection extensions.

Looking at your code, it seems the issue would be resolved by reducing the code to:

services.AddSingleton<IDistributedCache, CosmosCache>(sp =>
{
    var config = sp.GetRequiredService<IOptions<CosmosCacheConfiguration>>().Value;
    var tokenCredential = sp.GetRequiredService<TokenCredential>();

    CosmosCacheOptions o = new CosmosCacheOptions();
    o.ClientBuilder = new CosmosClientBuilder(config.AccountEndpoint, tokenCredential);
    o.ContainerName = config.ContainerName;
    o.DatabaseName = config.DatabaseName;

    return new CosmosCache(o);
});

The restrictions are related to the Dependency Injection framework you use itself. A DI framework is really not required to use this library, you could use it in a console application.

ApacheTech commented 7 months ago

That's pretty much exactly what I've ended up doing. It just seems really hacky. I was suprised there was no out-of-the-box support for it.

internal class CosmosCacheRegistrar : IServiceRegistrar
{
    public void Register(IServiceCollection services, IConfiguration configuration)
    {
        services.Configure<CosmosCacheConfiguration>(configuration.GetRequiredSection("CosmosCache"));
        services.AddSingleton<IDistributedCache, CosmosCache>(sp =>
        {
            var config = sp.GetRequiredService<IOptions<CosmosCacheConfiguration>>().Value;
            var tokenCredential = sp.GetRequiredService<TokenCredential>();
            var options = new CosmosCacheOptions
            {
            ClientBuilder = new CosmosClientBuilder(config.AccountEndpoint, tokenCredential),
            ContainerName = config.ContainerName,
            DatabaseName = config.DatabaseName
            };
            return new CosmosCache(options);
        });
    }
}

As a feature request, it would be nice to have a more fluent builder to set a lot of this stuff. The samples within this repo are outdated, still using Startup.cs. It would be nice to have some documentation on how to set this up for different scenarios, to see how the developers intended on user to add stuff like the token credentials. The way I've done it here seems like a patch, rather than intended use. How did your team see this being registered?

ealsur commented 7 months ago

Thanks for your answer.

Let me reiterate a point: This library has no dependency on a specific DI framework, you could use the library without any DI framework.

There is no dependency of the library on, for example in this case, Microsoft.Extensions.DependencyInjection, so even if we wanted to, we could not add new extension or shortcuts to make this wire further, we cannot depend on any type of API that is related to this package.

When looking at the documentation of this DI framework (https://learn.microsoft.com/en-us/dotnet/core/extensions/dependency-injection), I see that that's basically the way where you coordinate multiple registered components that require coordination among themselves.

The limitations or lack-of-thereof of a particular DI framework, while great feedback is nothing we can really process it in this repo.

Thanks for the feedback on the samples, I understand they might not cover the whole universe of scenarios, we could add one for a similar scenario that you described. Regarding the Startup.cs file, we can certainly update the samples to the new format.

ealsur commented 7 months ago

Additionally, looking at the official Distributed caching docs: https://learn.microsoft.com/en-us/aspnet/core/performance/caching/distributed?view=aspnetcore-8.0

And looking at the source code of other providers, like Redis:

https://github.com/dotnet/aspnetcore/blob/22ec4186f2f86469b45f712456a156c21803f455/src/Caching/StackExchangeRedis/src/StackExchangeRedisCacheServiceCollectionExtensions.cs#L24-L35

The extension methods are pretty similar.

ApacheTech commented 7 months ago

I think I may have been misunderstood, in part. There is never a requirement for any library to use any IOC container, or any specific DI framework.

That said, you do provide minimal support for Microsoft.Extensions.DependencyInjection within your repo, here. My feature request would be that this class is fleshed out, to provide a more friendly user experience. The only people who will ever use that file are the people who will use it within a Microsoft.Extensions.DependencyInjection based DI framework. The people who don't use any IOC don't enter into the equation at all.

In a way, using an IOC makes things more complicated than not, because you have the two-stage registration process, and you can't go back to the first, once the container is built.

When it was written, how did your team invisage people being able to use the TokenCredential overload of CosmosClientBuilder, within a DI container? Specifically, within the AddCosmosCache method you provide. It's there as an option to be able to set, but I don't see anything within the test suite that touches it, at all. You use a master key, but never show how to use a TokenCredential, or have any coverage of it's use.

I think my main point is that I don't feel that it should be necessary to have to decompile your library, and rip the guts out the extension method you provide, just to be able to use default Azure credential tokens with CosmosDB. Out of the box, your library provides no easy, or elegant solution, so each project that uses tokens and/or app settings will need to hack their own solution.

We can't even leave the setupAction out of it, and add that to the container elsewhere as an implementation factory, because you throw an exception if it's not present. There are a lot of improvements that can be made to that class. I'm not really interested in what other implementations of IDistributedCache do. Just because others don't offer elegant solutions, doesn't mean this library shouldn't either. You have a bare minimum support for DI. It could be a lot better.

ealsur commented 7 months ago

Thanks again for your feedback.

When it was written, how did your team invisage people being able to use the TokenCredential overload of CosmosClientBuilder.

It's possible to simply create the TokenCredential within the AddCosmosCache method, without registering as Singleton itself.

services.AddCosmosCache((op) =>
{
    string userAssignedClientId = "<your managed identity client Id>";
    // Or whatever constructor for the TokenCredential you want
    var credential = new DefaultAzureCredential(new DefaultAzureCredentialOptions { ManagedIdentityClientId = userAssignedClientId });
    op.ClientBuilder = new CosmosClientBuilder("endpoint", credential);
});

or if you want the TokenCredential to be used with other components in the same app:

string userAssignedClientId = "<your managed identity client Id>";
// Or whatever constructor for the TokenCredential you want
var tokenCredential = new DefaultAzureCredential(new DefaultAzureCredentialOptions { ManagedIdentityClientId = userAssignedClientId });
services.AddSingleton<TokenCredential>(tokenCredential);

services.AddCosmosCache((op) =>
{
    op.ClientBuilder = new CosmosClientBuilder("endpoint", tokenCredential);
});

There are a lot of possible scenarios, such as, creating a CosmosClient outside of the library and using it instead, so each of these scenarios might require different wiring. The reason the Options contain a CosmosClientBuilder is to give users the full customization control that the Cosmos DB .NET SDK has, and if the SDK keeps adding new options, these are all available for this library's consumers.

When looking at the APIs available when registering an Extension: https://github.com/Azure/Microsoft.Extensions.Caching.Cosmos/blob/6fea577a6d8134d4429b5eea883ea6a5fd58f577/src/CosmosCacheServiceCollectionExtensions.cs#L38-L48

There is a possibility to access the IServiceProvider but at that point, the options are already constructed, and the CosmosClientBuilder or CosmosClient is already set, we cannot modify them. And IServiceCollection.Configure does not allow for passing other parameters. I understand that this is not ideal for you and I'm sorry.

If you believe there is a better way to do the wiring, by all means, please send a PR and we'll review and incorporate it to the next version.

ealsur commented 7 months ago

One idea could be to use the IServiceProvider to test for CosmosClient or CosmosClientBuilder registered and use them. But you would still need to wire the registration to link the TokenCredential with the CosmosClient or CosmosClientBuilder.