Merge branch 'pipeline-api-return-code' into 'master'

Report pipeline creation success only when warranted

See merge request gitlab-org/gitlab!60746

app/services/ci/pipeline_trigger_service.rb

@@ -3,6 +3,7 @@
 module Ci
   class PipelineTriggerService < BaseService
     include Gitlab::Utils::StrongMemoize
+    include Services::ReturnServiceResponses
 
     def execute
       if trigger_from_token
@@ -20,7 +21,7 @@ module Ci
     private
 
     PAYLOAD_VARIABLE_KEY = 'TRIGGER_PAYLOAD'
-    PAYLOAD_VARIABLE_HIDDEN_PARAMS = %i(token).freeze
+    PAYLOAD_VARIABLE_HIDDEN_PARAMS = %i[token].freeze
 
     def create_pipeline_from_trigger(trigger)
       # this check is to not leak the presence of the project if user cannot read it
@@ -32,10 +33,17 @@ module Ci
           pipeline.trigger_requests.build(trigger: trigger)
         end
 
-      if pipeline.persisted?
+      pipeline_service_response(pipeline)
+    end
+
+    def pipeline_service_response(pipeline)
+      if pipeline.created_successfully?
         success(pipeline: pipeline)
+      elsif pipeline.persisted?
+        err = pipeline.errors.messages.presence || pipeline.failure_reason.presence || 'Could not create pipeline'
+        error(err, :unprocessable_entity)
       else
-        error(pipeline.errors.messages, 400)
+        error(pipeline.errors.messages, :bad_request)
       end
     end
 
@@ -61,11 +69,7 @@ module Ci
           pipeline.source_pipeline = source
         end
 
-      if pipeline.persisted?
-        success(pipeline: pipeline)
-      else
-        error(pipeline.errors.messages, 400)
-      end
+      pipeline_service_response(pipeline)
     end
 
     def job_from_token

app/services/concerns/services/return_service_responses.rb

+# frozen_string_literal: true
+
+module Services
+  # adapter for existing services over BaseServiceUtility - add error and
+  # success methods returning ServiceResponse objects
+  module ReturnServiceResponses
+    def error(message, http_status, pass_back: {})
+      ServiceResponse.error(message: message, http_status: http_status, payload: pass_back)
+    end
+
+    def success(payload)
+      ServiceResponse.success(payload: payload)
+    end
+  end
+end

app/services/service_response.rb

@@ -18,6 +18,14 @@ class ServiceResponse
     self.http_status = http_status
   end
 
+  def [](key)
+    to_h[key]
+  end
+
+  def to_h
+    (payload || {}).merge(status: status, message: message, http_status: http_status)
+  end
+
   def success?
     status == :success
   end

changelogs/unreleased/pipeline-api-return-code.yml

+---
+title: Report pipeline creation success only when warranted
+merge_request: 60746
+author:
+type: security

lib/api/triggers.rb

@@ -37,7 +37,7 @@ module API
         result = ::Ci::PipelineTriggerService.new(project, nil, params).execute
         not_found! unless result
 
-        if result[:http_status]
+        if result.error?
           render_api_error!(result[:message], result[:http_status])
         else
           present result[:pipeline], with: Entities::Ci::Pipeline

spec/services/ci/pipeline_trigger_service_spec.rb

@@ -20,6 +20,28 @@ RSpec.describe Ci::PipelineTriggerService do
       project.add_developer(user)
     end
 
+    shared_examples 'detecting an unprocessable pipeline trigger' do
+      context 'when the pipeline was not created successfully' do
+        let(:fail_pipeline) do
+          receive(:execute).and_wrap_original do |original, *args|
+            pipeline = original.call(*args)
+            pipeline.update!(failure_reason: 'unknown_failure')
+            pipeline
+          end
+        end
+
+        before do
+          allow_next(Ci::CreatePipelineService).to fail_pipeline
+        end
+
+        it 'has the correct status code' do
+          expect { result }.to change { Ci::Pipeline.count }
+          expect(result).to be_error
+          expect(result.http_status).to eq(:unprocessable_entity)
+        end
+      end
+    end
+
     context 'with a trigger token' do
       let(:trigger) { create(:ci_trigger, project: project, owner: user) }
 
@@ -63,7 +85,7 @@ RSpec.describe Ci::PipelineTriggerService do
 
             it 'ignores [ci skip] and create as general' do
               expect { result }.to change { Ci::Pipeline.count }.by(1)
-              expect(result[:status]).to eq(:success)
+              expect(result).to be_success
             end
           end
 
@@ -78,19 +100,22 @@ RSpec.describe Ci::PipelineTriggerService do
               expect(result[:pipeline].trigger_requests.last.variables).to be_nil
             end
           end
+
+          it_behaves_like 'detecting an unprocessable pipeline trigger'
         end
 
-        context 'when params have a non-existsed ref' do
+        context 'when params have a non-existant ref' do
           let(:params) { { token: trigger.token, ref: 'invalid-ref', variables: nil } }
 
           it 'does not trigger a pipeline' do
             expect { result }.not_to change { Ci::Pipeline.count }
-            expect(result[:http_status]).to eq(400)
+            expect(result).to be_error
+            expect(result.http_status).to eq(:bad_request)
           end
         end
       end
 
-      context 'when params have a non-existsed trigger token' do
+      context 'when params have a non-existant trigger token' do
         let(:params) { { token: 'invalid-token', ref: nil, variables: nil } }
 
         it 'does not trigger a pipeline' do
@@ -173,14 +198,17 @@ RSpec.describe Ci::PipelineTriggerService do
               expect(job.sourced_pipelines.last.pipeline_id).to eq(result[:pipeline].id)
             end
           end
+
+          it_behaves_like 'detecting an unprocessable pipeline trigger'
         end
 
-        context 'when params have a non-existsed ref' do
+        context 'when params have a non-existant ref' do
           let(:params) { { token: job.token, ref: 'invalid-ref', variables: nil } }
 
-          it 'does not job a pipeline' do
+          it 'does not trigger a job in the pipeline' do
             expect { result }.not_to change { Ci::Pipeline.count }
-            expect(result[:http_status]).to eq(400)
+            expect(result).to be_error
+            expect(result.http_status).to eq(:bad_request)
           end
         end
       end