From ca065e493e7a62f22cb330659e75f8567fa6131f Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 7 May 2018 16:04:11 +0200 Subject: Forbids combining named and unnamed variables in `gitlab.pot` This would break with an argument error when interpolating using `sprintf` in ruby. --- lib/gitlab/i18n/po_linter.rb | 99 +++++++++++++++++++++----------- lib/gitlab/i18n/translation_entry.rb | 20 ++++--- spec/lib/gitlab/i18n/po_linter_spec.rb | 102 ++++++++++++++++++++------------- 3 files changed, 140 insertions(+), 81 deletions(-) diff --git a/lib/gitlab/i18n/po_linter.rb b/lib/gitlab/i18n/po_linter.rb index 7d3ff8c7f58..ab79d4188dc 100644 --- a/lib/gitlab/i18n/po_linter.rb +++ b/lib/gitlab/i18n/po_linter.rb @@ -1,6 +1,8 @@ module Gitlab module I18n class PoLinter + include Gitlab::Utils::StrongMemoize + attr_reader :po_path, :translation_entries, :metadata_entry, :locale VARIABLE_REGEX = /%{\w*}|%[a-z]/.freeze @@ -48,7 +50,7 @@ module Gitlab translation_entries.each do |entry| errors_for_entry = validate_entry(entry) - errors[join_message(entry.msgid)] = errors_for_entry if errors_for_entry.any? + errors[entry.msgid] = errors_for_entry if errors_for_entry.any? end errors @@ -62,6 +64,7 @@ module Gitlab validate_newlines(errors, entry) validate_number_of_plurals(errors, entry) validate_unescaped_chars(errors, entry) + validate_translation(errors, entry) errors end @@ -117,41 +120,73 @@ module Gitlab end def validate_variables_in_message(errors, message_id, message_translation) - message_id = join_message(message_id) required_variables = message_id.scan(VARIABLE_REGEX) validate_unnamed_variables(errors, required_variables) - validate_translation(errors, message_id, required_variables) validate_variable_usage(errors, message_translation, required_variables) end - def validate_translation(errors, message_id, used_variables) + def validate_translation(errors, entry) + Gitlab::I18n.with_locale(locale) do + if entry.has_plural? + translate_plural(entry) + else + translate_singular(entry) + end + end + + # `sprintf` could raise an `ArgumentError` when invalid passing something + # other than a Hash when using named variables + # + # `sprintf` could raise `TypeError` when passing a wrong type when using + # unnamed variables + # + # FastGettext::Translation could raise `RuntimeError` (raised as a string), + # or as subclassess `NoTextDomainConfigured` & `InvalidFormat` + # + # `FastGettext::Translation` could raise `ArgumentError` as subclassess + # `InvalidEncoding`, `IllegalSequence` & `InvalidCharacter` + rescue ArgumentError, TypeError, RuntimeError => e + errors << "Failure translating to #{locale}: #{e.message}" + end + + def translate_singular(entry) + used_variables = entry.msgid.scan(VARIABLE_REGEX) variables = fill_in_variables(used_variables) - begin - Gitlab::I18n.with_locale(locale) do - translated = if message_id.include?('|') - FastGettext::Translation.s_(message_id) - else - FastGettext::Translation._(message_id) - end + translation = if entry.msgid.include?('|') + FastGettext::Translation.s_(entry.msgid) + else + FastGettext::Translation._(entry.msgid) + end - translated % variables - end + translation % variables if used_variables.any? + end + + def translate_plural(entry) + used_variables = entry.plural_id.scan(VARIABLE_REGEX) + variables = fill_in_variables(used_variables) + + numbers_covering_all_plurals.map do |number| + translation = FastGettext::Translation.n_(entry.msgid, entry.plural_id, number) + + translation % variables if used_variables.any? + end + end + + def numbers_covering_all_plurals + @numbers_covering_all_plurals ||= Array.new(metadata_entry.expected_plurals) do |index| + number_for_pluralization(index) + end + end + + def number_for_pluralization(counter) + pluralization_result = FastGettext.pluralisation_rule.call(counter) - # `sprintf` could raise an `ArgumentError` when invalid passing something - # other than a Hash when using named variables - # - # `sprintf` could raise `TypeError` when passing a wrong type when using - # unnamed variables - # - # FastGettext::Translation could raise `RuntimeError` (raised as a string), - # or as subclassess `NoTextDomainConfigured` & `InvalidFormat` - # - # `FastGettext::Translation` could raise `ArgumentError` as subclassess - # `InvalidEncoding`, `IllegalSequence` & `InvalidCharacter` - rescue ArgumentError, TypeError, RuntimeError => e - errors << "Failure translating to #{locale} with #{variables}: #{e.message}" + if pluralization_result.is_a?(TrueClass) || pluralization_result.is_a?(FalseClass) + counter + else + pluralization_result end end @@ -172,14 +207,16 @@ module Gitlab end def validate_unnamed_variables(errors, variables) - if variables.size > 1 && variables.any? { |variable_name| unnamed_variable?(variable_name) } + if variables.any? { |name| unnamed_variable?(name) } && variables.any? { |name| !unnamed_variable?(name) } + errors << 'is combining named variables with unnamed variables' + end + + if variables.select { |variable_name| unnamed_variable?(variable_name) }.size > 1 errors << 'is combining multiple unnamed variables' end end def validate_variable_usage(errors, translation, required_variables) - translation = join_message(translation) - # We don't need to validate when the message is empty. # In this case we fall back to the default, which has all the the # required variables. @@ -205,10 +242,6 @@ module Gitlab def validate_flags(errors, entry) errors << "is marked #{entry.flag}" if entry.flag end - - def join_message(message) - Array(message).join - end end end end diff --git a/lib/gitlab/i18n/translation_entry.rb b/lib/gitlab/i18n/translation_entry.rb index e6c95afca7e..13c544aed27 100644 --- a/lib/gitlab/i18n/translation_entry.rb +++ b/lib/gitlab/i18n/translation_entry.rb @@ -11,11 +11,11 @@ module Gitlab end def msgid - entry_data[:msgid] + @msgid ||= Array(entry_data[:msgid]).join end def plural_id - entry_data[:msgid_plural] + @plural_id ||= Array(entry_data[:msgid_plural]).join end def has_plural? @@ -23,12 +23,11 @@ module Gitlab end def singular_translation - all_translations.first if has_singular_translation? + all_translations.first.to_s if has_singular_translation? end def all_translations - @all_translations ||= entry_data.fetch_values(*translation_keys) - .reject(&:empty?) + @all_translations ||= translation_entries.map { |translation| Array(translation).join } end def translated? @@ -55,15 +54,15 @@ module Gitlab end def msgid_contains_newlines? - msgid.is_a?(Array) + entry_data[:msgid].is_a?(Array) end def plural_id_contains_newlines? - plural_id.is_a?(Array) + entry_data[:msgid_plural].is_a?(Array) end def translations_contain_newlines? - all_translations.any? { |translation| translation.is_a?(Array) } + translation_entries.any? { |translation| translation.is_a?(Array) } end def msgid_contains_unescaped_chars? @@ -84,6 +83,11 @@ module Gitlab private + def translation_entries + @translation_entries ||= entry_data.fetch_values(*translation_keys) + .reject(&:empty?) + end + def translation_keys @translation_keys ||= entry_data.keys.select { |key| key.to_s =~ /\Amsgstr(\[\d+\])?\z/ } end diff --git a/spec/lib/gitlab/i18n/po_linter_spec.rb b/spec/lib/gitlab/i18n/po_linter_spec.rb index 3a962ba7f22..9bbd51fe212 100644 --- a/spec/lib/gitlab/i18n/po_linter_spec.rb +++ b/spec/lib/gitlab/i18n/po_linter_spec.rb @@ -5,6 +5,25 @@ describe Gitlab::I18n::PoLinter do let(:linter) { described_class.new(po_path) } let(:po_path) { 'spec/fixtures/valid.po' } + def fake_translation(id:, translation:, plural_id: nil, plurals: []) + data = { msgid: id, msgid_plural: plural_id } + + if plural_id + [translation, *plurals].each_with_index do |plural, index| + allow(FastGettext::Translation).to receive(:n_).with(plural_id, index).and_return(plural) + data.merge!("msgstr[#{index}]" => plural) + end + else + allow(FastGettext::Translation).to receive(:_).with(id).and_return(translation) + data[:msgstr] = translation + end + + Gitlab::I18n::TranslationEntry.new( + data, + plurals.size + 1 + ) + end + describe '#errors' do it 'only calls validation once' do expect(linter).to receive(:validate_po).once.and_call_original @@ -155,9 +174,8 @@ describe Gitlab::I18n::PoLinter do describe '#validate_entries' do it 'keeps track of errors for entries' do - fake_invalid_entry = Gitlab::I18n::TranslationEntry.new( - { msgid: "Hello %{world}", msgstr: "Bonjour %{monde}" }, 2 - ) + fake_invalid_entry = fake_translation(id: "Hello %{world}", + translation: "Bonjour %{monde}") allow(linter).to receive(:translation_entries) { [fake_invalid_entry] } expect(linter).to receive(:validate_entry) @@ -177,6 +195,7 @@ describe Gitlab::I18n::PoLinter do expect(linter).to receive(:validate_newlines).with([], fake_entry) expect(linter).to receive(:validate_number_of_plurals).with([], fake_entry) expect(linter).to receive(:validate_unescaped_chars).with([], fake_entry) + expect(linter).to receive(:validate_translation).with([], fake_entry) linter.validate_entry(fake_entry) end @@ -201,13 +220,12 @@ describe Gitlab::I18n::PoLinter do end describe '#validate_variables' do - it 'validates both signular and plural in a pluralized string when the entry has a singular' do - pluralized_entry = Gitlab::I18n::TranslationEntry.new( - { msgid: 'Hello %{world}', - msgid_plural: 'Hello all %{world}', - 'msgstr[0]' => 'Bonjour %{world}', - 'msgstr[1]' => 'Bonjour tous %{world}' }, - 2 + it 'validates both singular and plural in a pluralized string when the entry has a singular' do + pluralized_entry = fake_translation( + id: 'Hello %{world}', + translation: 'Bonjour %{world}', + plural_id: 'Hello all %{world}', + plurals: ['Bonjour tous %{world}'] ) expect(linter).to receive(:validate_variables_in_message) @@ -221,11 +239,10 @@ describe Gitlab::I18n::PoLinter do end it 'only validates plural when there is no separate singular' do - pluralized_entry = Gitlab::I18n::TranslationEntry.new( - { msgid: 'Hello %{world}', - msgid_plural: 'Hello all %{world}', - 'msgstr[0]' => 'Bonjour %{world}' }, - 1 + pluralized_entry = fake_translation( + id: 'Hello %{world}', + translation: 'Bonjour %{world}', + plural_id: 'Hello all %{world}' ) expect(linter).to receive(:validate_variables_in_message) @@ -235,10 +252,7 @@ describe Gitlab::I18n::PoLinter do end it 'validates the message variables' do - entry = Gitlab::I18n::TranslationEntry.new( - { msgid: 'Hello', msgstr: 'Bonjour' }, - 2 - ) + entry = fake_translation(id: 'Hello', translation: 'Bonjour') expect(linter).to receive(:validate_variables_in_message) .with([], 'Hello', 'Bonjour') @@ -251,21 +265,34 @@ describe Gitlab::I18n::PoLinter do it 'detects when a variables are used incorrectly' do errors = [] - expected_errors = [' is missing: [%{hello}]', - ' is using unknown variables: [%{world}]', - 'is combining multiple unnamed variables'] + expected_errors = ['<%d hello %{world} %s> is missing: [%{hello}]', + '<%d hello %{world} %s> is using unknown variables: [%{world}]', + 'is combining multiple unnamed variables', + 'is combining named variables with unnamed variables'] - linter.validate_variables_in_message(errors, '%{hello} world %d', 'hello %{world} %d') + linter.validate_variables_in_message(errors, '%d %{hello} world %s', '%d hello %{world} %s') expect(errors).to include(*expected_errors) end + + it 'does not allow combining 1 `%d` unnamed variable with named variables' do + errors = [] + + linter.validate_variables_in_message(errors, + '%{type} detected %d vulnerability', + '%{type} detecteerde %d kwetsbaarheid') + + expect(errors).not_to be_empty + end end describe '#validate_translation' do + let(:entry) { fake_translation(id: 'Hello %{world}', translation: 'Bonjour %{world}') } + it 'succeeds with valid variables' do errors = [] - linter.validate_translation(errors, 'Hello %{world}', ['%{world}']) + linter.validate_translation(errors, entry) expect(errors).to be_empty end @@ -275,43 +302,38 @@ describe Gitlab::I18n::PoLinter do expect(FastGettext::Translation).to receive(:_) { raise 'broken' } - linter.validate_translation(errors, 'Hello', []) + linter.validate_translation(errors, entry) - expect(errors).to include('Failure translating to en with []: broken') + expect(errors).to include('Failure translating to en: broken') end it 'adds an error message when translating fails when translating with context' do + entry = fake_translation(id: 'Tests|Hello', translation: 'broken') errors = [] expect(FastGettext::Translation).to receive(:s_) { raise 'broken' } - linter.validate_translation(errors, 'Tests|Hello', []) + linter.validate_translation(errors, entry) - expect(errors).to include('Failure translating to en with []: broken') + expect(errors).to include('Failure translating to en: broken') end it "adds an error when trying to translate with incorrect variables when using unnamed variables" do + entry = fake_translation(id: 'Hello %s', translation: 'Hello %d') errors = [] - linter.validate_translation(errors, 'Hello %d', ['%s']) + linter.validate_translation(errors, entry) - expect(errors.first).to start_with("Failure translating to en with") + expect(errors.first).to start_with("Failure translating to en") end it "adds an error when trying to translate with named variables when unnamed variables are expected" do + entry = fake_translation(id: 'Hello %s', translation: 'Hello %{thing}') errors = [] - linter.validate_translation(errors, 'Hello %d', ['%{world}']) - - expect(errors.first).to start_with("Failure translating to en with") - end - - it 'adds an error when translated with incorrect variables using named variables' do - errors = [] - - linter.validate_translation(errors, 'Hello %{thing}', ['%d']) + linter.validate_translation(errors, entry) - expect(errors.first).to start_with("Failure translating to en with") + expect(errors.first).to start_with("Failure translating to en") end end -- cgit v1.2.1