Move CI external file validation from the initialize method

While we were discussing in another MR, we decided that the validate
method does not belong to the initialize method. So, in this commit
we are extracting it.

lib/gitlab/ci/config/external/file/base.rb

@@ -16,8 +16,6 @@ module Gitlab
               @params = params
               @context = context
               @errors = []
-
-              validate!
             end
 
             def matching?
@@ -48,6 +46,15 @@ module Gitlab
               expanded_content_hash
             end
 
+            def validate!
+              context.logger.instrument(:config_file_validation) do
+                validate_execution_time!
+                validate_location!
+                validate_content! if errors.none?
+                validate_hash! if errors.none?
+              end
+            end
+
             protected
 
             def expanded_content_hash
@@ -66,13 +73,6 @@ module Gitlab
               nil
             end
 
-            def validate!
-              validate_execution_time!
-              validate_location!
-              validate_content! if errors.none?
-              validate_hash! if errors.none?
-            end
-
             def validate_execution_time!
               context.check_execution_time!
             end

lib/gitlab/ci/config/external/mapper.rb

@@ -50,6 +50,7 @@ module Gitlab
               .map(&method(:expand_variables))
               .each(&method(:verify_duplicates!))
               .map(&method(:select_first_matching))
+              .each(&method(:verify!))
           end
 
           def normalize_location(location)
@@ -147,6 +148,10 @@ module Gitlab
             matching.first
           end
 
+          def verify!(location_object)
+            location_object.validate!
+          end
+
           def expand_variables(data)
             logger.instrument(:config_mapper_variables) do
               expand_variables_without_instrumentation(data)

spec/lib/gitlab/ci/config/external/file/artifact_spec.rb

@@ -29,14 +29,15 @@ RSpec.describe Gitlab::Ci::Config::External::File::Artifact do
   end
 
   describe '#valid?' do
-    shared_examples 'is invalid' do
-      it 'is not valid' do
-        expect(external_file).not_to be_valid
-      end
+    subject(:valid?) do
+      external_file.validate!
+      external_file.valid?
+    end
 
+    shared_examples 'is invalid' do
       it 'sets the expected error' do
-        expect(external_file.errors)
-          .to contain_exactly(expected_error)
+        expect(valid?).to be_falsy
+        expect(external_file.errors).to contain_exactly(expected_error)
       end
     end
 
@@ -142,7 +143,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Artifact do
 
                 context 'when file is not empty' do
                   it 'is valid' do
-                    expect(external_file).to be_valid
+                    expect(valid?).to be_truthy
                     expect(external_file.content).to be_present
                   end
 
@@ -154,6 +155,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Artifact do
                       user: anything
                     }
                     expect(context).to receive(:mutate).with(expected_attrs).and_call_original
+                    external_file.validate!
                     external_file.content
                   end
                 end

spec/lib/gitlab/ci/config/external/file/base_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Base do
     end
   end
 
-  subject { test_class.new(location, context) }
+  subject(:file) { test_class.new(location, context) }
 
   before do
     allow_any_instance_of(test_class)
@@ -31,7 +31,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Base do
       let(:location) { 'some-location' }
 
       it 'returns true' do
-        expect(subject).to be_matching
+        expect(file).to be_matching
       end
     end
 
@@ -39,40 +39,45 @@ RSpec.describe Gitlab::Ci::Config::External::File::Base do
       let(:location) { nil }
 
       it 'returns false' do
-        expect(subject).not_to be_matching
+        expect(file).not_to be_matching
       end
     end
   end
 
   describe '#valid?' do
+    subject(:valid?) do
+      file.validate!
+      file.valid?
+    end
+
     context 'when location is not a string' do
       let(:location) { %w(some/file.txt other/file.txt) }
 
-      it { is_expected.not_to be_valid }
+      it { is_expected.to be_falsy }
     end
 
     context 'when location is not a YAML file' do
       let(:location) { 'some/file.txt' }
 
-      it { is_expected.not_to be_valid }
+      it { is_expected.to be_falsy }
     end
 
     context 'when location has not a valid naming scheme' do
       let(:location) { 'some/file/.yml' }
 
-      it { is_expected.not_to be_valid }
+      it { is_expected.to be_falsy }
     end
 
     context 'when location is a valid .yml extension' do
       let(:location) { 'some/file/config.yml' }
 
