Merge branch 'mergeability_check' into 'master'

Introduce new mergeability approach

See merge request gitlab-org/gitlab!68312

app/models/merge_request.rb

@@ -1111,15 +1111,23 @@ class MergeRequest < ApplicationRecord
     can_be_merged? && !should_be_rebased?
   end
 
+  # rubocop: disable CodeReuse/ServiceClass
   def mergeable_state?(skip_ci_check: false, skip_discussions_check: false)
     return false unless open?
     return false if work_in_progress?
     return false if broken?
-    return false unless skip_ci_check || mergeable_ci_state?
     return false unless skip_discussions_check || mergeable_discussions_state?
 
-    true
+    if Feature.enabled?(:improved_mergeability_checks, self.project, default_enabled: :yaml)
+      additional_checks = MergeRequests::Mergeability::RunChecksService.new(merge_request: self, params: { skip_ci_check: skip_ci_check })
+      additional_checks.execute.all?(&:success?)
+    else
+      return false unless skip_ci_check || mergeable_ci_state?
+
+      true
+    end
   end
+  # rubocop: enable CodeReuse/ServiceClass
 
   def ff_merge_possible?
     project.repository.ancestor?(target_branch_sha, diff_head_sha)

app/services/merge_requests/mergeability/check_base_service.rb

+# frozen_string_literal: true
+module MergeRequests
+  module Mergeability
+    class CheckBaseService
+      attr_reader :merge_request, :params
+
+      def initialize(merge_request:, params:)
+        @merge_request = merge_request
+        @params = params
+      end
+
+      def skip?
+        raise NotImplementedError
+      end
+
+      # When this method is true, we need to implement a cache_key
+      def cacheable?
+        raise NotImplementedError
+      end
+
+      def cache_key
+        raise NotImplementedError
+      end
+
+      private
+
+      def success(*args)
+        Gitlab::MergeRequests::Mergeability::CheckResult.success(*args)
+      end
+
+      def failure(*args)
+        Gitlab::MergeRequests::Mergeability::CheckResult.failed(*args)
+      end
+    end
+  end
+end

app/services/merge_requests/mergeability/check_ci_status_service.rb

+# frozen_string_literal: true
+module MergeRequests
+  module Mergeability
+    class CheckCiStatusService < CheckBaseService
+      def execute
+        if merge_request.mergeable_ci_state?
+          success
+        else
+          failure
+        end
+      end
+
+      def skip?
+        params[:skip_ci_check].present?
+      end
+
+      def cacheable?
+        false
+      end
+    end
+  end
+end

app/services/merge_requests/mergeability/run_checks_service.rb

+# frozen_string_literal: true
+module MergeRequests
+  module Mergeability
+    class RunChecksService
+      include Gitlab::Utils::StrongMemoize
+
+      # We want to have the cheapest checks first in the list,
+      # that way we can fail fast before running the more expensive ones
+      CHECKS = [
+        CheckCiStatusService
+      ].freeze
+
+      def initialize(merge_request:, params:)
+        @merge_request = merge_request
+        @params = params
+      end
+
+      def execute
+        CHECKS.each_with_object([]) do |check_class, results|
+          check = check_class.new(merge_request: merge_request, params: params)
+
+          next if check.skip?
+
+          check_result = run_check(check)
+          results << check_result
+
+          break results if check_result.failed?
+        end
+      end
+
+      private
+
+      attr_reader :merge_request, :params
+
+      def run_check(check)
+        return check.execute unless Feature.enabled?(:mergeability_caching, merge_request.project, default_enabled: :yaml)
+        return check.execute unless check.cacheable?
+
+        cached_result = results.read(merge_check: check)
+        return cached_result if cached_result.respond_to?(:status)
+
+        check.execute.tap do |result|
+          results.write(merge_check: check, result_hash: result.to_hash)
+        end
+      end
+
+      def results
+        strong_memoize(:results) do
+          Gitlab::MergeRequests::Mergeability::ResultsStore.new(merge_request: merge_request)
+        end
+      end
+    end
+  end
+end

