From 62fa05b7c162d144ddee5fa76b3df4e7e24f0979 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Tue, 7 May 2024 16:57:54 +0200 Subject: [PATCH 1/4] Use page version's `updated_at` timestamp as cache key Currently, it's hard for us to invalidate caches for Alchemy pages when content that's referenced through ingredients with related objects changes. For example, in a situation where a user combines Alchemy and Solidus using the `alchemy_solidus` gem, a page's cache key does not update when a product that's referenced through a SpreeProduct ingredient changes. There's a PR up on the alchemy_solidus gem that touches ingredients in these situations, and with this change, that touching can be used for breaking caches. [1] In the unlikely event the requested version is not available, we use the page's updated_at timestamp as a fallback. The core of the logic has been moved to a new `#last_modified_at` method that can be used for the last-modified header in HTTP responses. [1](https://github.com/AlchemyCMS/alchemy-solidus/pull/108) --- app/models/alchemy/page/page_natures.rb | 23 +++++---- spec/models/alchemy/page_spec.rb | 62 ++++++++++++++++++++++--- 2 files changed, 70 insertions(+), 15 deletions(-) diff --git a/app/models/alchemy/page/page_natures.rb b/app/models/alchemy/page/page_natures.rb index 728d2c54af..e04d53b0cd 100644 --- a/app/models/alchemy/page/page_natures.rb +++ b/app/models/alchemy/page/page_natures.rb @@ -101,18 +101,23 @@ def layout_partial_name page_layout.parameterize.underscore end - # Returns the version that's taken for Rails' recycable cache key. + # Returns the string version of the +last_modified_at+ timestamp. # - # Uses the +published_at+ value that's updated when the user publishes the page. + def cache_version + last_modified_at.to_s + end + + # Returns the timestamp that the page was last modified at, regardless of through. + # publishing or editing page, or through a change of related objects through ingredients. + # Respects the public version not changing if editing a preview. # - # If the page is the current preview it uses the +updated_at+ value as cache key. + # In preview mode, it will take the draft version's updated_at timestamp. + # In public mode, it will take the public version's updated_at timestamp. + # If no version is available, it will take the page's updated_at timestamp. # - def cache_version - if Current.preview_page == self - updated_at.to_s - else - published_at.to_s - end + def last_modified_at + relevant_page_version = (Current.preview_page == self) ? draft_version : public_version + relevant_page_version&.updated_at || updated_at end # Returns true if the page cache control headers should be set. diff --git a/spec/models/alchemy/page_spec.rb b/spec/models/alchemy/page_spec.rb index 30e50c6c3f..c415a2e4a5 100644 --- a/spec/models/alchemy/page_spec.rb +++ b/spec/models/alchemy/page_spec.rb @@ -704,14 +704,36 @@ module Alchemy end describe "#cache_version" do + let(:page) { build(:alchemy_page) } + + before do + allow(page).to receive(:last_modified_at).and_return(1.day.ago) + end + + around do |example| + travel_to(Time.parse("2019-01-01 12:00:00")) do + example.run + end + end + + it "returns a cache version string" do + expect(page.cache_version).to eq("2018-12-31 11:00:00 UTC") + end + end + + describe "#last_modified_at" do let(:now) { Time.current } + let(:yesterday) { Time.current - 1.day } let(:last_week) { Time.current - 1.week } let(:page) do - build_stubbed(:alchemy_page, updated_at: now, published_at: last_week) + build_stubbed(:alchemy_page, public_version: public_version, draft_version: draft_version, updated_at: yesterday) end - subject { page.cache_version } + let(:public_version) { build_stubbed(:alchemy_page_version, updated_at: last_week) } + let(:draft_version) { build_stubbed(:alchemy_page_version, updated_at: now) } + + subject { page.last_modified_at } before do expect(Current).to receive(:preview_page).and_return(preview) @@ -720,16 +742,44 @@ module Alchemy context "when current page rendered in preview mode" do let(:preview) { page } - it "uses updated_at" do - is_expected.to eq(now.to_s) + it "uses draft version's updated_at" do + is_expected.to be_within(1.second).of(now) end end context "when current page not in preview mode" do let(:preview) { nil } - it "uses published_at" do - is_expected.to eq(last_week.to_s) + it "uses public version's updated at" do + is_expected.to be_within(1.second).of(last_week) + end + end + + context "if page has no public version" do + let(:public_version) { nil } + + context "in preview mode" do + let(:preview) { page } + + it "uses draft versions updated_at" do + is_expected.to be_within(1.second).of(now) + end + + context "if page has no draft version" do + let(:draft_version) { nil } + + it "uses page's updated_at" do + is_expected.to be_within(1.second).of(yesterday) + end + end + end + + context "not in preview mode" do + let(:preview) { nil } + + it "uses page's updated_at" do + is_expected.to be_within(1.second).of(yesterday) + end end end end From 7383a02d166b652107f837fe3d1e3d0c97ca2f44 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Mon, 13 May 2024 12:28:56 +0200 Subject: [PATCH 2/4] Touch page version's updated at when updating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without this, the previous commit will not do what we expect. If we don´t update the page version's updated at, it will not serve as a good cache invalidator. --- app/models/alchemy/page/publisher.rb | 1 + spec/models/alchemy/page/publisher_spec.rb | 9 ++++++++- spec/models/alchemy/page_spec.rb | 4 ++-- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/app/models/alchemy/page/publisher.rb b/app/models/alchemy/page/publisher.rb index 62b76907f8..766438de14 100644 --- a/app/models/alchemy/page/publisher.rb +++ b/app/models/alchemy/page/publisher.rb @@ -35,6 +35,7 @@ def publish!(public_on:) end end end + version.update(updated_at: public_on) page.update(published_at: public_on) end end diff --git a/spec/models/alchemy/page/publisher_spec.rb b/spec/models/alchemy/page/publisher_spec.rb index dc7962547d..de09eff093 100644 --- a/spec/models/alchemy/page/publisher_spec.rb +++ b/spec/models/alchemy/page/publisher_spec.rb @@ -55,8 +55,9 @@ end context "with published version existing" do + let(:yesterday) { Date.yesterday.to_time } let!(:public_version) do - create(:alchemy_page_version, :with_elements, element_count: 3, public_on: Date.yesterday.to_time, page: page) + create(:alchemy_page_version, :with_elements, element_count: 3, public_on: yesterday, page: page) end let!(:nested_element) do @@ -67,6 +68,12 @@ expect { publish }.to_not change(page.public_version, :public_on) end + it "updates public version's updated_at timestamp" do + # Need to do this here, because the nested element touches the version on creation. + public_version.update_columns(updated_at: yesterday) + expect { publish }.to change(page.public_version, :updated_at) + end + it "does not create another public version" do expect { publish }.to_not change(page.versions, :count) end diff --git a/spec/models/alchemy/page_spec.rb b/spec/models/alchemy/page_spec.rb index c415a2e4a5..4f6c00433b 100644 --- a/spec/models/alchemy/page_spec.rb +++ b/spec/models/alchemy/page_spec.rb @@ -711,13 +711,13 @@ module Alchemy end around do |example| - travel_to(Time.parse("2019-01-01 12:00:00")) do + travel_to(Time.parse("2019-01-01 12:00:00 UTC")) do example.run end end it "returns a cache version string" do - expect(page.cache_version).to eq("2018-12-31 11:00:00 UTC") + expect(page.cache_version).to eq("2018-12-31 12:00:00 UTC") end end From 74dfcab6a9119385a0104e706c2afbffc1c6d388 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Tue, 14 May 2024 13:48:42 +0200 Subject: [PATCH 3/4] Do not have a cache version if there's no version present We should not cache empty responses. --- app/models/alchemy/page/page_natures.rb | 4 ++-- spec/models/alchemy/page_spec.rb | 32 +++++++++++++++++-------- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/app/models/alchemy/page/page_natures.rb b/app/models/alchemy/page/page_natures.rb index e04d53b0cd..44c577d2c0 100644 --- a/app/models/alchemy/page/page_natures.rb +++ b/app/models/alchemy/page/page_natures.rb @@ -104,7 +104,7 @@ def layout_partial_name # Returns the string version of the +last_modified_at+ timestamp. # def cache_version - last_modified_at.to_s + last_modified_at&.to_s end # Returns the timestamp that the page was last modified at, regardless of through. @@ -117,7 +117,7 @@ def cache_version # def last_modified_at relevant_page_version = (Current.preview_page == self) ? draft_version : public_version - relevant_page_version&.updated_at || updated_at + relevant_page_version&.updated_at end # Returns true if the page cache control headers should be set. diff --git a/spec/models/alchemy/page_spec.rb b/spec/models/alchemy/page_spec.rb index 4f6c00433b..fd7c4fc617 100644 --- a/spec/models/alchemy/page_spec.rb +++ b/spec/models/alchemy/page_spec.rb @@ -706,18 +706,30 @@ module Alchemy describe "#cache_version" do let(:page) { build(:alchemy_page) } - before do - allow(page).to receive(:last_modified_at).and_return(1.day.ago) - end - around do |example| travel_to(Time.parse("2019-01-01 12:00:00 UTC")) do example.run end end - it "returns a cache version string" do - expect(page.cache_version).to eq("2018-12-31 12:00:00 UTC") + context "last modified is a time object" do + before do + allow(page).to receive(:last_modified_at).and_return(1.day.ago) + end + + it "returns a cache version string" do + expect(page.cache_version).to eq("2018-12-31 12:00:00 UTC") + end + end + + context "last modified at is nil" do + before do + allow(page).to receive(:last_modified_at).and_return(nil) + end + + it "returns a cache version string" do + expect(page.cache_version).to be(nil) + end end end @@ -768,8 +780,8 @@ module Alchemy context "if page has no draft version" do let(:draft_version) { nil } - it "uses page's updated_at" do - is_expected.to be_within(1.second).of(yesterday) + it "is nil" do + is_expected.to be(nil) end end end @@ -777,8 +789,8 @@ module Alchemy context "not in preview mode" do let(:preview) { nil } - it "uses page's updated_at" do - is_expected.to be_within(1.second).of(yesterday) + it "is nil" do + is_expected.to be(nil) end end end From 6787e9683506931dcf379309e910355d6ab1efec Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Tue, 14 May 2024 14:03:33 +0200 Subject: [PATCH 4/4] Use last-modified-at in PagesController --- app/controllers/alchemy/pages_controller.rb | 2 +- app/models/alchemy/page/page_natures.rb | 5 ++--- .../alchemy/page_request_caching_spec.rb | 16 +++++++--------- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/app/controllers/alchemy/pages_controller.rb b/app/controllers/alchemy/pages_controller.rb index e077919e37..67116df226 100644 --- a/app/controllers/alchemy/pages_controller.rb +++ b/app/controllers/alchemy/pages_controller.rb @@ -216,7 +216,7 @@ def page_etag def render_fresh_page? must_not_cache? || stale?( etag: page_etag, - last_modified: @page.published_at, + last_modified: @page.last_modified_at, public: !@page.restricted, template: "pages/show" ) diff --git a/app/models/alchemy/page/page_natures.rb b/app/models/alchemy/page/page_natures.rb index 44c577d2c0..e42cdc5c3e 100644 --- a/app/models/alchemy/page/page_natures.rb +++ b/app/models/alchemy/page/page_natures.rb @@ -101,19 +101,18 @@ def layout_partial_name page_layout.parameterize.underscore end - # Returns the string version of the +last_modified_at+ timestamp. + # Returns the version string that's taken for Rails' recycable cache key. # def cache_version last_modified_at&.to_s end - # Returns the timestamp that the page was last modified at, regardless of through. + # Returns the timestamp that the page was last modified at, regardless of through # publishing or editing page, or through a change of related objects through ingredients. # Respects the public version not changing if editing a preview. # # In preview mode, it will take the draft version's updated_at timestamp. # In public mode, it will take the public version's updated_at timestamp. - # If no version is available, it will take the page's updated_at timestamp. # def last_modified_at relevant_page_version = (Current.preview_page == self) ? draft_version : public_version diff --git a/spec/requests/alchemy/page_request_caching_spec.rb b/spec/requests/alchemy/page_request_caching_spec.rb index c0994c69a5..272c41a096 100644 --- a/spec/requests/alchemy/page_request_caching_spec.rb +++ b/spec/requests/alchemy/page_request_caching_spec.rb @@ -3,8 +3,7 @@ require "rails_helper" RSpec.describe "Page request caching" do - let!(:page) { create(:alchemy_page, :public, published_at: published_at) } - let(:published_at) { nil } + let!(:page) { create(:alchemy_page, :public) } context "when caching is disabled in app" do before do @@ -81,18 +80,17 @@ expect(response.headers).to have_key("ETag") end - it "does not set last-modified header" do - get "/#{page.urlname}" - expect(response.headers).to_not have_key("Last-Modified") - end + context "and public version is present" do + let(:jan_first) { Time.new(2020, 1, 1) } - context "and published_at is set" do - let(:published_at) { DateTime.yesterday } + before do + allow_any_instance_of(Alchemy::Page).to receive(:last_modified_at) { jan_first } + end it "sets last-modified header" do get "/#{page.urlname}" expect(response.headers).to have_key("Last-Modified") - expect(response.headers["Last-Modified"]).to eq(published_at.httpdate) + expect(response.headers["Last-Modified"]).to eq(jan_first.httpdate) end end end