Forbids combining named and unnamed variables in `gitlab.pot`

This would break with an argument error when interpolating using
`sprintf` in ruby.

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 = ['<hello %{world} %d> is missing: [%{hello}]',
-                         '<hello %{world} %d> 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