API tags: improve caching and purging (#5673) [deploy]

* Add pagination to /tags and proper caching to /tags and /tags/onboarding

* Correctly purge tags

* Fix broken specs

* Make Travis happy

app/controllers/api/v0/tags_controller.rb

@@ -1,20 +1,24 @@
 module Api
   module V0
     class TagsController < ApplicationController
-      caches_action :index,
-                    cache_path: proc { |c| c.params.permit! },
-                    expires_in: 10.minutes
-      before_action :set_cache_control_headers, only: %i[onboarding] # essentially static content
       respond_to :json
 
+      before_action :set_cache_control_headers, only: %i[index onboarding]
+
       def index
-        @page = params[:page]
-        @tags = Tag.order(taggings_count: :desc).page(@page).per(10)
+        page = params[:page]
+        per_page = (params[:per_page] || 10).to_i
+        num = [per_page, 1000].min
+
+        @tags = Tag.order(taggings_count: :desc).page(page).per(num)
+
+        set_surrogate_key_header Tag.table_key, @tags.map(&:record_key)
       end
 
       def onboarding
-        set_surrogate_key_header "siteconfig/onboarding-tags"
         @tags = Tag.where(name: SiteConfig.suggested_tags)
+
+        set_surrogate_key_header Tag.table_key, @tags.map(&:record_key)
       end
     end
   end

app/jobs/tags/bust_cache_job.rb

@@ -2,8 +2,11 @@ module Tags
   class BustCacheJob < ApplicationJob
     queue_as :tags_bust_cache
 
-    def perform(name, cache_buster = CacheBuster)
-      cache_buster.bust_tag(name)
+    def perform(tag_name)
+      tag = Tag.find_by(name: tag_name)
+      return unless tag
+
+      CacheBuster.bust_tag(tag)
     end
   end
 end

app/labor/cache_buster.rb

@@ -113,11 +113,13 @@ module CacheBuster
     bust "/#{slug}?i=i"
   end
 
-  def self.bust_tag(name)
-    bust("/t/#{name}")
-    bust("/t/#{name}?i=i")
-    bust("/t/#{name}/?i=i")
-    bust("/t/#{name}/")
+  def self.bust_tag(tag)
+    tag.purge
+
+    bust("/t/#{tag.name}")
+    bust("/t/#{tag.name}?i=i")
+    bust("/t/#{tag.name}/?i=i")
+    bust("/t/#{tag.name}/")
     bust("/tags")
   end
 

spec/jobs/tags/bust_cache_job_spec.rb

@@ -1,18 +1,27 @@
 require "rails_helper"
 
 RSpec.describe Tags::BustCacheJob do
-  let(:cache_buster) { class_double(CacheBuster) }
-
   before do
-    allow(cache_buster).to receive(:bust_tag)
+    allow(CacheBuster).to receive(:bust_tag)
   end
 
-  include_examples "#enqueues_job", "tags_bust_cache", "PHP"
+  include_examples "#enqueues_job", "tags_bust_cache", "php"
 
   describe "#perform_now" do
     it "busts cache" do
-      described_class.perform_now("PHP", cache_buster)
-      expect(cache_buster).to have_received(:bust_tag).with("PHP")
+      tag = create(:tag)
+
+      described_class.perform_now(tag.name)
+
+      expect(CacheBuster).to have_received(:bust_tag).with(tag)
+    end
+
+    it "doesn't call the cache buster if the tag does not exist" do
+      tag_name = "definitelyatagthatdoesnotexist"
+
+      described_class.perform_now(tag_name)
+
+      expect(CacheBuster).not_to have_received(:bust_tag)
     end
   end
 end

spec/labor/cache_buster_spec.rb

@@ -9,6 +9,7 @@ RSpec.describe CacheBuster, type: :labor do
   let(:listing) { create(:classified_listing, user_id: user.id, category: "cfp") }
   let(:podcast) { create(:podcast) }
   let(:podcast_episode) { create(:podcast_episode, podcast_id: podcast.id) }
+  let(:tag) { create(:tag) }
 
   describe "#bust_comment" do
     it "busts comment" do
@@ -40,7 +41,7 @@ RSpec.describe CacheBuster, type: :labor do
 
   describe "#bust_tag" do
     it "busts tag name + tags" do
-      cache_buster.bust_tag("cfp")
+      expect { cache_buster.bust_tag(tag) }.not_to raise_error
     end
   end
 

spec/requests/api/v0/tags_spec.rb

@@ -0,0 +1,96 @@
+require "rails_helper"
+
+RSpec.describe "Api::V0::Tags", type: :request do
+  describe "GET /api/tags" do
+    it "returns tags" do
+      create(:tag, taggings_count: 10)
+
+      get api_tags_path
+
+      expect(response.parsed_body.size).to eq(1)
+    end
+
+    it "returns tags with the correct json representation" do
+      tag = create(:tag, taggings_count: 10)
+
+      get api_tags_path
+
+      response_tag = response.parsed_body.first
+      expect(response_tag.keys).to match_array(%w[id name bg_color_hex text_color_hex])
+      expect(response_tag["id"]).to eq(tag.id)
+      expect(response_tag["name"]).to eq(tag.name)
+      expect(response_tag["bg_color_hex"]).to eq(tag.bg_color_hex)
+      expect(response_tag["text_color_hex"]).to eq(tag.text_color_hex)
+    end
+
+    it "orders tags by taggings_count in a descending order" do
+      tag = create(:tag, taggings_count: 10)
+      other_tag = create(:tag, taggings_count: tag.taggings_count + 1)
+
+      get api_tags_path
+
+      expected_result = [other_tag.id, tag.id]
+      expect(response.parsed_body.map { |t| t["id"] }).to eq(expected_result)
+    end
+
+    it "supports pagination" do
+      create_list(:tag, 3)
+
+      get api_tags_path, params: { page: 1, per_page: 2 }
+      expect(response.parsed_body.length).to eq(2)
+
+      get api_tags_path, params: { page: 2, per_page: 2 }
+      expect(response.parsed_body.length).to eq(1)
+    end
+
+    it "sets the correct edge caching surrogate key for all tags" do
+      tag = create(:tag, taggings_count: 10)
+
+      get api_tags_path
+
+      expected_key = ["tags", tag.record_key].to_set
+      expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key)
+    end
+  end
+
+  describe "GET /api/tags/onboarding" do
+    it "returns tags" do
+      create(:tag, name: SiteConfig.suggested_tags.first)
+
+      get onboarding_api_tags_path
+
+      expect(response.parsed_body.size).to eq(1)
+    end
+
+    it "returns tags with the correct json representation" do
+      tag = create(:tag, name: SiteConfig.suggested_tags.first)
+
+      get onboarding_api_tags_path
+
+      response_tag = response.parsed_body.first
+      expect(response_tag.keys).to match_array(%w[id name bg_color_hex text_color_hex following])
+      expect(response_tag["id"]).to eq(tag.id)
+      expect(response_tag["name"]).to eq(tag.name)
+      expect(response_tag["bg_color_hex"]).to eq(tag.bg_color_hex)
+      expect(response_tag["text_color_hex"]).to eq(tag.text_color_hex)
+      expect(response_tag["following"]).to be_nil
+    end
+
+    it "returns only suggested tags" do
+      not_suggested_tag = create(:tag, name: "definitelynotasuggestedtag")
+
+      get onboarding_api_tags_path
+
+      expect(response.parsed_body.filter { |t| t["name"] == not_suggested_tag.name }).to be_empty
+    end
+
+    it "sets the correct edge caching surrogate key for all tags" do
+      tag = create(:tag, name: SiteConfig.suggested_tags.first)
+
+      get onboarding_api_tags_path
+
+      expected_key = ["tags", tag.record_key].to_set
+      expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key)
+    end
+  end
+end

spec/requests/tags_api_spec.rb

@@ -1,29 +0,0 @@
-require "rails_helper"
-
-RSpec.describe "ArticlesApi", type: :request do
-  describe "GET /api/tags" do
-    it "returns tag objects" do
-      tag = create(:tag)
-      get "/api/tags"
-      expect(response.body).to include("bg_color_hex")
-      expect(response.body).to include("[")
-      expect(response.body).to include(tag.name)
-    end
-  end
-
-  describe "GET /api/tags/onboarding" do
-    it "returns onboarding tag objects" do
-      tag = create(:tag, name: "ruby")
-      get "/api/tags/onboarding"
-      expect(response.body).to include("bg_color_hex")
-      expect(response.body).to include("[")
-      expect(response.body).to include(tag.name)
-    end
-
-    it "does not return incorrect onboarding tag objects" do
-      tag = create(:tag, name: "dsdsdsdsdsdssddsdsds")
-      get "/api/tags/onboarding"
-      expect(response.body).not_to include(tag.name)
-    end
-  end
-end