From 1db3a578e4a217224fe16572efcc2f54779f6a55 Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Wed, 7 Dec 2022 15:46:35 +0000
Subject: [PATCH] PERF: Improve `Accept` header handling for stylesheets and
 theme-js (#19357)

The default behavior for Rails is to vary the response of an endpoint based on the `Accept:` header, and therefore it returns a `Vary:` header on responses. This instructs browsers and intermediate proxies to key their caches based on the value of the request's `Accept` header. In some cases (e.g. Akamai), the presence of a `Vary` header is enough to prevent caching entirely.

This commit restructures the Rails route definitions so that:
1. The "format" segment of the route is 'required'
2. The "format" segment of the route is constrained to a single value (e.g. `js` or `css`)

Now that the routes are guaranteed to have a `:format` segment, Rails will always prioritize that over the `Accept` header, and will therefore omit the `Vary` header.

Request specs are also added to test this behaviour for both stylesheets and theme-javascripts.
---
 config/routes.rb                              |  8 +++----
 spec/requests/stylesheets_controller_spec.rb  | 24 +++++++++++++++++++
 .../theme_javascripts_controller_spec.rb      | 19 +++++++++++++++
 3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/config/routes.rb b/config/routes.rb
index 3a557741405..e70f12b5a9c 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -561,11 +561,11 @@ Discourse::Application.routes.draw do
 
     get "highlight-js/:hostname/:version.js" => "highlight_js#show", constraints: { hostname: /[\w\.-]+/, format: :js }
 
-    get "stylesheets/:name.css.map" => "stylesheets#show_source_map", constraints: { name: /[-a-z0-9_]+/ }
-    get "stylesheets/:name.css" => "stylesheets#show", constraints: { name: /[-a-z0-9_]+/ }
+    get "stylesheets/:name" => "stylesheets#show_source_map", constraints: { name: /[-a-z0-9_]+/, format: /css\.map/ }, format: true
+    get "stylesheets/:name" => "stylesheets#show", constraints: { name: /[-a-z0-9_]+/, format: "css" }, format: true
     get "color-scheme-stylesheet/:id(/:theme_id)" => "stylesheets#color_scheme", constraints: { format: :json }
-    get "theme-javascripts/:digest.js" => "theme_javascripts#show", constraints: { digest: /\h{40}/ }
-    get "theme-javascripts/:digest.map" => "theme_javascripts#show_map", constraints: { digest: /\h{40}/ }
+    get "theme-javascripts/:digest" => "theme_javascripts#show", constraints: { digest: /\h{40}/, format: :js }, format: true
+    get "theme-javascripts/:digest" => "theme_javascripts#show_map", constraints: { digest: /\h{40}/, format: :map }, format: true
     get "theme-javascripts/tests/:theme_id-:digest.js" => "theme_javascripts#show_tests"
 
     post "uploads/lookup-metadata" => "uploads#metadata"
diff --git a/spec/requests/stylesheets_controller_spec.rb b/spec/requests/stylesheets_controller_spec.rb
index dac8e25e414..f5c0e3d0d61 100644
--- a/spec/requests/stylesheets_controller_spec.rb
+++ b/spec/requests/stylesheets_controller_spec.rb
@@ -59,6 +59,30 @@ RSpec.describe StylesheetsController do
     expect(response.status).to eq(200)
   end
 
+  it 'ignores Accept header and does not include Vary header' do
+    StylesheetCache.destroy_all
+    manager = Stylesheet::Manager.new(theme_id: nil)
+    builder = Stylesheet::Manager::Builder.new(target: 'desktop', manager: manager, theme: nil)
+    builder.compile
+
+    digest = StylesheetCache.first.digest
+
+    get "/stylesheets/desktop_#{digest}.css"
+    expect(response.status).to eq(200)
+    expect(response.headers["Content-Type"]).to eq("text/css")
+    expect(response.headers["Vary"]).to eq(nil)
+
+    get "/stylesheets/desktop_#{digest}.css", headers: { "Accept" => "text/html" }
+    expect(response.status).to eq(200)
+    expect(response.headers["Content-Type"]).to eq("text/css")
+    expect(response.headers["Vary"]).to eq(nil)
+
+    get "/stylesheets/desktop_#{digest}.css", headers: { "Accept" => "invalidcontenttype" }
+    expect(response.status).to eq(200)
+    expect(response.headers["Content-Type"]).to eq("text/css")
+    expect(response.headers["Vary"]).to eq(nil)
+  end
+
   describe "#color_scheme" do
     it 'works as expected' do
       scheme = ColorScheme.last
diff --git a/spec/requests/theme_javascripts_controller_spec.rb b/spec/requests/theme_javascripts_controller_spec.rb
index 56fe7b17cc9..f01afc2cd4a 100644
--- a/spec/requests/theme_javascripts_controller_spec.rb
+++ b/spec/requests/theme_javascripts_controller_spec.rb
@@ -75,6 +75,25 @@ RSpec.describe ThemeJavascriptsController do
         //# sourceMappingURL=#{digest}.map?__ws=test.localhost
       JS
     end
+
+    it "ignores Accept header and does not return Vary header" do
+      js_cache_url = "/theme-javascripts/#{javascript_cache.digest}.js"
+
+      get js_cache_url
+      expect(response.status).to eq(200)
+      expect(response.headers["Content-Type"]).to eq("text/javascript")
+      expect(response.headers["Vary"]).to eq(nil)
+
+      get js_cache_url, headers: { "Accept" => "text/html" }
+      expect(response.status).to eq(200)
+      expect(response.headers["Content-Type"]).to eq("text/javascript")
+      expect(response.headers["Vary"]).to eq(nil)
+
+      get js_cache_url, headers: { "Accept" => "invalidcontenttype" }
+      expect(response.status).to eq(200)
+      expect(response.headers["Content-Type"]).to eq("text/javascript")
+      expect(response.headers["Vary"]).to eq(nil)
+    end
   end
 
   describe "#show_map" do