Add post compose validation

Use post compose validation:
- ensure that bridge has a valid number of entries
- use simplifiable to construct need type
- use aspect with opt to indicate allowed opts

ee/lib/ee/gitlab/ci/config/entry/bridge.rb

@@ -23,34 +23,26 @@ module EE
               validates :name, presence: true
               validates :name, type: Symbol
 
-              validate do
-                unless trigger.present? || needs.present?
-                  errors.add(:config, 'should contain either a trigger or a needs:pipeline')
-                end
+              with_options allow_nil: true do
+                validates :when,
+                  inclusion: { in: %w[on_success on_failure always],
+                               message: 'should be on_success, on_failure or always' }
+                validates :extends, type: String
               end
 
-              validate do
-                next unless needs.present?
-
-                if [needs].flatten.any? { |need| !(Entry::Need::Bridge.matching?(need) || ::Gitlab::Ci::Config::Entry::Need::Pipeline.matching?(need)) }
-                  errors.add(:needs, 'can only have bridge or pipeline type needs')
+              validate on: :composed do
+                unless trigger.present? || bridge_needs.present?
+                  errors.add(:config, 'should contain either a trigger or a needs:pipeline')
                 end
               end
 
-              validate do
-                next unless needs.present?
+              validate on: :composed do
+                next unless bridge_needs.present?
 
-                if [needs].flatten.count { |need| Entry::Need::Bridge.matching?(need) } > 1
-                  errors.add(:needs, 'can only have one bridge type needs')
+                unless bridge_needs.one?
+                  errors.add(:config, 'should contain exactly one bridge need')
                 end
               end
-
-              with_options allow_nil: true do
-                validates :when,
-                  inclusion: { in: %w[on_success on_failure always],
-                               message: 'should be on_success, on_failure or always' }
-                validates :extends, type: String
-              end
             end
 
             entry :trigger, ::EE::Gitlab::Ci::Config::Entry::Trigger,
@@ -59,7 +51,8 @@ module EE
 
             entry :needs, ::Gitlab::Ci::Config::Entry::Needs,
               description: 'CI/CD Bridge needs dependency definition.',
-              inherit: false
+              inherit: false,
+              metadata: { allowed_needs: %i[bridge job] }
 
             entry :stage, ::Gitlab::Ci::Config::Entry::Stage,
               description: 'Pipeline stage this job will be executed into.',
@@ -114,6 +107,10 @@ module EE
             def overwrite_entry(deps, key, current_entry)
               deps.default[key] unless current_entry.specified?
             end
+
+            def bridge_needs
+              needs_value[:bridge] if needs_value
+            end
           end
         end
       end

ee/lib/ee/gitlab/ci/config/entry/need.rb

@@ -25,10 +25,6 @@ module EE
                 validates :pipeline, type: String, presence: true
               end
 
-              def self.matching?(config)
-                config.is_a?(Hash)
-              end
-
               def type
                 :bridge
               end

ee/lib/ee/gitlab/ci/config/entry/needs.rb

-# frozen_string_literal: true
-
-module EE
-  module Gitlab
-    module Ci
-      module Config
-        module Entry
-          module Needs
-            extend ActiveSupport::Concern
-
-            class_methods do
-              extend ::Gitlab::Utils::Override
-
-              override :all_types
-              def all_types
-                super + [Entry::Need::Bridge]
-              end
-            end
-          end
-        end
-      end
-    end
-  end
-end

ee/spec/lib/ee/gitlab/ci/config/entry/bridge_spec.rb

@@ -160,7 +160,19 @@ describe EE::Gitlab::Ci::Config::Entry::Bridge do
       end
     end
 
