Do not set a Last-Modified header

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.

app/controllers/alchemy/json_api/pages_controller.rb

@@ -3,16 +3,20 @@
 module Alchemy
   module JsonApi
     class PagesController < JsonApi::BaseController
+      JSONAPI_STALEMAKERS = [:include, :fields, :sort, :filter, :page]
+
       before_action :load_page_for_cache_key, only: :show
 
       def index
         allowed = Alchemy::Page.ransackable_attributes
 
         jsonapi_filter(page_scope, allowed) do |filtered_pages|
           @pages = filtered_pages.result
+          last_modified_page = @pages.select { |page| last_modified_for(page) }.max_by { |page| last_modified_for(page) }
+
           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(last_modified_page))
             render_pages_json(allowed)
           end
         end
@@ -25,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: @page.cache_key_with_version)
           # Only load page with all includes when browser cache is stale
           render jsonapi: api_page(load_page)
         end
@@ -61,7 +65,8 @@ def load_page_for_cache_key
       end
 
       def last_modified_for(page)
-        page.published_at
+        return unless page
+        page.respond_to?(:last_modified_at) ? page.last_modified_at : page.published_at
       end
 
       def jsonapi_meta(pages)
@@ -117,6 +122,12 @@ def api_page(page)
         Alchemy::JsonApi::Page.new(page, page_version_type: page_version_type)
       end
 
+      def etag(page)
+        return unless page
+        relevant_params = params.slice(JSONAPI_STALEMAKERS).values.compact.join("")
+        page.cache_key_with_version.concat(relevant_params)
+      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/pages_spec.rb

@@ -43,11 +43,32 @@
 
         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)
+          expect(response.headers["Last-Modified"]).to be_nil
           expect(response.headers["ETag"]).to eq('W/"0741fe32d81bfdabfeb47d9939c5f6b7"')
           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.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
           let(:page) do
             FactoryBot.create(
@@ -178,9 +199,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 +214,46 @@
 
           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: :asc})
+            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
+
           context "if one page is restricted" do
             let!(:restricted_page) do
               FactoryBot.create(
@@ -306,8 +362,8 @@
 
         it "sets cache headers of latest matching page" 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["Last-Modified"]).to be_nil
+          expect(response.headers["ETag"]).to eq('W/"becfffe3f6f8b8cc33e1ffc2f70d8869"')
           expect(response.headers["Cache-Control"]).to eq("max-age=10800, public, must-revalidate")
         end
       end