Merge pull request #11599 from saraycp/replace_exceptions_by_errors

Replace exceptions by errors

src/api/app/models/token/errors.rb

@@ -16,8 +16,4 @@ class SCMTokenInvalid < APIError
   class WorkflowsYamlNotParsable < APIError
     setup 400
   end
-
-  class InvalidWorkflowStepDefinition < APIError
-    setup 403
-  end
 end

src/api/app/validators/workflow_filters_validator.rb

@@ -1,5 +1,6 @@
 class WorkflowFiltersValidator < ActiveModel::Validator
   def validate(record)
+    @workflow = record
     @workflow_instructions = record.workflow_instructions
 
     valid_filters?
@@ -11,12 +12,15 @@ def valid_filters?
     # Filters aren't mandatory in a workflow
     return unless @workflow_instructions.key?(:filters)
 
-    raise Workflow::Errors::UnsupportedWorkflowFilters, "Unsupported filters: #{unsupported_filters.keys.to_sentence}" if unsupported_filters.present?
+    if unsupported_filters.present?
+      @workflow.errors.add(:base,
+                           "Unsupported filters: #{unsupported_filters.keys.to_sentence}")
+    end
 
     return unless unsupported_filter_types?
 
-    raise Workflow::Errors::UnsupportedWorkflowFilterTypes,
-          "Filters #{unsupported_filter_types.to_sentence} have unsupported keys. #{Workflow::SUPPORTED_FILTER_TYPES.to_sentence} are the only supported keys."
+    @workflow.errors.add(:base,
+                         "Filters #{unsupported_filter_types.to_sentence} have unsupported keys, #{Workflow::SUPPORTED_FILTER_TYPES.to_sentence} are the only supported keys")
   end
 
   def unsupported_filters

src/api/app/validators/workflow_steps_validator.rb

@@ -8,9 +8,8 @@ def validate(record)
   private
 
   def valid_steps?
-    raise Token::Errors::InvalidWorkflowStepDefinition, 'Invalid workflow. Steps are not present.' if no_steps?
-    raise Token::Errors::InvalidWorkflowStepDefinition, "Invalid workflow step definition: #{errors.to_sentence}" if
-      unsupported_steps.present? || invalid_steps.present?
+    @workflow.errors.add(:base, 'Invalid workflow. Steps are not present.') if no_steps?
+    @workflow.errors.add(:base, "Invalid workflow step definition: #{errors.to_sentence}") if unsupported_steps.present? || invalid_steps.present?
   end
 
   def unsupported_steps
@@ -29,15 +28,21 @@ def no_steps?
   end
 
   def errors
-    acc = []
+    error_messages = []
+    step_names = []
     unsupported_steps.each do |step_definition|
       step_definition.each do |step_name, _|
-        acc << "'#{step_name}' is not a supported step"
+        step_names << step_name
       end
     end
+
+    if step_names.size.positive?
+      error_messages << "#{step_names.to_sentence} #{step_names.size > 1 ? 'are not supported steps' : 'is not a supported step'}"
+    end
+
     invalid_steps.each do |step|
-      acc << step.errors.full_messages.to_sentence
+      error_messages << step.errors.full_messages
     end
-    acc
+    error_messages.flatten
   end
 end

src/api/spec/models/token/workflow_spec.rb

@@ -108,23 +108,6 @@
       end
     end
 
-    context 'when the step is not valid' do
-      let(:scm) { 'github' }
-      let(:event) { 'pull_request' }
-      let(:payload) { github_payload }
-      let(:invalid_steps_workflows_yml_file) { File.expand_path(Rails.root.join('spec/support/files/invalid_steps_workflows.yml')) }
-      let(:downloader) { instance_double(Workflows::YAMLDownloader) }
-
-      before do
-        allow(Workflows::YAMLDownloader).to receive(:new).and_return(downloader)
-        allow(downloader).to receive(:call).and_return(invalid_steps_workflows_yml_file)
-      end
-
-      it 'raises an "Invalid workflow step definition" error' do
-        expect { subject }.to raise_error(Token::Errors::InvalidWorkflowStepDefinition)
-      end
-    end
-
     context 'when the workflows.yml do not exist on the reference branch' do
       let(:octokit_client) { instance_double(Octokit::Client) }
       let(:scm) { 'github' }

src/api/spec/models/workflow_spec.rb

@@ -85,6 +85,10 @@
   end
 
   describe '#valid' do
+    before do
+      subject.valid?
+    end
+
     # Steps validations
 
     context 'with a supported step' do
@@ -106,24 +110,28 @@
       it { expect(subject).to be_valid }
     end
 
-    context 'with several unsupported steps' do
+    context 'with unsupported steps' do
       let(:yaml) do
