[FEATURE] 🔂 Conditional Refresh

jodydonetti commented 1 year ago

Problem

Sometimes the payload to receive from the so called single source of truth (eg: the database, a remote service, etc) can be quite big, and in those situations it is a waste to get and process it (eg: deserializing) each and every time even when the data has not changed.

Ideally there should be a way to keep track of the "version" of the cached data and, when requesting for a refresh of that, be able to handle a scenario where the data is not changed, so to keep the "stale" data which, in such a case, would not actually be stale.

As pointed out quite well by the community member @aKzenT in their original issue, it would be nice if FusionCache natively supported what's commonly known as conditional requests, but for when refreshing the cache.

And so the name: Conditional Refresh.

Solution

Native support should be added to FusionCache, thanks to the addition of a 2 new tracked props: string? ETag and DateTimeOffset? LastModified: these new props will be available in the FusionCacheFactoryExecutionContext<TValue> (see below for more), which is the ctx param we normally see when calling some of the available overloads of the GetOrSetAsync(...) method.

Also in the context we'll need to add support to for accessing the stale value, if any, thanks to the new StaleValue prop, of type MaybeValue<TValue> (so we can check if it actually has a stale value or not).

This is how the code would look like:

var product = await cache.GetOrSetAsync<Product>(
    $"product:{id}",
    async (ctx, ct) =>
    {
        using var req = new HttpRequestMessage(HttpMethod.Get, $"/api/product/{id}");

        if (ctx.HasETag && ctx.HasStaleValue)
        {
            // LAST MODIFIED + STALE VALUE -> TRY WITH A CONDITIONAL GET
            req.Headers.Add("ETag", ctx.ETag);
        }

        using var resp = await client.SendAsync(req, ct);

        resp.EnsureSuccessStatusCode();

        if (resp.StatusCode == HttpStatusCode.NotModified)
        {
            // NOT MODIFIED -> RETURN STALE VALUE
            return ctx.NotModified();
        }

        // NORMAL RESPONSE: SAVE ETAG + RETURN VALUE
        return ctx.Modified(
                  await resp.Content.ReadFromJsonAsync<Product>(),
                  resp.Headers.ETag?.ToString()
                );
    },
    opt => opt.SetDuration(duration).SetFailSafe(true)
);

It should be noted that this will require a breaking change, since previously the context was of type FusionCacheFactoryExecutionContext and now it should become FusionCacheFactoryExecutionContext<TValue>, so that the stale value would accessible, and typed correctly.

This may break existing calling sites, but only the ones that use the overloads WITH the context AND that do NOT specify the type (eg: GetOrSet(...) instead of GetOrSet<TValue>(...)). This is because the current C# compiler is not able to infer the type of the context based on the inner return type of the lambda (factory).

The added value is so big, and the existing call sites should not be that many, that this is a worthy breaking change (and most probably the last one) before the final v1.0.

aKzenT commented 1 year ago

Hey,

happy to see that feature coming to light!

The proposed design is similar to my original proposal except for one thing. I originally proposed that the factory returns a "NotModified" sentinel when the value is unchanged. In your version you are simply passing the stale value to the factory which can choose to return that stale value. I think your design is more flexible and therefore preferible.

So, I don't see any issues from my side.

jodydonetti commented 1 year ago

The proposed design is similar to my original proposal except for one thing. I originally proposed that the factory returns a "NotModified" sentinel when the value is unchanged.

I thought about that, but that would've meant not returning a value from the lambda, which is not possible: actually, there is a way to do that, but basically it meant having a method on the context that internally would have thrown a special exception caught "outside", but I don't like using exceptions for control flow.

In your version you are simply passing the stale value to the factory which can choose to return that stale value. I think your design is more flexible and therefore preferible.

So, I don't see any issues from my side.

Good 👍

Another detail is that I've added 2 props, one for the LastModified and one for the ETag: the idea was that in case of a DateTimeOffset it would have allocated less space than a string.

But.

But then i remembered something I've read in the specs for the Last-Modified header, and the format for the value is not really that friendly to be parsed as a DateTimeOffset (eg: it can be just Wed, 21 Oct 1978 16:42:00 GMT), so I can probably just use 1 prop with a generic name, for both the ETag case and the Last-Modified case, and leave the meaning of the string itself to the implementers (like in your original proposal).

Thoughts?

aKzenT commented 1 year ago

Hey,

while that is true, as far as I see, .NET parses these header values to DateTimeOffset in most places. So you would probably not need to work with the raw strings.

We could still unify that to one prop, but then maybe we would need another name and might consider making it an "object". Or even providing a generic Metadata dictionary that users and plugins can fill as they see fit.

jodydonetti commented 1 year ago

