Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

3887 – Better error handling in PermaCC #412

Merged
merged 7 commits into from
Oct 26, 2023
Merged

3887 – Better error handling in PermaCC #412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c91b664
add PermaCcError
vasconsaurus Oct 25, 2023
f0c6d7b
update error handling in perma_cc
vasconsaurus Oct 26, 2023
32b736c
update test
vasconsaurus Oct 26, 2023
87daf05
update test and update perma_cc
vasconsaurus Oct 26, 2023
0148fd6
fix test
vasconsaurus Oct 26, 2023
bf28237
update perma cc logic to be similar to archive org
vasconsaurus Oct 26, 2023
bc99a6a
skip tests related to video archiving
vasconsaurus Oct 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion app/models/concerns/media_perma_cc_archiver.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,17 @@ def send_to_perma_cc(url, key_id, _supported = nil)
data = { location: 'http://perma.cc/' + body['guid'] }
Media.notify_webhook_and_update_cache('perma_cc', url, data, key_id)
else
raise Pender::Exception::RetryLater, "(#{response.code}) #{response.message}"
data = { error: { message: response.message, code: Lapis::ErrorCodes::const_get('ARCHIVER_ERROR') }}
Media.notify_webhook_and_update_cache('perma_cc', url, data, key_id)
if response&.body.include?("You've reached your usage limit")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to have an integration test for this case, just for us to capture if they change their message. On the other hand, this is hard because we would need to have a dedicated account to use only for this test. Hopefully we can track that in the generic Pender::Exception::PermaCcError exception report. So let's not change anything for now, this is good to go.

PenderSentry.notify(
Pender::Exception::TooManyCaptures.new(response.message),
url: url,
response_body: response.body
)
else
raise Pender::Exception::PermaCcError, "(#{response.code}) #{response.message}"
end
end
end
end
Expand Down
5 changes: 5 additions & 0 deletions lib/pender/exception/perma_cc_error.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module Pender
module Exception
class PermaCcError < StandardError; end
end
end
94 changes: 69 additions & 25 deletions test/models/archiver_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,46 +455,73 @@ def quietly_redefine_constant(klass, constant, new_value)
WebMock.disable!
end

test "should update media with error when archive to Perma.cc fails" do
test "when Perma.cc fails with Pender::Exception::PermaCcError it should update media with error and retry" do
WebMock.enable!
WebMock.disable_net_connect!(allow: [/minio/])
Sidekiq::Testing.inline!
api_key = create_api_key application_settings: { config: { 'perma_cc_key': 'my-perma-key' }, 'webhook_url': 'https://example.com/webhook.php', 'webhook_token': 'test' }
url = 'https://example.com'

Media.any_instance.unstub(:archive_to_perma_cc)
WebMock.stub_request(:get, url).to_return(status: 200, body: '<html>A page</html>')
WebMock.stub_request(:post, /safebrowsing\.googleapis\.com/).to_return(status: 200, body: '{}')
WebMock.stub_request(:post, /example.com\/webhook/).to_return(status: 200, body: '')
WebMock.stub_request(:post, /api.perma.cc/).to_return(status: [400, 'Bad Request'], body: { 'error': "A random error." }.to_json)

Media.any_instance.stubs(:follow_redirections)
Media.any_instance.stubs(:get_canonical_url).returns(true)
Media.any_instance.stubs(:try_https)
Media.any_instance.stubs(:parse)
Media.any_instance.stubs(:archive)
m = Media.new url: url, key: api_key
assert_raises StandardError do
m.as_json(archivers: 'perma_cc')
end
media_data = Pender::Store.current.read(Media.get_id(url), :json)
assert_equal Lapis::ErrorCodes::const_get('ARCHIVER_ERROR'), media_data.dig('archives', 'perma_cc', 'error', 'code')
assert_equal '(400) Bad Request', media_data.dig('archives', 'perma_cc', 'error', 'message')
ensure
WebMock.disable!
end

a = create_api_key application_settings: { config: { 'perma_cc_key': 'my-perma-key' }, 'webhook_url': 'https://example.com/webhook.php', 'webhook_token': 'test' }
url = 'http://example.com'
test "when Perma.cc fails with Pender::Exception::TooManyCaptures it should update media with error and not retry" do
WebMock.enable!
WebMock.disable_net_connect!(allow: [/minio/])
Sidekiq::Testing.inline!
api_key = create_api_key application_settings: { config: { 'perma_cc_key': 'my-perma-key' }, 'webhook_url': 'https://example.com/webhook.php', 'webhook_token': 'test' }
url = 'https://example.com'

