Merge branch 'tz-cache-project-avatars' into 'master'

Improve caching of assets (project/group avatars/note attachments)

See merge request gitlab-org/gitlab!18345

app/controllers/concerns/uploads_actions.rb

@@ -29,13 +29,15 @@ module UploadsActions
   def show
     return render_404 unless uploader&.exists?
 
-    if cache_publicly?
-      # We need to reset caching from the applications controller to get rid of the no-store value
-      headers['Cache-Control'] = ''
-      expires_in 5.minutes, public: true, must_revalidate: false
-    else
-      expires_in 0.seconds, must_revalidate: true, private: true
-    end
+    # We need to reset caching from the applications controller to get rid of the no-store value
+    headers['Cache-Control'] = ''
+    headers['Pragma'] = ''
+
+    ttl, directives = *cache_settings
+    ttl ||= 6.months
+    directives ||= { private: true, must_revalidate: true }
+
+    expires_in ttl, directives
 
     disposition = uploader.embeddable? ? 'inline' : 'attachment'
 
@@ -120,8 +122,8 @@ module UploadsActions
     nil
   end
 
-  def cache_publicly?
-    false
+  def cache_settings
+    []
   end
 
   def model

app/controllers/uploads_controller.rb

@@ -81,8 +81,13 @@ class UploadsController < ApplicationController
     end
   end
 
-  def cache_publicly?
-    User === model || Appearance === model
+  def cache_settings
+    case model
+    when User, Appearance
+      [5.minutes, { public: true, must_revalidate: false }]
+    when Project, Group
+      [5.minutes, { private: true, must_revalidate: true }]
+    end
   end
 
   def secret?

spec/controllers/uploads_controller_spec.rb

 # frozen_string_literal: true
 
 require 'spec_helper'
-shared_examples 'content not cached without revalidation' do
+shared_examples 'content 5 min private cached with revalidation' do
   it 'ensures content will not be cached without revalidation' do
-    expect(subject['Cache-Control']).to eq('max-age=0, private, must-revalidate')
+    expect(subject['Cache-Control']).to eq('max-age=300, private, must-revalidate')
   end
 end
 
-shared_examples 'content not cached without revalidation and no-store' do
+shared_examples 'content long term private cached with revalidation' do
   it 'ensures content will not be cached without revalidation' do
-    # Fixed in newer versions of ActivePack, it will only output a single `private`.
-    expect(subject['Cache-Control']).to eq('max-age=0, private, must-revalidate, no-store')
+    expect(subject['Cache-Control']).to eq('max-age=15778476, private, must-revalidate')
   end
 end
 
@@ -285,7 +284,7 @@ describe UploadsController do
             expect(response).to have_gitlab_http_status(200)
           end
 
-          it_behaves_like 'content not cached without revalidation' do
+          it_behaves_like 'content 5 min private cached with revalidation' do
             subject do
               get :show, params: { model: 'project', mounted_as: 'avatar', id: project.id, filename: 'dk.png' }
 
@@ -305,7 +304,7 @@ describe UploadsController do
             expect(response).to have_gitlab_http_status(200)
           end
 
-          it_behaves_like 'content not cached without revalidation and no-store' do
+          it_behaves_like 'content 5 min private cached with revalidation' do
             subject do
               get :show, params: { model: 'project', mounted_as: 'avatar', id: project.id, filename: 'dk.png' }
 
@@ -358,7 +357,7 @@ describe UploadsController do
                 expect(response).to have_gitlab_http_status(200)
               end
 
-              it_behaves_like 'content not cached without revalidation and no-store' do
+              it_behaves_like 'content 5 min private cached with revalidation' do
                 subject do
                   get :show, params: { model: 'project', mounted_as: 'avatar', id: project.id, filename: 'dk.png' }
 
@@ -390,7 +389,7 @@ describe UploadsController do
             expect(response).to have_gitlab_http_status(200)
           end
 
-          it_behaves_like 'content not cached without revalidation' do
+          it_behaves_like 'content 5 min private cached with revalidation' do
             subject do
               get :show, params: { model: 'group', mounted_as: 'avatar', id: group.id, filename: 'dk.png' }
 
@@ -410,7 +409,7 @@ describe UploadsController do
             expect(response).to have_gitlab_http_status(200)
           end
 
-          it_behaves_like 'content not cached without revalidation and no-store' do
+          it_behaves_like 'content 5 min private cached with revalidation' do
             subject do
               get :show, params: { model: 'group', mounted_as: 'avatar', id: group.id, filename: 'dk.png' }
 
@@ -454,7 +453,7 @@ describe UploadsController do
                 expect(response).to have_gitlab_http_status(200)
               end
 
-              it_behaves_like 'content not cached without revalidation and no-store' do
+              it_behaves_like 'content 5 min private cached with revalidation' do
                 subject do
                   get :show, params: { model: 'group', mounted_as: 'avatar', id: group.id, filename: 'dk.png' }
 
@@ -491,7 +490,7 @@ describe UploadsController do
             expect(response).to have_gitlab_http_status(200)
           end
 
-          it_behaves_like 'content not cached without revalidation' do
+          it_behaves_like 'content long term private cached with revalidation' do
             subject do
               get :show, params: { model: 'note', mounted_as: 'attachment', id: note.id, filename: 'dk.png' }
 
@@ -511,7 +510,7 @@ describe UploadsController do
             expect(response).to have_gitlab_http_status(200)
           end
 
-          it_behaves_like 'content not cached without revalidation and no-store' do
+          it_behaves_like 'content long term private cached with revalidation' do
             subject do
               get :show, params: { model: 'note', mounted_as: 'attachment', id: note.id, filename: 'dk.png' }
 
@@ -564,7 +563,7 @@ describe UploadsController do
                 expect(response).to have_gitlab_http_status(200)
               end
 
-              it_behaves_like 'content not cached without revalidation and no-store' do
+              it_behaves_like 'content long term private cached with revalidation' do
                 subject do
                   get :show, params: { model: 'note', mounted_as: 'attachment', id: note.id, filename: 'dk.png' }
 

spec/features/snippets/user_creates_snippet_spec.rb

@@ -45,7 +45,9 @@ describe 'User creates snippet', :js do
       link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
       expect(link).to match(%r{/uploads/-/system/user/#{user.id}/\h{32}/banana_sample\.gif\z})
 
-      reqs = inspect_requests { visit(link) }
+      # Adds a cache buster for checking if the image exists as Selenium is now handling the cached regquests
+      # not anymore as requests when they come straight from memory cache.
+      reqs = inspect_requests { visit("#{link}?ran=#{SecureRandom.base64(20)}") }
       expect(reqs.first.status_code).to eq(200)
     end
   end
@@ -63,7 +65,7 @@ describe 'User creates snippet', :js do
     link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
     expect(link).to match(%r{/uploads/-/system/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z})
 
-    reqs = inspect_requests { visit(link) }
+    reqs = inspect_requests { visit("#{link}?ran=#{SecureRandom.base64(20)}") }
     expect(reqs.first.status_code).to eq(200)
   end
 
@@ -88,7 +90,7 @@ describe 'User creates snippet', :js do
     link = find('a.no-attachment-icon img[alt="banana_sample"]')['src']
     expect(link).to match(%r{/uploads/-/system/personal_snippet/#{Snippet.last.id}/\h{32}/banana_sample\.gif\z})
 
-    reqs = inspect_requests { visit(link) }
+    reqs = inspect_requests { visit("#{link}?ran=#{SecureRandom.base64(20)}") }
     expect(reqs.first.status_code).to eq(200)
   end