Draft: Pre-fetch vulnerability remediations
What does this MR do?
Related to #323059 (closed)
Similar to !55642 (merged), this pre-fetches existing remediations for security findings. This addresses an N+1 where we would fetch these per each finding in a loop:
33 SELECT "vulnerability_remediations".* FROM "vulnerability_remediations" WHERE "vulnerability_remediations"."project_id" = $1 AND 1=0 /*application:test,correlation_id:f7a9466d04197597d406af4d407361ee*/[0m [["project_id", 2]]This is now one query.
The impact will be on a similar scale as !55642 (merged), since this was a query that fired per finding, but its impact depends on the number of reports and existing remediations.
This was a lot more difficult to do since I had to untangle the creation of vulnerability_findings from updating them in a subsequent step, where remediations will be used. So I had to flip some of this code inside out. The service now first finds-or-creates vulnerability findings, then fetches remediations in one step, then proceeds to update findings.
There are still many problems with this service, and there is in fact another N+1 that also applies to remediations, where StoreReportService will call into Vulnerabilities::CreateService which then again performs N+1s.
I think the proper way forward for how this service to operate is:
- fetch all date require for updates upfront
- perform bulk-inserts or upserts for all vulnerability findings and vulnerabilities
This is essentially a rewrite, but perhaps we can get there step by step.
Does this MR meet the acceptance criteria?
Conformity
-
📋 Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because _____.
-
-
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec -
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team