app/services/merge_requests/update_service.rb

@@ -248,7 +248,7 @@ module MergeRequests
     def merge_from_quick_action(merge_request)
       last_diff_sha = params.delete(:merge)
 
-      MergeRequests::MergeOrchestrationService
+      ::MergeRequests::MergeOrchestrationService
         .new(project, current_user, { sha: last_diff_sha })
         .execute(merge_request)
     end

config/feature_flags/development/improved_mergeability_checks.yml

+---
+name: improved_mergeability_checks
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68312
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/342386
+milestone: '14.4'
+type: development
+group: group::code review
+default_enabled: false

config/feature_flags/development/mergeability_caching.yml

+---
+name: mergeability_caching
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68312
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/340810
+milestone: '14.4'
+type: development
+group: group::code review
+default_enabled: false

lib/gitlab/email/handler/create_merge_request_handler.rb

@@ -61,7 +61,7 @@ module Gitlab
         private
 
         def build_merge_request
-          MergeRequests::BuildService.new(project: project, current_user: author, params: merge_request_params).execute
+          ::MergeRequests::BuildService.new(project: project, current_user: author, params: merge_request_params).execute
         end
 
         def create_merge_request
@@ -78,7 +78,7 @@ module Gitlab
           if merge_request.errors.any?
             merge_request
           else
-            MergeRequests::CreateService.new(project: project, current_user: author).create(merge_request)
+            ::MergeRequests::CreateService.new(project: project, current_user: author).create(merge_request)
           end
         end
 

lib/gitlab/merge_requests/mergeability/check_result.rb

+# frozen_string_literal: true
+module Gitlab
+  module MergeRequests
+    module Mergeability
+      class CheckResult
+        SUCCESS_STATUS = :success
+        FAILED_STATUS = :failed
+
+        attr_reader :status, :payload
+
+        def self.default_payload
+          { last_run_at: Time.current }
+        end
+
+        def self.success(payload: {})
+          new(status: SUCCESS_STATUS, payload: default_payload.merge(payload))
+        end
+
+        def self.failed(payload: {})
+          new(status: FAILED_STATUS, payload: default_payload.merge(payload))
+        end
+
+        def self.from_hash(data)
+          new(
+            status: data.fetch(:status),
+            payload: data.fetch(:payload))
+        end
+
+        def initialize(status:, payload: {})
+          @status = status
+          @payload = payload
+        end
+
+        def to_hash
+          { status: status, payload: payload }
+        end
+
+        def failed?
+          status == FAILED_STATUS
+        end
+
+        def success?
+          status == SUCCESS_STATUS
+        end
+      end
+    end
+  end
+end

lib/gitlab/merge_requests/mergeability/redis_interface.rb

+# frozen_string_literal: true
+module Gitlab
+  module MergeRequests
+    module Mergeability
+      class RedisInterface
+        EXPIRATION = 6.hours
+        VERSION = 1
+
+        def save_check(merge_check:, result_hash:)
+          Gitlab::Redis::SharedState.with do |redis|
+            redis.set(merge_check.cache_key + ":#{VERSION}", result_hash.to_json, ex: EXPIRATION)
+          end
+        end
+
+        def retrieve_check(merge_check:)
+          Gitlab::Redis::SharedState.with do |redis|
+            Gitlab::Json.parse(redis.get(merge_check.cache_key + ":#{VERSION}"))
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/merge_requests/mergeability/results_store.rb

+# frozen_string_literal: true
+module Gitlab
+  module MergeRequests
+    module Mergeability
+      class ResultsStore
+        def initialize(interface: RedisInterface.new, merge_request:)
+          @interface = interface
+          @merge_request = merge_request
+        end
+
+        def read(merge_check:)
+          interface.retrieve_check(merge_check: merge_check)
+        end
+
+        def write(merge_check:, result_hash:)
+          interface.save_check(merge_check: merge_check, result_hash: result_hash)
+        end
+
+        private
+
+        attr_reader :interface
+      end
+    end
+  end
+end

