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.

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

Update GlotPress export-translations requests to avoid rate limiting #361

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -282,9 +282,13 @@ def self.download_from_glotpress(res_dir:, glotpress_project_url:, glotpress_fil
#
def self.download_glotpress_export_file(project_url:, locale:, filters:)
query_params = filters.transform_keys { |k| "filters[#{k}]" }.merge(format: 'android')
uri = URI.parse("#{project_url.chomp('/')}/#{locale}/default/export-translations?#{URI.encode_www_form(query_params)}")
uri = URI.parse("#{project_url.chomp('/')}/#{locale}/default/export-translations/?#{URI.encode_www_form(query_params)}")

# Set an unambiguous User Agent so GlotPress won't rate-limit us
options = { 'User-Agent' => Wpmreleasetoolkit::USER_AGENT }

begin
uri.open { |f| Nokogiri::XML(f.read.gsub("\t", ' '), nil, Encoding::UTF_8.to_s) }
uri.open(options) { |f| Nokogiri::XML(f.read.gsub("\t", ' '), nil, Encoding::UTF_8.to_s) }
rescue StandardError => e
UI.error "Error downloading #{locale} - #{e.message}"
return nil
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,13 @@ def self.generate_strings_file_from_hash(translations:, output_path:)
#
def self.download_glotpress_export_file(project_url:, locale:, filters:, destination:)
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)}")
uri = URI.parse("#{project_url.chomp('/')}/#{locale}/default/export-translations/?#{URI.encode_www_form(query_params)}")

# Set an unambiguous User Agent so GlotPress won't rate-limit us
options = { 'User-Agent' => Wpmreleasetoolkit::USER_AGENT }

begin
IO.copy_stream(uri.open, destination)
IO.copy_stream(uri.open(options), destination)
rescue StandardError => e
UI.error "Error downloading locale `#{locale}` — #{e.message}"
return nil
Expand Down
5 changes: 5 additions & 0 deletions lib/fastlane/plugin/wpmreleasetoolkit/helper/user_agent.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module Fastlane
module Wpmreleasetoolkit
USER_AGENT = 'Automattic App Release Automator; https://github.com/wordpress-mobile/release-toolkit/'.freeze
end
end
Comment on lines +1 to +5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure whether this is the best way to define a constant scoped to the whole toolkit codebase. I copied this approach from version.rb.

37 changes: 34 additions & 3 deletions spec/android_localize_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def generated_file(code)
Dir[File.join(stubs_dir, '*.xml')].each do |path|
# Each file in stubs_dir is a `{locale_code}.xml` whose content is what we want to use as stub for glotpress requests to `locale_code`
locale_code = File.basename(path, '.xml')
url = "#{gp_fake_url.chomp('/')}/#{locale_code}/default/export-translations?filters%5Bstatus%5D=current&format=android"
url = "#{gp_fake_url.chomp('/')}/#{locale_code}/default/export-translations/?filters%5Bstatus%5D=current&format=android"
stub_request(:get, url).to_return(status: 200, body: File.read(path))
end

Expand Down Expand Up @@ -215,7 +215,7 @@ def generated_file(code)

# Arrange: Prepare request stubs
custom_gp_urls = TEST_LOCALES_MAP.map do |locale|
"#{gp_fake_url.chomp('/')}/#{locale[:glotpress]}/default/export-translations?filters%5Bstatus%5D=custom-status&filters%5Bwarnings%5D=yes&format=android"
"#{gp_fake_url.chomp('/')}/#{locale[:glotpress]}/default/export-translations/?filters%5Bstatus%5D=custom-status&filters%5Bwarnings%5D=yes&format=android"
end
custom_gp_urls.each do |url|
stub_request(:get, url)
Expand Down Expand Up @@ -244,7 +244,7 @@ def generated_file(code)
statuses = %w[current waiting fuzzy]
statuses.each do |status|
stub_path = File.join(fixtures_dir, 'filters', "#{status}.xml")
stub_request(:get, "#{gp_fake_url.chomp('/')}/fakegploc/default/export-translations?filters%5Bstatus%5D=#{status}&format=android")
stub_request(:get, "#{gp_fake_url.chomp('/')}/fakegploc/default/export-translations/?filters%5Bstatus%5D=#{status}&format=android")
.to_return(status: 200, body: File.read(stub_path))
end

Expand All @@ -264,5 +264,36 @@ def generated_file(code)
expect(File.read(generated_file_path)).to eq(expected_merged_content)
end
end

it 'sets a predefined User Agent so GlotPress will not rate-limit us' do
# Arrange
#
# Note that in this test we don't care about what is downloaded or how it's processed,
# only whether the request is made with the expected User Agent.
# Therefore there is only minimum fixtures setup, unlike in the examples before
FileUtils.mkdir_p(File.dirname(generated_file(nil)))
FileUtils.cp(expected_file(nil), generated_file(nil))

