Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace file doesn't remove previously generated thumbnails #17477

Closed
WoLfulus opened this issue Feb 9, 2023 · 8 comments
Closed

Replace file doesn't remove previously generated thumbnails #17477

WoLfulus opened this issue Feb 9, 2023 · 8 comments
Assignees
Labels

Comments

@WoLfulus
Copy link
Contributor

WoLfulus commented Feb 9, 2023

Describe the Bug

Replacing an image with another image doesn't invalidate the other variant images generated through transformation presets.

To Reproduce

  • Upload an image
  • Access the asset using transformation keys
  • Replace file through admin UI
  • Access the same asset using the same transformation keys
  • Served file is still the old one

Errors Shown

No response

What version of Directus are you using?

9.22.4

What version of Node.js are you using?

Docker

What database are you using?

PG 14

What browser are you using?

Edge

How are you deploying Directus?

Docker

@github-actions
Copy link

github-actions bot commented Feb 9, 2023

Linear: ENG-682

@emrahnazif
Copy link

Maybe this bug can be considered along side with #13559

@Nitwel Nitwel self-assigned this Mar 17, 2023
@Nitwel
Copy link
Member

Nitwel commented Mar 17, 2023

This seems to be caused by CACHE_TTL having a default of 30 days which sets the header Cache-Control: max-age=86400. You probably have to change the name of the url when you replace it for the time being.
We could introduce versioning to files meaning that each file will have a new version field that only ever gets changed when you replace a file. That way you can add a query parameter to the request that busts the cache when the file got replaced. Thoughts @rijkvanzanten?

@rijkvanzanten
Copy link
Member

The /assets endpoint already allows any arbitrary text behind the UUID, so you could do this as-is 👍🏻

@DanielBiegler
Copy link
Contributor

Tested on v10.6.2 as container in production:

Curious how Firefox apparently does receive the new image but refuses to replace it:

image

@DanielBiegler
Copy link
Contributor

DanielBiegler commented Sep 22, 2023

So after a little bit of research I found a couple of approaches we could take here but first, let's specify exactly whats happening.

  1. Currently we actually do remove all generated transformations when replacing a file (v10.6.2)
  2. We do serve the new file as per my screenshot here.

Technically, if

  • you provide cache disabling headers for /assets
  • the browser ignores caching itself
  • you use your own cachebusting param with a new value, for example /assets/<hash>?cachebust=abc123

the browser will see the new resource. Going further along without the mentioned options:

  1. The new transformations get persisted once upon request for the new resource
  2. Now the browser refuses to replace the cached resource because it's still fresh defined by us setting the max-age.
  3. max-age seems to have a higher priority than other caching options

What we could do

  1. Get rid off the max-age header completely and switch to a conditional request where we can reply with an ETag and or Last-Modified header to either return the resource or reply with body-less and tiny 304 Not Modified

This'd allow to always get the latest resource but with the cost of an extra request. Probably for most people the following would be best:

  1. Use either the ETag and or modified-headers to be able to reply with tiny 304 responses, but make the max-age default from ASSETS_CACHE_TTL way shorter than the current default of 30days. This'd allow for good prevention of unnecessary requests and the quick revalidation via the 304 reply where we don't have to transmit the image at all after max-age.

Messing with caches can and will impact every Directus instance so I'd love to be very careful with this.

In my opinion this is more of an improvement/optimization than a current bug, because like I said, we actually do remove previously generated thumbnails and the user could implement cachebusting via parameters or set ASSETS_CACHE_TTL to a lower value if their resources change often.

I'd close this and open a feature request with my findings in case people want to vote for conditional requests, thoughts @rijkvanzanten ?

@rijkvanzanten
Copy link
Member

That makes sense 👍🏻

@DanielBiegler
Copy link
Contributor

Created a discussion for this on #19768

@DanielBiegler DanielBiegler closed this as not planned Won't fix, can't repro, duplicate, stale Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Archived in project
Development

No branches or pull requests

5 participants