From bcd85452e9aa41eb36f0a0750d6866298584d088 Mon Sep 17 00:00:00 2001 From: rhymes Date: Wed, 15 Jan 2020 16:45:56 +0100 Subject: [PATCH] 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 | 39 +++++++-- app/jobs/comments/bust_cache_job.rb | 1 + .../api/v0/comments/_comment.json.jbuilder | 6 ++ .../_comment_with_children.json.jbuilder | 6 ++ .../_comments_with_children.json.jbuilder | 4 + app/views/api/v0/comments/index.json.jbuilder | 44 +--------- app/views/api/v0/comments/show.json.jbuilder | 39 +-------- spec/requests/api/v0/comments_spec.rb | 82 +++++++++++-------- 8 files changed, 99 insertions(+), 122 deletions(-) create mode 100644 app/views/api/v0/comments/_comment.json.jbuilder create mode 100644 app/views/api/v0/comments/_comment_with_children.json.jbuilder create mode 100644 app/views/api/v0/comments/_comments_with_children.json.jbuilder diff --git a/app/controllers/api/v0/comments_controller.rb b/app/controllers/api/v0/comments_controller.rb index 51b28880a834..4c135d8d8634 100644 --- a/app/controllers/api/v0/comments_controller.rb +++ b/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 diff --git a/app/jobs/comments/bust_cache_job.rb b/app/jobs/comments/bust_cache_job.rb index ffe181c8352a..2d676a8133f8 100644 --- a/app/jobs/comments/bust_cache_job.rb +++ b/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 diff --git a/app/views/api/v0/comments/_comment.json.jbuilder b/app/views/api/v0/comments/_comment.json.jbuilder new file mode 100644 index 000000000000..c73822823acc --- /dev/null +++ b/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 diff --git a/app/views/api/v0/comments/_comment_with_children.json.jbuilder b/app/views/api/v0/comments/_comment_with_children.json.jbuilder new file mode 100644 index 000000000000..ebbfbd6c9af2 --- /dev/null +++ b/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 diff --git a/app/views/api/v0/comments/_comments_with_children.json.jbuilder b/app/views/api/v0/comments/_comments_with_children.json.jbuilder new file mode 100644 index 000000000000..18df6b1f508a --- /dev/null +++ b/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 diff --git a/app/views/api/v0/comments/index.json.jbuilder b/app/views/api/v0/comments/index.json.jbuilder index 746b7d211729..8503ae14c6c4 100644 --- a/app/views/api/v0/comments/index.json.jbuilder +++ b/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 diff --git a/app/views/api/v0/comments/show.json.jbuilder b/app/views/api/v0/comments/show.json.jbuilder index 3c38a45a5921..d854798de5d2 100644 --- a/app/views/api/v0/comments/show.json.jbuilder +++ b/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 diff --git a/spec/requests/api/v0/comments_spec.rb b/spec/requests/api/v0/comments_spec.rb index ac924a428ea8..df1cb48d156c 100644 --- a/spec/requests/api/v0/comments_spec.rb +++ b/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