-        { 'steps' => [{ 'unsupported_step' => {} },
+        { 'steps' => [{ 'unsupported_step_1' => {} },
+                      { 'unsupported_step_2' => {} },
                       { 'branch_package' => { source_project: 'project',
                                               source_package: 'package' } }] }
       end
 
-      it { expect { subject.valid? }.to raise_error("Invalid workflow step definition: 'unsupported_step' is not a supported step") }
+      it 'sets validation errors' do
+        expect(subject.errors.full_messages).to match_array(
+          ['Invalid workflow step definition: unsupported_step_1 and unsupported_step_2 are not supported steps']
+        )
+      end
     end
 
     context 'when steps are not provided' do
       let(:yaml) do
         { 'steps' => [{}] }
       end
 
-      it 'raises an exception for non-present steps' do
-        expect { subject.valid? }.to raise_error(Token::Errors::InvalidWorkflowStepDefinition,
-                                                 'Invalid workflow. Steps are not present.')
+      it 'sets validation errors' do
+        expect(subject.errors.full_messages).to match_array(['Invalid workflow. Steps are not present.'])
       end
     end
 
@@ -132,10 +140,11 @@
         { 'steps' => [{ 'branch_package' => {} }] }
       end
 
-      it 'raises an exception for non-present instructions' do
-        expect { subject.valid? }.to raise_error(Token::Errors::InvalidWorkflowStepDefinition,
-                                                 "Invalid workflow step definition: Source project name can't be blank, The 'source_project' key is missing, The 'source_package'
-                                                  key is missing, and Source package name can't be blank".squish)
+      it 'sets validation errors' do
+        expect(subject.errors.full_messages).to match_array(
+          ["Invalid workflow step definition: Source project name can't be blank, The 'source_project' key is missing, The 'source_package' key is missing, \
+and Source package name can't be blank"]
+        )
       end
     end
 
@@ -145,9 +154,24 @@
                                               source_package: nil } }] }
       end
 
-      it 'raises an exception for invalid instructions' do
-        expect { subject.valid? }.to raise_error(Token::Errors::InvalidWorkflowStepDefinition,
-                                                 "Invalid workflow step definition: Source project name can't be blank and Source package name can't be blank")
+      it 'sets validation errors' do
+        expect(subject.errors.full_messages).to match_array(
+          ["Invalid workflow step definition: Source project name can't be blank and Source package name can't be blank"]
+        )
+      end
+    end
+
+    context 'with a combination of unsupported steps and invalid step configuration' do
+      let(:yaml) do
+        { 'steps' => [{ 'unsupported_step_1' => {} },
+                      { 'branch_package' => { source_project: nil } }] }
+      end
+
+      it 'sets validation errors' do
+        expect(subject.errors.full_messages).to match_array(
+          ["Invalid workflow step definition: unsupported_step_1 is not a supported step, Source project name can't be blank, \
+The 'source_package' key is missing, and Source package name can't be blank"]
+        )
       end
     end
 
@@ -165,10 +189,10 @@
           }
         end
 
-        it 'raises a user-friendly error message' do
-          expect do
-            subject.valid?
-          end.to raise_error(Workflow::Errors::UnsupportedWorkflowFilterTypes, "Filters #{filter} have unsupported keys. only and ignore are the only supported keys.")
+        it 'sets validation errors' do
+          expect(subject.errors.full_messages).to match_array(
+            ["Filters #{filter} have unsupported keys, only and ignore are the only supported keys"]
+          )
         end
       end
     end
@@ -185,8 +209,48 @@
         }
       end
 
-      it 'raises a user-friendly error message' do
-        expect { subject.valid? }.to raise_error(Workflow::Errors::UnsupportedWorkflowFilters, 'Unsupported filters: unsupported_1 and unsupported_2')
+      it 'sets validation errors' do
+        expect(subject.errors.full_messages).to match_array(
+          ['Unsupported filters: unsupported_1 and unsupported_2']
+        )
+      end
+    end
+
+    context 'with a combination of unsupported filters and non-valid types' do
+      let(:yaml) do
+        {
+          'filters' => {
+            'unsupported_1' => { 'only' => ['foo'] },
+            'repositories' => { 'onlyyy' => [{ 'non_valid' => ['ppc'] }, 'x86_64'], 'ignore' => ['i586'] }
+          },
+          'steps' => [{ 'branch_package' => { source_project: 'project',
+                                              source_package: 'package' } }]
+        }
+      end
+
+      it 'sets validation errors' do
+        expect(subject.errors.full_messages).to match_array(
+          ['Filters repositories have unsupported keys, only and ignore are the only supported keys', 'Unsupported filters: unsupported_1']
+        )
+      end
+    end
+
+    context 'with a combination of invalid filters and invalid steps' do
+      let(:yaml) do
+        {
+          'filters' => {
+            'unsupported_1' => { 'only' => ['foo'] }
+          },
+          'steps' => [{ 'branch_package' => { source_project: nil,
+                                              source_package: 'package' },
+                        'unsuported_step_1' => { source_project: 'project' } }]
+        }
+      end
+
+      it 'sets validation errors' do
+        expect(subject.errors.full_messages).to match_array(
+          ["Invalid workflow step definition: unsuported_step_1 is not a supported step and Source project name can't be blank", 'Unsupported filters: unsupported_1']
+        )
       end
     end
   end