Merge branch 'make-graphql-errors-more-meaningful-218685' into 'master'

Include validation errors when DAST service fails

See merge request gitlab-org/gitlab!34484

ee/app/graphql/mutations/pipelines/run_dast_scan.rb

@@ -36,7 +36,7 @@ module Mutations
         service = Ci::RunDastScanService.new(project: project, user: current_user)
         pipeline = service.execute(branch: branch, target_url: target_url)
         success_response(project: project, pipeline: pipeline)
-      rescue *Ci::RunDastScanService::EXCEPTIONS => err
+      rescue Ci::RunDastScanService::RunError => err
         error_response(err)
       end
 
@@ -58,7 +58,7 @@ module Mutations
       end
 
       def error_response(err)
-        { errors: [err.message] }
+        { errors: err.full_messages }
       end
     end
   end

ee/app/services/ci/run_dast_scan_service.rb

@@ -4,13 +4,14 @@ module Ci
   class RunDastScanService
     DEFAULT_SHA_FOR_PROJECTS_WITHOUT_COMMITS = :placeholder
 
-    EXCEPTIONS = [
-      NotAllowed = Class.new(StandardError),
-      CreatePipelineError = Class.new(StandardError),
-      CreateStageError = Class.new(StandardError),
-      CreateBuildError = Class.new(StandardError),
-      EnqueueError = Class.new(StandardError)
-    ].freeze
+    class RunError < StandardError
+      attr_reader :full_messages
+
+      def initialize(msg, full_messages = [])
+        @full_messages = full_messages.unshift(msg)
+        super(msg)
+      end
+    end
 
     def initialize(project:, user:)
       @project = project
@@ -18,7 +19,10 @@ module Ci
     end
 
     def execute(branch:, target_url:)
-      raise NotAllowed unless allowed?
+      unless allowed?
+        msg = Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR
+        raise RunError.new(msg)
+      end
 
       ActiveRecord::Base.transaction do
         pipeline = create_pipeline!(branch)
@@ -38,7 +42,7 @@ module Ci
     end
 
     def create_pipeline!(branch)