lib/gitlab/quick_actions/merge_request_actions.rb

@@ -148,7 +148,7 @@ module Gitlab
           quick_action_target.persisted? && quick_action_target.can_be_approved_by?(current_user)
         end
         command :approve do
-          success = MergeRequests::ApprovalService.new(project: quick_action_target.project, current_user: current_user).execute(quick_action_target)
+          success = ::MergeRequests::ApprovalService.new(project: quick_action_target.project, current_user: current_user).execute(quick_action_target)
 
           next unless success
 
@@ -162,7 +162,7 @@ module Gitlab
           quick_action_target.persisted? && quick_action_target.can_be_unapproved_by?(current_user)
         end
         command :unapprove do
-          success = MergeRequests::RemoveApprovalService.new(project: quick_action_target.project, current_user: current_user).execute(quick_action_target)
+          success = ::MergeRequests::RemoveApprovalService.new(project: quick_action_target.project, current_user: current_user).execute(quick_action_target)
 
           next unless success
 
@@ -275,7 +275,7 @@ module Gitlab
       end
 
       def merge_orchestration_service
-        @merge_orchestration_service ||= MergeRequests::MergeOrchestrationService.new(project, current_user)
+        @merge_orchestration_service ||= ::MergeRequests::MergeOrchestrationService.new(project, current_user)
       end
 
       def preferred_auto_merge_strategy(merge_request)

spec/lib/gitlab/merge_requests/mergeability/check_result_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::MergeRequests::Mergeability::CheckResult do
+  subject(:check_result) { described_class }
+
+  let(:time) { Time.current }
+
+  around do |example|
+    freeze_time do
+      example.run
+    end
+  end
+
+  describe '.default_payload' do
+    it 'returns the expected defaults' do
+      expect(check_result.default_payload).to eq({ last_run_at: time })
+    end
+  end
+
+  describe '.success' do
+    subject(:success) { check_result.success(payload: payload) }
+
+    let(:payload) { {} }
+
+    it 'creates a success result' do
+      expect(success.status).to eq described_class::SUCCESS_STATUS
+    end
+
+    it 'uses the default payload' do
+      expect(success.payload).to eq described_class.default_payload
+    end
+
+    context 'when given a payload' do
+      let(:payload) { { last_run_at: time + 1.day, test: 'test' } }
+
+      it 'uses the payload passed' do
+        expect(success.payload).to eq payload
+      end
+    end
+  end
+
+  describe '.failed' do
+    subject(:failed) { check_result.failed(payload: payload) }
+
+    let(:payload) { {} }
+
+    it 'creates a failure result' do
+      expect(failed.status).to eq described_class::FAILED_STATUS
+    end
+
+    it 'uses the default payload' do
+      expect(failed.payload).to eq described_class.default_payload
+    end
+
+    context 'when given a payload' do
+      let(:payload) { { last_run_at: time + 1.day, test: 'test' } }
+
+      it 'uses the payload passed' do
+        expect(failed.payload).to eq payload
+      end
+    end
+  end
+
+  describe '.from_hash' do
+    subject(:from_hash) { described_class.from_hash(hash) }
+
+    let(:status) { described_class::SUCCESS_STATUS }
+    let(:payload) { { test: 'test' } }
+    let(:hash) do
+      {
+        status: status,
+        payload: payload
+      }
+    end
+
+    it 'returns the expected status and payload' do
+      expect(from_hash.status).to eq status
+      expect(from_hash.payload).to eq payload
+    end
+  end
+
+  describe '#to_hash' do
+    subject(:to_hash) { described_class.new(**hash).to_hash }
+
+    let(:status) { described_class::SUCCESS_STATUS }
+    let(:payload) { { test: 'test' } }
+    let(:hash) do
+      {
+        status: status,
+        payload: payload
+      }
+    end
+
+    it 'returns the expected hash' do
+      expect(to_hash).to eq hash
+    end
+  end
+
+  describe '#failed?' do
+    subject(:failed) { described_class.new(status: status).failed? }
+
+    context 'when it has failed' do
+      let(:status) { described_class::FAILED_STATUS }
+
+      it 'returns true' do
+        expect(failed).to eq true
+      end
+    end
+
+    context 'when it has succeeded' do
+      let(:status) { described_class::SUCCESS_STATUS }
+
+      it 'returns false' do
+        expect(failed).to eq false
+      end
+    end
+  end
+
+  describe '#success?' do
+    subject(:success) { described_class.new(status: status).success? }
+
+    context 'when it has failed' do
+      let(:status) { described_class::FAILED_STATUS }
+
+      it 'returns false' do
+        expect(success).to eq false
+      end
+    end
+
+    context 'when it has succeeded' do
+      let(:status) { described_class::SUCCESS_STATUS }
+
+      it 'returns true' do
+        expect(success).to eq true
+      end
+    end
+  end
+end

