1. 概要
日課で@y_yagiさんのrails commit log流し読みを読んでいるのですが、
コミットの内容を読んでみて、どうしてその修正でバグが直るのかわからないものがありました
そこで、その修正内容をしっかりと理解する為に調査したりしたので、どうやって理解するに至ったかなどを書いていきたいと思います
今回読んでいたエントリはこちらです
issue
github.com
該当PR
github.com
修正内容を抜粋
2. PRを理解するためにやってみたこと
1. まず、PRのコメントを読んでみる
Fix merging relation that order including `?`
The `Relation::Merger` has a problem that order values would be merged
as nested array.
That was caused an issue #33664 since if array value is passed to
`order` and first element in the array includes `?`, the array is
regarded as a prepared statement and bind variables.
https://api.rubyonrails.org/classes/ActiveRecord/Sanitization/ClassMethods.html#method-i-sanitize_sql_for_order
Just merging that as splat args like other values would fix the issue.
Fixes #33664.
ネストしたArrayで最初の値に'?'が含まれている場合、prepared statementとみなされてエラーとなるというバグとのことでした
issueには再現コードも書かれていました
github.com
どういったバグになるのかが何となく理解できたところで、コードを追ってみます
2. 変数の前に*をつけるとどんな動きなのか調べてみる
修正の対象になった箇所はこちらです
def merge_multi_values
if other.reordering_value
relation.reorder!(*other.order_values)
elsif other.order_values.any?
relation.order!(*other.order_values)
end
extensions = other.extensions - relation.extensions
relation.extending!(*extensions) if extensions.any?
end
Ruby * 等でGoogleで調べてみると、こちらの記事がヒットしたので、読んでみました
alpaca.tc
変数の前に* を書くと、配列に変換してくれるようです
なんとなく修正内容を理解したところで、コードを読んでみます
3. relation.reorder! を読んでみる
rails/activerecord/lib/active_record/relation/query_methods.rb
def reorder!(*args)
preprocess_order_args(args)
self.reordering_value = true
self.order_values = args
self
end
reorder! の引数をそのまま preprocess_order_argsに渡しているので、こちらの処理を追ってみます
4. preprocess_order_args を読む
rails/activerecord/lib/active_record/relation/query_methods.rb
def preprocess_order_args(order_args)
order_args.map! do |arg|
klass.sanitize_sql_for_order(arg)
end
order_args.flatten!
@klass.enforce_raw_sql_whitelist(
order_args.flat_map { |a| a.is_a?(Hash) ? a.keys : a },
whitelist: AttributeMethods::ClassMethods::COLUMN_NAME_ORDER_WHITELIST
)
省略
end
引数を.map! して 配列の要素を sanitize_sql_for_order に渡しているようなので、こちらを読んでみます
5. sanitize_sql_for_order を読む
PRのコメントに書いてあったエラーの原因のメソッドまでいきつきました
That was caused an issue #33664 since if array value is passed to
order and first element in the array includes ?, the array is
regarded as a prepared statement and bind variables.
https://api.rubyonrails.org/classes/ActiveRecord/Sanitization/ClassMethods.html#method-i-sanitize_sql_for_order
こちらのドキュメントを読んで、挙動を確認してみます
sanitize_sql_for_order(condition)
Accepts an array, or string of SQL conditions and sanitizes them into a valid SQL fragment for an ORDER clause.
sanitize_sql_for_order(condition)
sanitize_sql_for_order(["field(id, ?)", [1,3,2]])
sanitize_sql_for_order("id ASC")
引用：Ruby on Rails 5.2.1#sanitize_sql_for_order
そしてコードを読んでみると、
rails/activerecord/lib/active_record/sanitization.rb
def sanitize_sql_for_order(condition)
if condition.is_a?(Array) && condition.first.to_s.include?("?")
enforce_raw_sql_whitelist([condition.first],
whitelist: AttributeMethods::ClassMethods::COLUMN_NAME_ORDER_WHITELIST
)
if condition.first.kind_of?(String) && !condition.first.instance_of?(String)
condition = [String.new(condition.first), *condition[1..-1]]
end
Arel.sql(sanitize_sql_array(condition))
else
condition
end
end
conditionが配列かつ要素の最初の値に'?'が含まれている場合に、prepared statementと判断し、処理をやってくれるみたいです
なので、ここまで読んでみて推測すると、正しい挙動では引数conditionは
こちらの条件
if condition.is_a?(Array) && condition.first.to_s.include?("?")
には該当せず、
else
condition
end
こちらになるのが正しい挙動のようです
ここまで読んでみて仮設を立てるとpreprocess_order_args(order_args) の引数が
- バグのケースだと
[['?', '!']] が引数となり、
- 正しいケースだと
['?', '!']
となるのではないかなと思いました
しかしなぜこちらのパターン[['?', '!']] になるのかわからず困っていたのですが、
会社の先輩に相談したところこちらの記事を教えていただきました
qiita.com
変数の前に* を書くと、配列に変換してくれるではなく、
可変長引数を受け取るメソッドに、配列を渡す際に変数に* をつけないともう一回り配列で包まれてしまうというこちらの挙動ではないかとのことでした
なので、こちらの修正を行うことで
other.order_values の戻り値の配列がさらにもうひと回り配列で包まれることが無くなるということだと思われます
本当に戻り値が配列なのか確認するために、さらにコードを読んでいきます
6. other.order_values の戻り値を確認してみる
order_valuesの定義箇所を探してみようと思い def other.order_values でgrepしてみてもヒットしませんでした
activerecord/lib/active_record/relation/merger.rb ファイルの中を見てみると、lock_value, create_with_valueなどのメソッドがあり、こちらも定義箇所が見つからなかったので、define_method などでsuffixに_valueを設定し、動的にメソッド定義しているのではないかと予想し探し方を変えてみました
Merger クラス内で other を初期化している箇所を探してみると(RubyMineで⌘+B)
こちらにいきつきました
activerecord/lib/active_record/relation/merger.rb
省略
class Merger
attr_reader :relation, :values, :other
def initialize(relation, other)
@relation = relation
@values = other.values
@other = other
end
省略
Mergeクラスを初期化する際に引数でotherを入れているので、初期化処理の箇所がないか調べてみました
こちらを見てみるとotherはRelationクラスの可能性がありそうです
rails/activerecord/lib/active_record/relation/spawn_methods.rb
省略
def merge!(other)
if other.is_a?(Hash)
Relation::HashMerger.new(self, other).merge
elsif other.is_a?(Relation)
Relation::Merger.new(self, other).merge
elsif other.respond_to?(:to_proc)
instance_exec(&other)
else
raise ArgumentError, "#{other.inspect} is not an ActiveRecord::Relation"
end
end
省略
こちらのファイルを見てみると、
activerecord/lib/active_record/relation.rb
module ActiveRecord
class Relation
MULTI_VALUE_METHODS = [:includes, :eager_load, :preload, :select, :group,
:order, :joins, :left_outer_joins, :references,
:extending, :unscope]
SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :reordering,
:reverse_order, :distinct, :create_with, :skip_query_cache]
CLAUSE_METHODS = [:where, :having, :from]
INVALID_METHODS_FOR_DELETE_ALL = [:distinct, :group, :having]
VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS + CLAUSE_METHODS
include Enumerable
include FinderMethods, Calculations, SpawnMethods, QueryMethods, Batches, Explain, Delegation
定数の中にorder とあるので、この定数を利用してメソッド定義しているのではないかと予想しました
MULTI_VALUE_METHODS で検索すると
こちらでしか利用してなさそうなので、
ruby
VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS + CLAUSE_METHODS
activerecord/lib/active_record配下で、 VALUE_METHODSを使用しているところを探してみます
こちらで定義していました
rails/activerecord/lib/active_record/relation/query_methods.rb
省略
Relation::VALUE_METHODS.each do |name|
method_name = \
case name
when *Relation::MULTI_VALUE_METHODS then "#{name}_values"
when *Relation::SINGLE_VALUE_METHODS then "#{name}_value"
when *Relation::CLAUSE_METHODS then "#{name}_clause"
end
class_eval <<-CODE, __FILE__, __LINE__ + 1
def #{method_name} # def includes_values
get_value(#{name.inspect}) # get_value(:includes)
end # end
def #{method_name}=(value) # def includes_values=(value)
set_value(#{name.inspect}, value) # set_value(:includes, value)
end # end
CODE
end
省略
動的にメソッドを定義し、メソッド内部では get_value を呼び出しているので、こちらの定義箇所を読んでみます
def get_value(name)
@values.fetch(name, DEFAULT_VALUES[name])
end
fetchの第二引数を指定すると、キーが存在しない場合、デフォルト値を返すようになっているので、DEFAULT_VALUES を探してみます
DEFAULT_VALUES ハッシュにはorderが定義されていないのですが
DEFAULT_VALUES = {
create_with: FROZEN_EMPTY_HASH,
where: Relation::WhereClause.empty,
having: Relation::WhereClause.empty,
from: Relation::FromClause.empty
}
Relation::MULTI_VALUE_METHODS.each do |value|
DEFAULT_VALUES[value] ||= FROZEN_EMPTY_ARRAY
end
こちらで追加していました
Relation::MULTI_VALUE_METHODS.each do |value|
DEFAULT_VALUES[value] ||= FROZEN_EMPTY_ARRAY
end
配列をループしている内部の処理は、左辺が未定義または偽の場合に右辺の値を代入するという意味になります
Rubyの||=というイディオムは左辺が未定義または偽なら代入の意味 -- ぺけみさお
なので配列の要素のこちらをひとつずつ取り出しながら
MULTI_VALUE_METHODS = [:includes, :eager_load, :preload, :select, :group,
:order, :joins, :left_outer_joins, :references,
:extending, :unscope]
未定義の場合は、FROZEN_EMPTY_ARRAYを追加していくという処理です
DEFAULT_VALUES[value] ||= FROZEN_EMPTY_ARRAY
FROZEN_EMPTY_ARRAYは空の配列になります
FROZEN_EMPTY_ARRAY = [].freeze
なので、other.order_values の戻り値は配列になるということでした
今回のPRの修正、reoader! メソッドの引数に * をつけることで配列で包まれることなく、配列を渡せるようになったというのが修正内容でした
