leonibr / community-extensions-cache-postgres

A PostgreSQL Implementation of IDistributedCache interface. Using Postgresql as distributed cache in Asp.Net Core. NetStandard 2.0
50 stars 16 forks source link

[Enhancement] Option to Only Update Expiration on IDistributedCache.Refresh (for use with ASPNET Core Session) #65

Open kenbailey opened 2 weeks ago

kenbailey commented 2 weeks ago

The ASPNET Core 8 session handling is calling a IDistributedCache.Refresh at the end of the request lifecycle if there were no Session.SetString(...) calls. If there was a Session.SetString(...) call, then ASPNET Core appears to not call the IDistributedCache.Refresh. This package uses a call to GetCacheItem(key, includeValue: false) for the refresh, but is also updating the expiration on the GetCacheItem call as well.

My particular use case is for ASPNET Core Session handling and the sliding expiration is quite long (30 mins). As such, I would prefer to not have two sliding window update DB calls per request, but just the single one for performance reasons.

I'm wondering if you would accept a PR that would provide an configuration option for UpdateExpirationOnGet? It could be defaulted to true to keep existing functionality.

There would only need to be a couple of changes to GetCacheItem and GetCacheItemAsync as follows:

if (_updateOnGetCacheItem && (UpdateExpirationOnGet || !includeValue))
{
    var updateCacheItem = new CommandDefinition(
        SqlCommands.UpdateCacheItemSql,
        new ItemIdUtcNow { Id = key, UtcNow = utcNow });
    connection.Execute(updateCacheItem);
}

Thanks in advance.

kenbailey commented 2 weeks ago

Looking at the code some more, the already existing UpdateOnGetCacheItem configuration parameter is actually what I want to use, I just don't want it to also disable the updating of expiration when IDistributedCache.Refresh is called.

So I don't think we need to add a new option, but I would propose that we change the behavior such that the IDistributedCache.Refresh implementation does cause the expiration to be updated even if the UpdateOnGetCacheItem is set to false.

In other words:

// includeValue is false when it is being called via IDistributedCache.Refresh
if (_updateOnGetCacheItem || !includeValue))

If the use-case of read-only databases is an issue, I would propose that a separate ReadOnlyMode (default false) parameter be added to prevent all writes (not just expiration updates on the Get/GetAsync calls.

kenbailey commented 2 weeks ago

@leonibr, submitted a PR for this. Let me know if I need to make any adjustments for it to be included. Thanks!