Add Collection::Sorted class and possible_var_reference?

This class performs a topological sort of an array of runner variables

lib/expand_variables.rb

@@ -16,6 +16,12 @@ module ExpandVariables
       end
     end
 
+    def possible_var_reference?(value)
+      return unless value
+
+      %w[$ %].any? { |symbol| value.include?(symbol) }
+    end
+
     private
 
     def replace_with(value, variables)

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Variables
+      class Collection
+        class Sorted
+          include TSort
+          include Gitlab::Utils::StrongMemoize
+
+          def initialize(variables)
+            @variables = variables
+          end
+
+          def valid?
+            return @valid unless @valid.nil?
+
+            @valid = check_errors.nil?
+          end
+
+          # check_errors sorts an array of variables, ignoring unknown variable references,
+          # and returning an error string if a circular variable reference is found
+          def check_errors
+            return if Feature.disabled?(:variable_inside_variable)
+
+            message = nil
+
+            # Check for cyclic dependencies and build error message in that case
+            each_strongly_connected_component do |component|
+              if component.size > 1
+                message = "circular variable reference detected: #{component.map { |v| v[:key] }.inspect}"
+                break
+              end
+            end
+
+            message
+          end
+
+          # sort sorts an array of variables, ignoring unknown variable references.
+          # If a circular variable reference is found, the original array is returned
+          def sort
+            return @variables if Feature.disabled?(:variable_inside_variable)
+
+            begin
+              # Perform a topological sort
+              variables = tsort
+              @valid = true
+            rescue TSort::Cyclic
+              variables = @variables
+              @valid = false
+            end
+
+            variables
+          end
+
+          private
+
+          def tsort_each_node(&block)
+            @variables.each(&block)
+          end
+
+          def tsort_each_child(variable, &block)
+            each_variable_reference(variable[:value], &block)
+          end
+
+          def input_vars
+            strong_memoize(:inclusion) do
+              @variables.index_by { |env| env.fetch(:key) }
+            end
+          end
+
+          def walk_references(value)
+            return unless ExpandVariables.possible_var_reference?(value)
+
+            value.scan(ExpandVariables::VARIABLES_REGEXP) do |var_ref|
+              yield(input_vars, var_ref.first)
+            end
+          end
+
+          def each_variable_reference(value)
+            walk_references(value) do |vars_hash, ref_var_name|
+              variable = vars_hash.dig(ref_var_name)
+              yield variable if variable
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/lib/expand_variables_spec.rb

@@ -224,4 +224,41 @@ RSpec.describe ExpandVariables do
       end
     end
   end
+
+  describe '#possible_var_reference?' do
+    context 'table tests' do
+      using RSpec::Parameterized::TableSyntax
+
+      where do
+        {
+          "empty value": {
+            value: '',
+            result: false
+          },
+          "normal value": {
+            value: 'some value',
+            result: false
+          },
+          "simple expansions": {
+            value: 'key$variable',
+            result: true
+          },
+          "complex expansions": {
+            value: 'key${variable}${variable2}',
+            result: true
+          },
+          "complex expansions for Windows": {
+            value: 'key%variable%%variable2%',
+            result: true
+          }
+        }
+      end
+
+      with_them do
+        subject { ExpandVariables.possible_var_reference?(value) }
+
+        it { is_expected.to eq(result) }
+      end
+    end
+  end
 end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Variables::Collection::Sorted do