spec/lib/gitlab/merge_requests/mergeability/redis_interface_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::MergeRequests::Mergeability::RedisInterface, :clean_gitlab_redis_shared_state do
+  subject(:redis_interface) { described_class.new }
+
+  let(:merge_check) { double(cache_key: '13') }
+  let(:result_hash) { { 'test' => 'test' } }
+  let(:expected_key) { "#{merge_check.cache_key}:#{described_class::VERSION}" }
+
+  describe '#save_check' do
+    it 'saves the hash' do
+      expect(Gitlab::Redis::SharedState.with { |redis| redis.get(expected_key) }).to be_nil
+
+      redis_interface.save_check(merge_check: merge_check, result_hash: result_hash)
+
+      expect(Gitlab::Redis::SharedState.with { |redis| redis.get(expected_key) }).to eq result_hash.to_json
+    end
+  end
+
+  describe '#retrieve_check' do
+    it 'returns the hash' do
+      Gitlab::Redis::SharedState.with { |redis| redis.set(expected_key, result_hash.to_json) }
+
+      expect(redis_interface.retrieve_check(merge_check: merge_check)).to eq result_hash
+    end
+  end
+end

spec/lib/gitlab/merge_requests/mergeability/results_store_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::MergeRequests::Mergeability::ResultsStore do
+  subject(:results_store) { described_class.new(merge_request: merge_request, interface: interface) }
+
+  let(:merge_check) { double }
+  let(:interface) { double }
+  let(:merge_request) { double }
+
+  describe '#read' do
+    it 'calls #retrieve on the interface' do
+      expect(interface).to receive(:retrieve_check).with(merge_check: merge_check)
+
+      results_store.read(merge_check: merge_check)
+    end
+  end
+
+  describe '#write' do
+    let(:result_hash) { double }
+
+    it 'calls #save_check on the interface' do
+      expect(interface).to receive(:save_check).with(merge_check: merge_check, result_hash: result_hash)
+
+      results_store.write(merge_check: merge_check, result_hash: result_hash)
+    end
+  end
+end

spec/models/merge_request_spec.rb

@@ -3089,7 +3089,7 @@ RSpec.describe MergeRequest, factory_default: :keep do
     end
   end
 
-  describe '#mergeable_state?' do
+  shared_examples 'for mergeable_state' do
     subject { create(:merge_request) }
 
     it 'checks if merge request can be merged' do
@@ -3130,33 +3130,61 @@ RSpec.describe MergeRequest, factory_default: :keep do
     end
 
     context 'when failed' do
-      context 'when #mergeable_ci_state? is false' do
-        before do
-          allow(subject).to receive(:mergeable_ci_state?) { false }
-        end
+      shared_examples 'failed skip_ci_check' do
+        context 'when #mergeable_ci_state? is false' do
+          before do
+            allow(subject).to receive(:mergeable_ci_state?) { false }
+          end
 
-        it 'returns false' do
-          expect(subject.mergeable_state?).to be_falsey
+          it 'returns false' do
+            expect(subject.mergeable_state?).to be_falsey
+          end
+
+          it 'returns true when skipping ci check' do
+            expect(subject.mergeable_state?(skip_ci_check: true)).to be(true)
+          end
         end
 
