From 85cde8a8df1c3afecab9d9af56ba806e91729e2d Mon Sep 17 00:00:00 2001 From: Sam Vevang Date: Tue, 1 Aug 2023 21:04:51 -0500 Subject: [PATCH 01/11] Don't use constants as parameters --- app/models/apple/api_waiting.rb | 24 +++++++++++++----------- test/models/apple/api_waiting_test.rb | 13 +++---------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/app/models/apple/api_waiting.rb b/app/models/apple/api_waiting.rb index f7534879e..042b4f197 100644 --- a/app/models/apple/api_waiting.rb +++ b/app/models/apple/api_waiting.rb @@ -1,24 +1,26 @@ +API_WAIT_INTERVAL = 2.seconds +API_WAIT_TIMEOUT = 5.minutes + module Apple module ApiWaiting - API_WAIT_INTERVAL = 2.seconds - API_WAIT_TIMEOUT = 5.minutes - extend ActiveSupport::Concern included do - def self.wait_for(remaining_records) + def self.wait_for(remaining_records, wait_timeout: API_WAIT_TIMEOUT, wait_interval: API_WAIT_INTERVAL) t_beg = Time.now.utc - loop do - Rails.logger.info(".wait_for", {remaining_record_count: remaining_records.count, have_waited: Time.now.utc - t_beg}) + waited = 0 + loop do + # All done, return `timeout == false` + break [false, []] if remaining_records.empty? # Return `timeout == true` if we've waited too long - break [true, remaining_records] if Time.now.utc - t_beg > self::API_WAIT_TIMEOUT + break [true, remaining_records] if waited > wait_timeout - remaining_records = yield(remaining_records) + sleep(wait_interval) - # All done, return `timeout == false` - break [false, []] if remaining_records.empty? + waited = Time.now.utc - t_beg + Rails.logger.info(".wait_for", {remaining_record_count: remaining_records.count, have_waited: waited}) - sleep(self::API_WAIT_INTERVAL) + remaining_records = yield(remaining_records) end end end diff --git a/test/models/apple/api_waiting_test.rb b/test/models/apple/api_waiting_test.rb index dbac3278c..56b47e532 100644 --- a/test/models/apple/api_waiting_test.rb +++ b/test/models/apple/api_waiting_test.rb @@ -2,15 +2,8 @@ require "test_helper" -class TestWait +class Test include Apple::ApiWaiting - API_WAIT_INTERVAL = 0.seconds -end - -class TestTimeout - include Apple::ApiWaiting - API_WAIT_INTERVAL = 0.seconds - API_WAIT_TIMEOUT = 0.seconds end describe Apple::ApiWaiting do @@ -25,7 +18,7 @@ class TestTimeout step = 0 - (timed_out, remaining) = TestWait.wait_for(records) do |remaining| + (timed_out, remaining) = Test.wait_for(records, wait_interval: 0.seconds) do |remaining| rem = remaining.dup assert_equal intervals[step], rem @@ -42,7 +35,7 @@ class TestTimeout end it "times out" do - (timed_out, remaining) = TestTimeout.wait_for(["a", "b", "c"]) do |remaining| + (timed_out, remaining) = Test.wait_for(["a", "b", "c"], wait_interval: 0.seconds, wait_timeout: 0.seconds) do |remaining| remaining end From 8c87bc17386dde423bf3f2042fdde6b11607ad44 Mon Sep 17 00:00:00 2001 From: Sam Vevang Date: Tue, 1 Aug 2023 21:06:53 -0500 Subject: [PATCH 02/11] Extract the current time method --- app/models/apple/api_waiting.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/models/apple/api_waiting.rb b/app/models/apple/api_waiting.rb index 042b4f197..d15561e6e 100644 --- a/app/models/apple/api_waiting.rb +++ b/app/models/apple/api_waiting.rb @@ -5,8 +5,12 @@ module Apple module ApiWaiting extend ActiveSupport::Concern included do + def self.current_time + Time.now.utc + end + def self.wait_for(remaining_records, wait_timeout: API_WAIT_TIMEOUT, wait_interval: API_WAIT_INTERVAL) - t_beg = Time.now.utc + t_beg = current_time waited = 0 loop do @@ -17,7 +21,7 @@ def self.wait_for(remaining_records, wait_timeout: API_WAIT_TIMEOUT, wait_interv sleep(wait_interval) - waited = Time.now.utc - t_beg + waited = current_time - t_beg Rails.logger.info(".wait_for", {remaining_record_count: remaining_records.count, have_waited: waited}) remaining_records = yield(remaining_records) From a5c327413584b2246e9bf82dab26503d75fb1873 Mon Sep 17 00:00:00 2001 From: Sam Vevang Date: Wed, 2 Aug 2023 12:14:35 -0500 Subject: [PATCH 03/11] Break out logging --- app/models/apple/api_waiting.rb | 12 +++++++++++- test/models/apple/api_waiting_test.rb | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/app/models/apple/api_waiting.rb b/app/models/apple/api_waiting.rb index d15561e6e..bed7a595e 100644 --- a/app/models/apple/api_waiting.rb +++ b/app/models/apple/api_waiting.rb @@ -9,6 +9,15 @@ def self.current_time Time.now.utc end + def self.wait_timed_out?(waited, wait_timeout) + if waited > wait_timeout + Rails.logger.info("Timed out waiting for Apple API to process", waited: waited, wait_timeout: wait_timeout) + true + else + false + end + end + def self.wait_for(remaining_records, wait_timeout: API_WAIT_TIMEOUT, wait_interval: API_WAIT_INTERVAL) t_beg = current_time @@ -17,7 +26,8 @@ def self.wait_for(remaining_records, wait_timeout: API_WAIT_TIMEOUT, wait_interv # All done, return `timeout == false` break [false, []] if remaining_records.empty? # Return `timeout == true` if we've waited too long - break [true, remaining_records] if waited > wait_timeout + + break [true, remaining_records] if wait_timed_out?(waited, wait_timeout) sleep(wait_interval) diff --git a/test/models/apple/api_waiting_test.rb b/test/models/apple/api_waiting_test.rb index 56b47e532..808c65f8d 100644 --- a/test/models/apple/api_waiting_test.rb +++ b/test/models/apple/api_waiting_test.rb @@ -42,5 +42,22 @@ class Test assert_equal timed_out, true assert_equal remaining, ["a", "b", "c"] end + + it "times out by default after 5 minutes" do + current_time = Time.utc(2021, 1, 1, 0, 0, 0) + index = 0 + + Test.stub(:current_time, -> { + current_time += index.minutes + index += 1 + }) do + (timed_out, remaining) = Test.wait_for(["a", "b", "c"], wait_interval: 0.seconds) do |remaining| + remaining + end + + assert_equal timed_out, true + assert_equal remaining, ["a", "b", "c"] + end + end end end From 127f59f80081ac2ca459f6d0c1087baeb42c89d4 Mon Sep 17 00:00:00 2001 From: Sam Vevang Date: Wed, 2 Aug 2023 12:45:37 -0500 Subject: [PATCH 04/11] Increase wait timeout for the episode asset state --- app/models/apple/episode.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/models/apple/episode.rb b/app/models/apple/episode.rb index 9808d11c9..5d7c37eef 100644 --- a/app/models/apple/episode.rb +++ b/app/models/apple/episode.rb @@ -12,11 +12,14 @@ class Episode AUDIO_ASSET_FAILURE = "FAILURE" AUDIO_ASSET_SUCCESS = "SUCCESS" + EPISODE_ASSET_WAIT_TIMEOUT = 8.minutes.freeze + EPISODE_ASSET_WAIT_INTERVAL = 10.seconds.freeze + # In the case where the episodes state is not yet ready to publish, but the # underlying models are ready. Poll the episodes audio asset state but # guard against waiting for episode assets that will never be processed. def self.wait_for_asset_state(api, eps) - wait_for(eps) do |remaining_eps| + wait_for(eps, wait_timeout: EPISODE_ASSET_WAIT_TIMEOUT, wait_interval: EPISODE_ASSET_WAIT_INTERVAL) do |remaining_eps| Rails.logger.info("Probing for episode audio asset state") unwrapped = get_episodes(api, remaining_eps) From 8981dc0f30069ff4052ef07f943386c19d884fd2 Mon Sep 17 00:00:00 2001 From: Sam Vevang Date: Wed, 2 Aug 2023 13:21:09 -0500 Subject: [PATCH 05/11] Add logging for done with waiting --- app/models/apple/api_waiting.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/app/models/apple/api_waiting.rb b/app/models/apple/api_waiting.rb index bed7a595e..6d82d609d 100644 --- a/app/models/apple/api_waiting.rb +++ b/app/models/apple/api_waiting.rb @@ -18,13 +18,22 @@ def self.wait_timed_out?(waited, wait_timeout) end end + def self.work_done?(remaining_records, waited, wait_timeout) + if remaining_records.empty? + Rails.logger.info("Done waiting for Apple Api work", waited: waited, wait_timeout: wait_timeout) + true + else + false + end + end + def self.wait_for(remaining_records, wait_timeout: API_WAIT_TIMEOUT, wait_interval: API_WAIT_INTERVAL) t_beg = current_time waited = 0 loop do # All done, return `timeout == false` - break [false, []] if remaining_records.empty? + break [false, []] if work_done?(remaining_records, waited, wait_timeout) # Return `timeout == true` if we've waited too long break [true, remaining_records] if wait_timed_out?(waited, wait_timeout) From 1d193f4d4802932499f18c21a5a800dc5c64ceea Mon Sep 17 00:00:00 2001 From: Sam Vevang Date: Tue, 1 Aug 2023 14:21:42 -0500 Subject: [PATCH 06/11] Apple config tracks apple data --- app/models/apple/config.rb | 16 ++++++++++++++++ app/models/apple/podcast_container.rb | 2 +- app/models/apple/podcast_delivery.rb | 2 +- app/models/apple/podcast_delivery_file.rb | 2 +- 4 files changed, 19 insertions(+), 3 deletions(-) diff --git a/app/models/apple/config.rb b/app/models/apple/config.rb index 8feff016f..fa26f125e 100644 --- a/app/models/apple/config.rb +++ b/app/models/apple/config.rb @@ -40,5 +40,21 @@ def build_publisher def apple_key Base64.decode64(apple_key_pem_b64) end + + def apple_data + episode_data = + ::Episode.where(podcast: podcast).map do |episode| + [episode.apple_sync_log, episode.podcast_container] + end.flatten.compact + + podcast_delivery_data = [ + Apple::PodcastDelivery.with_deleted.where(episode: podcast.episodes), + Apple::PodcastDeliveryFile.with_deleted.where(episode: podcast.episodes) + ] + + feed_data = [public_feed.apple_sync_log, private_feed.apple_sync_log].compact + + [podcast_delivery_data, episode_data, feed_data] + end end end diff --git a/app/models/apple/podcast_container.rb b/app/models/apple/podcast_container.rb index f1ecf6e50..59ba8c073 100644 --- a/app/models/apple/podcast_container.rb +++ b/app/models/apple/podcast_container.rb @@ -8,7 +8,7 @@ class PodcastContainer < ApplicationRecord default_scope { includes(:apple_sync_log) } - has_one :apple_sync_log, -> { podcast_containers }, foreign_key: :feeder_id, class_name: "SyncLog" + has_one :apple_sync_log, -> { podcast_containers }, foreign_key: :feeder_id, class_name: "SyncLog", dependent: :destroy has_many :podcast_deliveries, dependent: :destroy has_many :podcast_delivery_files, through: :podcast_deliveries belongs_to :episode, class_name: "::Episode" diff --git a/app/models/apple/podcast_delivery.rb b/app/models/apple/podcast_delivery.rb index e65f33a24..9bcb844b4 100644 --- a/app/models/apple/podcast_delivery.rb +++ b/app/models/apple/podcast_delivery.rb @@ -8,7 +8,7 @@ class PodcastDelivery < ApplicationRecord default_scope { includes(:apple_sync_log) } - has_one :apple_sync_log, -> { podcast_deliveries }, foreign_key: :feeder_id, class_name: "SyncLog" + has_one :apple_sync_log, -> { podcast_deliveries }, foreign_key: :feeder_id, class_name: "SyncLog", dependent: :destroy has_many :podcast_delivery_files, dependent: :destroy belongs_to :episode, class_name: "::Episode" belongs_to :podcast_container, class_name: "::Apple::PodcastContainer" diff --git a/app/models/apple/podcast_delivery_file.rb b/app/models/apple/podcast_delivery_file.rb index fa460b269..76c6606de 100644 --- a/app/models/apple/podcast_delivery_file.rb +++ b/app/models/apple/podcast_delivery_file.rb @@ -9,7 +9,7 @@ class PodcastDeliveryFile < ApplicationRecord default_scope { includes(:apple_sync_log) } - has_one :apple_sync_log, -> { podcast_delivery_files }, foreign_key: :feeder_id, class_name: "SyncLog", autosave: true + has_one :apple_sync_log, -> { podcast_delivery_files }, foreign_key: :feeder_id, class_name: "SyncLog", autosave: true, dependent: :delete belongs_to :podcast_delivery has_one :podcast_container, through: :podcast_delivery belongs_to :episode, class_name: "::Episode" From 91a27045b94974fd38b22ad12074f3f593e855b0 Mon Sep 17 00:00:00 2001 From: Sam Vevang Date: Wed, 2 Aug 2023 14:51:18 -0500 Subject: [PATCH 07/11] Skip updates completely --- app/models/apple/show.rb | 51 ++++++++++++++++++++++------------ test/models/apple/show_test.rb | 8 ++++-- 2 files changed, 39 insertions(+), 20 deletions(-) diff --git a/app/models/apple/show.rb b/app/models/apple/show.rb index 577f08673..45a848c21 100644 --- a/app/models/apple/show.rb +++ b/app/models/apple/show.rb @@ -63,21 +63,32 @@ def category_data [{"type" => "categories", "id" => "1301"}] end - def show_data + def update_attributes + create_attributes.except(:releaseFrequency) + end + + def create_attributes { - data: { - type: "shows", - relationships: { - allowedCountriesAndRegions: {data: api.countries_and_regions} - }, - attributes: { - kind: "RSS", - rssUrl: feed_published_url, - releaseFrequency: "OPTOUT", - thirdPartyRights: "HAS_RIGHTS_TO_THIRD_PARTY_CONTENT" + kind: "RSS", + rssUrl: feed_published_url, + releaseFrequency: "WEEKLY", + thirdPartyRights: "HAS_RIGHTS_TO_THIRD_PARTY_CONTENT" + } + end + + def show_data(attributes, id: nil) + res = + { + data: { + type: "shows", + relationships: { + allowedCountriesAndRegions: {data: api.countries_and_regions} + }, + attributes: attributes } } - } + res[:data][:id] = id if id.present? + res end def apple_id @@ -106,7 +117,7 @@ def sync! end def create_show! - data = show_data + data = show_data(create_attributes) Rails.logger.info("Creating show", show_data: data) resp = api.post("shows", data) @@ -114,11 +125,17 @@ def create_show! end def update_show!(sync) - show_data_with_id = show_data - show_data_with_id[:data][:id] = sync.external_id - Rails.logger.info("Updating show", show_data: show_data_with_id) - resp = api.patch("shows/#{sync.external_id}", show_data_with_id) + Rails.logger.warn("Skipping update for existing show!") + nil + + # TODO, map out the cases where we'd actually need to update a show + # data = show_data(update_attributes, id: apple_id) + # Rails.logger.info("Updating show", show_data: data) + # resp = api.patch("shows/#{sync.external_id}", data) + # + # api.response(resp) + resp = api.get("shows/#{sync.external_id}") api.response(resp) end diff --git a/test/models/apple/show_test.rb b/test/models/apple/show_test.rb index 9064dfece..9cd6b282e 100644 --- a/test/models/apple/show_test.rb +++ b/test/models/apple/show_test.rb @@ -107,7 +107,9 @@ describe "#sync!" do it "runs sync!" do - apple_show.api.stub(:patch, OpenStruct.new(body: {"data" => {"id" => "123", "attributes" => {"foo" => "bar"}}}.to_json, code: "200")) do + # TODO, do we need update the show? + # apple_show.api.stub(:patch, OpenStruct.new(body: {"data" => {"id" => "123", "attributes" => {"foo" => "bar"}}}.to_json, code: "200")) do + apple_show.api.stub(:get, OpenStruct.new(body: {"data" => {"id" => "123", "attributes" => {"foo" => "bar"}}}.to_json, code: "200")) do assert apple_show.sync_log.present? apple_show.sync_log.update!(api_response: {}) @@ -122,7 +124,7 @@ end it "creates a sync log if one does not exist" do - apple_show.api.stub(:patch, OpenStruct.new(body: {"data" => {"id" => "123", "attributes" => {"foo" => "bar"}}}.to_json, code: "200")) do + apple_show.api.stub(:get, OpenStruct.new(body: {"data" => {"id" => "123", "attributes" => {"foo" => "bar"}}}.to_json, code: "200")) do assert apple_show.sync_log.present? sync = apple_show.sync! @@ -161,7 +163,7 @@ describe "#show_data" do it "returns a hash" do - assert_equal apple_show.show_data.class, Hash + assert_equal apple_show.show_data({}).class, Hash end end From 45a5d66e3b1809ec45495f185684f76a412e9487 Mon Sep 17 00:00:00 2001 From: Sam Vevang Date: Wed, 2 Aug 2023 16:27:25 -0500 Subject: [PATCH 08/11] Lint, remove old return value --- app/models/apple/show.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/models/apple/show.rb b/app/models/apple/show.rb index 45a848c21..f47bbcc9a 100644 --- a/app/models/apple/show.rb +++ b/app/models/apple/show.rb @@ -126,8 +126,6 @@ def create_show! def update_show!(sync) Rails.logger.warn("Skipping update for existing show!") - nil - # TODO, map out the cases where we'd actually need to update a show # data = show_data(update_attributes, id: apple_id) # Rails.logger.info("Updating show", show_data: data) From 79b9e2e3b79f2c400c5331888a938e8261aa2e73 Mon Sep 17 00:00:00 2001 From: Sam Vevang Date: Thu, 3 Aug 2023 10:10:47 -0500 Subject: [PATCH 09/11] The empty set of delivery files is settled --- app/models/apple/podcast_container.rb | 13 +++++------ test/models/apple/episode_test.rb | 4 ++-- test/models/apple/podcast_container_test.rb | 25 +++++++++++++++++++++ 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/app/models/apple/podcast_container.rb b/app/models/apple/podcast_container.rb index f1ecf6e50..212afbb5b 100644 --- a/app/models/apple/podcast_container.rb +++ b/app/models/apple/podcast_container.rb @@ -249,7 +249,8 @@ def missing_podcast_audio? end def delivered? - return false if podcast_delivery_files.length == 0 + # because we cannot infer if the podcast delivery files have expired + return true if podcast_delivery_files.length == 0 (podcast_delivery_files.all?(&:delivered?) && podcast_delivery_files.all?(&:processed?)) @@ -262,15 +263,9 @@ def processed_errors? end def delivery_settled? - return false if podcast_delivery_files.length == 0 - delivered? && !processed_errors? end - def skip_delivery? - container_upload_satisfied? - end - def container_upload_satisfied? # Sets us up for a retry if something prevented the audio from being # marked as uploaded and then processed and validated. Assuming that we @@ -279,6 +274,10 @@ def container_upload_satisfied? has_podcast_audio? && delivery_settled? end + def skip_delivery? + container_upload_satisfied? + end + def needs_delivery? !skip_delivery? end diff --git a/test/models/apple/episode_test.rb b/test/models/apple/episode_test.rb index 3372e33a0..86719675d 100644 --- a/test/models/apple/episode_test.rb +++ b/test/models/apple/episode_test.rb @@ -91,9 +91,9 @@ assert_equal true, apple_episode.waiting_for_asset_state? end - it "should be false if there are no podcast delivery files" do + it "should be true if there are no podcast delivery files and the asset state is UNSPECIFIED" do apple_episode.podcast_container.stub(:podcast_delivery_files, []) do - assert_equal false, apple_episode.waiting_for_asset_state? + assert_equal true, apple_episode.waiting_for_asset_state? end end diff --git a/test/models/apple/podcast_container_test.rb b/test/models/apple/podcast_container_test.rb index a8dfa7024..784f23085 100644 --- a/test/models/apple/podcast_container_test.rb +++ b/test/models/apple/podcast_container_test.rb @@ -256,6 +256,31 @@ class Apple::PodcastContainerTest < ActiveSupport::TestCase end end + describe "retry sentinals and gatekeeping" do + let(:container) { Apple::PodcastContainer.new } + describe "#delivery_settled?" do + it "should be settled if there are no delivery files" do + container.stub(:podcast_delivery_files, []) do + assert container.delivery_settled? + assert container.delivered? + end + end + end + + describe "#container_upload_satisfied?" do + it "should be satisfied with podcast files and a settled delivery" do + container.stub(:files, [ + {status: Apple::PodcastContainer::FILE_STATUS_SUCCESS, + assetRole: Apple::PodcastContainer::FILE_ASSET_ROLE_PODCAST_AUDIO}.with_indifferent_access + ]) do + container.stub(:podcast_delivery_files, []) do + assert container.container_upload_satisfied? + end + end + end + end + end + describe "#destroy" do it "should destroy the podcast container and cascade to the delivery and delivery file" do apple_episode.stub(:apple_id, apple_episode_id) do From df1740dd3ad2fca845cccc6c88a38af2115dbb54 Mon Sep 17 00:00:00 2001 From: Sam Vevang Date: Wed, 2 Aug 2023 21:54:56 -0500 Subject: [PATCH 10/11] Guard for nil podcast_container --- app/models/apple/episode.rb | 4 +++- test/models/apple/episode_test.rb | 12 ++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/app/models/apple/episode.rb b/app/models/apple/episode.rb index 9808d11c9..80d5dcc37 100644 --- a/app/models/apple/episode.rb +++ b/app/models/apple/episode.rb @@ -386,7 +386,9 @@ def drafting? end def container_upload_complete? - feeder_episode.apple_podcast_container.container_upload_satisfied? + return false unless has_container? + + podcast_container.container_upload_satisfied? end def audio_asset_vendor_id diff --git a/test/models/apple/episode_test.rb b/test/models/apple/episode_test.rb index 86719675d..8b4786559 100644 --- a/test/models/apple/episode_test.rb +++ b/test/models/apple/episode_test.rb @@ -147,6 +147,18 @@ assert_equal false, ep.synced_with_apple? end end + + it "should be false when the podcast_container is nil" do + ep = build(:apple_episode) + assert ep.podcast_container.nil? + + # it returns early via the guard + ep.stub(:podcast_container, -> { raise "shouldn't happen" }) do + ep.stub(:has_container?, false) do + refute ep.container_upload_complete? + end + end + end end describe "#enclosure_filename" do From 69a919985692540210ed97c121649abd42342395 Mon Sep 17 00:00:00 2001 From: Sam Vevang Date: Thu, 3 Aug 2023 11:27:06 -0500 Subject: [PATCH 11/11] Use if instead of unless --- app/models/apple/episode.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/apple/episode.rb b/app/models/apple/episode.rb index 80d5dcc37..f11193f0b 100644 --- a/app/models/apple/episode.rb +++ b/app/models/apple/episode.rb @@ -386,7 +386,7 @@ def drafting? end def container_upload_complete? - return false unless has_container? + return false if missing_container? podcast_container.container_upload_satisfied? end