Railsのコミットでわからないものを調べてみた | rails commit log流し読みを読んでみた

f:id:sktktk1230:20180726121250p:plain

1. 概要

日課で@y_yagiさんのrails commit log流し読みを読んでいるのですが、コミットの内容を読んでみて、どうしてその修正でバグが直るのかわからないものがありました
そこで、その修正内容をしっかりと理解する為に調査したりしたので、どうやって理解するに至ったかなどを書いていきたいと思います
今回読んでいたエントリはこちらです

issue

github.com

該当PR

github.com

修正内容を抜粋

f:id:sktktk1230:20180822134705p:plain

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
            # override any order specified in the original relation
            relation.reorder!(*other.order_values)
          elsif other.order_values.any?
            # merge in order_values from relation
            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

# Same as #reorder but operates on relation in-place instead of copying.
def reorder!(*args) # :nodoc:
  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]])
# => "field(id, 1,3,2)"

sanitize_sql_for_order("id ASC")
# => "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
    )

    # Ensure we aren't dealing with a subclass of String that might
    # override methods we use (eg. Arel::Nodes::SqlLiteral).
    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

変数の前に* を書くと、配列に変換してくれるではなく、可変長引数を受け取るメソッドに、配列を渡す際に変数に* をつけないともう一回り配列で包まれてしまうというこちらの挙動ではないかとのことでした

なので、こちらの修正を行うことで f:id:sktktk1230:20180822134705p:plain

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 # :nodoc:
      attr_reader :relation, :values, :other

      def initialize(relation, other)
        @relation = relation
        @values   = other.values
        @other    = other
      end

省略

Mergeクラスを初期化する際に引数でotherを入れているので、初期化処理の箇所がないか調べてみました f:id:sktktk1230:20180823105415p:plain

こちらを見てみるとotherはRelationクラスの可能性がありそうです

rails/activerecord/lib/active_record/relation/spawn_methods.rb

省略

def merge!(other) # :nodoc:
      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

# frozen_string_literal: true

module ActiveRecord
  # = Active Record \Relation
  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を使用しているところを探してみます

f:id:sktktk1230:20180823110148p:plain

こちらで定義していました

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 を呼び出しているので、こちらの定義箇所を読んでみます

    # Returns a relation value with a given name
    def get_value(name) # :nodoc:
      @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! メソッドの引数に * をつけることで配列で包まれることなく、配列を渡せるようになったというのが修正内容でした

1. 概要

issue

該当PR

修正内容を抜粋

2. PRを理解するためにやってみたこと

1. まず、PRのコメントを読んでみる

2. 変数の前に*をつけるとどんな動きなのか調べてみる

3. relation.reorder! を読んでみる

rails/activerecord/lib/active_record/relation/query_methods.rb

4. preprocess_order_args を読む

rails/activerecord/lib/active_record/relation/query_methods.rb

5. sanitize_sql_for_order を読む

rails/activerecord/lib/active_record/sanitization.rb

6. other.order_values の戻り値を確認してみる

activerecord/lib/active_record/relation/merger.rb

rails/activerecord/lib/active_record/relation/spawn_methods.rb

activerecord/lib/active_record/relation.rb

rails/activerecord/lib/active_record/relation/query_methods.rb

2. 変数の前に`*`をつけるとどんな動きなのか調べてみる

3. `relation.reorder!` を読んでみる

4. `preprocess_order_args` を読む

5. `sanitize_sql_for_order` を読む

6. `other.order_values` の戻り値を確認してみる