From 87167de8c5c0c381feb2dcbc96f6e697e7d4b06f Mon Sep 17 00:00:00 2001
From: rhymes <rhymesete@gmail.com>
Date: Thu, 23 Jan 2020 14:40:19 +0100
Subject: [PATCH] Use correct cache keys on the articles API index endpoint
 (#5606) [deploy]

Surrogate keys are about setting expectations on cache purging.

Presently we use a combination of all parameters to set changing surrogate keys depending on the result of the `ArticleApiIndexService`. This approach has two limitations:

- if two set of params lead to the same exact set of articles, we still create two different caches

- deleted articles will remain for 24 hours in the cache because the cache key doesn't take the record keys into account

By simply using record keys we make sure that different params leading to the same result set will share the same cache and that deleted keys won't appear anymore in the cache.
---
 app/controllers/api/v0/articles_controller.rb |  11 +-
 app/labor/cache_buster.rb                     |   2 +
 spec/requests/api/v0/articles_spec.rb         | 172 +++++++++++-------
 3 files changed, 110 insertions(+), 75 deletions(-)

diff --git a/app/controllers/api/v0/articles_controller.rb b/app/controllers/api/v0/articles_controller.rb
index e733e308d..3f0430c7f 100644
--- a/app/controllers/api/v0/articles_controller.rb
+++ b/app/controllers/api/v0/articles_controller.rb
@@ -21,16 +21,7 @@ module Api
       def index
         @articles = ArticleApiIndexService.new(params).get
 
-        key_headers = [
-          "articles_api",
-          params[:tag],
-          params[:page],
-          params[:username],
-          params[:signature],
-          params[:state],
-          params[:collection_id],
-        ]
-        set_surrogate_key_header key_headers.join("_")
+        set_surrogate_key_header "articles", @articles.map(&:record_key)
       end
 
       def show
diff --git a/app/labor/cache_buster.rb b/app/labor/cache_buster.rb
index 159648965..19c107bc9 100644
--- a/app/labor/cache_buster.rb
+++ b/app/labor/cache_buster.rb
@@ -33,6 +33,8 @@ module CacheBuster
   end
 
   def self.bust_article(article)
+    article.purge
+
     bust(article.path)
     bust("/#{article.user.username}")
     bust("#{article.path}/")
diff --git a/spec/requests/api/v0/articles_spec.rb b/spec/requests/api/v0/articles_spec.rb
index 3d472537a..c424df2c5 100644
--- a/spec/requests/api/v0/articles_spec.rb
+++ b/spec/requests/api/v0/articles_spec.rb
@@ -1,12 +1,8 @@
 require "rails_helper"
 
 RSpec.describe "Api::V0::Articles", type: :request do
-  let_it_be(:organization) { create(:organization) } # not used by every spec but lower times overall
-  let_it_be(:article) { create(:article, featured: true, tags: "discuss") }
-
-  def json_response
-    JSON.parse(response.body)
-  end
+  let_it_be_readonly(:organization) { create(:organization) } # not used by every spec but lower times overall
+  let_it_be_changeable(:article) { create(:article, featured: true, tags: "discuss") }
 
   describe "GET /api/articles" do
     it "has correct keys in the response" do
@@ -20,19 +16,19 @@ RSpec.describe "Api::V0::Articles", type: :request do
         published_timestamp user organization flare_tag
       ]
 
-      expect(json_response.first.keys).to match_array index_keys
+      expect(response.parsed_body.first.keys).to match_array index_keys
     end
 
     it "returns correct tag list" do
       get api_articles_path
 
-      expect(json_response.first["tag_list"]).to be_a_kind_of Array
+      expect(response.parsed_body.first["tag_list"]).to be_a_kind_of Array
     end
 
     it "returns correct tags" do
       get api_articles_path
 
-      expect(json_response.first["tags"]).to be_a_kind_of String
+      expect(response.parsed_body.first["tags"]).to be_a_kind_of String
     end
 
     context "without params" do
@@ -43,68 +39,83 @@ RSpec.describe "Api::V0::Articles", type: :request do
 
       it "returns nothing if params state=all is not found" do
         get api_articles_path(state: "all")
-        expect(json_response.size).to eq(0)
+        expect(response.parsed_body.size).to eq(0)
       end
 
       it "returns featured articles if no param is given" do
         article.update_column(:featured, true)
         get api_articles_path
