ruby: Adjust qhelp and query to avoid async calls (also rename)

yoff · yoff · commit d85366e9a25a · 2025-01-29T23:26:27.000+01:00
diff --git a/ruby/ql/src/queries/performance/CouldBeHoisted.qhelp b/ruby/ql/src/queries/performance/CouldBeHoisted.qhelp
@@ -11,14 +11,13 @@ In those cases, where the query includes a <code>pluck</code> call, the query co
 </p>
 </overview>
 <recommendation>
-<p>If possible, split the loop into two. The first creates a map of promises resolving to the async query results. The second runs throuhg this map, waiting on each promise, and does whatever the original loop did with the result of the query.</p>
+<p>If possible, pull the query out of the loop, thus replacing the many calls with a single one.
+</p>
 </recommendation>
 <example>
-<p>The following (suboptimal) example code executes a series of ActiveRecord queries in a loop. The queries are independent of each other, so they could be executed in parallel to improve performance.</p>
+<p>The following (suboptimal) example code queries the User object in each iteration of the loop:</p>
 <sample src="examples/straight_loop.rb" />
-<p>To be able to fetch the necessary information asynchronously, we first pull it out into its own (implicit) loop:
+<p>To improve the performance, we instead query the User object once outside the loop, gathereing all necessary information:
 <sample src="examples/preload.rb" />
-<p>We can now use the <code>async_pluck</code> method to execute the queries in parallel.</p>
-<sample src="examples/async_pluck.rb" />
 </example>
 </qhelp>
diff --git a/ruby/ql/src/queries/performance/CouldBeHoisted.ql b/ruby/ql/src/queries/performance/CouldBeHoisted.ql
@@ -1,10 +1,10 @@
 /**
- * @name Could be async
- * @description Use `ActiveRecord::Relation#load_async` to load records asynchronously.
+ * @name Could be hoisted
+ * @description Hoist Rails `ActiveRecord::Relation` query calls out of loops.
  * @kind problem
  * @problem.severity info
  * @precision high
- * @id rb/could-be-async
+ * @id rb/could-be-hoisted
  * @tags performance
  */
 
@@ -82,18 +82,14 @@ DataFlow::Node guardForLoopControl(ConditionalExpr cond, Stmt control) {
   control = cond.getBranch(_).getAChild()
 }
 
-from LoopingCall loop, DataFlow::CallNode call, string message
+from LoopingCall loop, DataFlow::CallNode call
 where
+  // TODO: Filter loops over constants
   not call.getLocation().getFile().getAbsolutePath().matches("%test%") and
-  not call = any(PluckCall p).chaines() and
+  // not call = any(PluckCall p).chaines() and
   not usedInLoopControlGuard(call, _) and
   happensInInnermostLoop(loop, call) and
-  (
-    call instanceof ActiveRecordModelFinderCall and
-    not call.getMethodName() in ["new", "create"] and
-    message = "could be chained with load_async"
-    or
-    call instanceof PluckCall and
-    message = "could be async_pluck"
-  )
-select call, "This call happens inside $@, and " + message, loop, "this loop"
+  call instanceof ActiveRecordModelFinderCall and
+  not call.getMethodName() in ["new", "create"] //and
+// call.getLocation().getFile().getAbsolutePath().matches("%app/models/stafftools/%")
+select call, "This call happens inside $@, and could be hoisted.", loop, "this loop"
diff --git a/ruby/ql/src/queries/performance/examples/preload.rb b/ruby/ql/src/queries/performance/examples/preload.rb
@@ -1,14 +1,14 @@
 # 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
+  [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
diff --git a/ruby/ql/src/queries/performance/rb-could-be-hoisted.md b/ruby/ql/src/queries/performance/rb-could-be-hoisted.md
@@ -2,11 +2,11 @@
 
 ## Description
 
-When an AcriveRecord query is executed in a loop, it potentially an n+1 problem.
+When a Rails ActiveRecord query is executed in a loop, it is potentially an n+1 problem.
 This query identifies situations where an ActiveRecord query execution could be pulled out of a loop.
 
 ## Recommendation
-Pull the query out of the loop, thus replacing the many calls with a single one.
+If possible, pull the query out of the loop, thus replacing the many calls with a single one.
 
 ## Examples
 
@@ -31,8 +31,8 @@ To improve the performance, we instead query the User object once outside the lo
 ```ruby
 # 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
+  [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]
