Merge branch 'pedropombeiro/sort-variables' into 'master'

Add Collection::Sorted class

See merge request gitlab-org/gitlab!50156

config/feature_flags/development/variable_inside_variable.yml

+---
+name: variable_inside_variable
+introduced_by_url:
+rollout_issue_url:
+milestone: '13.7'
+type: development
+group: group::runner
+default_enabled: false

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?
+            errors.nil?
+          end
+
+          # errors sorts an array of variables, ignoring unknown variable references,
+          # and returning an error string if a circular variable reference is found
+          def errors
+            return if Feature.disabled?(:variable_inside_variable)
+
+            strong_memoize(:errors) do
+              # Check for cyclic dependencies and build error message in that case
+              errors = each_strongly_connected_component.filter_map do |component|
+                component.map { |v| v[:key] }.inspect if component.size > 1
+              end
+
+              "circular variable reference detected: #{errors.join(', ')}" if errors.any?
+            end
+          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)
+            return @variables if errors
+
+            tsort
+          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(:input_vars) 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
+  describe '#errors' do
+    context 'when FF :variable_inside_variable is disabled' do
+      before do
+        stub_feature_flags(variable_inside_variable: false)
+      end
+
+      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.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
+      before do
+        stub_feature_flags(variable_inside_variable: true)
+      end
+
+      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 'errors matches expected validation result' do
+            expect(subject.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
+      before do
+        stub_feature_flags(variable_inside_variable: false)
+      end
+
+      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
+      before do
+        stub_licensed_features(group_saml_group_sync: true)
+        stub_feature_flags(saml_group_links: true)
+        stub_feature_flags(variable_inside_variable: true)
+      end
+
+      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