Code review

[Eshva]

Namaste!

I am an author of similar package and discovered your repo with help of the author of FusionCache. Of course I was curious and wanted to check an alternative. I checked your code and found some problems in your code and want to give you my feedback. Possibly I'm biased a bit but anyway please accept my feedback. I can't find other possibility to make this code review on GitHub (if you know how to do it outside of PR please point me to it) so I use issue form and will cite your code.

---

First of all I noticed you use KV-store and it's feature entry TTL. I also evaluated it for my library but understood it's not a viable solution. NATS doesn't provide possibility to change entry TTL and you need it if you want to implement sliding expiry. And you have to implement both sliding and absolute expiry if you implement MS's distributed cache. Otherwise it's not a valid implementation. It's not only an interface but behavior as well. If you don't you break LSP from SOLID. I see only 2 possible solutions using NATS KV-store:

1. Embed the entry metadata into entry data (some kind of an envelop containing both of them).
2. Use 2 KV-store (buckets): one for data and another for metadata.

In my implementation for KV-variant I chose the second solution. I have an object store based cache as well, but it doesn't need this compromise 'cause object store entry have metadata dictionary.

    public NatsDistributedCache(IOptions<NatsDistributedCacheOptions> options, INatsConnection connection)
    {
        var _streamContext = connection.CreateJetStreamContext();
        _kvContext = _streamContext.CreateKeyValueStoreContext();
        var createOrUpdateTask = Task.Run(async () =>
        {
            _natsKvStore = await _kvContext.CreateOrUpdateStoreAsync(new NatsKVConfig(options.Value.BucketName) {  LimitMarkerTTL = TimeSpan.FromMinutes(10)}).ConfigureAwait(false);
            //await _natsKvStore.JetStreamContext.UpdateStreamAsync(new NATS.Client.JetStream.Models.StreamConfig(_natsKvStore.) { AllowMsgTTL = true }).ConfigureAwait(false);
        });

        createOrUpdateTask.Wait();
        if (_natsKvStore == null)
        {
            throw new InvalidOperationException("Failed to create or update the NATS KeyValue Store.");
        }
    }

1. It's better to accept a INatsKVStore object in constructor, not a NATS-connection. I understand you want to guaranty TTL on entries but it's better to do outside of constructor.
2. Constructor should be free of any asynchrony.
3. In production you can't create buckets on your will. A secured NATS server should not allow to create buckets or change their properties. You should use what ever you get from DevOps guys.

 public void Remove(string key) => RemoveAsync(key).Wait();

Why do you wait here and block the thread from using it for different tasks? You use .GetAwaiter().GetResult(); approach in other places and it's better in my mind.

public async Task RemoveAsync(string key, CancellationToken token = default) => await _natsKvStore.DeleteAsync(key, cancellationToken: token).ConfigureAwait(false);

1. It's better to use PurgeAsync in this context. DeleteAsync will only mark the entry as deleted.
2. If something go wrong in DeleteAsyn and it will throw an exception you have leaking abstraction because clients of IDistributedCache should care from now about concrete implementation and they shouldn't care. It's better to wrap implementation specific exceptions into some more general like InvalidOperationException. It's pity MS doesn't document any exceptions in IDistributedCache docs but anyway it's better to not bind the user to implementation specifity.

#if NET8_0_OR_GREATER
        ArgumentNullException.ThrowIfNull(options);
#else
        if (options == null)
        {
            throw new ArgumentNullException(nameof(options));
        }
#endif

A minor point here. It's easer to use just the variant in 'else', it works in any .NET version.

You don't verify validity of key in any method of your class. It also possibly bind user to implementation.

        return ttl.HasValue
            ? _natsKvStore.CreateAsync(key, value, ttl.Value, cancellationToken: token).AsTask()
            : _natsKvStore.CreateAsync(key, value, cancellationToken: token).AsTask();

Again here is a leaking abstraction. Client code can get an implementation specific exception. You have to await the method call.

    public async ValueTask<bool> TryGetAsync(string key, IBufferWriter<byte> destination, CancellationToken token = default)
    {
        var result = await _natsKvStore.TryGetEntryAsync<NatsMemoryOwner<byte>>(key, cancellationToken: token).ConfigureAwait(false);
        if (result.Success)
        {
            using var memoryOwner = result.Value.Value;
            destination.Write(memoryOwner.Memory.Span);
            return true;
        }

        return false;
    }

1. To reduce nesting it's better to invert condition.
2. You have to log problems. result.Error have exception object with error details and throw something like InvalidOperationException.

    private static TimeSpan? ValidateCacheOptionsAndDetermineTtl(DistributedCacheEntryOptions options)
    {
        if (options.SlidingExpiration.HasValue)
        {
            return ThrowSlidingExpirationNotSupportedException();
        }
        else if (options.AbsoluteExpirationRelativeToNow.HasValue)
        {
            return options.AbsoluteExpirationRelativeToNow.Value;
        }
        else
        {
            return options.AbsoluteExpiration.HasValue ? (options.AbsoluteExpiration.Value - DateTimeOffset.UtcNow) : null;
        }
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static TimeSpan? ThrowSlidingExpirationNotSupportedException()
    {
        throw new NotSupportedException("Sliding expiration is not supported by this cache implementation yet...");
    }
}

1. Return a value from a throwing method is very strange. If you like to cache something, cache the message string. But I wouldn't do it ether. Strings are interned, the compiler will do it for you. You need just throw in place. DRY most of the time is anti-pattern.
2. Usage of DateTimeOffset.UtcNow is anti-pattern. It's very difficult to test such a class. It's better use external 'clock' service. .NET CLR has TimeProvider class for this.

    public static IServiceCollection AddNatsDistributedCache(this IServiceCollection services, Action<NatsDistributedCacheOptions> setupAction)
    {
#if NET8_0_OR_GREATER
        ArgumentNullException.ThrowIfNull(services);
#else
        if (services == null)
        {
            throw new ArgumentNullException(nameof(services));
        }
#endif

        // Add NATS distributed cache options
        services.Configure(setupAction);

        // Register the distributed cache implementations
        services.TryAddSingleton<IDistributedCache, NatsDistributedCache>();
        services.TryAddSingleton<IBufferDistributedCache, NatsDistributedCache>();

        return services;
    }
}

1. With caches it's not enough to provide possibility to register only one cache. Usually for each kind of cached object there is a separate cache. It's better to support keyed services as well.
2. Single NATS connection is not a viable solution. For caches there could be a separate NATS-server or cluster. So each cache object should have a way to accept a specific connection object (or an object depended on this connection).
3. IOptions pattern is good but it's better to register in DI-container an object of the settings class. Something like this:

    services
      .AddOptions<KeyValueBasedCacheSettings>()
      .BindConfiguration(ThumbnailsCacheSectionPath)
      .ValidateDataAnnotations()
      .ValidateOnStart();
    services.AddKeyedSingleton<KeyValueBasedCacheSettings>(
      ApplicationBootstrapping.ThumbnailsCacheName,
      (diContainer, _) => diContainer.GetRequiredService<IOptions<KeyValueBasedCacheSettings>>().Value);

It's good you have out-of-process tests but it's better to start required environment (NATS in docker container in this case) on start of the tests. The same about benchmarks. You can see how to do it my repository.