stub = stub_request(:get, "#{gp_fake_url}fr/default/export-translations/")
.with(
query: { format: 'android', 'filters[status]': 'current' },
# Note that the syntax below merely checks that the given headers are present in the request,
# it does not restrict the request to have only those headers.
Comment on lines +280 to +281
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

#
# See:
# - https://github.com/bblimke/webmock/tree/33d8810c2828fc17010e15cc3f21ad2c726a966f#matching-requests
# - https://github.com/bblimke/webmock/issues/276#issuecomment-28625436
headers: { 'User-Agent' => 'Automattic App Release Automator; https://github.com/wordpress-mobile/release-toolkit/' }
).to_return(status: 200, body: '')

# Act
described_class.download_from_glotpress(
res_dir: tmpdir,
glotpress_project_url: gp_fake_url,
locales_map: TEST_LOCALES_MAP.select { |l| l[:android] == 'fr' } # only run for `fr` because we only stubbed that
)

expect(stub).to have_been_made.once
end
end
end
2 changes: 1 addition & 1 deletion spec/ios_download_strings_files_from_glotpress_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
let(:locales_subset) { { 'fr-FR': 'fr', 'zh-cn': 'zh-Hans' } }

def gp_stub(locale:, query:)
stub_request(:get, "#{gp_fake_url}/#{locale}/default/export-translations").with(query: query)
stub_request(:get, "#{gp_fake_url}/#{locale}/default/export-translations/").with(query: query)
end

describe 'downloading export files from GlotPress' do
Expand Down
31 changes: 26 additions & 5 deletions spec/ios_l10n_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,31 @@ def file_encoding(path)
describe '#download_glotpress_export_file' do
let(:gp_fake_url) { 'https://stub.glotpress.com/rspec-fake-project' }

it 'sets a predefined User Agent so GlotPress will not rate-limit us' do
# Arrange
stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations/")
.with(
query: { format: 'strings' },
# Note that the syntax below merely checks that the given headers are present in the request,
# it does not restrict the request to have only those headers.
#
# See:
# - https://github.com/bblimke/webmock/tree/33d8810c2828fc17010e15cc3f21ad2c726a966f#matching-requests
# - https://github.com/bblimke/webmock/issues/276#issuecomment-28625436
headers: { 'User-Agent' => 'Automattic App Release Automator; https://github.com/wordpress-mobile/release-toolkit/' }
).to_return(body: 'content')
dest = StringIO.new
# Act
described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest)
# Assert
expect(stub).to have_been_made.once
expect(dest.string).to eq('content')
end
Comment on lines +190 to +209
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nitpick: The android test has this example at the bottom, while here is at the top. I think we can live with that...


describe 'request query parameters' do
it 'passes the expected params when no filters are provided' do
# Arrange
stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations").with(query: { format: 'strings' }).to_return(body: 'content')
stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations/").with(query: { format: 'strings' }).to_return(body: 'content')
dest = StringIO.new
# Act
described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest)
Expand All @@ -201,7 +222,7 @@ def file_encoding(path)

it 'passes the expected params when a list of filters is provided' do
# Arrange
stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations")
stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations/")
.with(query: { format: 'strings', 'filters[status]': 'current', 'filters[term]': 'foobar' }).to_return(body: 'content')
dest = StringIO.new
# Act
Expand All @@ -221,7 +242,7 @@ def file_encoding(path)
# Note: in practice it seems that GlotPress's `.strings` exports are using UTF-8 (but served as `application/octet-stream`)
# but it does not hurt to ensure the download to a file can work with UTF-16 (and copy the binary stream verbatim)
body = File.read(fixture('Localizable-utf16.strings'))
stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations").with(query: { format: 'strings' }).to_return(body: body)
stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations/").with(query: { format: 'strings' }).to_return(body: body)
dest = File.join(tmp_dir, 'export.strings')
# Act
described_class.download_glotpress_export_file(project_url: gp_fake_url, locale: 'fr', filters: nil, destination: dest)
Expand All @@ -236,7 +257,7 @@ def file_encoding(path)
describe 'invalid parameters' do
it 'prints an `UI.error` if passed a non-existing locale (or any other 404)' do
# Arrange
stub = stub_request(:get, "#{gp_fake_url}/invalid/default/export-translations").with(query: { format: 'strings' }).to_return(status: [404, 'Not Found'])
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) }
dest = StringIO.new
Expand All @@ -249,7 +270,7 @@ def file_encoding(path)

it 'prints an `UI.error` if the destination cannot be written to' do
# Arrange
stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations").with(query: { format: 'strings' }).to_return(body: 'content')
stub = stub_request(:get, "#{gp_fake_url}/fr/default/export-translations/").with(query: { format: 'strings' }).to_return(body: 'content')
error_messages = []
allow(FastlaneCore::UI).to receive(:error) { |message| error_messages.append(message) }
dest = '/these/are/not/the/droids/you/are/looking/for.strings'
Expand Down