diff --git a/.ruby-version b/.ruby-version index 74500cee1..be94e6f53 100644 --- a/.ruby-version +++ b/.ruby-version @@ -1 +1 @@ -2.7.4 \ No newline at end of file +3.2.2 diff --git a/CHANGELOG.md b/CHANGELOG.md index 77c08e3d6..2c5f13307 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ _None_ ### New Features -_None_ +- `ios_download_string_files_from_glotpress` supports automatic retry when receiving a 429 response, enabled by default [#516] ### Bug Fixes diff --git a/Gemfile.lock b/Gemfile.lock index 9ee698136..c183b9fce 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -29,7 +29,7 @@ GEM i18n (>= 1.6, < 2) minitest (>= 5.1) tzinfo (~> 2.0) - addressable (2.8.4) + addressable (2.8.5) public_suffix (>= 2.0.2, < 6.0) algoliasearch (1.27.5) httpclient (~> 2.8, >= 2.8.3) @@ -320,7 +320,7 @@ GEM trailblazer-option (>= 0.1.1, < 0.2.0) uber (< 0.2.0) retriable (3.1.2) - rexml (3.2.5) + rexml (3.2.6) rmagick (4.3.0) rouge (2.0.7) rspec (3.12.0) @@ -392,7 +392,7 @@ GEM unf_ext unf_ext (0.0.8.2) unicode-display_width (2.4.2) - webmock (3.18.1) + webmock (3.19.1) addressable (>= 2.8.0) crack (>= 0.3.2) hashdiff (>= 0.4.0, < 2.0.0) diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb new file mode 100644 index 000000000..17e00e3f9 --- /dev/null +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/glotpress_downloader.rb @@ -0,0 +1,52 @@ +require 'net/http' + +module Fastlane + module Helper + class GlotpressDownloader + AUTO_RETRY_SLEEP_TIME = 20 + MAX_AUTO_RETRY_ATTEMPTS = 30 + + def initialize( + auto_retry: true, + auto_retry_sleep_time: 20, + auto_retry_max_attempts: 30 + ) + @auto_retry = auto_retry + @auto_retry_sleep_time = auto_retry_sleep_time + @auto_retry_max_attempts = auto_retry_max_attempts + @auto_retry_attempt_counter = 0 + end + + def download(glotpress_url, options = {}) + uri = URI(glotpress_url) + response = Net::HTTP.get_response(uri, options) + + case response.code + when '200' # All good pass the result along + response + when '301' # Follow the redirect + UI.message("Received 301 for `#{response.uri}`. Following redirect...") + download(response.header['location']) + when '429' # We got rate-limited, auto_retry or offer to try again with a prompt + if @auto_retry + if @auto_retry_attempt_counter < @auto_retry_max_attempts + UI.message("Received 429 for `#{response.uri}`. Auto retrying in #{@auto_retry_sleep_time} seconds...") + sleep(@auto_retry_sleep_time) + @auto_retry_attempt_counter += 1 + download(response.uri, options) + else + UI.error("Abandoning `#{response.uri}` download after #{@auto_retry_attempt_counter} retries.") + end + elsif UI.confirm("Retry downloading `#{response.uri}` after receiving 429 from the API?") + download(response.uri, options) + else + UI.error("Abandoning `#{response.uri}` download as requested.") + end + else + message = "Received unexpected #{response.code} from request to URI #{response.uri}." + UI.abort_with_message!(message) unless UI.confirm("#{message} Continue anyway?") + end + end + end + end +end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb index e7197fd8a..97d08efa3 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/ios/ios_l10n_helper.rb @@ -161,19 +161,33 @@ def self.generate_strings_file_from_hash(translations:, output_path:) # Typical examples include `{ status: 'current' }` or `{ status: 'review' }`. # @param [String, IO] destination The path or `IO`-like instance, where to write the downloaded file on disk. # - def self.download_glotpress_export_file(project_url:, locale:, filters:, destination:) + def self.download_glotpress_export_file( + project_url:, + locale:, + filters:, + destination:, + autoretry: true, + autoretry_max: Fastlane::Helper::GlotpressDownloader::MAX_AUTO_RETRY_ATTEMPTS, + autoretry_sleep: Fastlane::Helper::GlotpressDownloader::AUTO_RETRY_SLEEP_TIME + ) query_params = (filters || {}).transform_keys { |k| "filters[#{k}]" }.merge(format: 'strings') uri = URI.parse("#{project_url.chomp('/')}/#{locale}/default/export-translations/?#{URI.encode_www_form(query_params)}") + downloader = Fastlane::Helper::GlotpressDownloader.new( + auto_retry: autoretry, + auto_retry_sleep_time: autoretry_sleep, + auto_retry_max_attempts: autoretry_max + ) + # Set an unambiguous User Agent so GlotPress won't rate-limit us options = { 'User-Agent' => Wpmreleasetoolkit::USER_AGENT } + response = downloader.download(uri.to_s, options) + begin - IO.copy_stream(uri.open(options), destination) + IO.copy_stream(StringIO.new(response.body), destination) rescue StandardError => e - UI.error "Error downloading locale `#{locale}` — #{e.message} (#{uri})" - retry if e.is_a?(OpenURI::HTTPError) && UI.confirm("Retry downloading `#{locale}`?") - return nil + UI.error "Error saving locale `#{locale}` — #{e.message} (#{uri})" end end end diff --git a/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb b/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb index 9745306d1..231224e42 100644 --- a/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb +++ b/lib/fastlane/plugin/wpmreleasetoolkit/helper/metadata_download_helper.rb @@ -4,23 +4,24 @@ module Fastlane module Helper class MetadataDownloader - AUTO_RETRY_SLEEP_TIME = 20 - MAX_AUTO_RETRY_ATTEMPTS = 30 - attr_reader :target_folder, :target_files - def initialize(target_folder, target_files, auto_retry) + def initialize(target_folder, target_files, auto_retry, auto_retry_sleep_time = 20) @target_folder = target_folder @target_files = target_files @auto_retry = auto_retry + @auto_retry_sleep_time = auto_retry_sleep_time @alternates = {} @auto_retry_attempt_counter = 0 end # Downloads data from GlotPress, in JSON format def download(target_locale, glotpress_url, is_source) - uri = URI(glotpress_url) - response = Net::HTTP.get_response(uri) + downloader = GlotpressDownloader.new( + auto_retry: @auto_retry, + auto_retry_sleep_time: @auto_retry_sleep_time + ) + response = downloader.download(glotpress_url) handle_glotpress_download(response: response, locale: target_locale, is_source: is_source) end @@ -112,22 +113,6 @@ def handle_glotpress_download(response:, locale:, is_source:) loc_data = JSON.parse(response.body) rescue loc_data = nil parse_data(locale, loc_data, is_source) reparse_alternates(target_locale, loc_data, is_source) unless @alternates.empty? - when '301' - # Follow the redirect - UI.message("Received 301 for `#{locale}`. Following redirect...") - download(locale, response.header['location'], is_source) - when '429' - # We got rate-limited, auto_retry or offer to try again with a prompt - if @auto_retry && @auto_retry_attempt_counter <= MAX_AUTO_RETRY_ATTEMPTS - UI.message("Received 429 for `#{locale}`. Auto retrying in #{AUTO_RETRY_SLEEP_TIME} seconds...") - sleep(AUTO_RETRY_SLEEP_TIME) - @auto_retry_attempt_counter += 1 - download(locale, response.uri, is_source) - elsif UI.confirm("Retry downloading `#{locale}` after receiving 429 from the API?") - download(locale, response.uri, is_source) - else - UI.error("Abandoning `#{locale}` download as requested.") - end else message = "Received unexpected #{response.code} from request to URI #{response.uri}." UI.abort_with_message!(message) unless UI.confirm("#{message} Continue anyway?") diff --git a/spec/glotpress_downloader_spec.rb b/spec/glotpress_downloader_spec.rb new file mode 100644 index 000000000..a0b1a5c88 --- /dev/null +++ b/spec/glotpress_downloader_spec.rb @@ -0,0 +1,116 @@ +require 'spec_helper' + +describe Fastlane::Helper::GlotpressDownloader do + describe 'downloading' do + context 'when auto retry is enabled' do + context 'when GlotPress returs a 200 code' do + it 'returns the response, without retrying' do + downloader = described_class.new(auto_retry: true) + fake_url = 'https://test.com' + + stub_request(:get, fake_url).to_return(status: 200, body: 'OK') + + response = downloader.download(fake_url) + + expect(response.code).to eq('200') + expect(response.body).to eq('OK') + # Make sure there was no retry + assert_requested(:get, fake_url, times: 1) + end + end + + context 'when GlotPress returs a 429 code' do + it 'retries automatically' do + sleep_time = 0.1 + downloader = described_class.new(auto_retry: true, auto_retry_sleep_time: sleep_time) + fake_url = 'https://test.com' + + count = 0 + stub_request(:get, fake_url).to_return do + count += 1 + if count == 1 + { status: 429, body: 'Too Many Requests' } + else + { status: 200, body: 'OK' } + end + end + + expect(Fastlane::UI).to receive(:message) + .with(/Received 429 for `#{fake_url}`. Auto retrying in #{sleep_time} seconds.../) + + response = downloader.download(fake_url) + + assert_requested(:get, fake_url, times: 2) + expect(response.code).to eq('200') + end + + context 'when the maximum number of retries is reached' do + it 'aborts' do + sleep_time = 0.1 + max_retries = 3 + downloader = described_class.new(auto_retry: true, auto_retry_sleep_time: sleep_time, auto_retry_max_attempts: max_retries) + fake_url = 'https://test.com' + + count = 0 + stub_request(:get, fake_url).to_return do + count += 1 + { status: 429, body: 'Too Many Requests' } + end + + expect(Fastlane::UI).to receive(:message) + .with(/Received 429 for `#{fake_url}`. Auto retrying in #{sleep_time} seconds.../) + .exactly(max_retries).times + + expect(Fastlane::UI).to receive(:error) + .with(/Abandoning `#{fake_url}` download after #{max_retries} retries./) + + downloader.download(fake_url) + + assert_requested(:get, fake_url, times: max_retries + 1) # the original request plus the retries + end + end + end + end + + context 'when auto retry is disabled' do + context 'when GlotPress returs a 200 code' do + it 'returns the response, without retrying' do + downloader = described_class.new(auto_retry: false) + fake_url = 'https://test.com' + + stub_request(:get, fake_url).to_return(status: 200, body: 'OK') + + response = downloader.download(fake_url) + + expect(response.code).to eq('200') + expect(response.body).to eq('OK') + # Make sure there was no retry + assert_requested(:get, fake_url, times: 1) + end + end + + context 'when GlotPress returs a 429 code' do + it 'prompt the user for confirmation, ignoring the max auto retry parameter' do + sleep_time = 0.1 + max_retries = 2 + downloader = described_class.new(auto_retry: false, auto_retry_sleep_time: sleep_time, auto_retry_max_attempts: max_retries) + fake_url = 'https://test.com' + + stub_request(:get, fake_url).to_return(status: 429, body: 'Too Many Requests') + + count = 0 + allow(Fastlane::UI).to receive(:confirm).with("Retry downloading `#{fake_url}` after receiving 429 from the API?") do + count += 1 + # Simulate user retrying more times that the max retries, then aborting + count <= max_retries + end + + # When the user aborts, with finish with an error + expect(Fastlane::UI).to receive(:error).with(/Abandoning `#{fake_url}` download as requested./) + + downloader.download(fake_url) + end + end + end + end +end diff --git a/spec/ios_l10n_helper_spec.rb b/spec/ios_l10n_helper_spec.rb index 3d8333535..38a8657d4 100644 --- a/spec/ios_l10n_helper_spec.rb +++ b/spec/ios_l10n_helper_spec.rb @@ -259,14 +259,14 @@ def file_encoding(path) # Arrange stub = stub_request(:get, "#{gp_fake_url}/invalid/default/export-translations/").with(query: { format: 'strings' }).to_return(status: [404, 'Not Found']) error_messages = [] - allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) } + allow(FastlaneCore::UI).to receive(:abort_with_message!) { |message| error_messages.append(message) } allow(FastlaneCore::UI).to receive(:confirm).and_return(false) # as we will be asked if we want to retry when getting a network error dest = StringIO.new # Act described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'invalid', filters: nil, destination: dest) # Assert expect(stub).to have_been_made.once - expect(error_messages).to eq(["Error downloading locale `invalid` — 404 Not Found (#{gp_fake_url}/invalid/default/export-translations/?format=strings)"]) + expect(error_messages).to eq(["Error downloading locale `invalid` — Received unexpected 404 from request to URI #{gp_fake_url}/invalid/default/export-translations/?format=strings"]) end it 'prints an `UI.error` if the destination cannot be written to' do @@ -280,7 +280,7 @@ def file_encoding(path) # Assert expect(stub).to have_been_made.once expect(File).not_to exist(dest) - expect(error_messages).to eq(["Error downloading locale `fr` — No such file or directory @ rb_sysopen - #{dest} (#{gp_fake_url}/fr/default/export-translations/?format=strings)"]) + expect(error_messages).to eq(["Error saving locale `fr` — No such file or directory @ rb_sysopen - #{dest} (#{gp_fake_url}/fr/default/export-translations/?format=strings)"]) end end end diff --git a/spec/metadata_download_helper_spec.rb b/spec/metadata_download_helper_spec.rb new file mode 100644 index 000000000..45cb1feb2 --- /dev/null +++ b/spec/metadata_download_helper_spec.rb @@ -0,0 +1,64 @@ +require 'spec_helper' + +describe Fastlane::Helper::MetadataDownloader do + describe 'downloading from GlotPress' do + it 'save the downloaded data to the target folder and file, including locale name' do + in_tmp_dir do |tmp_dir| + destination_name = 'target-file-name.txt' + dummy_url = 'https://test.com' + metadata_downloader = described_class.new(tmp_dir, { key: { desc: destination_name } }, true) + + stub_request(:get, dummy_url).to_return( + status: 200, + # GlotPress responses have a custom format. + body: { "key\u0004test metadata" => ['test metadata'] }.to_json + ) + + metadata_downloader.download('en-AU', dummy_url, false) + + destination_path_with_locale = File.join(tmp_dir, 'en-AU', destination_name) + expect(File.exist?(destination_path_with_locale)).to be true + # We also expect a trailing new line. + expect(File.read(destination_path_with_locale)).to eq("test metadata\n") + end + end + + context 'when GlotPress returs a 429 code' do + it 'automatically retries' do + in_tmp_dir do |tmp_dir| + metadata_downloader = described_class.new( + tmp_dir, + { key: { desc: 'target-file-name.txt' } }, + true, + 0.1 + ) + + fake_url = 'https://test.com' + + count = 0 + stub_request(:get, fake_url).to_return do + count += 1 + if count == 1 + { status: 429, body: 'Too Many Requests' } + else + { status: 200, body: 'OK' } + end + end + + expect(Fastlane::UI).to receive(:message) + .with(/Received 429 for `#{fake_url}`. Auto retrying in 0.1 seconds.../) + + expect(Fastlane::UI).to receive(:message) + .with(/No translation available for en-AU/) + + expect(Fastlane::UI).to receive(:success) + .with(/Successfully downloaded `en-AU`./) + + metadata_downloader.download('en-AU', fake_url, false) + + assert_requested(:get, fake_url, times: 2) + end + end + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 80cff9bfc..25cc6ae15 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -103,3 +103,14 @@ def with_tmp_file(named: nil, content: '') EMPTY_FIREBASE_TEST_LOG_PATH = File.join(__dir__, 'test-data', 'empty.json') PASSED_FIREBASE_TEST_LOG_PATH = File.join(__dir__, 'test-data', 'firebase', 'firebase-test-lab-run-passed.log') FAILED_FIREBASE_TEST_LOG_PATH = File.join(__dir__, 'test-data', 'firebase', 'firebase-test-lab-run-failure.log') + +# Monkey-patch WebMock to work around bug where response.uri is not set +# See https://github.com/bblimke/webmock/issues/469 +WebMock::HttpLibAdapters::NetHttpAdapter.instance_variable_get(:@webMockNetHTTP).class_eval do + old_request = instance_method :request + define_method :request do |request, &block| + old_request.bind(self).call(request, &block).tap do |response| + response.uri = request.uri + end + end +end