Ruby: Query for database calls in a loop

[yoff]

Add query for hoisting Rails ActiveRecord calls.
It uses ActiveRecordModelFinderCall to identify relevant calls and this PR is making that class public. Please let me know if that is not acceptable.

Possible Improvements;

• We could consider Associations.
  Associations are lazy-loading by default, so something like
  in a loop over article do
  article.book
  if you have 1000 articles it will do a 1000 calls to book.
  If you already did article includes book, there should be no problem.
• We could also consider instances of ActiveRecordInstanceMethodCall, for instance
  calls to pluck. (An earlier version added exactly calls to pluck which added exactly 1 result on gh/gh; currently the query has 195 results, so the complication did not seem worth it.)

The query comes with a .qhelp and autofixes have been validated on a small set (~15) of alerts.

This query is intended for CCR, so I am not sure if we want a change note.

[github-actions]

QHelp previews:

ruby/ql/src/queries/performance/DatabaseQueryInLoop.qhelp

Database query in a loop

When a database query operation, for example a call to a query method in the Rails `ActiveRecord::Relation` class, is executed in a loop, this can lead to a performance issue known as an "n+1 query problem". The database query will be executed in each iteration of the loop. Performance can usually be improved by performing a single database query outside of a loop, which retrieves all the required objects in a single operation.

Recommendation

If possible, pull the database query out of the loop and rewrite it to retrieve all the required objects. This replaces multiple database operations with a single one.

Example

The following (suboptimal) example code queries the User object in each iteration of the loop:

repo_names_by_owner.map do |owner_slug, repo_names|
    owner_id, owner_type = User.where(login: owner_slug).pluck(:id, :type).first
    owner_type = owner_type == "User" ? "USER" : "ORGANIZATION"
    rel_conditions = { owner_id: owner_id, name: repo_names }

    nwo_rel = nwo_rel.or(RepositorySecurityCenterConfig.where(rel_conditions)) unless neg
    nwo_rel = nwo_rel.and(RepositorySecurityCenterConfig.where.not(rel_conditions)) if neg
  end

To improve the performance, we instead query the User object once outside the loop, gathering all necessary information in a single query:

# Preload User data
user_data = User.where(login: repo_names_by_owner.keys).pluck(:login, :id, :type).to_h do |login, id, type|
  [login, { id: id, type: type == "User" ? "USER" : "ORGANIZATION" }]
end

repo_names_by_owner.each do |owner_slug, repo_names|
  owner_info = user_data[owner_slug]
  owner_id = owner_info[:id]
  owner_type = owner_info[:type]
  rel_conditions = { owner_id: owner_id, name: repo_names }

  nwo_rel = nwo_rel.or(RepositorySecurityCenterConfig.where(rel_conditions)) unless neg
  nwo_rel = nwo_rel.and(RepositorySecurityCenterConfig.where.not(rel_conditions)) if neg
end

[adityasharad]

Thanks for putting this together - the general direction looks reasonable and I like the simple statement of the problem. Suggestions mainly involve documenting the logic, adding tests, and clarifying the help text for both readers and autofix.

I also recommend a PR title and description stating exactly what you are adding, so we can easily find and understand it later.
I agree you don't need a change note yet.

[adityasharad]

Let's file internal issues for both ideas.

[adityasharad]

LGTM, ready to merge when DCA is happy.

[hvitved]

Could we please use InlineExpectationsTestQuery.ql instead, like e.g. https://github.com/github/codeql/blob/main/ruby/ql/test/query-tests/security/cwe-022/PathInjection.qlref?

[yoff]

Ooh, yes!

[yoff]

Should in_batches be excluded since it seems to signal some reflection on looping.

[adityasharad]

Let's do this later.

[adityasharad]

@hvitved are you happy with test changes (Qlucie run seemed to pass, although I was surprised we didn't have to change any of the inline comments)?

[yoff]

@hvitved are you happy with test changes (Qlucie run seemed to pass, although I was surprised we didn't have to change any of the inline comments)?

Inline comments are added. The existing comments do not start with # $ so they are only read by humans :-)