while that is true, as far as I see, .NET parses these header values to DateTimeOffset in most places. So you would probably not need to work with the raw strings.

Yeah I've made some tests, and it seems that it is true. On the other hand, the way DateTimeOffset are currently normally serialized is such that the timezone specifier is usually converted to UTC and then lost, so basically it would mean getting GMT from the server, storing it as UTC and then sending it back as UTC, maybe resulting in a different match.

I have to do some tests...

We could still unify that to one prop, but then maybe we would need another name

I'm playing locally with Version, plain and simple.

and might consider making it an "object".

Nope, that would not work with the 2nd level (eg: serialization).

Or even providing a generic Metadata dictionary that users and plugins can fill as they see fit.

Mmmh, probably too heavyweight.

Will update you soon on these issues.

jodydonetti commented 1 year ago

Hi @aKzenT , I've played with the 2 different approaches:

1 prop: string? Version
2 props: string? ETag + DateTimeOffset? LastModified

After some back and forth, here are my thoughts.

`Version` prop

Having only string? Version seems easier, and we may store into it either and ETag or a Last-Modified header in the shape of a single string. On the other hand though it can also seem limiting since for different reasons we may want to also store a potential Last-Modified header: for example the specs suggest, when we have both, to send them both in a refresh request, even though the server must use ETag first. Finally, the name "Version" may feels fine with "ETag" (they are both version identifiers), but a little bit less so with "LastModified" because yes, such a value may be used to identify a version of a value, but it's not a version itself. We may pick a different name, but I don't feel there's a single name that can be used to confidently communicate it can be used for both (even though this is a small issues, because of xml docs, tutorials, etc). But still.

`LastModified` + `ETag` props

The Last-Modified header seems to be in fact easily parsable by the DateTimeOffset class: on top of that, there are ready-made methods and props that does that automatically in a strongly type way like this and this.

The ETag header instead is quite easier to work with, since it's just a plain string?

Having both an ETag and a LastModified props can cover more situations.

Make It Easier

While I was at it, I've also added to the context class 2 new props and 2 new methods:

public bool HasStaleValue { get; }: to quickly check if there is a stale value
public bool HasETag { get; }: to quickly check if there is an ETag specified
public bool HasLastModified { get; }: to quickly check if there is a LastModified specified
public TValue NotModified(): used to clearly show in code the intent of returning the currently cached value, because it was not in fact modified and is still valid
public TValue Modified(TValue value, string? etag = null, DateTimeOffset? lastModified = null): used to clearly show in code the intent of returning a modified (eg: new) value, potentially along with a value for the ETag, the LastModified, or both of them (the props are also directly modifiable on the context, of course)

Working Branch

I've pushed a working branch with all of this, if you want to take a look at it.

Example

The resulting code using all of this looks something like this.

To me it seems quite clear and concise: what do you think?

More?

It's important to avoid re-creating parts of an http client caching handling with something like a dictionary, because frankly it seems too heavyweight. And just give the most common use-cases a quick and easy way of being handled, which means having the 2 most common props: ETag and LastModified, with ETag being the most used.

For any scenario that needs more advanced stuff (like storing all the response headers, etc) users can just create their own "wrapping" class with whatever extra props they may need, and cache that instead, like this:

public class MyResponse<TValue> {
  public Dictionary<string, string> Headers {get; set; }
  public int StatusCode {get; set; }
  public TValue Value {get; set; }
}

// ...

cache.GetOrSet<MyResponse<Product>>(
  ...
);

By caching this + using the new ctx.StaleValue prop + the 2 new ctx.NotModified() and ctx.Modified(...) methods, it should be pretty darn easy to implement more advanced conditional refresh scenarios, without having to pay the extra cpu and memory every single time for anybody else out there.

Your thoughts?

What do you think about this train of thoughts? Any suggestion or corrections?

aKzenT commented 1 year ago

I agree with that reasoning. I also feel that "Version" could mean a lot of things in a caching framework and it is not obvious that it should be used for ETags, etc. without reading the docs.

The example looks good to me. I think that would work nicely.

Long-term I still think there would be a benefit to having the entry class be a bit more extensible. While wrapper classes work for many scenarios, they shift caching-concerns into the consumer instead of allowing them to live separately, e.g., in the form of a plugin. But that is a separate discussion, which does not need to be addressed in this PR.

jodydonetti commented 1 year ago

Hi all, v0.21.0-preview1 is out which includes this, too.

jodydonetti commented 1 year ago

Hi, v0.21.0-preview2 is out: one more week of baking and the official v0.21.0 will be released 🎉

jodydonetti commented 1 year ago

Hi all, I just release v0.21.0 which includes this 🎉

ZiggyCreatures / FusionCache