+  let(:variable_inside_variable_enabled) { false }
+
+  before do
+    stub_feature_flags(variable_inside_variable: variable_inside_variable_enabled)
+  end
+
+  describe '#check_errors' do
+    context 'when FF :variable_inside_variable is disabled' do
+      let(:variable_inside_variable_enabled) { false }
+
+      context 'table tests' do
+        using RSpec::Parameterized::TableSyntax
+
+        where do
+          {
+            "empty array": {
+              variables: []
+            },
+            "simple expansions": {
+              variables: [
+                { key: 'variable', value: 'value' },
+                { key: 'variable2', value: 'result' },
+                { key: 'variable3', value: 'key$variable$variable2' }
+              ]
+            },
+            "complex expansion": {
+              variables: [
+                { key: 'variable', value: 'value' },
+                { key: 'variable2', value: 'key${variable}' }
+              ]
+            },
+            "complex expansions with missing variable for Windows": {
+              variables: [
+                { key: 'variable', value: 'value' },
+                { key: 'variable3', value: 'key%variable%%variable2%' }
+              ]
+            },
+            "out-of-order variable reference": {
+              variables: [
+                { key: 'variable2', value: 'key${variable}' },
+                { key: 'variable', value: 'value' }
+              ]
+            },
+            "array with cyclic dependency": {
+              variables: [
+                { key: 'variable', value: '$variable2' },
+                { key: 'variable2', value: '$variable3' },
+                { key: 'variable3', value: 'key$variable$variable2' }
+              ]
+            }
+          }
+        end
+
+        with_them do
+          subject { Gitlab::Ci::Variables::Collection::Sorted.new(variables) }
+
+          it 'does not report error' do
+            expect(subject.check_errors).to eq(nil)
+          end
+
+          it 'valid? reports true' do
+            expect(subject.valid?).to eq(true)
+          end
+        end
+      end
+    end
+
+    context 'when FF :variable_inside_variable is enabled' do
+      let(:variable_inside_variable_enabled) { true }
+
+      context 'table tests' do
+        using RSpec::Parameterized::TableSyntax
+
+        where do
+          {
+            "empty array": {
+              variables: [],
+              validation_result: nil
+            },
+            "simple expansions": {
+              variables: [
+                { key: 'variable', value: 'value' },
+                { key: 'variable2', value: 'result' },
+                { key: 'variable3', value: 'key$variable$variable2' }
+              ],
+              validation_result: nil
+            },
+            "cyclic dependency": {
+              variables: [
+                { key: 'variable', value: '$variable2' },
+                { key: 'variable2', value: '$variable3' },
+                { key: 'variable3', value: 'key$variable$variable2' }
+              ],
+              validation_result: 'circular variable reference detected: ["variable", "variable2", "variable3"]'
+            }
+          }
+        end
+
+        with_them do
+          subject { Gitlab::Ci::Variables::Collection::Sorted.new(variables) }
+
+          it 'check_errors matches expected validation result' do
+            expect(subject.check_errors).to eq(validation_result)
+          end
+
+          it 'valid? matches expected validation result' do
+            expect(subject.valid?).to eq(validation_result.nil?)
+          end
+        end
+      end
+    end
+  end
+
+  describe '#sort' do
+    context 'when FF :variable_inside_variable is disabled' do
+      let(:variable_inside_variable_enabled) { false }
+
+      context 'table tests' do
+        using RSpec::Parameterized::TableSyntax
+
+        where do
+          {
+            "empty array": {
+              variables: []
+            },
+            "simple expansions": {
+              variables: [
+                { key: 'variable', value: 'value' },
+                { key: 'variable2', value: 'result' },
+                { key: 'variable3', value: 'key$variable$variable2' }
+              ]
+            },
+            "complex expansion": {
+              variables: [
+                { key: 'variable', value: 'value' },
+                { key: 'variable2', value: 'key${variable}' }
+              ]
+            },
+            "complex expansions with missing variable for Windows": {
+              variables: [
+                { key: 'variable', value: 'value' },
+                { key: 'variable3', value: 'key%variable%%variable2%' }
+              ]
+            },
+            "out-of-order variable reference": {
+              variables: [
+                { key: 'variable2', value: 'key${variable}' },
+                { key: 'variable', value: 'value' }
+              ]
+            },
+            "array with cyclic dependency": {
+              variables: [
+                { key: 'variable', value: '$variable2' },
+                { key: 'variable2', value: '$variable3' },
+                { key: 'variable3', value: 'key$variable$variable2' }
+              ]
+            }
+          }
+        end
+
+        with_them do
+          subject { Gitlab::Ci::Variables::Collection::Sorted.new(variables) }
+
+          it 'does not expand variables' do
+            expect(subject.sort).to eq(variables)
+          end
+        end
+      end
+    end
+
+    context 'when FF :variable_inside_variable is enabled' do
+      let(:variable_inside_variable_enabled) { true }
+
+      context 'table tests' do
+        using RSpec::Parameterized::TableSyntax
+
+        where do
+          {
+            "empty array": {
+              variables: [],
+              result: []
+            },
+            "simple expansions, no reordering needed": {
+              variables: [
+                { key: 'variable', value: 'value' },
+                { key: 'variable2', value: 'result' },
+                { key: 'variable3', value: 'key$variable$variable2' }
+              ],
+              result: %w[variable variable2 variable3]
+            },
+            "complex expansion, reordering needed": {
+              variables: [
+                { key: 'variable2', value: 'key${variable}' },
+                { key: 'variable', value: 'value' }
+              ],
+              result: %w[variable variable2]
+            },
+            "unused variables": {
+              variables: [
+                { key: 'variable', value: 'value' },
+                { key: 'variable4', value: 'key$variable$variable3' },
+                { key: 'variable2', value: 'result2' },
+                { key: 'variable3', value: 'result3' }
+              ],
+              result: %w[variable variable3 variable4 variable2]
+            },
+            "missing variable": {
+              variables: [
+                { key: 'variable2', value: 'key$variable' }
+              ],
+              result: %w[variable2]
+            },
+            "complex expansions with missing variable": {
+              variables: [
+                { key: 'variable4', value: 'key${variable}${variable2}${variable3}' },
+                { key: 'variable', value: 'value' },
+                { key: 'variable3', value: 'value3' }
+              ],
+              result: %w[variable variable3 variable4]
+            },
+            "cyclic dependency causes original array to be returned": {
+              variables: [
+                { key: 'variable2', value: '$variable3' },
+                { key: 'variable3', value: 'key$variable$variable2' },
+                { key: 'variable', value: '$variable2' }
+              ],
+              result: %w[variable2 variable3 variable]
+            }
+          }
+        end
+
+        with_them do
+          subject { Gitlab::Ci::Variables::Collection::Sorted.new(variables) }
+
+          it 'sort returns correctly sorted variables' do
+            expect(subject.sort.map { |var| var[:key] }).to eq(result)
+          end
+        end
+      end
+    end
+  end
+end