-    context 'when bridge has bridge and pipeline needs' do
+    context 'when bridge has only job needs' do
+      let(:config) do
+        {
+          needs: ['some_job']
+        }
+      end
+
+      describe '#valid?' do
+        it { is_expected.not_to be_valid }
+      end
+    end
+
+    context 'when bridge has bridge and job needs' do
       let(:config) do
         {
           trigger: 'other-project',
@@ -187,7 +199,7 @@ describe EE::Gitlab::Ci::Config::Entry::Bridge do
 
       describe '#errors' do
         it 'returns an error about too many bridge needs' do
-          expect(subject.errors).to contain_exactly('bridge needs can only have one bridge type needs')
+          expect(subject.errors).to contain_exactly('bridge config should contain exactly one bridge need')
         end
       end
     end

ee/spec/lib/ee/gitlab/ci/config/entry/needs_spec.rb

@@ -5,6 +5,10 @@ require 'spec_helper'
 describe ::Gitlab::Ci::Config::Entry::Needs do
   subject(:needs) { described_class.new(config) }
 
+  before do
+    needs.metadata[:allowed_needs] = %i[job bridge]
+  end
+
   describe 'validations' do
     before do
       needs.compose!
@@ -27,8 +31,8 @@ describe ::Gitlab::Ci::Config::Entry::Needs do
 
       describe '#errors' do
         it 'returns error about incorrect type' do
-          expect(needs.errors)
-            .to contain_exactly('needs need has an unsupported type')
+          expect(needs.errors).to contain_exactly(
+            'need has an unsupported type')
         end
       end
     end
@@ -50,9 +54,16 @@ describe ::Gitlab::Ci::Config::Entry::Needs do
         needs.compose!
       end
 
+      it 'is valid' do
+        expect(needs).to be_valid
+      end
+
       describe '#value' do
         it 'returns key value' do
-          expect(needs.value).to eq(pipeline: [{ name: 'first_job_name' }], bridge: [{ pipeline: 'some/project' }])
+          expect(needs.value).to eq(
+            job: [{ name: 'first_job_name' }],
+            bridge: [{ pipeline: 'some/project' }]
+          )
         end
       end
 

ee/spec/lib/gitlab/ci/config/entry/job_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Ci::Config::Entry::Job do
+  let(:entry) { described_class.new(config, name: :rspec) }
+
+  describe 'validations' do
+    before do
+      entry.compose!
+    end
+
+    context 'when entry value is not correct' do
+      context 'when has needs' do
+        context 'when needs is bridge type' do
+          let(:config) do
+            {
+              script: 'echo',
+              stage: 'test',
+              needs: { pipeline: 'some/project' }
+            }
+          end
+
+          it 'returns error about invalid needs type' do
+            expect(entry).not_to be_valid
+            expect(entry.errors).to contain_exactly('needs config uses bridge type(s)')
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/ci/config/entry/job.rb

@@ -57,14 +57,6 @@ module Gitlab
             validates :start_in, duration: { limit: '1 day' }, if: :delayed?
             validates :start_in, absence: true, if: -> { has_rules? || !delayed? }
 
-            validate do
-              next unless needs.present?
-
-              if [needs].flatten.any? { |need| !Entry::Need::Pipeline.matching?(need) }
-                errors.add(:needs, 'can only have pipeline type needs')
-              end
-            end
-
             validate do
               next unless dependencies.present?
               next unless needs.present?
@@ -122,7 +114,8 @@ module Gitlab
             inherit: false
 
           entry :needs, Entry::Needs,
-            description: 'Needs configuration for this job.'
+            description: 'Needs configuration for this job.',
+            metadata: { allowed_needs: %i[job] }
 
           entry :variables, Entry::Variables,
             description: 'Environment variables available for this job.',

lib/gitlab/ci/config/entry/need.rb

@@ -5,9 +5,9 @@ module Gitlab
     class Config
       module Entry
         class Need < ::Gitlab::Config::Entry::Simplifiable
-          strategy :Pipeline, if: -> (config) { config.is_a?(String) }
+          strategy :Job, if: -> (config) { config.is_a?(String) }
 
-          class Pipeline < ::Gitlab::Config::Entry::Node
+          class Job < ::Gitlab::Config::Entry::Node
             include ::Gitlab::Config::Entry::Validatable
 
             validations do
@@ -15,12 +15,8 @@ module Gitlab
               validates :config, type: String
             end
 
-            def self.matching?(config)
-              config.is_a?(String)
-            end
-
             def type
-              :pipeline
+              :job
             end
 
             def value
@@ -29,6 +25,15 @@ module Gitlab
           end
 
           class UnknownStrategy < ::Gitlab::Config::Entry::Node
+            def type
+            end
+
+            def value
+            end
+
+            def errors
+              ["#{location} has an unsupported type"]
+            end
           end
         end
       end

lib/gitlab/ci/config/entry/needs.rb

@@ -15,33 +15,18 @@ module Gitlab
 
             validate do
               unless config.is_a?(Hash) || config.is_a?(Array)
-                errors.add(:config, 'can only be a hash or an array')
+                errors.add(:config, 'can only be a Hash or an Array')
               end
             end
 
-            validate do
-              [config].flatten.each do |need|
-                if Needs.find_type(need).nil?
-                  errors.add(:need, 'has an unsupported type')
-                end
+            validate on: :composed do
+              extra_keys = value.keys - opt(:allowed_needs)
+              if extra_keys.any?
+                errors.add(:config, "uses #{extra_keys.join(', ')} type(s)")
               end
             end
           end
 
-          TYPES = [Entry::Need::Pipeline].freeze
-
-          private_constant :TYPES
-
-          def self.all_types
-            TYPES
-          end
-
-          def self.find_type(config)
-            self.all_types.find do |type|
-              type.matching?(config)
-            end
-          end
-
           def compose!(deps = nil)
             super(deps) do
               [@config].flatten.each_with_index do |need, index|
@@ -58,7 +43,8 @@ module Gitlab
           end
 
           def value
-            @entries.values.group_by(&:type).transform_values do |values|
+            values = @entries.values.select(&:type)
+            values.group_by(&:type).transform_values do |values|
               values.map(&:value)
             end
           end
@@ -67,5 +53,3 @@ module Gitlab
     end
   end
 end
-
-::Gitlab::Ci::Config::Entry::Needs.prepend_if_ee('::EE::Gitlab::Ci::Config::Entry::Needs')

lib/gitlab/ci/yaml_processor.rb

@@ -40,7 +40,7 @@ module Gitlab
           environment: job[:environment_name],
           coverage_regex: job[:coverage],
           yaml_variables: yaml_variables(name),
-          needs_attributes: job.dig(:needs, :pipeline),
+          needs_attributes: job.dig(:needs, :job),
           interruptible: job[:interruptible],
           rules: job[:rules],
           options: {
@@ -159,11 +159,11 @@ module Gitlab
       end
 
       def validate_job_needs!(name, job)
-        return unless job.dig(:needs, :pipeline)
+        return unless job.dig(:needs, :job)
 
         stage_index = @stages.index(job[:stage])
 
-        job.dig(:needs, :pipeline).each do |need|
+        job.dig(:needs, :job).each do |need|
           need_job_name = need[:name]
 
           raise ValidationError, "#{name} job: undefined need: #{need_job_name}" unless @jobs[need_job_name.to_sym]

lib/gitlab/config/entry/configurable.rb

@@ -29,22 +29,24 @@ module Gitlab
         def compose!(deps = nil)
           return unless valid?
 
-          self.class.nodes.each do |key, factory|
-            # If we override the config type validation
-            # we can end with different config types like String
-            next unless config.is_a?(Hash)
+          super do
+            self.class.nodes.each do |key, factory|
+              # If we override the config type validation
+              # we can end with different config types like String
+              next unless config.is_a?(Hash)
 
-            factory
-              .value(config[key])
-              .with(key: key, parent: self)
+              factory
+                .value(config[key])
+                .with(key: key, parent: self)
 
-            entries[key] = factory.create!
-          end
+              entries[key] = factory.create!
+            end
 
-          yield if block_given?
+            yield if block_given?
 
-          entries.each_value do |entry|
-            entry.compose!(deps)
+            entries.each_value do |entry|
+              entry.compose!(deps)
+            end
           end
         end
         # rubocop: enable CodeReuse/ActiveRecord
@@ -67,12 +69,13 @@ module Gitlab
           private
 
           # rubocop: disable CodeReuse/ActiveRecord
-          def entry(key, entry, description: nil, default: nil, inherit: nil, reserved: nil)
+          def entry(key, entry, description: nil, default: nil, inherit: nil, reserved: nil, metadata: {})
             factory = ::Gitlab::Config::Entry::Factory.new(entry)
               .with(description: description)
               .with(default: default)
               .with(inherit: inherit)
               .with(reserved: reserved)
+              .metadata(metadata)
 
             (@nodes ||= {}).merge!(key.to_sym => factory)
           end

lib/gitlab/config/entry/node.rb

@@ -112,6 +112,10 @@ module Gitlab
           @aspects ||= []
         end
 
+        def self.with_aspect(blk)
+          self.aspects.append(blk)
+        end
+
         private
 
         attr_reader :entries

lib/gitlab/config/entry/simplifiable.rb

@@ -8,7 +8,7 @@ module Gitlab
 
         attr_reader :subject
 
-        def initialize(config, **metadata)
+        def initialize(config, **metadata, &blk)
           unless self.class.const_defined?(:UnknownStrategy)
             raise ArgumentError, 'UndefinedStrategy not available!'
           end
@@ -19,9 +19,8 @@ module Gitlab
 
           entry = self.class.entry_class(strategy)
 
-          @subject = entry.new(config, metadata)
+          @subject = entry.new(config, metadata, &blk)
 
-          yield(@subject) if block_given?
           super(@subject)
         end
 

lib/gitlab/config/entry/validatable.rb

@@ -7,14 +7,27 @@ module Gitlab
         extend ActiveSupport::Concern
 
         def self.included(node)
-          node.aspects.append -> do
-            @validator = self.class.validator.new(self)
-            @validator.validate(:new)
+          node.with_aspect -> do
+            validate(:new)
           end
         end
 
+        def validator
+          @validator ||= self.class.validator.new(self)
+        end
+
+        def validate(context = nil)
+          validator.validate(context)
+        end
+
+        def compose!(deps = nil, &blk)
+          super(deps, &blk)
+
+          validate(:composed)
+        end
+
         def errors
-          @validator.messages + descendants.flat_map(&:errors) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+          validator.messages + descendants.flat_map(&:errors)
         end
 
         class_methods do

spec/lib/gitlab/ci/config/entry/job_spec.rb

@@ -413,21 +413,6 @@ describe Gitlab::Ci::Config::Entry::Job do
             expect(entry.errors).to include 'job config missing required keys: stage'
           end
         end
-
-        context 'when needs is bridge type' do
-          let(:config) do
-            {
-              script: 'echo',
-              stage: 'test',
-              needs: { pipeline: 'some/project' }
-            }
-          end
-
-          it 'returns error about invalid needs type' do
-            expect(entry).not_to be_valid
-            expect(entry.errors).to contain_exactly('job needs can only have pipeline type needs')
-          end
-        end
       end
 
       context 'when timeout value is not correct' do

spec/lib/gitlab/ci/config/entry/need_spec.rb

@@ -29,7 +29,7 @@ describe ::Gitlab::Ci::Config::Entry::Need do
     describe '#errors' do
       it 'is returns an error about an empty config' do
         expect(need.errors)
-          .to contain_exactly("pipeline config can't be blank")
+          .to contain_exactly("job config can't be blank")
       end
     end
   end

spec/lib/gitlab/ci/config/entry/needs_spec.rb

@@ -5,6 +5,10 @@ require 'spec_helper'
 describe ::Gitlab::Ci::Config::Entry::Needs do
   subject(:needs) { described_class.new(config) }
 
+  before do
+    needs.metadata[:allowed_needs] = %i[job]
+  end
+
   describe 'validations' do
     before do
       needs.compose!
@@ -42,8 +46,8 @@ describe ::Gitlab::Ci::Config::Entry::Needs do
 
       describe '#errors' do
         it 'returns error about incorrect type' do
-          expect(needs.errors)
-            .to contain_exactly('needs need has an unsupported type')
+          expect(needs.errors).to contain_exactly(
+            'need has an unsupported type')
         end
       end
     end
@@ -59,7 +63,12 @@ describe ::Gitlab::Ci::Config::Entry::Needs do
 
       describe '#value' do
         it 'returns key value' do
-          expect(needs.value).to eq(pipeline: [{ name: 'first_job_name' }, { name: 'second_job_name' }])
+          expect(needs.value).to eq(
+            job: [
+              { name: 'first_job_name' },
+              { name: 'second_job_name' }
+            ]
+          )
         end
       end