Merge branch 'pedropombeiro/1809/handle-overridden-variables-in-tsort' into 'master'

Preserve relative order of overridden variables

See merge request gitlab-org/gitlab!69653

lib/gitlab/ci/variables/collection.rb

@@ -10,7 +10,7 @@ module Gitlab
 
         def initialize(variables = [], errors = nil)
           @variables = []
-          @variables_by_key = {}
+          @variables_by_key = Hash.new { |h, k| h[k] = [] }
           @errors = errors
 
           variables.each { |variable| self.append(variable) }
@@ -19,7 +19,7 @@ module Gitlab
         def append(resource)
           item = Collection::Item.fabricate(resource)
           @variables.append(item)
-          @variables_by_key[item[:key]] = item
+          @variables_by_key[item[:key]] << item
 
           self
         end
@@ -46,7 +46,12 @@ module Gitlab
         end
 
         def [](key)
-          @variables_by_key[key]
+          all(key)&.last
+        end
+
+        def all(key)
+          vars = @variables_by_key[key]
+          vars unless vars.empty?
         end
 
         def size
@@ -72,7 +77,7 @@ module Gitlab
             match = Regexp.last_match
             if match[:key]
               # we matched variable
-              if variable = @variables_by_key[match[:key]]
+              if variable = self[match[:key]]
                 variable.value
               elsif keep_undefined
                 match[0]

lib/gitlab/ci/variables/collection/sort.rb

@@ -42,7 +42,7 @@ module Gitlab
             depends_on = var_item.depends_on
             return unless depends_on
 
-            depends_on.filter_map { |var_ref_name| @collection[var_ref_name] }.each(&block)
+            depends_on.filter_map { |var_ref_name| @collection.all(var_ref_name) }.flatten.each(&block)
           end
         end
       end

spec/lib/gitlab/ci/variables/collection/sort_spec.rb

@@ -5,20 +5,10 @@ require 'rspec-parameterized'
 
 RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
   describe '#initialize with non-Collection value' do
-    context 'when FF :variable_inside_variable is disabled' do
-      subject { Gitlab::Ci::Variables::Collection::Sort.new([]) }
+    subject { Gitlab::Ci::Variables::Collection::Sort.new([]) }
 
-      it 'raises ArgumentError' do
-        expect { subject }.to raise_error(ArgumentError, /Collection object was expected/)
-      end
-    end
-
-    context 'when FF :variable_inside_variable is enabled' do
-      subject { Gitlab::Ci::Variables::Collection::Sort.new([]) }
-
-      it 'raises ArgumentError' do
-        expect { subject }.to raise_error(ArgumentError, /Collection object was expected/)
-      end
+    it 'raises ArgumentError' do
+      expect { subject }.to raise_error(ArgumentError, /Collection object was expected/)
     end
   end
 
@@ -182,5 +172,33 @@ RSpec.describe Gitlab::Ci::Variables::Collection::Sort do
         expect { subject }.to raise_error(TSort::Cyclic)
       end
     end
+
+    context 'with overridden variables' do
+      let(:variables) do
+        [
+          { key: 'PROJECT_VAR',          value: '$SUBGROUP_VAR' },
+          { key: 'SUBGROUP_VAR',         value: '$TOP_LEVEL_GROUP_NAME' },
+          { key: 'SUBGROUP_VAR',         value: '$SUB_GROUP_NAME' },
+          { key: 'TOP_LEVEL_GROUP_NAME', value: 'top-level-group' },
+          { key: 'SUB_GROUP_NAME',       value: 'vars-in-vars-subgroup' }
+        ]
+      end
+
+      let(:collection) { Gitlab::Ci::Variables::Collection.new(variables) }
+
+      subject do
+        Gitlab::Ci::Variables::Collection::Sort.new(collection).tsort.map { |v| { v[:key] => v.value } }
+      end
+
+      it 'preserves relative order of overridden variables' do
+        is_expected.to eq([
+          { 'TOP_LEVEL_GROUP_NAME' => 'top-level-group' },
+          { 'SUBGROUP_VAR'         => '$TOP_LEVEL_GROUP_NAME' },
+          { 'SUB_GROUP_NAME'       => 'vars-in-vars-subgroup' },
+          { 'SUBGROUP_VAR'         => '$SUB_GROUP_NAME' },
+          { 'PROJECT_VAR'          => '$SUBGROUP_VAR' }
+        ])
+      end
+    end
   end
 end

