Replace .latest scope with .merge
What does this MR do and why?
This MR removes a redundant extra join on the security_scans
table when Security::FindingsFinder#all_security_findings
is called.
This fixes the related issue #384102 (closed) - see !105097 (comment 1186339891) for the original discussion.
Database review
Below are the relevant queries before and after these MR changes, first with deprecate_vulnerabilities_feedback
enabled and disabled. The query was generated by calling Security::FindingsFinder.new(Ci::Pipeline.last).execute
Query before and after MR with `deprecate_vulnerabilities_feedback` enabled
Before
SELECT
"security_findings".*
FROM
"security_findings"
INNER JOIN "security_scans" ON "security_findings"."scan_id" = "security_scans"."id"
INNER JOIN "security_scans" "scans_security_findings" ON "scans_security_findings"."id" = "security_findings"."scan_id"
WHERE
"security_scans"."pipeline_id" = 568
AND "security_findings"."deduplicated" = TRUE
AND "security_scans"."latest" = TRUE
AND "security_scans"."status" = 1
AND (NOT EXISTS (
SELECT
1
FROM
"vulnerabilities"
INNER JOIN "vulnerability_occurrences" ON "vulnerability_occurrences"."vulnerability_id" = "vulnerabilities"."id"
WHERE
"vulnerabilities"."state" = 2
AND (vulnerability_occurrences.uuid = security_findings.uuid::text)))
ORDER BY
"security_findings"."severity" DESC,
"security_findings"."id" ASC
LIMIT 20 OFFSET 0
After
SELECT
"security_findings".*
FROM
"security_findings"
INNER JOIN "security_scans" ON "security_findings"."scan_id" = "security_scans"."id"
WHERE
"security_scans"."pipeline_id" = 568
AND "security_findings"."deduplicated" = TRUE
AND "security_scans"."latest" = TRUE
AND "security_scans"."status" = 1
AND (NOT EXISTS (
SELECT
1
FROM
"vulnerabilities"
INNER JOIN "vulnerability_occurrences" ON "vulnerability_occurrences"."vulnerability_id" = "vulnerabilities"."id"
WHERE
"vulnerabilities"."state" = 2
AND (vulnerability_occurrences.uuid = security_findings.uuid::text)))
ORDER BY
"security_findings"."severity" DESC,
"security_findings"."id" ASC
LIMIT 20 OFFSET 0
Query before and after MR with `deprecate_vulnerabilities_feedback` disabled
Before
SELECT
"security_findings".*
FROM
"security_findings"
INNER JOIN "security_scans" ON "security_findings"."scan_id" = "security_scans"."id"
INNER JOIN "security_scans" "scans_security_findings" ON "scans_security_findings"."id" = "security_findings"."scan_id"
WHERE
"security_scans"."pipeline_id" = 568
AND "security_findings"."deduplicated" = TRUE
AND "security_scans"."latest" = TRUE
AND "security_scans"."status" = 1
AND (NOT EXISTS (
SELECT
1
FROM
"security_scans"
INNER JOIN "projects" ON "projects"."id" = "security_scans"."project_id"
INNER JOIN "vulnerability_feedback" ON "vulnerability_feedback"."project_id" = "projects"."id"
WHERE (vulnerability_feedback.category = (security_scans.scan_type - 1))
AND "vulnerability_feedback"."feedback_type" = 0
AND (security_scans.id = security_findings.scan_id)
AND (vulnerability_feedback.finding_uuid = security_findings.uuid)))
ORDER BY
"security_findings"."severity" DESC,
"security_findings"."id" ASC
LIMIT 20 OFFSET 0
After
SELECT
"security_findings".*
FROM
"security_findings"
INNER JOIN "security_scans" ON "security_findings"."scan_id" = "security_scans"."id"
WHERE
"security_scans"."pipeline_id" = 568
AND "security_findings"."deduplicated" = TRUE
AND "security_scans"."latest" = TRUE
AND "security_scans"."status" = 1
AND (NOT EXISTS (
SELECT
1
FROM
"security_scans"
INNER JOIN "projects" ON "projects"."id" = "security_scans"."project_id"
INNER JOIN "vulnerability_feedback" ON "vulnerability_feedback"."project_id" = "projects"."id"
WHERE (vulnerability_feedback.category = (security_scans.scan_type - 1))
AND "vulnerability_feedback"."feedback_type" = 0
AND (security_scans.id = security_findings.scan_id)
AND (vulnerability_feedback.finding_uuid = security_findings.uuid)))
ORDER BY
"security_findings"."severity" DESC,
"security_findings"."id" ASC
LIMIT 20 OFFSET 0
In both cases above, the only difference between the 2 queries is the removal of the redundant join.
--- before-ff-enabled.sql 2023-01-19 11:56:58.054630035 +1300
+++ after-ff-enabled.sql 2023-01-19 11:59:48.840539552 +1300
@@ -3,7 +3,6 @@
FROM
"security_findings"
INNER JOIN "security_scans" ON "security_findings"."scan_id" = "security_scans"."id"
- INNER JOIN "security_scans" "scans_security_findings" ON "scans_security_findings"."id" = "security_findings"."scan_id"
WHERE
"security_scans"."pipeline_id" = 568
AND "security_findings"."deduplicated" = TRUE
--- before-ff-disabled.sql 2023-01-19 12:05:29.296346186 +1300
+++ after-ff-disabled.sql 2023-01-19 12:04:15.683523130 +1300
@@ -3,7 +3,6 @@
FROM
"security_findings"
INNER JOIN "security_scans" ON "security_findings"."scan_id" = "security_scans"."id"
- INNER JOIN "security_scans" "scans_security_findings" ON "scans_security_findings"."id" = "security_findings"."scan_id"
WHERE
"security_scans"."pipeline_id" = 568
AND "security_findings"."deduplicated" = TRUE
How to set up and validate locally
- Find a pipeline
pipeline = Ci::Pipeline.last
- Enable
deprecate_vulnerabilities_feedback
for the pipeline projectFeature.enable(:deprecate_vulnerabilities_feedback, pipeline.project)
- Execute the finder to trigger the query and examine the logged SQL
Security::FindingsFinder.new(pipeline).execute
- Disable
deprecate_vulnerabilities_feedback
for the pipeline projectFeature.disable(:deprecate_vulnerabilities_feedback, pipeline.project)
- Repeat step 3.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #384102 (closed)