assert_raises Pender::Exception::RetryLater do
m = Media.new url: url, key: a
m.as_json
assert m.data.dig('archives', 'perma_cc').nil?
Media.send_to_perma_cc(url.to_s, a.id, 20)
media_data = Pender::Store.current.read(Media.get_id(url), :json)
assert_equal Lapis::ErrorCodes::const_get('ARCHIVER_FAILURE'), media_data.dig('archives', 'perma_cc', 'error', 'code')
assert_equal "401 Unauthorized", media_data.dig('archives', 'perma_cc', 'error', 'message')
Media.any_instance.unstub(:archive_to_perma_cc)
WebMock.stub_request(:get, url).to_return(status: 200, body: '<html>A page</html>')
WebMock.stub_request(:post, /safebrowsing\.googleapis\.com/).to_return(status: 200, body: '{}')
WebMock.stub_request(:post, /example.com\/webhook/).to_return(status: 200, body: '')
WebMock.stub_request(:post, /api.perma.cc/).to_return(status: [400, 'Bad Request'], body: { 'error': "Perma can't create this link. You've reached your usage limit. Visit your Usage Plan page for information and plan options." }.to_json)

m = Media.new url: url, key: api_key
assert_nothing_raised do
m.as_json(archivers: 'perma_cc')
end

media_data = Pender::Store.current.read(Media.get_id(url), :json)
assert_equal Lapis::ErrorCodes::const_get('ARCHIVER_ERROR'), media_data.dig('archives', 'perma_cc', 'error', 'code')
assert_equal 'Bad Request', media_data.dig('archives', 'perma_cc', 'error', 'message')
ensure
WebMock.disable!
end

test "should add disabled Perma.cc archiver error message if perma_key is not present" do
test "should add disabled Perma.cc archiver error message if perma_key is not defined" do
WebMock.enable!
WebMock.disable_net_connect!(allow: [/minio/])
Sidekiq::Testing.inline!
api_key = create_api_key application_settings: { 'webhook_url': 'https://example.com/webhook.php', 'webhook_token': 'test' }
url = 'https://example.com/'

WebMock.stub_request(:get, url).to_return(status: 200, body: '<html>A Page</html>')
Media.any_instance.unstub(:archive_to_perma_cc)
Media.stubs(:available_archivers).returns(['perma_cc'])
Metrics.stubs(:schedule_fetching_metrics_from_facebook).returns(nil)
WebMock.stub_request(:get, url).to_return(status: 200, body: '<html>A Page</html>')
WebMock.stub_request(:post, /safebrowsing\.googleapis\.com/).to_return(status: 200, body: '{}')
WebMock.stub_request(:post, "https://example.com/webhook.php").to_return(status: 200, body: '')

m = Media.new url: url, key: api_key
m.as_json(archivers: 'perma_cc')

id = Media.get_id(url)

assert_raises Pender::Exception::RetryLater do
m = Media.new url: url, key: nil
m.as_json(archivers: 'perma_cc')
end
cached = Pender::Store.current.read(id, :json)[:archives]
assert_match 'missing authentication', cached.dig('perma_cc', 'error', 'message').downcase
assert_equal Lapis::ErrorCodes::const_get('ARCHIVER_MISSING_KEY'), cached.dig('perma_cc', 'error', 'code')
Expand All @@ -514,6 +541,7 @@ def quietly_redefine_constant(klass, constant, new_value)
end

test "should call youtube-dl and call video upload when archive video" do
skip('we are not supporting archiving videos with youtube-dl anymore, will remove this on a separate ticket')
WebMock.enable!
WebMock.stub_request(:post, /example.com\/webhook/).to_return(status: 200, body: '')

Expand All @@ -535,6 +563,10 @@ def quietly_redefine_constant(klass, constant, new_value)
end

test "should return false and add error to data when video archiving is not supported" do
skip('we are not supporting archiving videos with youtube-dl anymore, will remove this on a separate ticket')
WebMock.enable!
api_key = create_api_key_with_webhook

Media.unstub(:supported_video?)
Media.any_instance.stubs(:parse)
Metrics.stubs(:schedule_fetching_metrics_from_facebook)
Expand Down Expand Up @@ -565,11 +597,13 @@ def quietly_redefine_constant(klass, constant, new_value)
end

