Respawnsive / Apizr

Refit based web api client management, but resilient (retry, connectivity, cache, auth, log, priority, etc...)
https://www.apizr.net
Apache License 2.0
140 stars 12 forks source link

Caching not working properly not respecting CacheKey #20

Closed SOFSPEEL closed 5 months ago

SOFSPEEL commented 5 months ago

I have setup Caching as follows:

    [Post("/site/SRConsAPI?method=getSingleSignOnToken"), Cache(CacheMode.GetOrFetch, "00:30:00")]
    Task<singleSignOnTokenResult> GetSingleSignOnTokenAsync(
        [AliasAs("cons_id"), CacheKey] string constituentId);

Issue is when the constituentId parameter changes, the cached value based on the previous constituentId is returned.

I think the issue may arise here:

https://github.com/Respawnsive/Apizr/blob/078b3f987cd1a469645b2e7f855b5390c082f84e/Apizr/Src/Apizr/ApizrManager.cs#L2171C52-L2171C69

where this code returns without considering the [CacheKey].

             // Did we ask for it already ?
                if (_cachingMethodsSet.TryGetValue(methodToCacheData, out var methodCacheDetails))
                {
                    // Yes we did so get saved specific details
                    cacheAttribute = methodCacheDetails.cacheAttribute;
                    cacheKey = methodCacheDetails.cacheKey;

                    return cacheAttribute != null && !string.IsNullOrWhiteSpace(cacheKey);
                }

Here is a some client pseudo-code that will fail:

var singleSignOnTokenResult1 =
            await _apizrManager.ExecuteAsync(api => api.GetSingleSignOnTokenAsync("blah1"));

  var singleSignOnTokenResult2 =
            await _apizrManager.ExecuteAsync(api => api.GetSingleSignOnTokenAsync("blah2"));

singleSignOnTokenResult2 equals singleSignOnTokenResult1 as its not considering the passed in argument and hence pulls the cached value from the first call.

Here is a PR which may fix the issue: https://github.com/Respawnsive/Apizr/pull/21, but consider getting rid of _cachingMethodsSet too.

JeremyBP commented 5 months ago

OMG this one is a little tiny misstake with big consequences! My fix was just to not cache the cachekey into the methodset when cachekey is built from dynamic parameters values. Thanks for pointing that out to me!

JeremyBP commented 5 months ago

Just pushed the preview 3 on NuGet including a fix for it. Let me know if it does actually fix it for you.

SOFSPEEL commented 5 months ago

@JeremyBP Caching seems to be working properly now thanks for the quick fix.

Love your framework BTW!