From 4adc5937783bba0b52b52dd71742a5acf8d47cf8 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Mon, 13 May 2024 10:53:00 +0200 Subject: [PATCH] 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. --- .../json_api/admin/pages_controller.rb | 2 +- .../alchemy/json_api/pages_controller.rb | 21 +++-- .../json_api/admin/layout_pages_spec.rb | 2 +- .../alchemy/json_api/admin/pages_spec.rb | 2 +- spec/requests/alchemy/json_api/pages_spec.rb | 88 +++++++++++++++++-- 5 files changed, 98 insertions(+), 17 deletions(-) diff --git a/app/controllers/alchemy/json_api/admin/pages_controller.rb b/app/controllers/alchemy/json_api/admin/pages_controller.rb index 0bf5660..9f4071f 100644 --- a/app/controllers/alchemy/json_api/admin/pages_controller.rb +++ b/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 diff --git a/app/controllers/alchemy/json_api/pages_controller.rb b/app/controllers/alchemy/json_api/pages_controller.rb index fde1739..0e7a72b 100644 --- a/app/controllers/alchemy/json_api/pages_controller.rb +++ b/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) diff --git a/spec/requests/alchemy/json_api/admin/layout_pages_spec.rb b/spec/requests/alchemy/json_api/admin/layout_pages_spec.rb index fa209e2..c93dc69 100644 --- a/spec/requests/alchemy/json_api/admin/layout_pages_spec.rb +++ b/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 diff --git a/spec/requests/alchemy/json_api/admin/pages_spec.rb b/spec/requests/alchemy/json_api/admin/pages_spec.rb index 939b761..e3aba0a 100644 --- a/spec/requests/alchemy/json_api/admin/pages_spec.rb +++ b/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 diff --git a/spec/requests/alchemy/json_api/pages_spec.rb b/spec/requests/alchemy/json_api/pages_spec.rb index 695d39d..61d322a 100644 --- a/spec/requests/alchemy/json_api/pages_spec.rb +++ b/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