Cache previews 404 responses

[PVince81]

Clients might not know whether a file type has a preview available, so they will connect and receive a 404. However I noticed that the 404 response contains "no-cache".

We should let browsers/clients cache 404 responses for previews to avoid further requests.

Note that in the browser's case we already send an additional parameter "c" that contains the instance id and etag, so if the file ever changes, a new preview would be loaded.

@oparoz @rullzer @georgehrke @DeepDiver1975 what do you think ?

Also note that with the WebDAV approach we might get more 404 as we won't know if previews are available either (#12353)

[oparoz]

Not sure about this. The browser will be able to detect changes via the etag, but clients won't and people will complain that they don't see the files they've just uploaded, no?

[PVince81]

If they upload new files, there will be new file names.
If they overwrite existing files, their etag will change so the preview URL's parameter will change, so technically a different URL.

[oparoz]

I think I'm missing some context here.
How does WebDAV serve previews? There is a special route for that like the thumbnail API or are we talking Files controller?

☕

[PVince81]

Webdav does not serve previews at all.

The Webdav branch still uses the existing previews API endpoint.
Currently that endpoint returns "allow cache" headers when the preview exists, which means the browser is allowed to cache existing previews.
However when the preview does not exist, the server returns 404 and "do not cache this!" headers, so the browser will never cache the 404 response (the fact that this preview does not exist). So it will re-request it from the server every time the list is shown.

[oparoz]

OK.
That endpoint is then not used by the Files app exclusively and those apps which use it may not have an etag they can send. I haven't checked, but I'm thinking about activity, bin, file comparison. There may be more. They will all have to be modified to use the etag, if they don't already.
External apps which are using that "preview.png" endpoint will also need to be modified, unless that's considered a private API.

[PVince81]

Hmmm good point.

I think this is mostly about JS apps that run the web browser, because the browser will use the headers. Not sure about external apps.

You don't actually need to send the etag, you can also send a random string but then you'd get it from a real server request each time.

I think changing the behavior with 404 will not harm more than the actual caching already does (if there is harm at all)

[oparoz]

OK, so ask the browser to cache requests and send a random string at every request for apps which don't handle etags.

[rullzer]

Wouldn't it make more sense to have the available preview type in the capabilities.... then clients would only request previews on types that actually support it.

[oparoz]

@rullzer - The problem is that a preview can fail (missing encryption key, corrupt file, missing file, etc.), so the frontend would still need to get some kind of error. 500 is probably better, but I think these cases still generate 404s.

[PVince81]

@rullzer of course it would be better to have OC.Previews with the preview types.
But I'm planning short term as I don't know whether OC.Previews will be available in time, while enabling caching for 404 seems quicker short term.

[PVince81]

OC.Previews ticket here #17543 (just linking)

[nickvergessen]

Duplicate of #13609 ?

[PVince81]

Not exactly. I think the caching from #13609 is about the gallery folder.

This ticket is about letting the browser cache the 404 response by setting the correct cache headers instead of setting a "don't cache this" header style.

[PVince81]

Likely obsolete, @VicDeo I think we got rid of those 404 ? If yes, please close.

[VicDeo]

@PVince81 404 caused by unsupported mime was fixed with #27558.
I'm not aware of any other kind of 404

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.