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

[API Proposal] Allow to disable caching for Metadata or use another cache mechanism #550

Open
joegoldman2 opened this issue Oct 6, 2024 · 3 comments
Milestone

Comments

@joegoldman2
Copy link
Contributor

Context

Today there is no easy way to use another cache than IDistributedCache (like the new HybridCache of .NET 9, FusionCache, etc) when using the metadata service. The AddCachedMetadataService method is the entry point for register the metadata service, registering a service that is based on IDistributedCache and IMemoryCache.

In addition, there is no way to completely disable the cache as it is directly implemented in the only IMetadataService implementation available out of the box (DistributedCacheMetadataService).

To solve this problem, it would be nice to have the possibility to register the service and repositories without any caching, which leaves the user the possibility to add the cache afterwards. Of course it will still be possible to register a default cache implementation that is based, as today, on IDistributedCache and IMemoryCache.

API Proposal

I propose the following API:

namespace Fido2NetLib;

+    public sealed class DefaultMetadataService : IMetadataService // An implementation without any cache mechanism
namespace Microsoft.Extensions.DependencyInjection;

public static class Fido2NetLibBuilderExtensions
{
-    public static void AddCachedMetadataService(this IFido2NetLibBuilder builder, Action<IFido2MetadataServiceBuilder> configAction);
     // Will register the default service without any cache
+    public static void AddMetadataService(this IFido2NetLibBuilder builder, Action<IFido2MetadataServiceBuilder> configure);
     // As the cache is currently implemented on service level (should be probably more flexible on repository level + decorator pattern but it's outside the scope of this issue),
     // this method will replace the current registered implementation by DistributedCacheMetadataService.
+    public static IFido2MetadataServiceBuilder UseDistributedCache(this IFido2MetadataServiceBuilder builder);
}

I am open to suggestions for the name of UseDistributedCache method for which I am not really inspired.

API Usage

var services = new ServiceCollection();
services.AddFido2(options => { ... }).AddMetadataService(options =>
{
    options.AddFidoMetadataRepository();
    options.UseDistributedCache();
});

instead of that, currently:

var services = new ServiceCollection();
services.AddFido2(options => { ... }).AddCachedMetadataService(options =>
{
    options.AddFidoMetadataRepository();
});

More context

#397 also reports some limitations of the current model

cc @abergs @iamcarbon

@abergs
Copy link
Collaborator

abergs commented Oct 29, 2024

Hey @joegoldman2 - I agree that the current API is problematic and I think you're API makes sense.

My only concern is that I think we are giving developers a footgun if the defaultprovider does not utilize any caching at all. The scenario being:

People will set everything up, start using MDS (although few should use MDS) and everything will work great. Until one day where the MDS endpoints are having a bit of maintenance/down time and things will fail in prod.

The fido MDS repo is built with the expectation that developers should cache the response.

If we can adress that in the default scenario, I think this is great for v4.

@abergs abergs added this to the Version 4 milestone Oct 29, 2024
@joegoldman2
Copy link
Contributor Author

The fido MDS repo is built with the expectation that developers should cache the response.

If we can adress that in the default scenario, I think this is great for v4.

I'm aligned with this approach. I will try to think about an API shape that matches this requirement. I'm also open to suggestion.

@abergs
Copy link
Collaborator

abergs commented Nov 18, 2024

@joegoldman2 Just a heads up, I'm pushing towards a 4.0.0 and it would be great to land this API redesign as part of it so that it doesn't have to wait until v5. However, if you won't have time to take a stab at it, it's likely not going to make it for v4.

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

No branches or pull requests

2 participants