-        expect(json_response.size).to eq(1)
+        expect(response.parsed_body.size).to eq(1)
       end
 
       it "supports pagination" do
         create_list(:article, 2, featured: true)
         get api_articles_path, params: { page: 1, per_page: 2 }
-        expect(json_response.length).to eq(2)
+        expect(response.parsed_body.length).to eq(2)
         get api_articles_path, params: { page: 2, per_page: 2 }
-        expect(json_response.length).to eq(1)
+        expect(response.parsed_body.length).to eq(1)
       end
 
       it "returns flare tag in the response" do
         get api_articles_path
-        response_article = JSON.parse(response.body)[0]
+        response_article = response.parsed_body.first
         expect(response_article["flare_tag"]).to be_present
         expect(response_article["flare_tag"].keys).to eq(%w[name bg_color_hex text_color_hex])
         expect(response_article["flare_tag"]["name"]).to eq("discuss")
       end
+
+      it "sets the correct edge caching surrogate key" do
+        get api_articles_path
+
+        expected_key = ["articles", article.record_key].to_set
+        expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key)
+      end
     end
 
     context "with username param" do
       it "returns user's articles for the given username" do
         create(:article, user: article.user)
         get api_articles_path(username: article.user.username)
-        expect(json_response.size).to eq(2)
+        expect(response.parsed_body.size).to eq(2)
       end
 
       it "returns nothing if given user is not found" do
         get api_articles_path(username: "foobar")
-        expect(json_response.size).to eq(0)
+        expect(response.parsed_body.size).to eq(0)
       end
 
       it "returns org's articles if org's slug is given" do
         create(:article, user: article.user, organization: organization)
         get api_articles_path(username: organization.slug)
-        expect(json_response.size).to eq(1)
+        expect(response.parsed_body.size).to eq(1)
       end
 
       it "supports pagination" do
         create_list(:article, 2, user: article.user)
         get api_articles_path(username: article.user.username), params: { page: 1, per_page: 2 }
-        expect(json_response.length).to eq(2)
+        expect(response.parsed_body.length).to eq(2)
         get api_articles_path(username: article.user.username), params: { page: 2, per_page: 2 }
-        expect(json_response.length).to eq(1)
+        expect(response.parsed_body.length).to eq(1)
+      end
+
+      it "sets the correct edge caching surrogate key" do
+        new_article = create(:article, user: article.user)
+        get api_articles_path(username: article.user.username)
+
+        expected_key = ["articles", article.record_key, new_article.record_key].to_set
+        expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key)
       end
     end
 
     context "with tag param" do
       it "returns tag's articles" do
         get api_articles_path(tag: article.tag_list.first)
-        expect(json_response.size).to eq(1)
+        expect(response.parsed_body.size).to eq(1)
       end
 
       it "returns top tag articles if tag and top param is present" do
         get api_articles_path(tag: article.tag_list.first, top: "7")
-        expect(json_response.size).to eq(1)
+        expect(response.parsed_body.size).to eq(1)
       end
 
       it "returns not tag articles if article and tag are not approved" do
@@ -113,15 +124,22 @@ RSpec.describe "Api::V0::Articles", type: :request do
         tag.update(requires_approval: true)
 
         get api_articles_path(tag: tag.name)
-        expect(JSON.parse(response.body).size).to eq(0)
+        expect(response.parsed_body.size).to eq(0)
       end
 
       it "supports pagination" do
         create_list(:article, 2, tags: "discuss")
         get api_articles_path(tag: article.tag_list.first), params: { page: 1, per_page: 2 }
-        expect(json_response.length).to eq(2)
+        expect(response.parsed_body.length).to eq(2)
         get api_articles_path(tag: article.tag_list.first), params: { page: 2, per_page: 2 }
-        expect(json_response.length).to eq(1)
+        expect(response.parsed_body.length).to eq(1)
+      end
+
+      it "sets the correct edge caching surrogate key" do
+        get api_articles_path(tag: article.tag_list.first)
+
+        expected_key = ["articles", article.record_key].to_set
+        expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key)
       end
     end
 
@@ -131,7 +149,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
         old_article = create(:article)
         old_article.update_column(:published_at, 10.days.ago)
         get api_articles_path(top: "7")
