[IMPROVE] Attachment download caching

[wreiske]

This pull request adds a cache to the file-uploads handler to help improve subsequent load times.

Currently Rocket.Chat re-downloads images every time they are viewed / reacted / starred / etc. This commit helps keep images that will never change from being re-requested, and re-downloaded. There's no reason why a file-upload should change. It has a unique ID and will forever contain the same content.

Before

After

[geekgonecrazy]

I can’t see a reason not to do this.. seems silly not to include this.

@rocketchat/core any reason not to?

[sampaiodiego]

@wreiske have you tested in which storage providers? I'm a bit concerned about S3 and GCP storage providers, as they sent a redirect to clients and I'm not sure how clients deal redirects and cache.. If they cache the redirect URL or the content itself..

[wreiske]

@wreiske have you tested in which storage providers? I'm a bit concerned about S3 and GCP storage providers, as they sent a redirect to clients and I'm not sure how clients deal redirects and cache.. If they cache the redirect URL or the content itself..

I have not tested with any other storage providers. If anything, we could add an if statement to check if the storage is local filesystem / GridFS and only set expires that way.

[d-gubert]

I've tested this with the Amazon S3 storage.

It looks like the browser simply ignores the Cache-Control header when the response also specifies a Location header, effectively redirecting that request to even another origin. Since it's being redirected, I figure the browser would respect the Cache-Control directive of the external resource instead.

It looks to me that there's no harm in merging this. @sampaiodiego do you think it needs more testing?

Update: see message below

[d-gubert]

So, further testing indicated that this does introduce a problem after all. I've opened my dev tools in the first attempt, inadvertently disabling Chrome's cache 🤦‍♂️

The added header caches the redirect URL sent by Rocket.Chat, but when using S3 and a timeout setting smaller than the specified max-age in Cache-Control, the browser will eventually end up with a stale URL for the redirecting.

The solution would be adding the Cache-Control header only for requests that are not redirected to external services (or maybe removing the header from requests that will be redirected... whatever needs less effort)

[wreiske]

I'm pretty busy with work stuff, maybe I'll have some time around the end of the week to dig this up again -- unless someone else can quickly add on to this what is needed.

Need to look to see what the current storage type is set to, if set to FileSystem OR GridFS, use the cache. Otherwise, don't.

If nobody does anything with this, I'll have this pull-request updated by the EOD Friday.

[d-gubert]

I'm gonna update it then :)

[wreiske]

🎉