kaminariにPRを出してみた
概要
OSSでRuby力を高めていきたいと思っている中，初のOSS挑戦にいいIssueがないかなと探していたところ下記Issueを発見。ただ，Issueが出されたのも2021/4で特に対応もされてなさそうだし，kaminariのgem自体も，もうそんなに活発に開発されているわけではないから意味あるかなと思いつつも，コードリーディングの機会も兼ねて出してみることに。
Issue
調査内容
total_countというメソッドは，対象のActiveRecordモデルのレコード数を返すもの。しかし，loadメソッドと一緒に呼び出すと，正しいレコード数が返って来ないことがあるというのが今回のIssueの内容。
まずは検証。
irb(main):015:0> User.count (4.8ms) SELECT COUNT(*) FROM "users" => 5 irb(main):016:0> User.page(1).padding(5) User Load (0.4ms) SELECT "users".* FROM "users" /* loading for inspect */ LIMIT ? OFFSET ? [["LIMIT", 11], ["OFFSET", 5]] => #<ActiveRecord::Relation []> irb(main):017:0> User.page(1).padding(5).total_count (0.3ms) SELECT COUNT(*) FROM "users" => 5 irb(main):018:0> User.page(1).padding(5).load User Load (0.2ms) SELECT "users".* FROM "users" LIMIT ? OFFSET ? [["LIMIT", 25], ["OFFSET", 5]] => #<ActiveRecord::Relation []> irb(main):019:0> User.page(1).padding(5).load.total_count User Load (0.3ms) SELECT "users".* FROM "users" LIMIT ? OFFSET ? [["LIMIT", 25], ["OFFSET", 5]] => 0
Issue通り， User.page(1).padding(5).total_countでは本来のユーザー数である5を返すが，loadを追加したUser.page(1).padding(5).load.total_countでは0を返してしまっている。
まずは，loadの実装から見ていく。
def load(&block) unless loaded? @records = exec_queries(&block) @loaded = true end self end
loadedはデフォルトでfalseであるため，unless内が実行される。このloadメソッドでしていることは3つ。@records，@loadedインスタンス変数への代入，そして，selfつまり，loadメソッドを呼び出したインスタンス自身を返す。
最初に@recordsについて考えていく。exec_queries(&block)の返り値が代入されるので，exec_queriesメソッドが少しボリューミーかつ，今回ブロック引数は渡さないため，pryで空の配列が返されることだけを確認した。
[4] pry(#<User::ActiveRecord_Relation>)> exec_queries => []
@loadedは実装通り，falseの値が代入される。
次に，total_countの実装を見ていく。
def total_count(column_name = :all, _options = nil) #:nodoc: return @total_count if defined?(@total_count) && @total_count # There are some cases that total count can be deduced from loaded records if loaded? # Total count has to be 0 if loaded records are 0 return @total_count = 0 if (current_page == 1) && @records.empty? # Total count is calculable at the last page return @total_count = (current_page - 1) * limit_value + @records.length if @records.any? && (@records.length < limit_value) end # #count overrides the #select which could include generated columns referenced in #order, so skip #order here, where it's irrelevant to the result anyway c = except(:offset, :limit, :order) # Remove includes only if they are irrelevant c = c.except(:includes) unless references_eager_loaded_tables? c = c.limit(max_pages * limit_value) if max_pages && max_pages.respond_to?(:*) # .group returns an OrderedHash that responds to #count c = c.count(column_name) @total_count = if c.is_a?(Hash) || c.is_a?(ActiveSupport::OrderedHash) c.count elsif c.respond_to? :count c.count(column_name) else c end end
まずは，一行目。
return @total_count if defined?(@total_count) && @total_count
@total_countというインスタンス変数にキャッシュしていて，キャッシュが存在すればその値を返し，なけれが後続の処理が走る。
次の処理を見ていく。
if loaded? # Total count has to be 0 if loaded records are 0 return @total_count = 0 if (current_page == 1) && @records.empty? # Total count is calculable at the last page return @total_count = (current_page - 1) * limit_value + @records.length if @records.any? && (@records.length < limit_value) end
先ほどのloadメソッドを実行していると，loaded?がtrueとなり，if文の中が実行されるため，if文の処理を追っていく。
まずは，一つ目の処理を見る。
return @total_count = 0 if (current_page == 1) && @records.empty?
&&の二つ目の@records.empty?に関しては，先ほどのloadメソッドで@recordsには空の配列が代入されることを確認したので，trueとなる。なので，current_pageについて調べる。current_pageの実装は以下。
def current_page offset_without_padding = offset_value offset_without_padding -= @_padding if defined?(@_padding) && @_padding offset_without_padding = 0 if offset_without_padding < 0 (offset_without_padding / limit_value) + 1 rescue ZeroDivisionError raise ZeroPerPageOperation, "Current page was incalculable. Perhaps you called .per(0)?" end
１行目から読んでいく。
offset_without_padding = offset_value
offset_valueという変数が出てきた。定義ジャンプしてもジャンプできないし，定義しているところも見つからなかったが，どうやらActiveRecordのoffsetメソッドを使用したときの引数の値が代入される模様。だが，User.page(1).padding(5).load.total_countの時を考えているから，offsetメソッドは特に呼び出していない。そこで，pageメソッドを見にいく。実装は以下。
def self.#{Kaminari.config.page_method_name}(num = nil) per_page = max_per_page && (default_per_page > max_per_page) ? max_per_page : default_per_page limit(per_page).offset(per_page * ((num = num.to_i - 1) < 0 ? 0 : num)).extending do include Kaminari::ActiveRecordRelationMethods include Kaminari::PageScopeMethods end end
Kaminari.config.page_method_name==pageとなる。limit(per_page).offset(per_page * ((num = num.to_i - 1) < 0 ? 0 : num)).extending do確かにoffsetメソッドが呼ばれていた。引数は，per_page * ((num = num.to_i - 1) < 0 ? 0 : num)。numは引数で，今回は1。また，per_pageはper_page = max_per_page && (default_per_page > max_per_page) ? max_per_page : default_per_pageであり，max_pageは定義されていないため，default_pageのデフォルトが使用され，デフォルトが25となり，per_pageは25。よって，offsetの引数は0となり，offset_valueも0となる。User.page(1).padding(5).load.total_countで検証してみる。
[1] pry(#<User::ActiveRecord_Relation>)> offset_value => 5
5となり，pageメソッドで呼ばれたoffset(0)とは異なる結果となっている。実は，paddingメソッドでもoffsetメソッドが呼ばれており（というより，現在のページ数からの開始位置を決定するのがこのメソッドの本来の役割），以下のようになっている。
def padding(num) num = num.to_i raise ArgumentError, "padding must not be negative" if num < 0 @_padding = num offset(offset_value + @_padding) end
offset(offset_value + @_padding)で，@_paddingは今回のpaddingメソッドの5となるため，offset(0 + 5)が呼ばれるため，offset_valueも5となる。
それでは，current_pageメソッドの続きに戻る。current_pageメソッドを再掲する。
def current_page offset_without_padding = offset_value offset_without_padding -= @_padding if defined?(@_padding) && @_padding offset_without_padding = 0 if offset_without_padding < 0 (offset_without_padding / limit_value) + 1 rescue ZeroDivisionError raise ZeroPerPageOperation, "Current page was incalculable. Perhaps you called .per(0)?" end
改めて処理を追っていく。
offset_without_padding = offset_value
offset_valueは5なので，offset_without_paddingには5が代入される。
offset_without_padding -= @_padding if defined?(@_padding) && @_padding
先ほどのpadding(5)メソッドにより，@_paddingには5が代入されているので，offset_without_paddingは0。
offset_without_padding = 0 if offset_without_padding < 0
offset_without_paddingは0なので，無視。
(offset_without_padding / limit_value) + 1
offset_without_paddingは0なので，current_pageは1となる。limit_valueは，ActiveRecordのlimitメソッドの引数となるみたいだが，offset_valueメソッドと同様にpageメソッドでlimitメソッドが呼ばれている。以下，実装。
limit(per_page).offset(per_page * ((num = num.to_i - 1) < 0 ? 0 : num)).extending do
per_pageは先ほど見たようにdefault_per_pageが適用されているため，limit(25)が呼ばれている。よって，limit_valueは25。ここまでで，current_pageの実装。
本題のtotal_countメソッドに戻る。先ほどまで見ていた箇所は以下。
return @total_count = 0 if (current_page == 1) && @records.empty?
@records.empty?はtrueとなり，current_pageは1であるため，ifはtrueとなり，@total_countが0となり，早期リターンされてしまう。ここが今回のバグの原因な模様。
次に，今回のIssueと同じ状況で考える。User.page(1).per(2).padding(4).load.total_countを考える。実際にメソッドを呼び出してみる。
[11] pry(#<User::ActiveRecord_Relation>)> User.count => 5 [12] pry(#<User::ActiveRecord_Relation>)> User.page(1).per(2).padding(4).total_count => 5 [13] pry(#<User::ActiveRecord_Relation>)> User.page(1).per(2).padding(4).load.total_count => 1
確かにIssueと同じ挙動となる。先ほどと同じようにtotal_countメソッドを読んでいく。
return @total_count = 0 if (current_page == 1) && @records.empty?
pryでcurrent_pageの値を確認する。
[6] pry(#<User::ActiveRecord_Relation>)> current_page => 1
current_pageは前回と同じ1である。1ページあたりデータ数が2つで，5個目のデータから始め，1ページ目を表示するため，当たり前の結果ではあるが。
次に，@recordsの中身をpryで確認する。
[7] pry(#<User::ActiveRecord_Relation>)> User.count => 5 [8] pry(#<User::ActiveRecord_Relation>)> @records => [#<User:0x00007f8ff8072f80 id: 5, name: "user_name5", avatar_url: "avatar5", created_at: Sat, 14 May 2022 13:02:25.931165000 UTC +00:00, updated_at: Sat, 14 May 2022 13:02:25.931165000 UTC +00:00>]
@recordsがnilではなくなっている。そのため，(current_page == 1) && @records.empty?はfalseとなり，1つ目の例の早期リターンの処理は行われない。@recordsへの代入には，loadメソッド内で，exec_queriesの返り値が使われている。先ほど，空の配列を常時返すというのは間違い。exec_queriesの実装を読んでいきたいところだが，少し重そうなため，今回はパス。ただ，先ほどの例と今回の例を見た感じloadメソッドを呼び出すインスタンスが返ってきそう。
一つ目の早期リターンが実行されないので，二つ目の処理を確認する。実装は以下。
return @total_count = (current_page - 1) * limit_value + @records.length if @records.any? && (@records.length < limit_value)
@records.any?に関しては，先ほど@recordsが存在することが確認できたので，trueが返る。また，current_pageにおいて，limit_valueやoffset_valueが使われるため，それらを再定義しているperメソッドの実装を追っていく。
def per(num, max_per_page: nil) max_per_page ||= ((defined?(@_max_per_page) && @_max_per_page) || self.max_per_page) @_per = (num || default_per_page).to_i if (n = num.to_i) < 0 || !(/^\d/ =~ num.to_s) self elsif n.zero? limit(n) elsif max_per_page && (max_per_page < n) limit(max_per_page).offset(offset_value / limit_value * max_per_page) else limit(n).offset(offset_value / limit_value * n) end end
まずは一行目から。
max_per_page ||= ((defined?(@_max_per_page) && @_max_per_page) || self.max_per_page)
@max_per_pageは定義されていないため，nilとなる。
@_per = (num || default_per_page).to_i
@_perには，引数で渡しているnum=2が入る。
次に，条件分岐を確認していく。
まずは，一つ目の/^\d/ =~ num.to_s。^\dは任意の数字で始まる文字列を意味する正規表現なので，今回の場合はtrueとなり，この条件式は実行されない。
二つ目の，n.zero?はnが2であるためfalseとなり，max_per_pageも定義されていないため，else式が実行される。実行される実装は以下。
limit(n).offset(offset_value / limit_value * n)
n == 2,offset_value == 0,limit_value == 25となるので，実際に実行されるのは，limit(2).offset(0)となり，limit_value == 2,offset_value == 0となる。
次に，padding(4)メソッドが実行されるので，その実行内容を前回同様に簡単に確認する。
def padding(num) num = num.to_i raise ArgumentError, "padding must not be negative" if num < 0 @_padding = num offset(offset_value + @_padding) end
@_padding=4であるため，offset(0 + 4)が実行され，offset_value=4となる。では，最後に本題である，total_countメソッドに戻る。
return @total_count = (current_page - 1) * limit_value + @records.length if @records.any? && (@records.length < limit_value)
@records.length == 1, limit_value == 2となり，この早期リターンが実行される。まずは，current_pageの値を実装から確認する。
def current_page offset_without_padding = offset_value offset_without_padding -= @_padding if defined?(@_padding) && @_padding offset_without_padding = 0 if offset_without_padding < 0 (offset_without_padding / limit_value) + 1 rescue ZeroDivisionError raise ZeroPerPageOperation, "Current page was incalculable. Perhaps you called .per(0)?" end
offset_value == 4, @padding == 4であり，offset_without_padding == 0となるため，current_page == 1となり，二つ目の例の冒頭で述べた結果と一致する。そこで，total_countメソッドの実装に戻る。
return @total_count = (current_page - 1) * limit_value + @records.length if @records.any? && (@records.length < limit_value)
current_page == 1であるため，(current_page - 1) * limit_valueの項は0となるため，total_countの返り値は@records.lengthつまり1となり，冒頭及びIssueの内容と一致する。
対応
ここまで今回のバグがなぜ起こっているのかを確認してきたわけだが，具体的な対応方法について検討する。
最初はloaded?の条件分岐を消せば，毎回SQLを叩きにいって解決とか安易に考えていたが，それだとSQLを叩かずに計算できる時がもったいない。ならば，今回の問題としては，paddingメソッドが実行された時に，total_countの挙動が上記で調査したようにおかしくなってしまうので，paddingメソッドが使用された時には，loaded?の条件分岐で行われていた計算を行わないようにすれば良い。
paddingメソッドが使用された時，@_paddingというインスタンス変数が定義され，このメソッドのみで@_paddingへの代入が行われていたので，以下のように条件を増やすという対応が適切だと思った。
if loaded? && !@_padding
