Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ruby: Query for N+1 queries problem #18317

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@
* Provides modeling for the `ActiveRecord` library.
*/

private import codeql.ruby.AST

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
codeql.ruby.dataflow.FlowSummary
.
private import codeql.ruby.Concepts
private import codeql.ruby.controlflow.CfgNodes
private import codeql.ruby.DataFlow

Check warning

Code scanning / CodeQL

Redundant import Warning

Redundant import, the module is already imported inside
codeql.ruby.dataflow.FlowSummary
.
private import codeql.ruby.dataflow.internal.DataFlowDispatch
private import codeql.ruby.ApiGraphs
private import codeql.ruby.frameworks.Stdlib
private import codeql.ruby.frameworks.Core
private import codeql.ruby.dataflow.FlowSummary

/// See https://api.rubyonrails.org/classes/ActiveRecord/Persistence.html
private string activeRecordPersistenceInstanceMethodName() {
Expand Down Expand Up @@ -634,6 +635,13 @@
result.getName().toLowerCase() = this.getTargetModelName()
}

/**
* Gets the referenced table or field as it appears in the first argument.
*
* For example, in `has_many :posts`, this is `posts`.
*/
string getName() { result = this.getArgument(0).getConstantValue().getStringlikeValue() }

/**
* Gets the (lowercase) name of the model this association targets.
* For example, in `has_many :posts`, this is `post`.
Expand Down Expand Up @@ -689,7 +697,7 @@
* do not yield ActiveRecord instances.
* https://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html
*/
private class ActiveRecordAssociationMethodCall extends DataFlow::CallNode {
class ActiveRecordAssociationMethodCall extends DataFlow::CallNode {
ActiveRecordAssociation assoc;

ActiveRecordAssociationMethodCall() {
Expand All @@ -715,7 +723,7 @@
)
}

ActiveRecordAssociation getAssociation() { result = assoc }

Check warning on line 726 in ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll

View workflow job for this annotation

GitHub Actions / qldoc

Missing QLdoc for member-predicate ActiveRecord::ActiveRecordAssociationMethodCall::getAssociation/0
}

/**
Expand Down Expand Up @@ -832,3 +840,15 @@
}
}
}

private class ConnectedTo extends SummarizedCallable {
ConnectedTo() { this = "ActiveRecord::Base.connected_to" }

override MethodCall getACallSimple() { result.getMethodName() = "connected_to" }

override predicate propagatesFlow(string input, string output, boolean preservesValue) {
input = "Argument[block].ReturnValue" and
output = "ReturnValue" and
preservesValue = true
}
}
2 changes: 1 addition & 1 deletion ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ module Kernel {
* arr.send("push", 5) # => [5]
* ```
*/
private predicate isPublicKernelMethod(string method) {
predicate isPublicKernelMethod(string method) {
method in [
"class", "clone", "frozen?", "tap", "then", "yield_self", "send", "public_send", "__send__",
"method", "public_method", "singleton_method"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>

<overview>
<p>

Performing database queries in a loop can be inefficient
compared to performing a single query up front that retrieves all the data at once.

</p>

<p>

ORMs such as ActiveRecord implicitly query the database when certain
members of an object are being accessed. But if such a member is accessed in a loop,
it can result in a separate query for each iteration. ActiveRecord
provides a way to eagerly load associated data using the <code>includes</code>
method, which can be used to speed up such queries.

</p>
</overview>

<recommendation>
<p>

In ActiveRecord, use <code>includes</code> to eagerly load associated
data that you expect to access in a loop.

</p>
</recommendation>

<example>

<p>

The following example code gets the names of all podcasts followed by a user:

</p>

<sample src="examples/ActiveRecordQueryInLoop.rb"/>

<p>

However, the call to <code>name</code> inside the loop will result in a separate
SQL query being executed to obtain the name. To speed this up, use
<code>includes</code> to eagerly load the associated data:

</p>

<sample src="examples/ActiveRecordQueryInLoopFixed.rb"/>

<p>

Now all the associated podcast names are loaded up front with a single SQL query,
and the call to <code>name</code> no longer results in a separate query.

</p>

</example>

<references>
<li>OWASP: <a href="https://www.owasp.org/index.php/Server_Side_Request_Forgery">SSRF</a></li>
<li>OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html">XSS Unvalidated Redirects and Forwards Cheat Sheet</a>.</li>
</references>
</qhelp>
Loading
Loading