coil-kt / coil

Image loading for Android and Compose Multiplatform.
https://coil-kt.github.io/coil/
Apache License 2.0
10.83k stars 664 forks source link

Coil 3.0.2 CacheControlCacheStrategy is not invalidating disk cache when ETag header value is changed #2668

Closed lmiskovic closed 1 week ago

lmiskovic commented 1 week ago

I have a constant url to load user profile image that contains accountId. Content behind that url can be changed and the change will be indicated with updated ETag header value.

CacheControlCacheStrategy is being used but when content behind url is changed (and ETag value is changed) Coil is still displaying old image.

There is an issue (https://github.com/coil-kt/coil/issues/1923) that was opened for older version of Coil where it is stated that "Coil will handle standard Cache-Control headers (including reverification) by default."

To me it looks like there is no reverification. I've added okhttp3.Interceptor to an OkHttpClient and by looking at the logs there is no HTTP request made for specific url, I can just see that image is pulled from DISK cache by using DebugLogger for that url.

colinrtwhite commented 1 week ago

Are you sure this is occurring on the latest version of Coil? I added a test here to verify and it passes as expected.

lmiskovic commented 1 week ago

Yep, I've updated due to fix made regarding https://github.com/coil-kt/coil/issues/2656 I can try to find some public url that works the same way and provide you with sample. One I'm trying is auth protected.

lmiskovic commented 1 week ago

@colinrtwhite What I realised now is that api is returning 302 Moved Temporarily with direct url for image, not sure if it affects the issue

colinrtwhite commented 1 week ago

OkHttp should handle those automatically unless you've set followRedirects(false). What's the response code, response body, and headers after following the redirect?

lmiskovic commented 1 week ago
Screenshot 2024-11-12 at 10 28 44 Screenshot 2024-11-12 at 10 28 49

Response is HTTP 200 Okhttp is handling redirect, not set to false.

Auth removed from url.

If Redirect happens what data (url) is used as cache key? I suppose redirected url as coil relies on OkHttp. Then afterwards shouldn't Coil reverification fail (cache miss) if redirected url is changed?

Initial url: https://caelor-demo.atlassian.net/wiki/aa-avatar/712020:5abacba1-429b-420c-8002-e05bd225d6ca

Redirected url: https://avatar-management--avatars.us-west-2.prod.public.atl-paas.net/712020:5abacba1-429b-420c-8002-e05bd225d6ca/9274d48c-4d75-478a-80fb-b3849b46bdad/128

colinrtwhite commented 1 week ago

@lmiskovic According to the headers set by the server's response I believe Coil is working correctly. Cache-Control on the redirected response is max-age=1814400, immutable. This means the response will continue to be served from the disk cache until it's older than max-age. Afterwards, Coil will reverify with the Etag. I believe no-cache must be set in the cache control to revalidate every time.

lmiskovic commented 1 week ago

Understood, is there any way I could get it to work for my usecase? Meaning if initial url redirects to another url different than the one that is still alive per max-age to display image from new url? Or I could just rewrite response header to use no-cache

colinrtwhite commented 1 week ago

Yep, using an OkHttp network interceptor to rewrite the cache header to include no-cache will force Coil to reverify the Etag every time.

lmiskovic commented 1 week ago

I'm not sure if this is the solution, it voids usage of disk cace completely. This probably goes out of the scope of this issue but is there a way to achieve behaviour that Coil stores image in disk cache and uses it while validating max-age and etag?

colinrtwhite commented 1 week ago

When the etag is verified if the server returns 304 it won't redownload the image so the disk cache can still be used. It's not possible to return the disk cache image at the same time as verifying the etag as each image request can only return one ImageResult. If we return the disk cache result and the etag has changed we'd have to return a second ImageResult which isn't possible.

Going to close this out as it is not a bug with Coil.