-        expect(json_response.size).to eq(1)
+        expect(response.parsed_body.size).to eq(1)
       end
 
       it "supports pagination" do
@@ -140,9 +158,16 @@ RSpec.describe "Api::V0::Articles", type: :request do
           old_article.update_column(:published_at, 10.days.ago)
         end
         get api_articles_path(top: "11"), params: { page: 1, per_page: 2 }
-        expect(json_response.length).to eq(2)
+        expect(response.parsed_body.length).to eq(2)
         get api_articles_path(top: "11"), params: { page: 2, per_page: 2 }
-        expect(json_response.length).to eq(1)
+        expect(response.parsed_body.length).to eq(1)
+      end
+
+      it "sets the correct edge caching surrogate key" do
+        get api_articles_path(top: "7")
+
+        expected_key = ["articles", article.record_key].to_set
+        expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key)
       end
     end
 
@@ -151,7 +176,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
         collection = create(:collection, user: article.user)
         article.update_columns(collection_id: collection.id)
         get api_articles_path(collection_id: collection.id)
-        expect(json_response[0]["collection_id"]).to eq collection.id
+        expect(response.parsed_body[0]["collection_id"]).to eq collection.id
       end
 
       it "supports pagination" do
@@ -162,9 +187,18 @@ RSpec.describe "Api::V0::Articles", type: :request do
           collection_article.update_columns(collection_id: collection.id)
         end
         get api_articles_path(collection_id: collection.id), params: { page: 1, per_page: 2 }
-        expect(json_response.length).to eq(2)
+        expect(response.parsed_body.length).to eq(2)
         get api_articles_path(collection_id: collection.id), params: { page: 2, per_page: 2 }
-        expect(json_response.length).to eq(1)
+        expect(response.parsed_body.length).to eq(1)
+      end
+
+      it "sets the correct edge caching surrogate key" do
+        collection = create(:collection, user: article.user)
+        article.update_columns(collection_id: collection.id)
+        get api_articles_path(collection_id: collection.id)
+
+        expected_key = ["articles", article.record_key].to_set
+        expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key)
       end
     end
 
@@ -193,9 +227,17 @@ RSpec.describe "Api::V0::Articles", type: :request do
         create_list(:article, 2, tags: "discuss", positive_reactions_count: 1, score: 1)
 
         get api_articles_path(state: "fresh"), params: { page: 1, per_page: 2 }
-        expect(json_response.length).to eq(2)
+        expect(response.parsed_body.length).to eq(2)
         get api_articles_path(state: "fresh"), params: { page: 2, per_page: 2 }
-        expect(json_response.length).to eq(1)
+        expect(response.parsed_body.length).to eq(1)
+      end
+
+      it "sets the correct edge caching surrogate key" do
+        article.update_columns(positive_reactions_count: 1, score: 1)
+
+        get api_articles_path(state: "fresh")
+        expected_key = ["articles", article.record_key].to_set
+        expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key)
       end
     end
   end
@@ -212,24 +254,24 @@ RSpec.describe "Api::V0::Articles", type: :request do
         published_timestamp body_html body_markdown user organization flare_tag
       ]
 
-      expect(json_response.keys).to match_array show_keys
+      expect(response.parsed_body.keys).to match_array show_keys
     end
 
     it "returns correct tag list" do
       get api_article_path(article.id)
 
-      expect(json_response["tag_list"]).to be_a_kind_of String
+      expect(response.parsed_body["tag_list"]).to be_a_kind_of String
     end
 
     it "returns correct tags" do
       get api_article_path(article.id)
 
-      expect(json_response["tags"]).to be_a_kind_of Array
+      expect(response.parsed_body["tags"]).to be_a_kind_of Array
     end
 
     it "returns proper article" do
       get api_article_path(article.id)
