Restructured the assets caching setup

New definitions for different types of Uploads

Updated Caching strategy for project + group avatars, Notes Uploads

Fixed upload actions specs + offenses

Added random string as cache buster

Fixing the double quoted string for interpolation

app/controllers/concerns/uploads_actions.rb

@@ -29,12 +29,17 @@ module UploadsActions
   def show
     return render_404 unless uploader&.exists?
 
+    # We need to reset caching from the applications controller to get rid of the no-store value
+    headers['Cache-Control'] = ''
+    headers['Pragma'] = ''
     if cache_publicly?
-      # We need to reset caching from the applications controller to get rid of the no-store value
-      headers['Cache-Control'] = ''
+      # Caching for user avatars
       expires_in 5.minutes, public: true, must_revalidate: false
+    elsif avatar?
+      # Caching for Project + Group Avatars
+      expires_in 5.minutes, private: true, must_revalidate: true
     else
-      expires_in 0.seconds, must_revalidate: true, private: true
+      expires_in 6.months, private: true, must_revalidate: true
     end
 
     disposition = uploader.embeddable? ? 'inline' : 'attachment'
@@ -124,6 +129,10 @@ module UploadsActions
     false
   end
 
+  def avatar?
+    false
+  end
+
   def model
     strong_memoize(:model) { find_model }
   end

app/controllers/uploads_controller.rb

@@ -85,6 +85,14 @@ class UploadsController < ApplicationController
     User === model || Appearance === model
   end
 
+  def avatar?
+    User === model || Project === model || Group === model
+  end
+
+  def note_upload?
+    Note === model
+  end
+
   def secret?
     params[:secret].present?
   end

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,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/user/#{user.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
   end
@@ -63,7 +63,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 +88,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