-        it 'returns true when skipping ci check' do
-          expect(subject.mergeable_state?(skip_ci_check: true)).to be(true)
+        context 'when #mergeable_discussions_state? is false' do
+          before do
+            allow(subject).to receive(:mergeable_discussions_state?) { false }
+          end
+
+          it 'returns false' do
+            expect(subject.mergeable_state?).to be_falsey
+          end
+
+          it 'returns true when skipping discussions check' do
+            expect(subject.mergeable_state?(skip_discussions_check: true)).to be(true)
+          end
         end
       end
 
-      context 'when #mergeable_discussions_state? is false' do
+      context 'when improved_mergeability_checks is on' do
+        it_behaves_like 'failed skip_ci_check'
+      end
+
+      context 'when improved_mergeability_checks is off' do
         before do
-          allow(subject).to receive(:mergeable_discussions_state?) { false }
+          stub_feature_flags(improved_mergeability_checks: false)
         end
 
-        it 'returns false' do
-          expect(subject.mergeable_state?).to be_falsey
-        end
+        it_behaves_like 'failed skip_ci_check'
+      end
+    end
+  end
 
-        it 'returns true when skipping discussions check' do
-          expect(subject.mergeable_state?(skip_discussions_check: true)).to be(true)
-        end
+  describe '#mergeable_state?' do
+    context 'when merge state caching is on' do
+      it_behaves_like 'for mergeable_state'
+    end
+
+    context 'when merge state caching is off' do
+      before do
+        stub_feature_flags(mergeability_caching: false)
       end
+
+      it_behaves_like 'for mergeable_state'
     end
   end
 

spec/services/merge_requests/mergeability/check_base_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::Mergeability::CheckBaseService do
+  subject(:check_base_service) { described_class.new(merge_request: merge_request, params: params) }
+
+  let(:merge_request) { double }
+  let(:params) { double }
+
+  describe '#merge_request' do
+    it 'returns the merge_request' do
+      expect(check_base_service.merge_request).to eq merge_request
+    end
+  end
+
+  describe '#params' do
+    it 'returns the params' do
+      expect(check_base_service.params).to eq params
+    end
+  end
+
+  describe '#skip?' do
+    it 'raises NotImplementedError' do
+      expect { check_base_service.skip? }.to raise_error(NotImplementedError)
+    end
+  end
+
+  describe '#cacheable?' do
+    it 'raises NotImplementedError' do
+      expect { check_base_service.skip? }.to raise_error(NotImplementedError)
+    end
+  end
+
+  describe '#cache_key?' do
+    it 'raises NotImplementedError' do
+      expect { check_base_service.skip? }.to raise_error(NotImplementedError)
+    end
+  end
+end