-      it { is_expected.to be_valid }
+      it { is_expected.to be_truthy }
     end
 
     context 'when location is a valid .yaml extension' do
       let(:location) { 'some/file/config.yaml' }
 
-      it { is_expected.to be_valid }
+      it { is_expected.to be_truthy }
     end
 
     context 'when there are YAML syntax errors' do
@@ -84,8 +89,8 @@ RSpec.describe Gitlab::Ci::Config::External::File::Base do
       end
 
       it 'is not a valid file' do
-        expect(subject).not_to be_valid
-        expect(subject.error_message).to match /does not have valid YAML syntax/
+        expect(valid?).to be_falsy
+        expect(file.error_message).to match /does not have valid YAML syntax/
       end
     end
   end
@@ -101,7 +106,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Base do
       end
 
       it 'does expand hash to include the template' do
-        expect(subject.to_hash).to include(:before_script)
+        expect(file.to_hash).to include(:before_script)
       end
     end
   end

spec/lib/gitlab/ci/config/external/file/local_spec.rb

@@ -54,6 +54,11 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
   end
 
   describe '#valid?' do
+    subject(:valid?) do
+      local_file.validate!
+      local_file.valid?
+    end
+
     context 'when is a valid local path' do
       let(:location) { '/lib/gitlab/ci/templates/existent-file.yml' }
 
@@ -61,25 +66,19 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
         allow_any_instance_of(described_class).to receive(:fetch_local_content).and_return("image: 'ruby2:2'")
       end
 
-      it 'returns true' do
-        expect(local_file.valid?).to be_truthy
-      end
+      it { is_expected.to be_truthy }
     end
 
     context 'when is not a valid local path' do
       let(:location) { '/lib/gitlab/ci/templates/non-existent-file.yml' }
 
-      it 'returns false' do
-        expect(local_file.valid?).to be_falsy
-      end
+      it { is_expected.to be_falsy }
     end
 
     context 'when is not a yaml file' do
       let(:location) { '/config/application.rb' }
 
-      it 'returns false' do
-        expect(local_file.valid?).to be_falsy
-      end
+      it { is_expected.to be_falsy }
     end
 
     context 'when the given sha is not valid' do
@@ -87,7 +86,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
       let(:sha) { ':' }
 
       it 'returns false and adds an error message stating that included file does not exist' do
-        expect(local_file).not_to be_valid
+        expect(valid?).to be_falsy
         expect(local_file.errors).to include("Sha #{sha} is not valid!")
       end
     end
@@ -128,6 +127,10 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
   describe '#error_message' do
     let(:location) { '/lib/gitlab/ci/templates/non-existent-file.yml' }
 
+    before do
+      local_file.validate!
+    end
+
     it 'returns an error message' do
       expect(local_file.error_message).to eq("Local file `#{location}` does not exist!")
     end
@@ -162,6 +165,8 @@ RSpec.describe Gitlab::Ci::Config::External::File::Local do
 
         allow(project.repository).to receive(:blob_data_at).with(sha, another_location)
           .and_return(another_content)
+
+        local_file.validate!
       end
 
       it 'does expand hash to include the template' do

spec/lib/gitlab/ci/config/external/file/project_spec.rb

@@ -65,6 +65,11 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
   end
 
   describe '#valid?' do
+    subject(:valid?) do
+      project_file.validate!
+      project_file.valid?
+    end
+
     context 'when a valid path is used' do
       let(:params) do
         { project: project.full_path, file: '/file.yml' }
@@ -76,15 +81,13 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
         stub_project_blob(root_ref_sha, '/file.yml') { 'image: ruby:2.7' }
       end
 
-      it 'returns true' do
-        expect(project_file).to be_valid
-      end
+      it { is_expected.to be_truthy }
 
       context 'when user does not have permission to access file' do
         let(:context_user) { create(:user) }
 
         it 'returns false' do
-          expect(project_file).not_to be_valid
+          expect(valid?).to be_falsy
           expect(project_file.error_message).to include("Project `#{project.full_path}` not found or access denied!")
         end
       end
@@ -101,9 +104,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
         stub_project_blob(ref_sha, '/file.yml') { 'image: ruby:2.7' }
       end
 
-      it 'returns true' do
-        expect(project_file).to be_valid
-      end
+      it { is_expected.to be_truthy }
     end
 
     context 'when an empty file is used' do
@@ -118,7 +119,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
       end
 
       it 'returns false' do
