Load only one scan_finding_rule per policy

by grouping them based on a policy index which
is passed from project to merge request rules.

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71858
https://gitlab.com/gitlab-org/gitlab/-/issues/341962

EE: true
Changelog: changed

db/migrate/20211108204736_add_policy_idx_to_approval_project_rule.rb

+# frozen_string_literal: true
+
+class AddPolicyIdxToApprovalProjectRule < Gitlab::Database::Migration[1.0]
+  enable_lock_retries!
+
+  def change
+    add_column :approval_project_rules, :orchestration_policy_idx, :integer, limit: 2
+  end
+end

db/migrate/20211122215001_add_policy_idx_to_approval_merge_request_rule.rb

+# frozen_string_literal: true
+
+class AddPolicyIdxToApprovalMergeRequestRule < Gitlab::Database::Migration[1.0]
+  enable_lock_retries!
+
+  def change
+    add_column :approval_merge_request_rules, :orchestration_policy_idx, :integer, limit: 2
+  end
+end

db/schema_migrations/20211108204736

+9e01b1817e4c578f5be7d7378dc12a8535c2bbbff5ecbc77f5a7cfdb148927f5
\ No newline at end of file

db/schema_migrations/20211122215001

+fc29e10717357f7dd57940042d69a6c43a0d17fdf3c951917a76eae8c1d93ba3
\ No newline at end of file

db/structure.sql

@@ -10547,6 +10547,7 @@ CREATE TABLE approval_merge_request_rules (
     report_type smallint,
     section text,
     modified_from_project_rule boolean DEFAULT false NOT NULL,
+    orchestration_policy_idx smallint,
     CONSTRAINT check_6fca5928b2 CHECK ((char_length(section) <= 255))
 );
 
