Rely on improved etag for caching

The Last-Modified header is a weaker cache key than the etag, and thus
we rely solely on the etag from now on. Browsers and Rails will ignore
it if it's not set while validating if a request is fresh.

Also sets the etag based on JSONAPI's typical set of params, so that we
get a different etag for different `include`, `sort`, `page`, `fields`,
or `filter` params.

app/controllers/alchemy/json_api/admin/pages_controller.rb

@@ -25,7 +25,7 @@ def set_current_preview
           end
         end
 
-        def last_modified_for(page)
+        def page_cache_key(page)
           page.updated_at
         end
 

app/controllers/alchemy/json_api/pages_controller.rb

@@ -4,6 +4,7 @@ module Alchemy
   module JsonApi
     class PagesController < JsonApi::BaseController
       THREE_HOURS = 10800
+      JSONAPI_STALEMAKERS = %i[include fields sort filter page]
 
       before_action :load_page_for_cache_key, only: :show
 
@@ -12,9 +13,10 @@ def index
 
         jsonapi_filter(page_scope, allowed) do |filtered_pages|
           @pages = filtered_pages.result
+
           if !@pages.all?(&:cache_page?)
             render_pages_json(allowed) && return
-          elsif stale?(last_modified: @pages.maximum(:published_at), etag: @pages.max_by(&:cache_key_with_version)&.cache_key_with_version)
+          elsif stale?(etag: etag(@pages))
             render_pages_json(allowed)
           end
         end
@@ -27,7 +29,7 @@ def show
           render(jsonapi: api_page(load_page)) && return
         end
 
-        if stale?(last_modified: last_modified_for(@page), etag: @page.cache_key_with_version)
+        if stale?(etag: etag(@page))
           # Only load page with all includes when browser cache is stale
           render jsonapi: api_page(load_page)
         end
@@ -62,10 +64,6 @@ def load_page_for_cache_key
           .or(page_scope.where(urlname: params[:path])).first!
       end
 
-      def last_modified_for(page)
-        page.published_at
-      end
-
       def jsonapi_meta(pages)
         pagination = jsonapi_pagination_meta(pages)
 
@@ -119,6 +117,17 @@ def api_page(page)
         Alchemy::JsonApi::Page.new(page, page_version_type: page_version_type)
       end
 
+      def etag(pages)
+        pages = Array.wrap(pages)
+        return unless pages.any?
+        relevant_params = params.to_unsafe_hash.slice(*JSONAPI_STALEMAKERS).flatten.compact
+        pages.map { |page| page_cache_key(page) }.concat(relevant_params)
+      end
+
+      def page_cache_key(page)
+        page.cache_key_with_version
+      end
+
       def base_page_scope
         # cancancan is not able to merge our complex AR scopes for logged in users
         if can?(:edit_content, ::Alchemy::Page)

spec/requests/alchemy/json_api/admin/layout_pages_spec.rb

@@ -112,7 +112,7 @@
 
         it "sets cache headers" do
           get alchemy_json_api.admin_layout_page_path(page)
-          expect(response.headers["Last-Modified"]).to eq(page.updated_at.utc.httpdate)
+          expect(response.headers["Last-Modified"]).to be nil
           expect(response.headers["ETag"]).to match(/W\/".+"/)
           expect(response.headers["Cache-Control"]).to eq("max-age=0, private, must-revalidate")
         end

spec/requests/alchemy/json_api/admin/pages_spec.rb

@@ -35,7 +35,7 @@
 
         it "sets cache headers" do
           get alchemy_json_api.admin_page_path(page)
-          expect(response.headers["Last-Modified"]).to eq(page.updated_at.utc.httpdate)
+          expect(response.headers["Last-Modified"]).to be(nil)
           expect(response.headers["ETag"]).to match(/W\/".+"/)
           expect(response.headers["Cache-Control"]).to eq("max-age=0, private, must-revalidate")
         end

spec/requests/alchemy/json_api/pages_spec.rb

@@ -43,9 +43,33 @@
 
         it "sets public cache headers" do
           get alchemy_json_api.page_path(page)
-          expect(response.headers["Last-Modified"]).to eq(page.published_at.utc.httpdate)
+          first_etag = response.headers["Last-Modified"]
+          expect(response.headers["ETag"]).to match(/W\/".+"/)
           expect(response.headers["ETag"]).to eq('W/"0741fe32d81bfdabfeb47d9939c5f6b7"')
           expect(response.headers["Cache-Control"]).to eq("max-age=10800, public, must-revalidate")
