Comments API: fix caching issues (#4744) [deploy]

* Use ancestry in the index and add test

* Add tests for show action

* Return complete comments trees for index, kill n+1 and use correct edge cache key

* Return complete comments tree for show, kill n+1 and use correct edge cache key

* Refactor tests and add surrogate keys tests

* Preload users for the whole subtree

* Clarify explanation of the edge cache keys construction

* Refactor partials

* Remove action caching

* Replace .map.flatten with .flat_map

* Fix surrogate key prefix according to Fastly docs

* Add comment.purge to Comments::BustCacheJob to use Fastly-Rails

* Fix surrogate key spec

app/controllers/api/v0/comments_controller.rb

@@ -3,21 +3,42 @@ module V0
     class CommentsController < ApiController
       respond_to :json
 
-      caches_action :index,
-                    cache_path: proc { |c| c.params.permit! },
-                    expires_in: 10.minutes
-
-      caches_action :show,
-                    cache_path: proc { |c| c.params.permit! },
-                    expires_in: 10.minutes
+      before_action :set_cache_control_headers, only: %i[index show]
 
       def index
         article = Article.find(params[:a_id])
-        @comments = article.comments.roots.includes(:user).select(%i[id processed_html user_id ancestry])
+
+        @comments = article.comments.includes(:user).select(%i[id processed_html user_id ancestry]).arrange
+
+        set_surrogate_key_header "comments", edge_cache_keys(@comments)
       end
 
       def show
-        @comment = Comment.find(params[:id].to_i(26))
+        tree_with_root_comment = Comment.subtree_of(params[:id].to_i(26)).
+          includes(:user).
+          select(%i[id processed_html user_id ancestry]).
+          arrange
+
+        # being only one tree we know that the root comment is the first (and only) key
+        @comment = tree_with_root_comment.keys.first
+        @comments = tree_with_root_comment[@comment]
+
+        set_surrogate_key_header "comments", edge_cache_keys(tree_with_root_comment)
+      end
+
+      private
+
+      # ancestry wraps a single or multiple trees of comments into a single hash,
+      # in the case of an article comments, the hash has multiple keys (the root comments),
+      # in the case of a comment and its descendants, the hash has only one key.
+      # Either way, we need to use recursion to extract all the comment cache keys
+      # collecting both the keys of each level of root comments and their descendants
+      # NOTE: the objects are already loaded in memory by "ancestry",
+      # so no additional SQL query is performed during this extraction, avoiding N+1s
+      def edge_cache_keys(comments_trees)
+        comments_trees.keys.flat_map do |comment|
+          Array.wrap(comment.record_key) + edge_cache_keys(comments_trees[comment])
+        end
       end
     end
   end

app/jobs/comments/bust_cache_job.rb

@@ -6,6 +6,7 @@ def perform(comment_id, service = EdgeCache::Commentable::Bust)
       comment = Comment.find_by(id: comment_id)
       return unless comment&.commentable
 
+      comment.purge
       service.call(comment.commentable)
     end
   end

app/views/api/v0/comments/_comment.json.jbuilder

@@ -0,0 +1,6 @@
+json.type_of    "comment"
+
+json.id_code    comment.id_code_generated
+json.body_html  comment.processed_html
+
+json.partial! "api/v0/shared/user", user: comment.user

app/views/api/v0/comments/_comment_with_children.json.jbuilder

@@ -0,0 +1,6 @@
+json.partial! "comment", comment: comment
+
+# recursively render the comment subtree
+json.children do
+  json.partial! "comments_with_children", comments: children
+end

app/views/api/v0/comments/_comments_with_children.json.jbuilder

@@ -0,0 +1,4 @@
+json.array! comments.keys do |root_comment|
+  # ancestry organizes root comments and their descendants in a hash structure
+  json.partial! "comment_with_children", comment: root_comment, children: comments[root_comment]
+end

app/views/api/v0/comments/index.json.jbuilder

@@ -1,43 +1 @@
-json.array! @comments do |comment|
-  json.type_of            "comment"
-
-  json.id_code            comment.id_code_generated
-  json.body_html          comment.processed_html
-  json.user do
-    json.name             comment.user.name
-    json.username         comment.user.username
-    json.twitter_username comment.user.twitter_username
-    json.github_username  comment.user.github_username
-    json.website_url      comment.user.processed_website_url
-    json.profile_image    ProfileImage.new(comment.user).get(640)
-    json.profile_image_90 ProfileImage.new(comment.user).get(90)
-  end
-
-  json.children comment.children.order(score: :desc).each do |children_comment|
-    json.id_code            children_comment.id_code_generated
-    json.body_html          children_comment.processed_html
-    json.user do
-      json.name             children_comment.user.name
-      json.username         children_comment.user.username
-      json.twitter_username children_comment.user.twitter_username
-      json.github_username  children_comment.user.github_username
-      json.website_url      children_comment.user.processed_website_url
-      json.profile_image    ProfileImage.new(children_comment.user).get(640)
-      json.profile_image_90 ProfileImage.new(children_comment.user).get(90)
-    end
-
-    json.children children_comment.children.order(score: :desc).each do |grandchild_comment|
-      json.id_code            grandchild_comment.id_code_generated
-      json.body_html          grandchild_comment.processed_html
-      json.user do
-        json.name             grandchild_comment.user.name
-        json.username         grandchild_comment.user.username
-        json.twitter_username grandchild_comment.user.twitter_username
-        json.github_username  grandchild_comment.user.github_username
-        json.website_url      grandchild_comment.user.processed_website_url
-        json.profile_image    ProfileImage.new(grandchild_comment.user).get(640)
-        json.profile_image_90 ProfileImage.new(grandchild_comment.user).get(90)
-      end
-    end
-  end
-end
+json.partial! "comments_with_children", comments: @comments

app/views/api/v0/comments/show.json.jbuilder

@@ -1,38 +1 @@
-json.type_of            "comment"
-json.id_code            @comment.id_code_generated
-json.body_html          @comment.processed_html
-json.user do
-  json.name             @comment.user.name
-  json.username         @comment.user.username
-  json.twitter_username @comment.user.twitter_username
-  json.github_username  @comment.user.github_username
-  json.website_url      @comment.user.processed_website_url
-  json.profile_image    ProfileImage.new(@comment.user).get(640)
-  json.profile_image_90 ProfileImage.new(@comment.user).get(90)
-end
-json.children @comment.children.order("score DESC").each do |children_comment|
-  json.id_code            children_comment.id_code_generated
-  json.body_html          children_comment.processed_html
-  json.user do
-    json.name             children_comment.user.name
-    json.username         children_comment.user.username
-    json.twitter_username children_comment.user.twitter_username
-    json.github_username  children_comment.user.github_username
-    json.website_url      children_comment.user.processed_website_url
-    json.profile_image    ProfileImage.new(children_comment.user).get(640)
-    json.profile_image_90 ProfileImage.new(children_comment.user).get(90)
-  end
-  json.children children_comment.children.order("score DESC").each do |grandchild_comment|
-    json.id_code            grandchild_comment.id_code_generated
-    json.body_html          grandchild_comment.processed_html
-    json.user do
-      json.name             grandchild_comment.user.name
-      json.username         grandchild_comment.user.username
-      json.twitter_username grandchild_comment.user.twitter_username
-      json.github_username  grandchild_comment.user.github_username
-      json.website_url      grandchild_comment.user.processed_website_url
-      json.profile_image    ProfileImage.new(grandchild_comment.user).get(640)
-      json.profile_image_90 ProfileImage.new(grandchild_comment.user).get(90)
-    end
-  end
-end
+json.partial! "comment_with_children", comment: @comment, children: @comments

spec/requests/api/v0/comments_spec.rb

@@ -6,12 +6,12 @@ def json_response
   end
 
   let_it_be(:article) { create(:article) }
+  let_it_be(:root_comment) { create(:comment, commentable: article) }
+  let_it_be(:child_comment) { create(:comment, commentable: article, parent: root_comment) }
+  let_it_be(:grandchild_comment) { create(:comment, commentable: article, parent: child_comment) }
+  let_it_be(:great_grandchild_comment) { create(:comment, commentable: article, parent: grandchild_comment) }
 
   describe "GET /api/comments" do
-    before do
-      create(:comment, commentable: article)
-    end
-
     it "returns not found if wrong article id" do
       get "/api/comments?a_id=gobbledygook"
       expect(response).to have_http_status(:not_found)
@@ -23,69 +23,87 @@ def json_response
     end
 
     it "does not include children comments in the root list" do
-      # create child comment
-      create(:comment, commentable: article, parent: article.comments.first)
-
       get "/api/comments?a_id=#{article.id}"
       expected_ids = article.comments.roots.map(&:id_code_generated)
       expect(json_response.map { |cm| cm["id_code"] }).to match_array(expected_ids)
     end
 
     it "includes children comments in the children list" do
-      # create child comment
-      parent_comment = article.comments.first
-      child_comment = create(:comment, commentable: article, parent: parent_comment)
-
       get "/api/comments?a_id=#{article.id}"
-      comment_with_children = json_response.detect { |cm| cm["id_code"] == parent_comment.id_code_generated }
+      comment_with_children = json_response.detect { |cm| cm["id_code"] == root_comment.id_code_generated }
       expect(comment_with_children["children"][0]["id_code"]).to eq(child_comment.id_code_generated)
     end
 
-    it "includes granchildren comments in the children-children list" do
-      # create granchild comment
-      root_comment = article.comments.first
-      child_comment = create(:comment, commentable: article, parent: root_comment)
-      granchild_comment = create(:comment, commentable: article, parent: child_comment)
+    it "includes grandchildren comments in the children-children list" do
+      get "/api/comments?a_id=#{article.id}"
+
+      comment_with_descendants = json_response.detect { |cm| cm["id_code"] == root_comment.id_code_generated }
+      expect(comment_with_descendants["children"][0]["children"][0]["id_code"]).to eq(grandchild_comment.id_code_generated)
+    end
 
+    it "includes great-grandchildren comments in the children-children-children list" do
       get "/api/comments?a_id=#{article.id}"
 
       comment_with_descendants = json_response.detect { |cm| cm["id_code"] == root_comment.id_code_generated }
-      expect(comment_with_descendants["children"][0]["children"][0]["id_code"]).to eq(granchild_comment.id_code_generated)
+      json_great_grandchild_id_code = comment_with_descendants["children"][0]["children"][0]["children"][0]["id_code"]
+      expect(json_great_grandchild_id_code).to eq(great_grandchild_comment.id_code_generated)
+    end
+
+    it "sets the correct edge caching surrogate key for all the comments" do
+      sibling_root_comment = create(:comment, commentable: article)
+
+      get "/api/comments?a_id=#{article.id}"
+
+      expected_key = [
+        "comments", sibling_root_comment.record_key,
+        root_comment.record_key, child_comment.record_key,
+        grandchild_comment.record_key, great_grandchild_comment.record_key
+      ].to_set
+      expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key)
     end
   end
 
   describe "GET /api/comments/:id" do