-        expect(project_file).not_to be_valid
+        expect(valid?).to be_falsy
         expect(project_file.error_message).to include("Project `#{project.full_path}` file `/file.yml` is empty!")
       end
     end
@@ -129,7 +130,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
       end
 
       it 'returns false' do
-        expect(project_file).not_to be_valid
+        expect(valid?).to be_falsy
         expect(project_file.error_message).to include("Project `#{project.full_path}` reference `I-Do-Not-Exist` does not exist!")
       end
     end
@@ -140,7 +141,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
       end
 
       it 'returns false' do
-        expect(project_file).not_to be_valid
+        expect(valid?).to be_falsy
         expect(project_file.error_message).to include("Project `#{project.full_path}` file `/invalid-file.yml` does not exist!")
       end
     end
@@ -151,7 +152,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Project do
       end
 
       it 'returns false' do
-        expect(project_file).not_to be_valid
+        expect(valid?).to be_falsy
         expect(project_file.error_message).to include('Included file `/invalid-file` does not have YAML extension!')
       end
     end

spec/lib/gitlab/ci/config/external/file/remote_spec.rb

@@ -53,22 +53,23 @@ RSpec.describe Gitlab::Ci::Config::External::File::Remote do
   end
 
   describe "#valid?" do
+    subject(:valid?) do
+      remote_file.validate!
+      remote_file.valid?
+    end
+
     context 'when is a valid remote url' do
       before do
         stub_full_request(location).to_return(body: remote_file_content)
       end
 
-      it 'returns true' do
-        expect(remote_file.valid?).to be_truthy
-      end
+      it { is_expected.to be_truthy }
     end
 
     context 'with an irregular url' do
       let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.gitlab-ci-1.yml' }
 
-      it 'returns false' do
-        expect(remote_file.valid?).to be_falsy
-      end
+      it { is_expected.to be_falsy }
     end
 
     context 'with a timeout' do
@@ -76,25 +77,19 @@ RSpec.describe Gitlab::Ci::Config::External::File::Remote do
         allow(Gitlab::HTTP).to receive(:get).and_raise(Timeout::Error)
       end
 
-      it 'is falsy' do
-        expect(remote_file.valid?).to be_falsy
-      end
+      it { is_expected.to be_falsy }
     end
 
     context 'when is not a yaml file' do
       let(:location) { 'https://asdasdasdaj48ggerexample.com' }
 
-      it 'is falsy' do
-        expect(remote_file.valid?).to be_falsy
-      end
+      it { is_expected.to be_falsy }
     end
 
     context 'with an internal url' do
       let(:location) { 'http://localhost:8080' }
 
-      it 'is falsy' do
-        expect(remote_file.valid?).to be_falsy
-      end
+      it { is_expected.to be_falsy }
     end
   end
 
@@ -141,7 +136,10 @@ RSpec.describe Gitlab::Ci::Config::External::File::Remote do
   end
 
   describe "#error_message" do
-    subject { remote_file.error_message }
+    subject(:error_message) do
+      remote_file.validate!
+      remote_file.error_message
+    end
 
     context 'when remote file location is not valid' do
       let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.gitlab-ci-1.yml' }

spec/lib/gitlab/ci/config/external/file/template_spec.rb

@@ -45,19 +45,22 @@ RSpec.describe Gitlab::Ci::Config::External::File::Template do
   end
 
   describe "#valid?" do
+    subject(:valid?) do
+      template_file.validate!
+      template_file.valid?
+    end
+
     context 'when is a valid template name' do
       let(:template) { 'Auto-DevOps.gitlab-ci.yml' }
 
-      it 'returns true' do
-        expect(template_file).to be_valid
-      end
+      it { is_expected.to be_truthy }
     end
 
     context 'with invalid template name' do
       let(:template) { 'Template.yml' }
 
       it 'returns false' do
-        expect(template_file).not_to be_valid
+        expect(valid?).to be_falsy
         expect(template_file.error_message).to include('Template file `Template.yml` is not a valid location!')
       end
     end
@@ -66,7 +69,7 @@ RSpec.describe Gitlab::Ci::Config::External::File::Template do
       let(:template) { 'I-Do-Not-Have-This-Template.gitlab-ci.yml' }
 
       it 'returns false' do
-        expect(template_file).not_to be_valid
+        expect(valid?).to be_falsy
         expect(template_file.error_message).to include('Included file `I-Do-Not-Have-This-Template.gitlab-ci.yml` is empty or does not exist!')
       end
     end