From 6bdb200ae2590720cc8060b2cb6d841a42032d8d Mon Sep 17 00:00:00 2001 From: mc_rocha Date: Mon, 6 Oct 2025 07:55:53 -0400 Subject: [PATCH 1/5] Adds policy_violations to vulnerability reference EE: true Changelog: added --- .../vulnerability/enhanced_proxy.rb | 10 ++- .../vulnerability/policy_violations.rb | 34 ++++++++++ .../elastic/record_proxy/vulnerability.rb | 5 +- .../elastic/references/vulnerability.rb | 16 ++++- ee/lib/security/policy_violations.rb | 11 +++ .../vulnerability/enhanced_proxy_spec.rb | 50 +++++++++++++- .../vulnerability/policy_violations_spec.rb | 67 +++++++++++++++++++ .../record_proxy/vulnerability_spec.rb | 23 ++++++- .../elastic/references/vulnerability_spec.rb | 36 ++++++++++ 9 files changed, 242 insertions(+), 10 deletions(-) create mode 100644 ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb create mode 100644 ee/lib/security/policy_violations.rb create mode 100644 ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb diff --git a/ee/lib/search/elastic/preloaders/vulnerability/enhanced_proxy.rb b/ee/lib/search/elastic/preloaders/vulnerability/enhanced_proxy.rb index 97b3b8254090db..a2a5df77ff728d 100644 --- a/ee/lib/search/elastic/preloaders/vulnerability/enhanced_proxy.rb +++ b/ee/lib/search/elastic/preloaders/vulnerability/enhanced_proxy.rb @@ -8,7 +8,7 @@ module Vulnerability # enhanced record proxies for elasticsearch indexing. # # This class: - # 1. Coordinates multiple individual preloaders (EPSS, CVE, Reachability, TokenStatus) + # 1. Coordinates multiple individual preloaders (EPSS, CVE, Reachability, TokenStatus, PolicyViolations) # 2. Creates enhanced proxies with all preloaded data # 3. Assigns proxies back to reference objects # 4. Provides a single entry point for all vulnerability data preloading @@ -33,6 +33,7 @@ def preload_and_enhance! def preload_all_data @reachability_data = Reachability.new(records).preload @token_status_data = TokenStatus.new(records).preload + @policy_violations_data = PolicyViolations.new(records).preload end # Create enhanced proxies and assign them to references @@ -52,7 +53,8 @@ def create_enhanced_proxy(record) vulnerability_id = record.vulnerability_id enhancements = { 'reachability' => fetch_reachability_value(vulnerability_id), - 'token_status' => fetch_token_status_value(vulnerability_id) + 'token_status' => fetch_token_status_value(vulnerability_id), + 'policy_violations' => fetch_policy_violation_value(vulnerability_id) }.with_indifferent_access ::Search::Elastic::RecordProxy::Vulnerability.create_with_enhancements(record, enhancements) @@ -65,6 +67,10 @@ def fetch_reachability_value(vulnerability_id) def fetch_token_status_value(vulnerability_id) @token_status_data[vulnerability_id] || ::Security::TokenStatus::UNKNOWN end + + def fetch_policy_violation_value(vulnerability_id) + @policy_violations_data[vulnerability_id] + end end end end diff --git a/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb b/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb new file mode 100644 index 00000000000000..f9645efa07605f --- /dev/null +++ b/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module Search + module Elastic + module Preloaders + module Vulnerability + class PolicyViolations < Base + def perform_preload + security_findings_uuids = records.map(&:uuid) + return {} if security_findings_uuids.empty? + + fetch_policy_violations_data(security_findings_uuids) + end + + private + + def fetch_policy_violations_data(security_findings_uuids) + project_ids = records.map(&:project_id) + + dismissed_uuids = ::Security::PolicyDismissal + .for_projects(project_ids) + .for_security_findings_uuids(security_findings_uuids) + .flat_map(&:security_findings_uuids).uniq + + records.each_with_object({}) do |record, result| + result[record.vulnerability_id] = + dismissed_uuids.include?(record.uuid) ? ::Security::PolicyViolations::DISMISSED_IN_MR : nil + end + end + end + end + end + end +end diff --git a/ee/lib/search/elastic/record_proxy/vulnerability.rb b/ee/lib/search/elastic/record_proxy/vulnerability.rb index 3d3d7370dd3938..b67860896a9369 100644 --- a/ee/lib/search/elastic/record_proxy/vulnerability.rb +++ b/ee/lib/search/elastic/record_proxy/vulnerability.rb @@ -5,7 +5,7 @@ module Elastic module RecordProxy # Vulnerability-specific record proxy that provides optimized access # to elasticsearch indexing data including EPSS scores, CVE values, - # reachability and token status information. + # reachability, token status, and policy_violations information. class Vulnerability < Base # Creates a vulnerability proxy with all optimizations applied def self.create_with_enhancements(record, enhancements) @@ -13,7 +13,8 @@ def self.create_with_enhancements(record, enhancements) proxy.enhance_with_data({ reachability: enhancements[:reachability], - token_status: enhancements[:token_status] + token_status: enhancements[:token_status], + policy_violations: enhancements[:policy_violations] }) proxy diff --git a/ee/lib/search/elastic/references/vulnerability.rb b/ee/lib/search/elastic/references/vulnerability.rb index 6703054034811f..32e30f304ad77d 100644 --- a/ee/lib/search/elastic/references/vulnerability.rb +++ b/ee/lib/search/elastic/references/vulnerability.rb @@ -7,7 +7,7 @@ class Vulnerability < Reference include Search::Elastic::Concerns::DatabaseReference include ::Gitlab::Utils::StrongMemoize - SCHEMA_VERSION = 25_37 + SCHEMA_VERSION = 25_38 DOC_TYPE = 'vulnerability' INDEX_NAME = 'vulnerabilities' @@ -70,6 +70,7 @@ def serialize self.class.join_delimited([klass, identifier, routing].compact) end + # rubocop: disable Metrics/AbcSize -- TODO # Generate the JSON representation for elasticsearch indexing override :as_indexed_json def as_indexed_json @@ -96,6 +97,10 @@ def as_indexed_json fields["token_status"] = fetch_record_attribute(database_record, :token_status) end + if policy_violations_migration_finished? + fields["policy_violations"] = fetch_record_attribute(database_record, :policy_violations) + end + if resolved_at_dismissed_at_migration_completed? fields["resolved_at"] = database_record.vulnerability.resolved_at fields["dismissed_at"] = database_record.vulnerability.dismissed_at @@ -103,6 +108,7 @@ def as_indexed_json internal_es_fields.merge(fields) end + # rubocop: enable Metrics/AbcSize override :index_name def index_name @@ -134,8 +140,10 @@ def internal_es_fields end def fetch_schema_version - if token_status_migration_finished? + if policy_violations_migration_finished? SCHEMA_VERSION + elsif token_status_migration_finished? + 25_37 elsif resolved_at_dismissed_at_migration_completed? 25_36 elsif reachability_migration_finished? @@ -157,6 +165,10 @@ def token_status_migration_finished? ) end + def policy_violations_migration_finished? + ::Elastic::DataMigrationService.migration_has_finished?(:add_policy_violations_field_to_vulnerability) + end + def resolved_at_dismissed_at_migration_completed? ::Elastic::DataMigrationService.migration_has_finished?( :add_resolved_at_dismissed_at_fields_to_vulnerability) diff --git a/ee/lib/security/policy_violations.rb b/ee/lib/security/policy_violations.rb new file mode 100644 index 00000000000000..e22b6bf2d30276 --- /dev/null +++ b/ee/lib/security/policy_violations.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Security + module PolicyViolations + DISMISSED_IN_MR = 0 + + VIOLATIONS_TYPES = { + dismissed_in_mr: DISMISSED_IN_MR + }.freeze + end +end diff --git a/ee/spec/lib/search/elastic/preloaders/vulnerability/enhanced_proxy_spec.rb b/ee/spec/lib/search/elastic/preloaders/vulnerability/enhanced_proxy_spec.rb index bc77efba9a34b8..8f02be1088606b 100644 --- a/ee/spec/lib/search/elastic/preloaders/vulnerability/enhanced_proxy_spec.rb +++ b/ee/spec/lib/search/elastic/preloaders/vulnerability/enhanced_proxy_spec.rb @@ -19,6 +19,9 @@ let(:mock_reachability_preloader) { instance_double(Search::Elastic::Preloaders::Vulnerability::Reachability) } let(:mock_token_status_preloader) { instance_double(Search::Elastic::Preloaders::Vulnerability::TokenStatus) } + let(:mock_policy_violations_preloader) do + instance_double(Search::Elastic::Preloaders::Vulnerability::PolicyViolations) + end let(:reachability_data) do { @@ -35,6 +38,13 @@ } end + let(:policy_violations_data) do + { + vulnerability_reads[0].vulnerability_id => ::Security::PolicyViolations::DISMISSED_IN_MR, + vulnerability_reads[1].vulnerability_id => nil + } + end + let(:enhanced_proxy) { described_class.new(refs, vulnerability_reads) } before do @@ -55,6 +65,15 @@ allow(mock_token_status_preloader) .to receive(:preload) .and_return(token_status_data) + + allow(Search::Elastic::Preloaders::Vulnerability::PolicyViolations) + .to receive(:new) + .with(vulnerability_reads) + .and_return(mock_policy_violations_preloader) + + allow(mock_policy_violations_preloader) + .to receive(:preload) + .and_return(policy_violations_data) end describe '#initialize' do @@ -86,6 +105,10 @@ expect(Search::Elastic::Preloaders::Vulnerability::TokenStatus) .to have_received(:new).with(vulnerability_reads) expect(mock_token_status_preloader).to have_received(:preload) + + expect(Search::Elastic::Preloaders::Vulnerability::PolicyViolations) + .to have_received(:new).with(vulnerability_reads) + expect(mock_policy_violations_preloader).to have_received(:preload) end it 'creates enhanced proxies for matching refs' do @@ -129,17 +152,20 @@ it 'enhances proxies with correct reachability and token_status data' do expected_enhancements_in_use = { 'reachability' => ::Enums::Sbom::REACHABILITY_TYPES[::Enums::Sbom::IN_USE], - 'token_status' => ::Security::TokenStatus::ACTIVE + 'token_status' => ::Security::TokenStatus::ACTIVE, + 'policy_violations' => ::Security::PolicyViolations::DISMISSED_IN_MR }.with_indifferent_access expected_enhancements_not_found = { 'reachability' => ::Enums::Sbom::REACHABILITY_TYPES[::Enums::Sbom::NOT_FOUND], - 'token_status' => ::Security::TokenStatus::INACTIVE + 'token_status' => ::Security::TokenStatus::INACTIVE, + 'policy_violations' => nil }.with_indifferent_access expected_enhancements_unknown = { 'reachability' => ::Enums::Sbom::REACHABILITY_TYPES[::Enums::Sbom::UNKNOWN], - 'token_status' => ::Security::TokenStatus::UNKNOWN + 'token_status' => ::Security::TokenStatus::UNKNOWN, + 'policy_violations' => nil }.with_indifferent_access expect(::Search::Elastic::RecordProxy::Vulnerability) @@ -168,6 +194,7 @@ expect(assigned_proxy.project_id).to eq(vulnerability_reads[0].project_id) expect(assigned_proxy.reachability).to eq(reachability_data[vulnerability_reads[0].vulnerability_id]) expect(assigned_proxy.token_status).to eq(token_status_data[vulnerability_reads[0].vulnerability_id]) + expect(assigned_proxy.policy_violations).to eq(policy_violations_data[vulnerability_reads[0].vulnerability_id]) end end @@ -217,4 +244,21 @@ expect(result).to eq(::Security::TokenStatus::UNKNOWN) end end + + describe '#fetch_policy_violations_value' do + it 'returns preloaded value when available' do + enhanced_proxy.instance_variable_set(:@policy_violations_data, + { 123 => ::Security::PolicyViolations::DISMISSED_IN_MR }) + + result = enhanced_proxy.send(:fetch_policy_violation_value, 123) + expect(result).to eq(::Security::PolicyViolations::DISMISSED_IN_MR) + end + + it 'returns nil for missing data' do + enhanced_proxy.instance_variable_set(:@policy_violations_data, {}) + + result = enhanced_proxy.send(:fetch_policy_violation_value, 999) + expect(result).to be_nil + end + end end diff --git a/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb b/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb new file mode 100644 index 00000000000000..aae7424fd45758 --- /dev/null +++ b/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Search::Elastic::Preloaders::Vulnerability::PolicyViolations, feature_category: :vulnerability_management do + let_it_be(:project) { create(:project) } + let_it_be(:reads_by_policy_violation_type) do + { + dismissed_in_mr: create_read_with_policy_violations(true, project), + no_violations: create_read_with_policy_violations(false, project) + } + end + + def create_read_with_policy_violations(has_violations, project) + vulnerability = create(:vulnerability, :with_finding, project: project) + + create(:policy_dismissal, project: project, security_findings_uuids: [vulnerability.finding.uuid]) if has_violations + + vulnerability.vulnerability_read + end + + describe '#preload' do + subject(:preloader) { described_class.new(records) } + + context 'with vulnerability records having different policy violation types' do + let(:records) { reads_by_policy_violation_type.values } + + it 'returns policy violation status for vulnerabilities' do + result = preloader.preload + + vulnerability_dismissed_in_mr = reads_by_policy_violation_type[:dismissed_in_mr].vulnerability_id + vulnerability_without_dismissal = reads_by_policy_violation_type[:no_violations].vulnerability_id + + expect(result[vulnerability_dismissed_in_mr]).to eq(described_class::VIOLATIONS_TYPES[:dismissed_in_mr]) + expect(result[vulnerability_without_dismissal]).to be_nil + end + end + + context 'with edge cases' do + context 'when records are empty' do + let(:records) { [] } + + it 'returns empty hash' do + expect(preloader.preload).to eq({}) + end + end + + context 'when database query fails' do + let(:records) { [reads_by_policy_violation_type[:dismissed_in_mr]] } + + before do + allow(::Security::PolicyDismissal).to receive(:by_security_findings_uuids).and_raise(StandardError, + 'Database error') + allow(::Gitlab::ErrorTracking).to receive(:track_exception) + end + + it 'handles errors gracefully and tracks exceptions' do + result = preloader.preload + + expect(result).to eq({}) + expect(::Gitlab::ErrorTracking).to have_received(:track_exception) + .with(instance_of(StandardError), class: described_class.name) + end + end + end + end +end diff --git a/ee/spec/lib/search/elastic/record_proxy/vulnerability_spec.rb b/ee/spec/lib/search/elastic/record_proxy/vulnerability_spec.rb index 2a6d450c94c23f..b984d6af48527a 100644 --- a/ee/spec/lib/search/elastic/record_proxy/vulnerability_spec.rb +++ b/ee/spec/lib/search/elastic/record_proxy/vulnerability_spec.rb @@ -22,7 +22,10 @@ end describe '.create_with_enhancements' do - let(:enhancements) { { reachability: 1, token_status: ::Security::TokenStatus::ACTIVE } } + let(:enhancements) do + { reachability: 1, token_status: ::Security::TokenStatus::ACTIVE, + policy_violations: ::Security::PolicyViolations::DISMISSED_IN_MR } + end it 'creates a new proxy instance' do proxy = described_class.create_with_enhancements(vulnerability_record, enhancements) @@ -48,6 +51,12 @@ expect(proxy.token_status).to eq(::Security::TokenStatus::ACTIVE) end + it 'enhances the proxy with policy_violations data' do + proxy = described_class.create_with_enhancements(vulnerability_record, enhancements) + + expect(proxy.policy_violations).to eq(::Security::PolicyViolations::DISMISSED_IN_MR) + end + context 'with missing reachability in enhancements' do let(:enhancements) { { token_status: ::Security::TokenStatus::INACTIVE } } @@ -80,6 +89,11 @@ .to receive(:new).and_return(instance_double( Search::Elastic::Preloaders::Vulnerability::TokenStatus, preload: {} )) + + allow(Search::Elastic::Preloaders::Vulnerability::PolicyViolations) + .to receive(:new).and_return(instance_double( + Search::Elastic::Preloaders::Vulnerability::PolicyViolations, preload: {} + )) end it 'defaults token_status to UNKNOWN when missing' do @@ -88,6 +102,13 @@ expect(proxy.token_status).to eq(::Security::TokenStatus::UNKNOWN) end + + it 'defaults policy_violations to nil when missing' do + enhanced_proxy.preload_and_enhance! + proxy = refs.first.database_record + + expect(proxy.policy_violations).to be_nil + end end end end diff --git a/ee/spec/lib/search/elastic/references/vulnerability_spec.rb b/ee/spec/lib/search/elastic/references/vulnerability_spec.rb index 3009f7f5d77a6c..eb9f4a7331b435 100644 --- a/ee/spec/lib/search/elastic/references/vulnerability_spec.rb +++ b/ee/spec/lib/search/elastic/references/vulnerability_spec.rb @@ -77,6 +77,7 @@ epss_scores: [], reachability: [], token_status: [], + policy_violations: [], type: described_class::DOC_TYPE, schema_version: described_class::SCHEMA_VERSION } @@ -246,6 +247,41 @@ end end + context 'with policy_violations migration mappings' do + context 'when policy_violations migration has finished' do + let(:policy_violations_data) { ::Security::PolicyViolations::DISMISSED_IN_MR } + + before do + set_elasticsearch_migration_to(:add_policy_violations_field_to_vulnerability) + + allow(object).to receive(:policy_violations).and_return(policy_violations_data) + allow(vulnerability_reference_object).to receive(:database_record).and_return(object) + end + + it 'returns schema version' do + expect(indexed_json[:schema_version]).to eq(25_38) + end + + it 'includes policy_violations in the indexed json' do + expect(indexed_json[:policy_violations]).to eq(policy_violations_data) + end + end + + context 'when migration has not completed' do + before do + set_elasticsearch_migration_to(:add_policy_violations_field_to_vulnerability, including: false) + end + + it 'returns schema version with token_status' do + expect(indexed_json[:schema_version]).to eq(25_37) + end + + it 'does not assign token_status on the indexed json', :aggregate_failures do + expect(indexed_json[:policy_violations]).to be_nil + end + end + end + context 'when all migrations have completed' do it 'returns the current schema version' do expect(indexed_json[:schema_version]).to eq(described_class::SCHEMA_VERSION) -- GitLab From 5034b66e59125033fef9ad4ab4ce65f89dc8dc6e Mon Sep 17 00:00:00 2001 From: mc_rocha Date: Wed, 8 Oct 2025 10:05:19 -0400 Subject: [PATCH 2/5] Address reviewer suggestion --- .../preloaders/vulnerability/policy_violations.rb | 10 +++++++--- ee/lib/search/elastic/references/vulnerability.rb | 2 +- ee/lib/security/policy_violations.rb | 4 +++- .../preloaders/vulnerability/policy_violations_spec.rb | 6 +++--- .../search/elastic/references/vulnerability_spec.rb | 2 +- 5 files changed, 15 insertions(+), 9 deletions(-) diff --git a/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb b/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb index f9645efa07605f..c3eacc72f44c10 100644 --- a/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb +++ b/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb @@ -20,13 +20,17 @@ def fetch_policy_violations_data(security_findings_uuids) dismissed_uuids = ::Security::PolicyDismissal .for_projects(project_ids) .for_security_findings_uuids(security_findings_uuids) - .flat_map(&:security_findings_uuids).uniq + .flat_map(&:security_findings_uuids).uniq.to_set records.each_with_object({}) do |record, result| - result[record.vulnerability_id] = - dismissed_uuids.include?(record.uuid) ? ::Security::PolicyViolations::DISMISSED_IN_MR : nil + result[record.vulnerability_id] = policy_violation(dismissed_uuids, record.uuid) end end + + def policy_violation(dismissed_uuids, uuid) + is_dismissed = dismissed_uuids.include?(uuid) + is_dismissed ? ::Security::PolicyViolations::DISMISSED_IN_MR : ::Security::PolicyViolations::NOT_APPLICABLE + end end end end diff --git a/ee/lib/search/elastic/references/vulnerability.rb b/ee/lib/search/elastic/references/vulnerability.rb index 32e30f304ad77d..e786abf315de79 100644 --- a/ee/lib/search/elastic/references/vulnerability.rb +++ b/ee/lib/search/elastic/references/vulnerability.rb @@ -7,7 +7,7 @@ class Vulnerability < Reference include Search::Elastic::Concerns::DatabaseReference include ::Gitlab::Utils::StrongMemoize - SCHEMA_VERSION = 25_38 + SCHEMA_VERSION = 25_42 DOC_TYPE = 'vulnerability' INDEX_NAME = 'vulnerabilities' diff --git a/ee/lib/security/policy_violations.rb b/ee/lib/security/policy_violations.rb index e22b6bf2d30276..9f420611d0c2c9 100644 --- a/ee/lib/security/policy_violations.rb +++ b/ee/lib/security/policy_violations.rb @@ -2,9 +2,11 @@ module Security module PolicyViolations - DISMISSED_IN_MR = 0 + NOT_APPLICABLE = 0 + DISMISSED_IN_MR = 1 VIOLATIONS_TYPES = { + not_applicable: NOT_APPLICABLE, dismissed_in_mr: DISMISSED_IN_MR }.freeze end diff --git a/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb b/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb index aae7424fd45758..7673e33ca524cd 100644 --- a/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb +++ b/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb @@ -31,8 +31,8 @@ def create_read_with_policy_violations(has_violations, project) vulnerability_dismissed_in_mr = reads_by_policy_violation_type[:dismissed_in_mr].vulnerability_id vulnerability_without_dismissal = reads_by_policy_violation_type[:no_violations].vulnerability_id - expect(result[vulnerability_dismissed_in_mr]).to eq(described_class::VIOLATIONS_TYPES[:dismissed_in_mr]) - expect(result[vulnerability_without_dismissal]).to be_nil + expect(result[vulnerability_dismissed_in_mr]).to eq(Security::PolicyViolations::DISMISSED_IN_MR) + expect(result[vulnerability_without_dismissal]).to eq(Security::PolicyViolations::NOT_APPLICABLE) end end @@ -49,7 +49,7 @@ def create_read_with_policy_violations(has_violations, project) let(:records) { [reads_by_policy_violation_type[:dismissed_in_mr]] } before do - allow(::Security::PolicyDismissal).to receive(:by_security_findings_uuids).and_raise(StandardError, + allow(::Security::PolicyDismissal).to receive(:for_projects).and_raise(StandardError, 'Database error') allow(::Gitlab::ErrorTracking).to receive(:track_exception) end diff --git a/ee/spec/lib/search/elastic/references/vulnerability_spec.rb b/ee/spec/lib/search/elastic/references/vulnerability_spec.rb index eb9f4a7331b435..045b1c2ba5834f 100644 --- a/ee/spec/lib/search/elastic/references/vulnerability_spec.rb +++ b/ee/spec/lib/search/elastic/references/vulnerability_spec.rb @@ -259,7 +259,7 @@ end it 'returns schema version' do - expect(indexed_json[:schema_version]).to eq(25_38) + expect(indexed_json[:schema_version]).to eq(25_42) end it 'includes policy_violations in the indexed json' do -- GitLab From 7764d7549d764bb6abde094de23cc9c505c7b1f7 Mon Sep 17 00:00:00 2001 From: mc_rocha Date: Fri, 10 Oct 2025 20:13:12 -0400 Subject: [PATCH 3/5] Address reviewer suggestion --- .../elastic/references/vulnerability.rb | 19 ++++++++------ .../elastic/references/vulnerability_spec.rb | 26 ++++++++++++++++++- 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/ee/lib/search/elastic/references/vulnerability.rb b/ee/lib/search/elastic/references/vulnerability.rb index e786abf315de79..5fc54783d40e3a 100644 --- a/ee/lib/search/elastic/references/vulnerability.rb +++ b/ee/lib/search/elastic/references/vulnerability.rb @@ -70,7 +70,6 @@ def serialize self.class.join_delimited([klass, identifier, routing].compact) end - # rubocop: disable Metrics/AbcSize -- TODO # Generate the JSON representation for elasticsearch indexing override :as_indexed_json def as_indexed_json @@ -82,12 +81,7 @@ def as_indexed_json fields[field] = database_record.method(:"#{field}_before_type_cast").call end - # Add other fields - fields["scanner_external_id"] = database_record.scanner&.external_id - fields["created_at"] = database_record.vulnerability.created_at - fields["updated_at"] = database_record.vulnerability.updated_at - fields["traversal_ids"] = database_record.project.namespace.elastic_namespace_ancestry - fields["epss_scores"] = fetch_record_attribute(database_record, :epss_scores) + fields.merge!(build_other_fields) if reachability_migration_finished? fields["reachability"] = fetch_record_attribute(database_record, :reachability) @@ -108,7 +102,6 @@ def as_indexed_json internal_es_fields.merge(fields) end - # rubocop: enable Metrics/AbcSize override :index_name def index_name @@ -125,6 +118,16 @@ def database_record private + def build_other_fields + { + scanner_external_id: database_record.scanner&.external_id, + created_at: database_record.vulnerability.created_at, + updated_at: database_record.vulnerability.updated_at, + traversal_ids: database_record.project.namespace.elastic_namespace_ancestry, + epss_scores: fetch_record_attribute(database_record, :epss_scores) + } + end + def fetch_record_attribute(record, attribute) return record.method(attribute).call if record.respond_to?(attribute) diff --git a/ee/spec/lib/search/elastic/references/vulnerability_spec.rb b/ee/spec/lib/search/elastic/references/vulnerability_spec.rb index 045b1c2ba5834f..edd0a00f9d6227 100644 --- a/ee/spec/lib/search/elastic/references/vulnerability_spec.rb +++ b/ee/spec/lib/search/elastic/references/vulnerability_spec.rb @@ -386,6 +386,21 @@ end end + context 'with policy_violations data' do + before do + create(:policy_dismissal, project: project, + security_findings_uuids: [vulnerability_read.uuid]) + end + + it 'preloads policy_violations' do + described_class.preload_indexing_data(refs) + + expect(vulnerability_ref.database_record.policy_violations).to eq(::Security::PolicyViolations::DISMISSED_IN_MR) + expect(user_vulnerability_ref.database_record.policy_violations).to eq( + ::Security::PolicyViolations::NOT_APPLICABLE) + end + end + context 'for database_record' do it 'without preloading returns an instance of model class' do expect(vulnerability_ref.database_record).to be_an_instance_of(described_class.model_klass) @@ -398,8 +413,10 @@ end end - context 'when checking for N+1 queries' do + context 'when checking for N+1 queries', :request_store do it 'does not have N+1 queries when preloading multiple references' do + stub_env('GITALY_DISABLE_REQUEST_LIMITS', 'true') + # Create initial test data project1 = create(:project, group: group) project2 = create(:project, namespace: create(:namespace)) @@ -436,6 +453,10 @@ occurrence: sbom_occurrence2, vulnerability_id: vulnerability_read2.vulnerability_id) + # Create Policy Dismissal occurrences for policy_violations data + create(:policy_dismissal, project: project1, security_findings_uuids: [vulnerability_read1.uuid]) + create(:policy_dismissal, project: project2, security_findings_uuids: [vulnerability_read2.uuid]) + # Create refs array for initial batch refs = [ described_class.new(vulnerability_read1.vulnerability_id, vulnerability_read1.es_parent) @@ -469,6 +490,9 @@ occurrence: sbom_occurrence3, vulnerability_id: vulnerability_read3.vulnerability_id) + # Create Policy Dismissal occurrences for policy_violations data + create(:policy_dismissal, project: project3, security_findings_uuids: [vulnerability_read3.uuid]) + # Add new refs to the array refs += [ described_class.new(vulnerability_read2.vulnerability_id, vulnerability_read2.es_parent), -- GitLab From ff469181594fc76bd26a8ce5ad0e118de2c5de6f Mon Sep 17 00:00:00 2001 From: mc_rocha Date: Tue, 14 Oct 2025 10:13:21 -0400 Subject: [PATCH 4/5] Address reviewers suggestions --- ee/app/models/security/policy_dismissal.rb | 1 + .../vulnerability/policy_violations.rb | 2 +- .../vulnerability/policy_violations_spec.rb | 43 +++++++++++++++++-- .../elastic/references/vulnerability_spec.rb | 3 +- 4 files changed, 43 insertions(+), 6 deletions(-) diff --git a/ee/app/models/security/policy_dismissal.rb b/ee/app/models/security/policy_dismissal.rb index 9a80f64b9140f2..81461ae5e580f8 100644 --- a/ee/app/models/security/policy_dismissal.rb +++ b/ee/app/models/security/policy_dismissal.rb @@ -25,6 +25,7 @@ class PolicyDismissal < ApplicationRecord scope :for_security_findings_uuids, ->(security_findings_uuids) do where("security_findings_uuids && ARRAY[?]::text[]", security_findings_uuids) end + scope :pluck_security_findings_uuid, -> { pluck(Arel.sql('DISTINCT unnest(security_findings_uuids)')) } # rubocop: disable Database/AvoidUsingPluckWithoutLimit -- pluck limited to batch size in ee/lib/search/elastic/references/vulnerability.rb#preload_indexing_data scope :including_merge_request_and_user, -> { includes(:user, :merge_request) } state_machine :status, initial: :open do diff --git a/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb b/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb index c3eacc72f44c10..443c895f2ba3fd 100644 --- a/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb +++ b/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb @@ -20,7 +20,7 @@ def fetch_policy_violations_data(security_findings_uuids) dismissed_uuids = ::Security::PolicyDismissal .for_projects(project_ids) .for_security_findings_uuids(security_findings_uuids) - .flat_map(&:security_findings_uuids).uniq.to_set + .pluck_security_findings_uuid.to_set records.each_with_object({}) do |record, result| result[record.vulnerability_id] = policy_violation(dismissed_uuids, record.uuid) diff --git a/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb b/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb index 7673e33ca524cd..b438b705c8e4f4 100644 --- a/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb +++ b/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb @@ -49,8 +49,7 @@ def create_read_with_policy_violations(has_violations, project) let(:records) { [reads_by_policy_violation_type[:dismissed_in_mr]] } before do - allow(::Security::PolicyDismissal).to receive(:for_projects).and_raise(StandardError, - 'Database error') + allow(::Security::PolicyDismissal).to receive(:for_projects).and_raise(StandardError, 'Database error') allow(::Gitlab::ErrorTracking).to receive(:track_exception) end @@ -59,9 +58,47 @@ def create_read_with_policy_violations(has_violations, project) expect(result).to eq({}) expect(::Gitlab::ErrorTracking).to have_received(:track_exception) - .with(instance_of(StandardError), class: described_class.name) + .with(instance_of(StandardError), class: described_class.name) end end end + + describe 'performance characteristics' do + let_it_be(:large_vulnerability_set) do + create_list(:vulnerability, 5, :with_read, project: project).map(&:vulnerability_read) + end + + let_it_be(:extra_vulnerability_set) do + create_list(:vulnerability, 2, :with_read, project: project).map(&:vulnerability_read) + end + + it 'performs efficient batch querying' do + preloader = described_class.new(large_vulnerability_set) + + expect { preloader.preload }.not_to exceed_query_limit(3) + end + + it 'avoids N+1 queries with larger datasets' do + # Setup some vulnerabilities with policy dismissals + large_vulnerability_set.first(3).each do |vulnerability_read| + create(:policy_dismissal, project: project, security_findings_uuids: [vulnerability_read.uuid]) + end + + preloader = described_class.new(large_vulnerability_set) + control_count = count_queries { preloader.preload } + + # Adding more records should not significantly increase query count + larger_set = large_vulnerability_set + extra_vulnerability_set + larger_preloader = described_class.new(larger_set) + + expect { larger_preloader.preload }.not_to exceed_query_limit(control_count + 1) + end + end + end + + private + + def count_queries(&block) + ActiveRecord::QueryRecorder.new(&block).count end end diff --git a/ee/spec/lib/search/elastic/references/vulnerability_spec.rb b/ee/spec/lib/search/elastic/references/vulnerability_spec.rb index edd0a00f9d6227..cc8585b7ac9580 100644 --- a/ee/spec/lib/search/elastic/references/vulnerability_spec.rb +++ b/ee/spec/lib/search/elastic/references/vulnerability_spec.rb @@ -388,8 +388,7 @@ context 'with policy_violations data' do before do - create(:policy_dismissal, project: project, - security_findings_uuids: [vulnerability_read.uuid]) + create(:policy_dismissal, project: project, security_findings_uuids: [vulnerability_read.uuid]) end it 'preloads policy_violations' do -- GitLab From d3cd0a8319979e085c43351473432b35990d8b41 Mon Sep 17 00:00:00 2001 From: mc_rocha Date: Wed, 15 Oct 2025 10:08:12 -0400 Subject: [PATCH 5/5] Update policy dismissal lookup query --- .../elastic/preloaders/vulnerability/policy_violations.rb | 1 + ee/spec/factories/security/policy_dismissal.rb | 4 ++++ .../preloaders/vulnerability/policy_violations_spec.rb | 7 +++++-- .../lib/search/elastic/references/vulnerability_spec.rb | 8 ++++---- 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb b/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb index 443c895f2ba3fd..4e7c20da3ea01e 100644 --- a/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb +++ b/ee/lib/search/elastic/preloaders/vulnerability/policy_violations.rb @@ -19,6 +19,7 @@ def fetch_policy_violations_data(security_findings_uuids) dismissed_uuids = ::Security::PolicyDismissal .for_projects(project_ids) + .with_status(:preserved) .for_security_findings_uuids(security_findings_uuids) .pluck_security_findings_uuid.to_set diff --git a/ee/spec/factories/security/policy_dismissal.rb b/ee/spec/factories/security/policy_dismissal.rb index 9e5cd02039490c..e607660bb09446 100644 --- a/ee/spec/factories/security/policy_dismissal.rb +++ b/ee/spec/factories/security/policy_dismissal.rb @@ -8,5 +8,9 @@ user dismissal_types { Security::PolicyDismissal::DISMISSAL_TYPES.values.sample(2) } security_findings_uuids { [SecureRandom.uuid] } + + trait :preserved do + status { 1 } + end end end diff --git a/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb b/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb index b438b705c8e4f4..2528ff1a8f288b 100644 --- a/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb +++ b/ee/spec/lib/search/elastic/preloaders/vulnerability/policy_violations_spec.rb @@ -14,7 +14,10 @@ def create_read_with_policy_violations(has_violations, project) vulnerability = create(:vulnerability, :with_finding, project: project) - create(:policy_dismissal, project: project, security_findings_uuids: [vulnerability.finding.uuid]) if has_violations + if has_violations + create(:policy_dismissal, :preserved, project: project, + security_findings_uuids: [vulnerability.finding.uuid]) + end vulnerability.vulnerability_read end @@ -81,7 +84,7 @@ def create_read_with_policy_violations(has_violations, project) it 'avoids N+1 queries with larger datasets' do # Setup some vulnerabilities with policy dismissals large_vulnerability_set.first(3).each do |vulnerability_read| - create(:policy_dismissal, project: project, security_findings_uuids: [vulnerability_read.uuid]) + create(:policy_dismissal, :preserved, project: project, security_findings_uuids: [vulnerability_read.uuid]) end preloader = described_class.new(large_vulnerability_set) diff --git a/ee/spec/lib/search/elastic/references/vulnerability_spec.rb b/ee/spec/lib/search/elastic/references/vulnerability_spec.rb index cc8585b7ac9580..827b9967064978 100644 --- a/ee/spec/lib/search/elastic/references/vulnerability_spec.rb +++ b/ee/spec/lib/search/elastic/references/vulnerability_spec.rb @@ -388,7 +388,7 @@ context 'with policy_violations data' do before do - create(:policy_dismissal, project: project, security_findings_uuids: [vulnerability_read.uuid]) + create(:policy_dismissal, :preserved, project: project, security_findings_uuids: [vulnerability_read.uuid]) end it 'preloads policy_violations' do @@ -453,8 +453,8 @@ vulnerability_id: vulnerability_read2.vulnerability_id) # Create Policy Dismissal occurrences for policy_violations data - create(:policy_dismissal, project: project1, security_findings_uuids: [vulnerability_read1.uuid]) - create(:policy_dismissal, project: project2, security_findings_uuids: [vulnerability_read2.uuid]) + create(:policy_dismissal, :preserved, project: project1, security_findings_uuids: [vulnerability_read1.uuid]) + create(:policy_dismissal, :preserved, project: project2, security_findings_uuids: [vulnerability_read2.uuid]) # Create refs array for initial batch refs = [ @@ -490,7 +490,7 @@ vulnerability_id: vulnerability_read3.vulnerability_id) # Create Policy Dismissal occurrences for policy_violations data - create(:policy_dismissal, project: project3, security_findings_uuids: [vulnerability_read3.uuid]) + create(:policy_dismissal, :preserved, project: project3, security_findings_uuids: [vulnerability_read3.uuid]) # Add new refs to the array refs += [ -- GitLab