Skip to content

Commit

Permalink
Merge pull request #80 from AlchemyCMS/use-updated-at-for-etag
Browse files Browse the repository at this point in the history
Improve etag generation
  • Loading branch information
tvdeyen authored May 15, 2024
2 parents 19af451 + 230fdea commit 1bd7cc0
Show file tree
Hide file tree
Showing 5 changed files with 97 additions and 18 deletions.
2 changes: 1 addition & 1 deletion app/controllers/alchemy/json_api/admin/pages_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def set_current_preview
end
end

def last_modified_for(page)
def page_cache_key(page)
page.updated_at
end

Expand Down
21 changes: 15 additions & 6 deletions app/controllers/alchemy/json_api/pages_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion spec/requests/alchemy/json_api/admin/layout_pages_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion spec/requests/alchemy/json_api/admin/pages_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
88 changes: 79 additions & 9 deletions spec/requests/alchemy/json_api/pages_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +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["ETag"]).to eq('W/"0741fe32d81bfdabfeb47d9939c5f6b7"')
first_etag = response.headers["Last-Modified"]
expect(response.headers["ETag"]).to match(/W\/".+"/)
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
Expand Down Expand Up @@ -178,9 +201,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] }
Expand All @@ -193,11 +216,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(
Expand Down Expand Up @@ -304,11 +369,16 @@
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["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
Expand Down

0 comments on commit 1bd7cc0

Please sign in to comment.