-    let_it_be(:comment) { create(:comment, commentable: article) }
-
     it "returns not found if wrong comment id" do
       get "/api/comments/foobar"
       expect(response).to have_http_status(:not_found)
     end
 
     it "returns the comment" do
-      get "/api/comments/#{comment.id_code_generated}"
-      expect(json_response["id_code"]).to eq(comment.id_code_generated)
+      get "/api/comments/#{root_comment.id_code_generated}"
+      expect(json_response["id_code"]).to eq(root_comment.id_code_generated)
     end
 
     it "includes children comments in the children list" do
-      # create child comment
-      child_comment = create(:comment, commentable: article, parent: comment)
+      get "/api/comments/#{root_comment.id_code_generated}"
 
-      get "/api/comments/#{comment.id_code_generated}"
       comment_with_children = json_response
       expect(comment_with_children["children"][0]["id_code"]).to eq(child_comment.id_code_generated)
     end
 
-    it "includes granchildren comments in the children-children list" do
-      # create granchild comment
-      root_comment = comment
-      child_comment = create(:comment, commentable: article, parent: root_comment)
-      granchild_comment = create(:comment, commentable: article, parent: child_comment)
+    it "includes grandchildren comments in the children-children list" do
+      get "/api/comments/#{root_comment.id_code_generated}"
+
+      comment_with_descendants = json_response
+      expect(comment_with_descendants["children"][0]["children"][0]["id_code"]).to eq(grandchild_comment.id_code_generated)
+    end
 
-      get "/api/comments/#{comment.id_code_generated}"
+    it "includes great-grandchildren comments in the children-children-children list" do
+      get "/api/comments/#{root_comment.id_code_generated}"
 
       comment_with_descendants = json_response
-      expect(comment_with_descendants["children"][0]["children"][0]["id_code"]).to eq(granchild_comment.id_code_generated)
+      json_great_grandchild_id_code = comment_with_descendants["children"][0]["children"][0]["children"][0]["id_code"]
+      expect(json_great_grandchild_id_code).to eq(great_grandchild_comment.id_code_generated)
+    end
+
+    it "sets the correct edge caching surrogate key for all the comments" do
+      get "/api/comments/#{root_comment.id_code_generated}"
+
+      expected_key = [
+        "comments", root_comment.record_key, child_comment.record_key,
+        grandchild_comment.record_key, great_grandchild_comment.record_key
+      ].to_set
+      expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key)
     end
   end
 end