-      reraise!(with: CreatePipelineError.new('Could not create pipeline')) do
+      reraise!(msg: 'Pipeline could not be created') do
         Ci::Pipeline.create!(
           project: project,
           ref: branch,
@@ -50,7 +54,7 @@ module Ci
     end
 
     def create_stage!(pipeline)
-      reraise!(with: CreateStageError.new('Could not create stage')) do
+      reraise!(msg: 'Stage could not be created') do
         Ci::Stage.create!(
           name: 'dast',
           pipeline: pipeline,
@@ -60,7 +64,7 @@ module Ci
     end
 
     def create_build!(pipeline, stage, branch, target_url)
-      reraise!(with: CreateBuildError.new('Could not create build')) do
+      reraise!(msg: 'Build could not be created') do
         Ci::Build.create!(
           name: 'DAST Scan',
           pipeline: pipeline,
@@ -75,16 +79,19 @@ module Ci
     end
 
     def enqueue!(build)
-      reraise!(with: EnqueueError.new('Could not enqueue build')) do
+      reraise!(msg: 'Build could not be enqueued') do
         build.enqueue!
       end
     end
 
-    def reraise!(with:)
+    def reraise!(msg:)
       yield
+    rescue ActiveRecord::RecordInvalid => err
+      Gitlab::ErrorTracking.track_exception(err)
+      raise RunError.new(msg, err.record.errors.full_messages)
     rescue => err
       Gitlab::ErrorTracking.track_exception(err)
-      raise with
+      raise RunError.new(msg)
     end
 
     def options

ee/spec/requests/api/graphql/mutations/pipelines/run_dast_scan_spec.rb

@@ -63,7 +63,7 @@ describe 'Running a DAST Scan' do
           allow(Ci::Pipeline).to receive(:create!).and_raise(StandardError)
         end
 
-        it_behaves_like 'a mutation that returns errors in the response', errors: ['Could not create pipeline']
+        it_behaves_like 'a mutation that returns errors in the response', errors: ['Pipeline could not be created']
       end
 
       context 'when the stage could not be created' do
@@ -71,7 +71,7 @@ describe 'Running a DAST Scan' do
           allow(Ci::Stage).to receive(:create!).and_raise(StandardError)
         end
 
-        it_behaves_like 'a mutation that returns errors in the response', errors: ['Could not create stage']
+        it_behaves_like 'a mutation that returns errors in the response', errors: ['Stage could not be created']
       end
 
       context 'when the build could not be created' do
@@ -79,7 +79,7 @@ describe 'Running a DAST Scan' do
           allow(Ci::Build).to receive(:create!).and_raise(StandardError)
         end
 
-        it_behaves_like 'a mutation that returns errors in the response', errors: ['Could not create build']
+        it_behaves_like 'a mutation that returns errors in the response', errors: ['Build could not be created']
       end
 
       context 'when the build could not be enqueued' do
@@ -87,7 +87,7 @@ describe 'Running a DAST Scan' do
           allow_any_instance_of(Ci::Build).to receive(:enqueue!).and_raise(StandardError)
         end
 
-        it_behaves_like 'a mutation that returns errors in the response', errors: ['Could not enqueue build']
+        it_behaves_like 'a mutation that returns errors in the response', errors: ['Build could not be enqueued']
       end
     end
   end

ee/spec/services/ci/run_dast_scan_service_spec.rb

@@ -12,8 +12,10 @@ describe Ci::RunDastScanService do
     subject { described_class.new(project: project, user: user).execute(branch: branch, target_url: target_url) }
 
     context 'when the user does not have permission to run a dast scan' do
-      it 'raises an exception'  do
-        expect { subject }.to raise_error(described_class::NotAllowed)
+      it 'raises an exeception with #full_messages populated' do
+        expect { subject }.to raise_error(Ci::RunDastScanService::RunError) do |error|
+          expect(error.full_messages[0]).to include('you don\'t have permission to perform this action')
+        end
       end
     end
 
@@ -110,8 +112,10 @@ describe Ci::RunDastScanService do
           allow(Ci::Pipeline).to receive(:create!).and_raise(StandardError)
         end
 
-        it 'raises an exception' do
-          expect { subject }.to raise_error(Ci::RunDastScanService::CreatePipelineError)
+        it 'raises an exeception with #full_messages populated' do
+          expect { subject }.to raise_error(Ci::RunDastScanService::RunError) do |error|
+            expect(error.full_messages).to include('Pipeline could not be created')
+          end
         end
       end
 
@@ -120,8 +124,10 @@ describe Ci::RunDastScanService do
           allow(Ci::Stage).to receive(:create!).and_raise(StandardError)
         end
 
-        it 'raises an exception' do
-          expect { subject }.to raise_error(Ci::RunDastScanService::CreateStageError)
+        it 'raises an exeception with #full_messages populated' do
+          expect { subject }.to raise_error(Ci::RunDastScanService::RunError) do |error|
+            expect(error.full_messages).to include('Stage could not be created')
+          end
         end
 
         it 'does not create a pipeline' do
@@ -134,8 +140,10 @@ describe Ci::RunDastScanService do
           allow(Ci::Build).to receive(:create!).and_raise(StandardError)
         end
 
-        it 'raises an exception' do
-          expect { subject }.to raise_error(Ci::RunDastScanService::CreateBuildError)
+        it 'raises an exeception with #full_messages populated' do
+          expect { subject }.to raise_error(Ci::RunDastScanService::RunError) do |error|
+            expect(error.full_messages).to include('Build could not be created')
+          end
         end
 
         it 'does not create a stage' do
@@ -148,14 +156,33 @@ describe Ci::RunDastScanService do
           allow_any_instance_of(Ci::Build).to receive(:enqueue!).and_raise(StandardError)
         end
 
-        it 'raises an exception' do
-          expect { subject }.to raise_error(Ci::RunDastScanService::EnqueueError)
+        it 'raises an exeception with #full_messages populated' do
+          expect { subject }.to raise_error(Ci::RunDastScanService::RunError) do |error|
+            expect(error.full_messages).to include('Build could not be enqueued')
+          end
         end
 
         it 'does not create a build' do
           expect { subject rescue nil }.not_to change(Ci::Pipeline, :count)
         end
       end
+
+      context 'when a validation error is raised' do
+        before do
+          klass = Ci::Pipeline
+          allow(klass).to receive(:create!).and_raise(
+            ActiveRecord::RecordInvalid, klass.new.tap do |pl|
+              pl.errors.add(:sha, 'can\'t be blank')
+            end
+          )
+        end
+
+        it 'raises an exeception with #full_messages populated' do
+          expect { subject }.to raise_error(Ci::RunDastScanService::RunError) do |error|
+            expect(error.full_messages).to include('Sha can\'t be blank')
+          end
+        end
+      end
     end
   end
 end