spec/lib/gitlab/ci/variables/collection_spec.rb

@@ -123,17 +123,102 @@ RSpec.describe Gitlab::Ci::Variables::Collection do
   end
 
   describe '#[]' do
-    variable = { key: 'VAR', value: 'value', public: true, masked: false }
+    subject { Gitlab::Ci::Variables::Collection.new(variables)[var_name] }
 
-    collection = described_class.new([variable])
+    shared_examples 'an array access operator' do
+      context 'for a non-existent variable name' do
+        let(:var_name) { 'UNKNOWN_VAR' }
 
-    it 'returns nil for a non-existent variable name' do
-      expect(collection['UNKNOWN_VAR']).to be_nil
+        it 'returns nil' do
+          is_expected.to be_nil
+        end
+      end
+
+      context 'for an existent variable name' do
+        let(:var_name) { 'VAR' }
+
+        it 'returns the last Item' do
+          is_expected.to be_an_instance_of(Gitlab::Ci::Variables::Collection::Item)
+          expect(subject.to_runner_variable).to eq(variables.last)
+        end
+      end
+    end
+
+    context 'with variable key with single entry' do
+      let(:variables) do
+        [
+          { key: 'VAR', value: 'value', public: true, masked: false }
+        ]
+      end
+
+      it_behaves_like 'an array access operator'
+    end
+
+    context 'with variable key with multiple entries' do
+      let(:variables) do
+        [
+          { key: 'VAR', value: 'value', public: true, masked: false },
+          { key: 'VAR', value: 'override value', public: true, masked: false }
+        ]
+      end
+
+      it_behaves_like 'an array access operator'
     end
+  end
+
+  describe '#all' do
+    subject { described_class.new(variables).all(var_name) }
 
-    it 'returns Item for an existent variable name' do
-      expect(collection['VAR']).to be_an_instance_of(Gitlab::Ci::Variables::Collection::Item)
-      expect(collection['VAR'].to_runner_variable).to eq(variable)
+    shared_examples 'a method returning all known variables or nil' do
+      context 'for a non-existent variable name' do
+        let(:var_name) { 'UNKNOWN_VAR' }
+
+        it 'returns nil' do
+          is_expected.to be_nil
+        end
+      end
+
+      context 'for an existing variable name' do
+        let(:var_name) { 'VAR' }
+
+        it 'returns all expected Items' do
+          is_expected.to eq(expected_variables.map { |v| Gitlab::Ci::Variables::Collection::Item.fabricate(v) })
+        end
+      end
+    end
+
+    context 'with variable key with single entry' do
+      let(:variables) do
+        [
+          { key: 'VAR', value: 'value', public: true, masked: false }
+        ]
+      end
+
+      it_behaves_like 'a method returning all known variables or nil' do
+        let(:expected_variables) do
+          [
+            { key: 'VAR', value: 'value', public: true, masked: false }
+          ]
+        end
+      end
+    end
+
+    context 'with variable key with multiple entries' do
+      let(:variables) do
+        [
+          { key: 'VAR', value: 'value', public: true, masked: false },
+          { key: 'VAR', value: 'override value', public: true, masked: false }
+        ]
+      end
+
+      it_behaves_like 'a method returning all known variables or nil' do
+        let(:expected_variables) do
+          [
+            { key: 'VAR', value: 'value', public: true, masked: false },
+            { key: 'VAR', value: 'override value', public: true, masked: false }
+          ]
+        end
+      end
     end
   end