spec/services/merge_requests/mergeability/check_ci_status_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::Mergeability::CheckCiStatusService do
+  subject(:check_ci_status) { described_class.new(merge_request: merge_request, params: params) }
+
+  let(:merge_request) { build(:merge_request) }
+  let(:params) { { skip_ci_check: skip_check } }
+  let(:skip_check) { false }
+
+  describe '#execute' do
+    before do
+      expect(merge_request).to receive(:mergeable_ci_state?).and_return(mergeable)
+    end
+
+    context 'when the merge request is in a mergable state' do
+      let(:mergeable) { true }
+
+      it 'returns a check result with status success' do
+        expect(check_ci_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::SUCCESS_STATUS
+      end
+    end
+
+    context 'when the merge request is not in a mergeable state' do
+      let(:mergeable) { false }
+
+      it 'returns a check result with status failed' do
+        expect(check_ci_status.execute.status).to eq Gitlab::MergeRequests::Mergeability::CheckResult::FAILED_STATUS
+      end
+    end
+  end
+
+  describe '#skip?' do
+    context 'when skip check is true' do
+      let(:skip_check) { true }
+
+      it 'returns true' do
+        expect(check_ci_status.skip?).to eq true
+      end
+    end
+
+    context 'when skip check is false' do
+      let(:skip_check) { false }
+
+      it 'returns false' do
+        expect(check_ci_status.skip?).to eq false
+      end
+    end
+  end
+
+  describe '#cacheable?' do
+    it 'returns false' do
+      expect(check_ci_status.cacheable?).to eq false
+    end
+  end
+end

spec/services/merge_requests/mergeability/run_checks_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequests::Mergeability::RunChecksService do
+  subject(:run_checks) { described_class.new(merge_request: merge_request, params: {}) }
+
+  let_it_be(:merge_request) { create(:merge_request) }
+
+  describe '#CHECKS' do
+    it 'contains every subclass of the base checks service' do
+      expect(described_class::CHECKS).to contain_exactly(*MergeRequests::Mergeability::CheckBaseService.subclasses)
+    end
+  end
+
+  describe '#execute' do
+    subject(:execute) { run_checks.execute }
+
+    let(:params) { {} }
+    let(:success_result) { Gitlab::MergeRequests::Mergeability::CheckResult.success }
+
+    context 'when every check is skipped' do
+      before do
+        MergeRequests::Mergeability::CheckBaseService.subclasses.each do |subclass|
+          expect_next_instance_of(subclass) do |service|
+            expect(service).to receive(:skip?).and_return(true)
+          end
+        end
+      end
+
+      it 'is still a success' do
+        expect(execute.all?(&:success?)).to eq(true)
+      end
+    end
+
+    context 'when a check is skipped' do
+      it 'does not execute the check' do
+        expect_next_instance_of(MergeRequests::Mergeability::CheckCiStatusService) do |service|
+          expect(service).to receive(:skip?).and_return(true)
+          expect(service).not_to receive(:execute)
+        end
+
+        expect(execute).to match_array([])
+      end
+    end
+
+    context 'when a check is not skipped' do
+      let(:cacheable) { true }
+      let(:merge_check) { instance_double(MergeRequests::Mergeability::CheckCiStatusService) }
+
+      before do
+        expect(MergeRequests::Mergeability::CheckCiStatusService).to receive(:new).and_return(merge_check)
+        expect(merge_check).to receive(:skip?).and_return(false)
+        allow(merge_check).to receive(:cacheable?).and_return(cacheable)
+        allow(merge_check).to receive(:execute).and_return(success_result)
+      end
+
+      context 'when the check is cacheable' do
+        context 'when the check is cached' do
+          it 'returns the cached result' do
+            expect_next_instance_of(Gitlab::MergeRequests::Mergeability::ResultsStore) do |service|
+              expect(service).to receive(:read).with(merge_check: merge_check).and_return(success_result)
+            end
+
+            expect(execute).to match_array([success_result])
+          end
+        end
+
+        context 'when the check is not cached' do
+          it 'writes and returns the result' do
+            expect_next_instance_of(Gitlab::MergeRequests::Mergeability::ResultsStore) do |service|
+              expect(service).to receive(:read).with(merge_check: merge_check).and_return(nil)
+              expect(service).to receive(:write).with(merge_check: merge_check, result_hash: success_result.to_hash).and_return(true)
+            end
+
+            expect(execute).to match_array([success_result])
+          end
+        end
+      end
+
+      context 'when check is not cacheable' do
+        let(:cacheable) { false }
+
+        it 'does not call the results store' do
+          expect(Gitlab::MergeRequests::Mergeability::ResultsStore).not_to receive(:new)
+
+          expect(execute).to match_array([success_result])
+        end
+      end
+
+      context 'when mergeability_caching is turned off' do
+        before do
+          stub_feature_flags(mergeability_caching: false)
+        end
+
+        it 'does not call the results store' do
+          expect(Gitlab::MergeRequests::Mergeability::ResultsStore).not_to receive(:new)
+
+          expect(execute).to match_array([success_result])
+        end
+      end
+    end
+  end
+end