-      expect(json_response).to include(
+      expect(response.parsed_body).to include(
         "title" => article.title,
         "body_markdown" => article.body_markdown,
         "tags" => article.decorate.cached_tag_list_array,
@@ -241,7 +283,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
         edited_at: 1.minute.from_now, crossposted_at: 2.minutes.ago, last_comment_at: 30.seconds.ago,
       )
       get api_article_path(article.id)
-      expect(json_response).to include(
+      expect(response.parsed_body).to include(
         "created_at" => article.created_at.utc.iso8601,
         "edited_at" => article.edited_at.utc.iso8601,
         "crossposted_at" => article.crossposted_at.utc.iso8601,
@@ -312,32 +354,32 @@ RSpec.describe "Api::V0::Articles", type: :request do
         create(:article, user: user)
         create(:article)
         get me_api_articles_path, params: { access_token: access_token.token }
-        expect(json_response.length).to eq(1)
-        expect(json_response[0]["body_markdown"]).not_to be_nil
+        expect(response.parsed_body.length).to eq(1)
+        expect(response.parsed_body[0]["body_markdown"]).not_to be_nil
       end
 
       it "supports pagination" do
         create_list(:article, 3, user: user)
         get me_api_articles_path, params: { access_token: access_token.token, page: 2, per_page: 2 }
-        expect(json_response.length).to eq(1)
+        expect(response.parsed_body.length).to eq(1)
       end
 
       it "only includes published articles by default" do
         create(:article, published: false, published_at: nil, user: user)
         get me_api_articles_path, params: { access_token: access_token.token }
-        expect(json_response.length).to eq(0)
+        expect(response.parsed_body.length).to eq(0)
       end
 
       it "only includes published articles when asking for published articles" do
         create(:article, published: false, published_at: nil, user: user)
         get me_api_articles_path(status: :published), params: { access_token: access_token.token }
-        expect(json_response.length).to eq(0)
+        expect(response.parsed_body.length).to eq(0)
       end
 
       it "only includes unpublished articles when asking for unpublished articles" do
         create(:article, published: false, published_at: nil, user: user)
         get me_api_articles_path(status: :unpublished), params: { access_token: access_token.token }
-        expect(json_response.length).to eq(1)
+        expect(response.parsed_body.length).to eq(1)
       end
 
       it "orders unpublished articles by reverse order when asking for unpublished articles" do
@@ -347,7 +389,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           newer = create(:article, published: false, published_at: nil, user: user)
         end
         get me_api_articles_path(status: :unpublished), params: { access_token: access_token.token }
-        expected_order = json_response.map { |resp| resp["id"] }
+        expected_order = response.parsed_body.map { |resp| resp["id"] }
         expect(expected_order).to eq([newer.id, older.id])
       end
 
@@ -355,7 +397,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
         create(:article, user: user)
         create(:article, published: false, published_at: nil, user: user)
         get me_api_articles_path(status: :all), params: { access_token: access_token.token }
-        expected_order = json_response.map { |resp| resp["published"] }
+        expected_order = response.parsed_body.map { |resp| resp["published"] }
         expect(expected_order).to eq([false, true])
       end
     end
@@ -407,7 +449,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
         tags = %w[meta discussion]
         post_article(body_markdown: "Yo ho ho", tags: tags)
         expect(response).to have_http_status(:unprocessable_entity)
-        expect(json_response["error"]).to be_present
+        expect(response.parsed_body["error"]).to be_present
       end
 
       it "fails if article contains tags with non-alphanumeric characters" do
@@ -419,13 +461,13 @@ RSpec.describe "Api::V0::Articles", type: :request do
       it "creates an article belonging to the user" do
         post_article(title: Faker::Book.title)
         expect(response).to have_http_status(:created)
-        expect(Article.find(json_response["id"]).user).to eq(user)
+        expect(Article.find(response.parsed_body["id"]).user).to eq(user)
       end
 
       it "creates an unpublished article by default" do
         post_article(title: Faker::Book.title)
         expect(response).to have_http_status(:created)
-        expect(Article.find(json_response["id"]).published).to be(false)
+        expect(Article.find(response.parsed_body["id"]).published).to be(false)
       end
 
       it "returns the location of the article" do
@@ -440,7 +482,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           post_article(title: title)
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        expect(Article.find(json_response["id"]).title).to eq(title)
+        expect(Article.find(response.parsed_body["id"]).title).to eq(title)
       end
 
       it "creates a published article" do
@@ -449,7 +491,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           post_article(title: title, published: true)
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        expect(Article.find(json_response["id"]).published).to be(true)
+        expect(Article.find(response.parsed_body["id"]).published).to be(true)
       end
 
       it "creates an article with a title and the markdown body" do
@@ -461,7 +503,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           )
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        expect(Article.find(json_response["id"]).body_markdown).to eq(body_markdown)
+        expect(Article.find(response.parsed_body["id"]).body_markdown).to eq(body_markdown)
       end
 
       it "creates an article with a title, body and a list of tags" do
@@ -474,7 +516,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           )
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        expect(Article.find(json_response["id"]).cached_tag_list).to eq(tags.join(", "))
+        expect(Article.find(response.parsed_body["id"]).cached_tag_list).to eq(tags.join(", "))
       end
 
       it "creates an unpublished article with the front matter in the body" do
@@ -483,7 +525,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           post_article(body_markdown: body_markdown)
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        article = Article.find(json_response["id"])
+        article = Article.find(response.parsed_body["id"])
         expect(article.title).to eq("Sample Article")
         expect(article.published).to be(false)
       end
@@ -494,7 +536,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           post_article(body_markdown: body_markdown)
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        article = Article.find(json_response["id"])
+        article = Article.find(response.parsed_body["id"])
         expect(article.title).to eq("Sample Article")
         expect(article.published).to be(true)
       end
@@ -507,7 +549,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           series: series,
         )
         expect(response).to have_http_status(:created)
