From 5444c53df22f28388741f670ea75e7033ce77987 Mon Sep 17 00:00:00 2001 From: rhymes Date: Mon, 27 Jan 2020 22:57:37 +0100 Subject: [PATCH] Improve API podcast episodes controller caching and fetching (#5772) [deploy] * Refactor caching and add some tests * Only select needed columns * Add more tests --- .../api/v0/podcast_episodes_controller.rb | 28 +++-- .../requests/api/v0/podcasts_episodes_spec.rb | 113 +++++++++++++++--- 2 files changed, 110 insertions(+), 31 deletions(-) diff --git a/app/controllers/api/v0/podcast_episodes_controller.rb b/app/controllers/api/v0/podcast_episodes_controller.rb index 8eebb689d..41df2d780 100644 --- a/app/controllers/api/v0/podcast_episodes_controller.rb +++ b/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 diff --git a/spec/requests/api/v0/podcasts_episodes_spec.rb b/spec/requests/api/v0/podcasts_episodes_spec.rb index f50eb9583..d811ebd1d 100644 --- a/spec/requests/api/v0/podcasts_episodes_spec.rb +++ b/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