test "should check if non-ascii URL support video download" do
skip('we are not supporting archiving videos with youtube-dl anymore, will remove this on a separate ticket')
Media.unstub(:supported_video?)
assert !Media.supported_video?('http://example.com/pages/category/Musician-Band/चौधरी-कमला-बाड़मेर-108960273957085')
end

test "should notify if URL was already parsed and has a location on data when archive video" do
skip('we are not supporting archiving videos with youtube-dl anymore, will remove this on a separate ticket')
WebMock.enable!
WebMock.stub_request(:post, /example.com\/webhook/).to_return(status: 200, body: '')

Expand All @@ -594,6 +628,7 @@ def quietly_redefine_constant(klass, constant, new_value)

# FIXME Mocking Youtube-DL to avoid `HTTP Error 429: Too Many Requests`
test "should archive video info subtitles, thumbnails and update cache" do
skip('we are not supporting archiving videos with youtube-dl anymore, will remove this on a separate ticket')
WebMock.enable!
WebMock.stub_request(:post, /example.com\/webhook/).to_return(status: 200, body: '')

Expand Down Expand Up @@ -632,6 +667,8 @@ def quietly_redefine_constant(klass, constant, new_value)
end

test "should raise retry error when video archiving fails" do
skip('we are not supporting archiving videos with youtube-dl anymore, will remove this on a separate ticket')
WebMock.enable!
Sidekiq::Testing.fake!
WebMock.enable!
WebMock.stub_request(:post, /example.com\/webhook/).to_return(status: 200, body: '')
Expand All @@ -658,6 +695,8 @@ def quietly_redefine_constant(klass, constant, new_value)
end

test "should update media with error when supported video call raises on video archiving" do
skip('we are not supporting archiving videos with youtube-dl anymore, will remove this on a separate ticket')
WebMock.enable!
Sidekiq::Testing.fake!
Media.any_instance.stubs(:follow_redirections)
Media.any_instance.stubs(:get_canonical_url).returns(true)
Expand All @@ -684,6 +723,7 @@ def quietly_redefine_constant(klass, constant, new_value)
end

test "should update media with error when video download fails when video archiving" do
skip('we are not supporting archiving videos with youtube-dl anymore, will remove this on a separate ticket')
WebMock.enable!
WebMock.stub_request(:post, /example.com\/webhook/).to_return(status: 200, body: '')

Expand Down Expand Up @@ -711,8 +751,9 @@ def quietly_redefine_constant(klass, constant, new_value)
end

test "should generate the public archiving folder for videos" do
a = create_api_key application_settings: { config: { storage_endpoint: 'http://minio:9000', storage_bucket: 'default-bucket', storage_video_asset_path: nil, storage_video_bucket: nil }}
ApiKey.current = a
skip('we are not supporting archiving videos with youtube-dl anymore, will remove this on a separate ticket')
api_key = create_api_key application_settings: { config: { storage_endpoint: 'http://minio:9000', storage_bucket: 'default-bucket', storage_video_asset_path: nil, storage_video_bucket: nil }}
ApiKey.current = api_key

assert_match /#{PenderConfig.get('storage_endpoint')}\/default-bucket\d*\/video/, Media.archiving_folder

Expand Down Expand Up @@ -758,6 +799,7 @@ def quietly_redefine_constant(klass, constant, new_value)
end

test "should send to video archiver when call archive to video" do
skip('we are not supporting archiving videos with youtube-dl anymore, will remove this on a separate ticket')
Media.any_instance.unstub(:archive_to_video)
Media.any_instance.stubs(:follow_redirections)
Media.any_instance.stubs(:get_canonical_url).returns(true)
Expand All @@ -773,6 +815,7 @@ def quietly_redefine_constant(klass, constant, new_value)
end

test "should get proxy to download video from api key if present" do
skip('we are not supporting archiving videos with youtube-dl anymore, will remove this on a separate ticket')
WebMock.enable!
WebMock.stub_request(:post, /example.com\/webhook/).to_return(status: 200, body: '')

Expand All @@ -791,6 +834,7 @@ def quietly_redefine_constant(klass, constant, new_value)
end

test "should use api key config when archiving video if present" do
skip('we are not supporting archiving videos with youtube-dl anymore, will remove this on a separate ticket')
WebMock.enable!
WebMock.stub_request(:post, /example.com\/webhook/).to_return(status: 200, body: '')

Expand Down
Loading