-        article = Article.find(json_response["id"])
+        article = Article.find(response.parsed_body["id"])
         expect(article.collection).to eq(Collection.find_by(slug: series))
         expect(article.collection.user).to eq(user)
       end
@@ -518,7 +560,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           post_article(body_markdown: body_markdown)
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        article = Article.find(json_response["id"])
+        article = Article.find(response.parsed_body["id"])
         expect(article.collection).to eq(Collection.find_by(slug: "a series"))
         expect(article.collection.user).to eq(user)
       end
@@ -533,7 +575,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           )
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        expect(Article.find(json_response["id"]).organization).to eq(organization)
+        expect(Article.find(response.parsed_body["id"]).organization).to eq(organization)
       end
 
       it "creates an article with a main/cover image" do
@@ -546,7 +588,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           )
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        expect(Article.find(json_response["id"]).main_image).to eq(image_url)
+        expect(Article.find(response.parsed_body["id"]).main_image).to eq(image_url)
       end
 
       it "creates an article with a main/cover image in the front matter" do
@@ -556,7 +598,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           post_article(body_markdown: body_markdown)
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        expect(Article.find(json_response["id"]).main_image).to eq(image_url)
+        expect(Article.find(response.parsed_body["id"]).main_image).to eq(image_url)
       end
 
       it "creates an article with a canonical url" do
@@ -569,7 +611,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           )
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        expect(Article.find(json_response["id"]).canonical_url).to eq(canonical_url)
+        expect(Article.find(response.parsed_body["id"]).canonical_url).to eq(canonical_url)
       end
 
       it "creates an article with a canonical url in the front matter" do
@@ -579,7 +621,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           post_article(body_markdown: body_markdown)
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        expect(Article.find(json_response["id"]).canonical_url).to eq(canonical_url)
+        expect(Article.find(response.parsed_body["id"]).canonical_url).to eq(canonical_url)
       end
 
       it "creates an article with the given description" do
@@ -592,7 +634,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           )
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        expect(Article.find(json_response["id"]).description).to eq(description)
+        expect(Article.find(response.parsed_body["id"]).description).to eq(description)
       end
 
       it "creates an article with description in the front matter" do
@@ -605,7 +647,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           )
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        expect(Article.find(json_response["id"]).description).not_to eq(description)
+        expect(Article.find(response.parsed_body["id"]).description).not_to eq(description)
       end
 
       it "creates an article with a part of the body as a description" do
@@ -616,7 +658,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
           )
           expect(response).to have_http_status(:created)
         end.to change(Article, :count).by(1)
-        expect(Article.find(json_response["id"]).description).to eq("yooo" * 20 + "y...")
+        expect(Article.find(response.parsed_body["id"]).description).to eq("yooo" * 20 + "y...")
       end
 
       it "does not raise an error if article params are missing" do
@@ -695,7 +737,7 @@ RSpec.describe "Api::V0::Articles", type: :request do
         put_article(title: Faker::Book.title)
         expect(response).to have_http_status(:ok)
         expect(article.reload.title).to eq(article.title)
-        expect(json_response["title"]).to eq(article.title)
+        expect(response.parsed_body["title"]).to eq(article.title)
       end
 
       it "updates the title and the body if given a title and a body" do