Skip to content

Investigate possible N+1 issue with PipelineSecurityReportFindingType vulnerabilities field

Summary

There is a possible N+1 problem with the PipelineSecurityReportFindingType.vulnerabilities field, as discussed !107491 (comment 1219189652) and related to Define the safe/correct way to write N+1 query ... (#385205). We need to check the actual results in the query and not depend on the QueryRecorder testing.

Steps to reproduce

Run the group of tests for the vulnerabilities field in ee/spec/graphql/types/pipeline_security_report_finding_type_spec.rb with logging or jumping into the code with binding.pry to check the query recorder logs.

Example Project

What is the current bug behavior?

What is the expected correct behavior?

Relevant logs and/or screenshots

Output of checks

Results of GitLab environment info

Expand for output related to GitLab environment info

(For installations with omnibus-gitlab package run and paste the output of:
`sudo gitlab-rake gitlab:env:info`)

(For installations from source run and paste the output of:
`sudo -u git -H bundle exec rake gitlab:env:info RAILS_ENV=production`)

Results of GitLab application Check

Expand for output related to the GitLab application check

(For installations with omnibus-gitlab package run and paste the output of: sudo gitlab-rake gitlab:check SANITIZE=true)

(For installations from source run and paste the output of: sudo -u git -H bundle exec rake gitlab:check RAILS_ENV=production SANITIZE=true)

(we will only investigate if the tests are passing)

Possible fixes

May need to add preloading to vulnerabilities batch loader.

Implementation Plan

The existing spec that tests the #merge_request field on the PipelineSecurityReportFinding type shows 21 queries from the Active::Record::QueryRecorder. This # seems high for what it is doing. I would expect something closer to loading each of the vulnerability feedback records in a single query (with a limit?) and then another query to find the associated merge request records as a separate query.

  • Verify usage of .all_preloaded to see if we're loading too much or too little data. (i.e. includes(:merge_request) instead of preload(:author, :comment_author, :project, :issue, :merge_request, :pipeline))
  • Check the query recorder spec to ensure that it's testing the total query count correctly.
Edited by mo khan