What does this MR do and why?

Do not eager load the app when running CI specs

By disabling eager loading, we don't need to read all 20k ruby files in the app before running any one test which should speed up the CI pipeline.

References

Screenshots or screen recordings

How to set up and validate locally

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Brian Williams (8e5ba831) at 18 Mar 12:23

@bala.kumar would you mind doing the initial review for this change?

I'm not sure why it is common convention to stub this. I first tried using factories but it did not work.

diff --git a/ee/spec/lib/search/elastic/references/vulnerability_spec.rb b/ee/spec/lib/search/elastic/references/vulnerability_spec.rb
index c626a1ba90e2..0b0c204e9b13 100644
--- a/ee/spec/lib/search/elastic/references/vulnerability_spec.rb
+++ b/ee/spec/lib/search/elastic/references/vulnerability_spec.rb
@@ -87,7 +87,7 @@
         type: described_class::DOC_TYPE,
         schema_version: described_class::SCHEMA_VERSION,
         security_project_tracked_context_id: object.security_project_tracked_context_id,
-        is_default: []
+        is_default: object.tracked_context&.is_default
       }
     end
 
@@ -373,13 +373,11 @@
 
     context 'with is_default migration mappings' do
       context 'when is_default migration has finished' do
-        let(:is_default_value) { true }
+        let(:tracked_context) { create(:security_project_tracked_context, :default) }
 
         before do
           set_elasticsearch_migration_to(:add_is_default_to_vulnerability)
-
-          allow(object).to receive(:is_default).and_return(is_default_value)
-          allow(vulnerability_reference_object).to receive(:database_record).and_return(object)
+          object.update!(security_project_tracked_context_id: tracked_context.id)
         end
 
         it 'returns schema version' do
@@ -389,14 +387,6 @@
         it 'includes is_default in the indexed json' do
           expect(indexed_json[:is_default]).to be(true)
         end
-
-        context 'when is_default is false' do
-          let(:is_default_value) { false }
-
-          it 'indexes false' do
-            expect(indexed_json[:is_default]).to be(false)
-          end
-        end
       end
 
       context 'when is_default migration has not completed' do
@@ -409,7 +399,7 @@
         end
 
         it 'does not assign is_default on the indexed json' do
-          expect(indexed_json[:is_default]).to be_blank
+          expect(indexed_json[:is_default]).to be_nil
         end
       end
 
@@ -420,7 +410,7 @@
         end
 
         it 'defaults is_default to false' do
-          expect(indexed_json[:is_default]).to be_blank
+          expect(indexed_json[:is_default]).to be_nil
         end
       end
     end

fetch_record_attribute returns [] if there is now value. Why is this? 🤔

Brian Williams (ec0d3c9f) at 17 Mar 23:48

@mclausen35 Just documenting what we discussed on our last sync call here: Vulnerabilities with at least one status change is the desired indicator, but we won't have status changes in-scope for the first closed beta release. Given that, we'll be recording metrics on the filter interactions for now.

@srushik Thanks for confirming that there are vulnerabilities on both contexts 😞

@Quintasan In scenario B the vulnerability history such as status changes would disappear which I think is a problem. Additionally, I don't think we have foreign keys on security_project_tracked_context_id for these tables yet so we would have to manually cascade the deletes. At that point, deleting is about the same level of effort as migrating.

Brian Williams (3613b35d) at 17 Mar 22:26

... and 705 more commits

Addressed in !227354 (merged).

Brian Williams (79e75024) at 17 Mar 22:00

... and 1 more commit

Brian Williams (75c1d5cd) at 17 Mar 22:00

What does this MR do and why?

These fields have a large cyclomatic complexity that continues growing as more fields are added. This change switches to a declarative pattern so that we do not have duplicated methods for each field, and can more easily reason about the business logic.

References

Relates to: #593575

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Brian Williams (effdbfba) at 17 Mar 20:45

... and 1 more commit

Brian Williams (40578357) at 17 Mar 20:45

What does this MR do and why?

Create new preloader namespace VulnerabilityRead with preloaders that key data by vulnerability_occurrence_id instead of vulnerability_id, for use with the new vulnerability_reads Elasticsearch index. New preloaders:

• EnhancedProxy: coordinator indexed by record[:id]
• FalsePositive: queries findings by occurrence ID
• PolicyAutoDismissed: keys results by occurrence ID
• PolicyViolations: keys results by occurrence ID
• Reachability: fetches via occurrence_id association
• RiskScore: queries findings by occurrence ID
• TokenStatus: queries findings by occurrence ID
• UndetectedSince: queries findings by occurrence ID Also adds filter_by_vulnerability_occurrence_id scope to Sbom::Occurrence and updates the vulnerability_read factory to include a vulnerability_occurrence."

DB Review

The preloaders for the old reference class are located at https://gitlab.com/gitlab-org/gitlab/-/tree/master/ee/lib/search/elastic/preloaders/vulnerability. The new preloaders use vulnerability_occurrence_id directly. So not a lot of change w.r.t to DB queries. These have been tested on the old preloaders. But I've listed down all the queries and their plans if anything has changed in the new preloader

1. FalsePositive

::Vulnerabilities::Finding.by_vulnerability changed to ::Vulnerabilities::Finding.id_in

https://postgres.ai/console/gitlab/gitlab-production-sec/sessions/49845/commands/148283

2. PolicyAutoDismissed

No change.

3. PolicyViolations

No change.

4. Reachability

::Sbom::Occurrence.filter_by_vulnerability_id changed to ::Sbom::Occurrence.filter_by_vulnerability_occurrence_id

https://postgres.ai/console/gitlab/gitlab-production-sec/sessions/49845/commands/148285

5. RiskScore

::Vulnerabilities::Finding.by_vulnerability changed to ::Vulnerabilities::Finding.id_in

https://postgres.ai/console/gitlab/gitlab-production-sec/sessions/49845/commands/148286

6. TokenStatus

::Vulnerabilities::Finding.by_vulnerability changed to ::Vulnerabilities::Finding.id_in

https://postgres.ai/console/gitlab/gitlab-production-sec/sessions/49845/commands/148287

7. UndetectedSince

::Vulnerabilities::Finding.by_vulnerability changed to ::Vulnerabilities::Finding.id_in

https://postgres.ai/console/gitlab/gitlab-production-sec/sessions/49845/commands/148288

References

#591562

Screenshots or screen recordings

How to set up and validate locally

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

What does this MR do and why?

This MR adds a new consistency check (OrphanedContextIdsCheck) to identify and clean up vulnerability-related records that reference orphaned security_project_tracked_context_id values.

The check targets four models:

• Vulnerabilities::Read
• Vulnerabilities::Finding
• Vulnerabilities::Statistic
• Vulnerabilities::HistoricalStatistic

When records in these models reference a security_project_tracked_context_id that no longer exists in the project's tracked contexts, they become orphaned and should be removed to maintain data consistency.

This is part of the consistency check orchestrator work to ensure data integrity in the vulnerability management system.

References

Relates to: #589837

SQL

How to set up and validate locally

1. Create a project with vulnerability records
2. Manually create orphaned records by setting invalid security_project_tracked_context_id values
3. Run the consistency check: Vulnerabilities::ConsistencyChecks::OrphanedContextIdsCheck.new(project).fix!
4. Verify that orphaned records are deleted while valid records remain

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.``

Thanks @terrichu! I addressed your comments. Could you please have another look?

Agreed! Changed in 39847a13.