@@ -10616,7 +10617,8 @@ CREATE TABLE approval_project_rules (
     vulnerabilities_allowed smallint DEFAULT 0 NOT NULL,
     severity_levels text[] DEFAULT '{}'::text[] NOT NULL,
     report_type smallint,
-    vulnerability_states text[] DEFAULT '{newly_detected}'::text[] NOT NULL
+    vulnerability_states text[] DEFAULT '{newly_detected}'::text[] NOT NULL,
+    orchestration_policy_idx smallint
 );
 
 CREATE TABLE approval_project_rules_groups (

ee/app/models/approval_project_rule.rb

@@ -83,7 +83,7 @@ class ApprovalProjectRule < ApplicationRecord
 
   def report_approver_attributes
     attributes
-      .slice('approvals_required', 'name')
+      .slice('approvals_required', 'name', 'orchestration_policy_idx')
       .merge(
         users: users,
         groups: groups,

ee/app/models/approval_state.rb

@@ -247,10 +247,10 @@ class ApprovalState
 
   def wrapped_rules
     strong_memoize(:wrapped_rules) do
-      merge_request_rules = merge_request.approval_rules.applicable_to_branch(target_branch)
+      grouped_merge_request_rules = merge_request.approval_rules.applicable_to_branch(target_branch).group_by(&:report_type)
 
-      merge_request_rules.map! do |rule|
-        ApprovalWrappedRule.wrap(merge_request, rule)
+      grouped_merge_request_rules.flat_map do |report_type, merge_request_rules|
+        Approvals::WrappedRuleSet.wrap(merge_request, merge_request_rules, report_type).wrapped_rules
       end
     end
   end

ee/app/models/approval_wrapped_rule.rb

@@ -23,7 +23,7 @@ class ApprovalWrappedRule
 
   def_delegators(:@approval_rule,
                  :regular?, :any_approver?, :code_owner?, :report_approver?,
-                 :overridden?, :id, :name, :users, :groups, :code_owner,
+                 :overridden?, :id, :users, :groups, :code_owner,
                  :source_rule, :rule_type, :approvals_required, :section, :to_global_id)
 
   def self.wrap(merge_request, rule)
@@ -109,6 +109,12 @@ class ApprovalWrappedRule
     approvers - approved_approvers
   end
 
+  def name
+    return approval_rule.name unless approval_rule.scan_finding?
+
+    approval_rule.name.gsub(/\s\d+\z/, '')
+  end
+
   private
 
   def filter_approvers(approvers)

ee/app/models/approvals/scan_finding_wrapped_rule_set.rb

+# frozen_string_literal: true
+module Approvals
+  class ScanFindingWrappedRuleSet < WrappedRuleSet
+    extend ::Gitlab::Utils::Override
+
+    override :wrapped_rules
+    def wrapped_rules
+      strong_memoize(:wrapped_rules) do
+        if ::Feature.enabled?(:scan_result_policy, merge_request.project)
+          grouped_merge_request_rules = approval_rules.group_by(&:orchestration_policy_idx)
+          grouped_merge_request_rules.map do |_, merge_request_rules|
+            wrapped_rules_sorted_by_approval(merge_request_rules).first
+          end
+        else
+          []
+        end
+      end
+    end
+
+    private
+
+    def wrapped_rules_sorted_by_approval(merge_request_rules)
+      merge_request_rules.map! do |rule|
+        ApprovalWrappedRule.wrap(merge_request, rule)
+      end
+      merge_request_rules.sort_by {|rule| rule.approved? ? 1 : 0}
+    end
+  end
+end

ee/app/models/approvals/wrapped_rule_set.rb

+# frozen_string_literal: true
+
+module Approvals
+  class WrappedRuleSet
+    include Gitlab::Utils::StrongMemoize
+
+    attr_reader :merge_request, :approval_rules
+
+    def self.wrap(merge_request, rules, report_type)
+      if report_type == Security::ScanResultPolicy::SCAN_FINDING
+        ScanFindingWrappedRuleSet.new(merge_request, rules)
+      else
+        WrappedRuleSet.new(merge_request, rules)
+      end
+    end
+
+    def initialize(merge_request, approval_rules)
+      @merge_request = merge_request
+      @approval_rules = approval_rules
+    end
+
+    def wrapped_rules
+      strong_memoize(:wrapped_rules) do
+        approval_rules.map do |rule|
+          ApprovalWrappedRule.wrap(merge_request, rule)
+        end
+      end
+    end
+  end
+end

ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb

@@ -5,9 +5,10 @@ module Security
     class ProcessScanResultPolicyService
       MAX_LENGTH = 25
 
-      def initialize(policy_configuration:, policy:)
+      def initialize(policy_configuration:, policy:, policy_index:)
         @policy_configuration = policy_configuration
         @policy = policy
+        @policy_index = policy_index
         @project = policy_configuration.project
         @author = policy_configuration.policy_last_updated_by
       end
@@ -20,7 +21,7 @@ module Security
 
       private
 
-      attr_reader :policy_configuration, :policy, :project, :author
+      attr_reader :policy_configuration, :policy, :project, :author, :policy_index
 
       def create_new_approval_rules
         action_info = policy[:actions].find { |action| action[:type] == Security::ScanResultPolicy::REQUIRE_APPROVAL }
@@ -43,12 +44,13 @@ module Security
           severity_levels: rule[:severity_levels],
           user_ids: project.users.get_ids_by_username(action_info[:approvers]),
           vulnerabilities_allowed: rule[:vulnerabilities_allowed],
-          report_type: :scan_finding
+          report_type: :scan_finding,
+          orchestration_policy_idx: policy_index
         }
       end
 
       def rule_name(policy_name, rule_index)
-        truncated = policy_name.truncate(MAX_LENGTH, omission: '')
+        truncated = policy_name.truncate(MAX_LENGTH)
         return truncated if rule_index == 0
 
         "#{truncated} #{rule_index + 1}"

ee/app/workers/security/create_orchestration_policy_worker.rb

@@ -28,9 +28,9 @@ module Security
 
           configuration.transaction do
             configuration.approval_rules.scan_finding.delete_all
-            configuration.active_scan_result_policies.each do |policy|
+            configuration.active_scan_result_policies.each_with_index do |policy, policy_index|
               Security::SecurityOrchestrationPolicies::ProcessScanResultPolicyService
-                .new(policy_configuration: configuration, policy: policy)
+                .new(policy_configuration: configuration, policy: policy, policy_index: policy_index)
                 .execute
             end
           end

ee/spec/models/approval_project_rule_spec.rb

@@ -139,7 +139,7 @@ RSpec.describe ApprovalProjectRule do
 
     context "when there is a project rule for each report type" do
       with_them do
-        subject { create(:approval_project_rule, report_type, :requires_approval, project: project) }
+        subject { create(:approval_project_rule, report_type, :requires_approval, project: project, orchestration_policy_idx: 1) }
 
         let!(:result) { subject.apply_report_approver_rules_to(merge_request) }
 
@@ -149,6 +149,7 @@ RSpec.describe ApprovalProjectRule do
         specify { expect(result.name).to be(:default_name) }
         specify { expect(result.rule_type).to be(:report_approver) }
         specify { expect(result.report_type).to be(:report_type) }
+        specify { expect(result.orchestration_policy_idx).to be 1 }
       end
     end
   end

ee/spec/models/approval_state_spec.rb

@@ -173,6 +173,24 @@ RSpec.describe ApprovalState do
       end
     end
 
+    context 'with multiple scan_finding rules' do
+      before do
+        2.times {create_rule({ rule_type: :report_approver, report_type: :scan_finding })}
+        2.times {create_rule({ rule_type: :report_approver, report_type: :scan_finding, orchestration_policy_idx: 1 })}
+      end
+
+      it 'returns one rule for each orchestration_policy_idx' do
+        approval_rules = subject.wrapped_approval_rules
+
+        expect(approval_rules.count).to be(2)
+        expect(approval_rules).to all(be_instance_of(ApprovalWrappedRule))
+
+        policy_indices = approval_rules.map { |rule| rule.approval_rule.orchestration_policy_idx }
+
+        expect(policy_indices).to contain_exactly(nil, 1)
+      end
+    end
+
     describe '#approval_needed?' do
       context 'when feature not available' do
         it 'returns false' do

ee/spec/models/approval_wrapped_rule_spec.rb

@@ -201,4 +201,27 @@ RSpec.describe ApprovalWrappedRule do
       expect(subject.approvals_required).to eq(19)
     end
   end
+
+  describe '#name' do
+    let(:rule_name) { 'approval rule 2' }
+    let(:rule) { create(:approval_merge_request_rule, report_type: report_type, name: rule_name) }
+
+    context 'with report_type set to scan_finding' do
+      let(:report_type) { :scan_finding }
+      let(:expected_rule_name) { 'approval rule' }
+
+      it 'returns rule name without the sequential notation' do
+        expect(subject.name).not_to eq(rule_name)
+        expect(subject.name).to eq(expected_rule_name)
+      end
+    end
+
+    context 'with report_type other than scan_finding' do
+      let(:report_type) { :vulnerability }
+
+      it 'returns rule name as is' do
+        expect(subject.name).to eq(rule_name)
+      end
+    end
+  end
 end

ee/spec/models/approvals/scan_finding_wrapped_rule_set_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Approvals::ScanFindingWrappedRuleSet do
+  let(:report_type) { Security::ScanResultPolicy::SCAN_FINDING }
+  let_it_be(:merge_request) { create(:merge_request) }
+  let_it_be(:approver) { create(:user) }
+  let_it_be(:approval_rules) { create_list(:approval_merge_request_rule, 2, :scan_finding, merge_request: merge_request, users: [approver]) }
+
+  let(:approval_rules_list) { approval_rules }
+
+  subject { described_class.wrap(merge_request, approval_rules_list, report_type).wrapped_rules }
+
+  describe '#wrapped_rules' do
+    context 'with feature flag disabled' do
+      before do
+        stub_feature_flags(scan_result_policy: false)
+      end
+
+      it {is_expected.to be_empty }
+    end
+
+    it 'returns only one rule' do
+      expect(subject.count).to be 1
+    end
+
+    context 'with various orchestration_policy_idx' do
+      let(:orchestration_policy_idx) { 0 }
+      let(:approval_rules_w_policy_idx) { create_list(:approval_merge_request_rule, 2, :scan_finding, merge_request: merge_request, orchestration_policy_idx: orchestration_policy_idx, users: [approver]) }
+      let(:approval_rules_list) { approval_rules + approval_rules_w_policy_idx }
+
+      it 'returns one rule for each orchestration_policy_idx' do
+        expect(subject.count).to be 2
+
+        orchestration_policy_indices = subject.map { |wrapped_rule| wrapped_rule.approval_rule.orchestration_policy_idx }
+
+        expect(orchestration_policy_indices).to contain_exactly(nil, orchestration_policy_idx)
+      end
+
+      context 'with unapproved rules' do
+        let(:unapproved_rule) { create(:approval_merge_request_rule, :scan_finding, merge_request: merge_request, orchestration_policy_idx: orchestration_policy_idx, users: [approver], approvals_required: 5) }
+        let(:approval_rules_list) { approval_rules + approval_rules_w_policy_idx + [unapproved_rule] }
+
+        it 'returns sorted based on approval' do
+          selected_rules = subject.select { |wrapped_rule| wrapped_rule.approval_rule.orchestration_policy_idx == orchestration_policy_idx }
+
+          expect(selected_rules.count).to be 1
+          expect(selected_rules.first.id).to be unapproved_rule.id
+        end
+      end
+    end
+  end
+end

ee/spec/models/approvals/wrapped_rule_set_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Approvals::WrappedRuleSet do
+  let(:report_type) { nil }
+  let(:merge_request) { build(:merge_request) }
+  let(:approval_merge_request_rule) { build(:approval_merge_request_rule, merge_request: merge_request, report_type: report_type) }
+  let(:grouped_approval_wrapped_rules) { described_class.wrap(merge_request, [approval_merge_request_rule], report_type) }
+
+  describe '.wrap' do
+    subject { grouped_approval_wrapped_rules }
+
+    context "with report_type set to #{Security::ScanResultPolicy::SCAN_FINDING}" do
+      let(:report_type) { Security::ScanResultPolicy::SCAN_FINDING }
+
+      it { is_expected.to be_instance_of(Approvals::ScanFindingWrappedRuleSet) }
+    end
+
+    context 'with any other report_type' do
+      it { is_expected.to be_instance_of(described_class) }
+    end
+  end
+
+  describe '#wrapped_rules' do
+    subject { grouped_approval_wrapped_rules.wrapped_rules }
+
+    it 'returns an array of ApprovalWrappedRule' do
+      expect(subject.count).to be 1
+      expect(subject.first).to be_instance_of(ApprovalWrappedRule)
+    end
+
+    it "returns ApprovalWrappedRule with attributes as provided to #{described_class.name}" do
+      expect(subject.first).to have_attributes(merge_request: merge_request, approval_rule: approval_merge_request_rule)
+    end
+  end
+end

ee/spec/services/security/security_orchestration_policies/process_scan_result_policy_service_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessScanResultPolicyS
     let(:policy) { build(:scan_result_policy, name: 'Test Policy') }
     let(:policy_yaml) { Gitlab::Config::Loader::Yaml.new(policy.to_yaml).load! }
     let(:project) { policy_configuration.project }
-    let(:service) { described_class.new(policy_configuration: policy_configuration, policy: policy) }
+    let(:service) { described_class.new(policy_configuration: policy_configuration, policy: policy, policy_index: 0) }
 
     before do
       allow(policy_configuration).to receive(:policy_last_updated_by).and_return(project.owner)

ee/spec/workers/security/create_orchestration_policy_worker_spec.rb

@@ -63,9 +63,9 @@ RSpec.describe Security::CreateOrchestrationPolicyWorker do
           end
         end
 
-        active_policies[:scan_result_policy].each do |policy|
+        active_policies[:scan_result_policy].each_with_index do |policy, policy_index|
           expect_next_instance_of(Security::SecurityOrchestrationPolicies::ProcessScanResultPolicyService,
-                                  policy_configuration: configuration, policy: policy) do |service|
+                                  policy_configuration: configuration, policy: policy, policy_index: policy_index) do |service|
             expect(service).to receive(:execute)
           end
         end