+          get alchemy_json_api.page_path(page)
+          expect(response.headers["Last-Modified"]).to eq(first_etag)
+        end
+
+        it "returns a different etag if different filters are present" do
+          get alchemy_json_api.page_path(page)
+          etag = response.headers["ETag"]
+          get alchemy_json_api.pages_path(filter: {page_layout_eq: "standard"})
+          expect(response.headers["ETag"]).not_to eq(etag)
+        end
+
+        it "returns a different etag if different include params are present" do
+          get alchemy_json_api.page_path(page)
+          etag = response.headers["ETag"]
+          get alchemy_json_api.pages_path(include: "all_elements.ingredients")
+          expect(response.headers["ETag"]).not_to eq(etag)
+        end
+
+        it "returns a different etag if different fields params are present" do
+          get alchemy_json_api.page_path(page)
+          etag = response.headers["ETag"]
+          get alchemy_json_api.pages_path(fields: "urlname")
+          expect(response.headers["ETag"]).not_to eq(etag)
         end
 
         context "if page is restricted" do
@@ -178,9 +202,9 @@
 
   describe "GET /alchemy/json_api/pages" do
     context "with layoutpages and unpublished pages" do
-      let!(:layoutpage) { FactoryBot.create(:alchemy_page, :layoutpage, :public) }
-      let!(:non_public_page) { FactoryBot.create(:alchemy_page) }
-      let!(:public_page) { FactoryBot.create(:alchemy_page, :public, published_at: published_at) }
+      let!(:layoutpage) { FactoryBot.create(:alchemy_page, :layoutpage, :public, page_layout: "standard") }
+      let!(:non_public_page) { FactoryBot.create(:alchemy_page, page_layout: "standard") }
+      let!(:public_page) { FactoryBot.create(:alchemy_page, :public, published_at: published_at, page_layout: "standard") }
 
       context "as anonymous user" do
         let!(:pages) { [public_page] }
@@ -193,11 +217,53 @@
 
           it "sets public cache headers of latest published page" do
             get alchemy_json_api.pages_path
-            expect(response.headers["Last-Modified"]).to eq(pages.max_by(&:published_at).published_at.utc.httpdate)
+            expect(response.headers["Last-Modified"]).to be_nil
             expect(response.headers["ETag"]).to match(/W\/".+"/)
             expect(response.headers["Cache-Control"]).to eq("max-age=10800, public, must-revalidate")
           end
 
+          it "returns a different etag if different filters are present" do
+            get alchemy_json_api.pages_path
+            etag = response.headers["ETag"]
+            get alchemy_json_api.pages_path(filter: {page_layout_eq: "standard"})
+            expect(response.headers["ETag"]).not_to eq(etag)
+          end
+
+          it "returns a different etag if different sort params are present" do
+            get alchemy_json_api.pages_path
+            etag = response.headers["ETag"]
+            get alchemy_json_api.pages_path(sort: "-id")
+            expect(response.headers["ETag"]).not_to eq(etag)
+          end
+
+          it "returns a different etag if different include params are present" do
+            get alchemy_json_api.pages_path
+            etag = response.headers["ETag"]
+            get alchemy_json_api.pages_path(include: "all_elements.ingredients")
+            expect(response.headers["ETag"]).not_to eq(etag)
+          end
+
+          it "returns a different etag if different fields params are present" do
+            get alchemy_json_api.pages_path
+            etag = response.headers["ETag"]
+            get alchemy_json_api.pages_path(fields: "urlname")
+            expect(response.headers["ETag"]).not_to eq(etag)
+          end
+
+          it "returns a different etag if different fields params are present" do
+            get alchemy_json_api.pages_path
+            etag = response.headers["ETag"]
+            get alchemy_json_api.pages_path(page: {number: 2, size: 1})
+            expect(response.headers["ETag"]).not_to eq(etag)
+          end
+
+          it "returns a different etag if different JSONAPI params have the same value" do
+            get alchemy_json_api.pages_path(sort: "author")
+            etag = response.headers["ETag"]
+            get alchemy_json_api.pages_path(fields: "author")
+            expect(response.headers["ETag"]).not_to eq(etag)
+          end
+
           context "if one page is restricted" do
             let!(:restricted_page) do
               FactoryBot.create(
@@ -304,11 +370,17 @@
           stub_alchemy_config(:cache_pages, true)
         end
 
-        it "sets cache headers of latest matching page" do
+        it "sets constant etag" do
           get alchemy_json_api.pages_path(filter: {page_layout_eq: "news"})
-          expect(response.headers["Last-Modified"]).to eq(news_page2.published_at.utc.httpdate)
-          expect(response.headers["ETag"]).to eq('W/"e7a1c8beb22b58e94a605594d79766ad"')
+          expect(response.headers["ETag"]).to match(/W\/".+"/)
+
+          first_etag = response.headers["Last-Modified"]
+
+          expect(response.headers["ETag"]).to eq('W/"d7591eb3123d21c747fd712fe6c08e44"')
           expect(response.headers["Cache-Control"]).to eq("max-age=10800, public, must-revalidate")
+
+          get alchemy_json_api.pages_path(filter: {page_layout_eq: "news"})
+          expect(response.headers["Last-Modified"]).to eq(first_etag)
         end
       end
     end