Improve API podcast episodes controller caching and fetching (#5772) [deploy]

* Refactor caching and add some tests

* Only select needed columns

* Add more tests

app/controllers/api/v0/podcast_episodes_controller.rb

@@ -1,29 +1,31 @@
 module Api
   module V0
     class PodcastEpisodesController < ApiController
-      caches_action :index,
-                    cache_path: proc { |c| c.params.permit! },
-                    expires_in: 10.minutes
       respond_to :json
 
       before_action :cors_preflight_check
       after_action :cors_set_access_control_headers
 
+      before_action :set_cache_control_headers, only: %i[index]
+
       def index
-        @page = params[:page]
+        page = params[:page]
+        per_page = (params[:per_page] || 30).to_i
+        num = [per_page, 1000].min
 
         if params[:username]
-          @podcast = Podcast.available.find_by!(slug: params[:username])
-          @podcast_episodes = @podcast.
-            podcast_episodes.reachable.order("published_at desc").
-            page(@page).
-            per(30)
+          podcast = Podcast.available.find_by!(slug: params[:username])
+          relation = podcast.podcast_episodes.reachable
         else
-          @podcast_episodes = PodcastEpisode.
-            reachable.
-            includes(:podcast).
-            order("published_at desc").page(@page).per(30)
+          relation = PodcastEpisode.includes(:podcast).reachable
         end
+
+        @podcast_episodes = relation.
+          select(:id, :slug, :title, :podcast_id).
+          order(published_at: :desc).
+          page(page).per(num)
+
+        set_surrogate_key_header PodcastEpisode.table_key, @podcast_episodes.map(&:record_key)
       end
     end
   end

spec/requests/api/v0/podcasts_episodes_spec.rb

@@ -5,34 +5,111 @@ RSpec.describe "Api::V0::PodcastEpisodes", type: :request do
 
   describe "GET /api/podcast_episodes" do
     it "returns json response" do
-      get "/api/podcast_episodes"
+      get api_podcast_episodes_path
+
       expect(response.content_type).to eq("application/json")
     end
 
-    it "returns correct attributes for an episode" do
-      create(:podcast_episode)
-      get "/api/podcast_episodes"
-      expected_attributes = %w[type_of id path image_url title podcast]
-      expect(JSON.parse(response.body).first.keys).to eq(expected_attributes)
+    it "does not return unreachable podcasts" do
+      create(:podcast_episode, reachable: false, podcast: podcast)
+
+      get api_podcast_episodes_path
+
+      expect(response.parsed_body.size).to eq(0)
+    end
+
+    it "does not return reachable podcast episodes belonging to unpublished podcasts" do
+      pe = create(:podcast_episode, reachable: true, podcast: create(:podcast, published: false))
+
+      get api_podcast_episodes_path
+
+      expect(response.parsed_body.map { |e| e["id"] }).not_to include(pe.id.to_s)
+    end
+
+    it "returns correct attributes for an episode", :aggregate_failures do
+      podcast_episode = create(:podcast_episode, podcast: podcast)
+
+      get api_podcast_episodes_path
+
+      response_episode = response.parsed_body.first
+      expect(response_episode.keys).to match_array(%w[type_of id path image_url title podcast])
+
+      expect(response_episode["type_of"]).to eq("podcast_episodes")
+      %w[id path title].each do |attr|
+        expect(response_episode[attr]).to eq(podcast_episode.public_send(attr))
+      end
+      expect(response_episode["image_url"]).to eq(podcast_episode.podcast.image_url)
+    end
+
+    it "returns the episode's podcast json representation" do
+      podcast_episode = create(:podcast_episode, podcast: podcast)
+
+      get api_podcast_episodes_path
+
+      response_episode = response.parsed_body.first
+      expect(response_episode["podcast"]["title"]).to eq(podcast_episode.podcast.title)
+      expect(response_episode["podcast"]["slug"]).to eq(podcast_episode.podcast.slug)
+      expect(response_episode["podcast"]["image_url"]).to eq(podcast_episode.podcast.image_url)
     end
 
     it "returns episodes in reverse publishing order" do
-      pe1 = create(:podcast_episode, published_at: 1.day.ago)
-      pe2 = create(:podcast_episode, published_at: 1.day.from_now)
-      get "/api/podcast_episodes"
-      expect(JSON.parse(response.body).map { |pe| pe["id"] }).to eq([pe2.id, pe1.id])
+      pe1 = create(:podcast_episode, published_at: 1.day.ago, podcast: podcast)
+      pe2 = create(:podcast_episode, published_at: 1.day.from_now, podcast: podcast)
+
+      get api_podcast_episodes_path
+      expect(response.parsed_body.map { |pe| pe["id"] }).to eq([pe2.id, pe1.id])
     end
 
-    it "returns only podcasts for a given username" do
-      pe1 = create(:podcast_episode, podcast: podcast)
-      create(:podcast_episode, podcast: create(:podcast))
-      get "/api/podcast_episodes?username=#{podcast.slug}"
-      expect(JSON.parse(response.body).map { |pe| pe["id"] }).to eq([pe1.id])
+    it "supports pagination" do
+      create_list(:podcast_episode, 3, podcast: podcast)
+
+      get api_podcast_episodes_path, params: { page: 1, per_page: 2 }
+      expect(response.parsed_body.length).to eq(2)
+
+      get api_podcast_episodes_path, params: { page: 2, per_page: 2 }
+      expect(response.parsed_body.length).to eq(1)
     end
 
-    it "returns not found if the username does not exist" do
-      get "/api/podcast_episodes?username=foobar"
-      expect(response).to have_http_status(:not_found)
+    it "sets the correct edge caching surrogate key for all tags" do
+      podcast_episode = create(:podcast_episode, reachable: true, podcast: podcast)
+
+      get api_podcast_episodes_path
+
+      expected_key = ["podcast_episodes", podcast_episode.record_key].to_set
+      expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key)
+    end
+
+    context "when given a username parameter" do
+      it "returns only podcasts for a given username" do
+        pe1 = create(:podcast_episode, podcast: podcast)
+        create(:podcast_episode, podcast: create(:podcast))
+
+        get api_podcast_episodes_path(username: podcast.slug)
+        expect(response.parsed_body.map { |pe| pe["id"] }).to eq([pe1.id])
+      end
+
+      it "returns not found if the podcast is unavailable" do
+        unavailable_podcast = create(:podcast, published: false)
+        create(:podcast_episode, podcast: unavailable_podcast)
+
+        get api_podcast_episodes_path(username: unavailable_podcast.slug)
+
+        expect(response).to have_http_status(:not_found)
+      end
+
+      it "returns not found if any of the podcast episodes are unreachable" do
+        create(:podcast_episode, reachable: false, podcast: podcast)
+
+        get api_podcast_episodes_path(username: podcast.slug)
+
+        expect(response).to have_http_status(:not_found)
+      end
+
+      it "returns not found if the username does not exist" do
+        get api_podcast_episodes_path(username: "foobar")
+
+        expect(response).to have_http_status(:not_found)
+      end
     end
   end
 end