この記事は Opt Technologies Advent Calendar 2019 13 日目の記事です。若干遅刻しました。

12 日目の記事は @gcchaan さんの なにか書きます です
14 日目の記事は @naru0504 さんの styled-componentでモダンなCSS設計入門 です

「RailsでActiveRecordでクエリが複雑でつらいしパフォーマンスも悪くなってきた」
よくある話だと思います。例に漏れず、自分のチームも同じ問題と向き合いました

クエリが複雑になりアプリケーションの修正時にコードリーディングが難しい・修正後の結果予測が困難になるという、まあ言ってしまえば典型的な事例です
これまでに同様の議論は多くなされてきたと思いますが、自分のチームがこの問題と向き合い、「何が問題で・どのように解決し・結果どうなったか・反省点」という観点で事例を書き残しておこうと思います

何が問題だったか

プロダクトにとっての問題点

ユーザーに利用してもらうにはあまりにも レスポンスが遅い という状態になってしまっていたことが第一です
プロダクトにとってとても優先度の高い機能を優先して実装していたのですが、それがリリースされたときにはもうだいぶ遅い（レスポンスに30sec以上平気でかかる・・・）状態になっていました

コードベースの問題点

クエリについて話しているのでお察しのことと思いますが、SQLがボトルネックでした
しかし、クエリを改善しようとするも、以下の状況が立ちはだかります

プロダクトの性質上、集計関数を用いたクエリが必要
基本方針として、よく使うscopeを実装し、その組み合わせによってクエリを実現していた
- レポーティングのクエリで、同じテーブルに対して複数の結果セットが欲しいので、とあるscopeが複数のクエリ呼び出しから参照されていた
scopeは別のscopeも参照していた
scopeの組み合わせるクエリ組み立て処理はControllerで書いていた
一部でクエリの結果に対しての複雑な加工処理もあった（クエリ組み立てがControllerで実施されているので、この処理もControllerで呼び出しされていた）
Controllerで呼び出されている処理について直接のテストはなかった
- あったのはrswagによるスキーマのテストぐらい
各scopeは結構丁寧にテストは書かれていた

イメージとしては以下のような実装をもっとFatにしたものになっていました

foo_controller.rb

class FooController < ApplicationController
  def index
    # Controllerでこのようにscopeチェインしてクエリを組み立てていた
    @foo = Foo.by_foo(params[:some_param])
      .of_baz(params[:some_condition], params[:some_condition2])

    render json: @foo
  end

  def other
    # 別のメソッドやControllerなどからも呼ばれることもある
    @foo_other = Foo.of_baz(params[:some_condition], params[:some_condition2])
      .by_other
    render json: @foo_other
  end
end

foo.rb

class Foo < ApplicationRecord
  scope: by_foo, ->(some_param) {
    # このように別のscopeを参照している     ---↓
    select(:foo).group(:foo).order(:foo).sum_bar
  }
  scope: sum_bar, ->() {
    select('SUM(bar) AS bar')
  }

  scope: of_baz, -> (cond1,cond2) {
    baz_condition = make_condition(cond1,cond2)
    where(baz: baz_condition)
  }
end

まとめると、でかいクエリが詳細なテストなしに複数あって、scopeの参照関係も複雑、という形です

どのように解決したか

タイトルにも記載しましたが、QueryObjectパターンを利用しました

QueryObjectについて参考にした資料
- 7 Patterns to Refactor Fat ActiveRecord Models
- Rails - ActiveRecord の scope を Query object で実装する

ただ適当にQueryObjectを利用しても上手く行かないと思い、以下のような指針でQueryObjectへの切り出しを実施しています

実装の方針

「最終的に欲しいクエリ」は必ずQueryObjectに定義する
実装自体は引き続きscopeのチェイン
ただし、scope内から別のscopeを(なるべく)参照しない
- 結果の予測が困難になる理由の一つだったため
- scopeの利用自体は許容（scopeを利用しないとQueryObject側の実装が複雑になりそうだったため）
「最終的に欲しいクエリ」同士で重複しているクエリは許容
- scopeという「クエリの断片を組み合わせる処理」を呼び出す部分を共通化するのは「早すぎる抽象化」になりそう

以上のような方針にすることで、以下のような恩恵を受けられます

最終的なクエリに対してのテストがQueryObjectの呼び出しだけで実施できるようになる
scopeの影響範囲が明快になり、変更の結果を予測しやすくなる・影響範囲を限定できる

リファクタの方針

方針を決定したので、リファクタをします
といってもこれ自体は「まずはテスト出来るような形にだけ変更」、「テストを書く」、「リファクタを実施」という鉄則に従って実施しただけです
幸いだったのが、「まずはテスト出来るような形にだけ変更」が非常に容易だった点です

先程の例を元に説明します

最初に FooController#index についてのみをQueryObjectに切り出す

foo_query.rb

class SumOfFooQuery
  class << self
    delegate :call, to: :new
  end

  def initialize(foo = Foo.all)
    @foo = foo
  end

  def call(params)
    # ここにFooController#indexに書かれていた処理をまるっとコピペ
    @foo.by_foo(params[:some_param])
      .of_baz(params[:some_condition], params[:some_condition2])
  end
end

モデルにQueryObjectを参照したscopeを定義

foo.rb

scope :sum_of_foo, SumOfFooQuery

FooControllerの実装を置き換え

foo_controller.rb

class FooController < ApplicationController
  def index
+   @foo = foo.sum_of_foo(params)
-   @foo = Foo.by_foo(params[:some_param])
-     .of_baz(params[:some_condition], params[:some_condition2])

    render json: @foo
  end

  def other
    @foo_other = Foo.of_baz(params[:some_condition], params[:some_condition2])
      .by_other
    render json: @foo_other
  end
end

テストを書く
リファクタする
- サンプルは割愛

以上のような流れでリファクタリングを順次実施していきました

結果どうなったか

クエリ単位でのテストがあり、影響範囲も狭められたので、特定のエンドポイントから順番に・独立してクエリチューニングを実施できるようになりました
（チューニングはそれはそれで大変だったのですがそれはまた別の話）

scope内から別のscopeを(なるべく)参照しない 、 「最終的に欲しいクエリ」同士で重複しているクエリは許容 といった方針も見込み通りにコードの読みやすさや変更しやすさに繋がったという感触です

その後運用していても、変更時に大きく困るような事態にはなっていないので、設計は上手くいったと思っています

反省点

Controllerに処理書いちゃだめだった
- ここを徹底すべきだった
scopeの先のscopeの先のscope・・・という道のりを辿った先でのselectを把握してコーディングするのは人間には無理だった
- 作るときはいいけど変更できない
- 「1つのメソッドを短くする」、「DRY」を徹底すればいいってもんじゃなかった
  - メソッドの定義をバラけさせればバラけさせるほど「結局何をやりたいのか」が分かりにくくなることもある
  - （かと言ってまとめりゃいいってもんでもないので難しい）
レスポンスに30sec以上という状態は流石にもっと早く手を打てたんじゃ・・・
- turai
- 開発が進むうちにデータが溜まって表出したものが多かったので、開発初期時点でデータを作れるなら作っておくという選択肢は今後頭に入れておきたい
- 機能追加の優先度が高かったので、パフォーマンスがヤバくなるかもしれないとわかってても優先度を変更するかは判断が難しかったと思う
  - ので、普段から変更に強い設計にしておく・設計を身に着けておくという再現性のない反省になってしまう・・・

ただ、以下のような良かった点もあって、この前提がなかったらもっと困難な課題になっていたと思います

scopeで非常に汎用的なクエリを表現していた
- 例えば見込み値の算出クエリなど
- このような複雑な処理がControllerに氾濫していたらQueryObjectへの移行が困難だったと思う
scope単位のユニットテストはあった
- テスト大事・・・
Controllerはクエリ組み立てとrender用の多少の加工だけでFatにはなっていなかった
- Fatになる前にちゃんとリファクタに手を付けられたとも言えるかも

おわりに

ということで、Railsでクエリが複雑になってきたのでQueryObjectパターンでリファクタをして良かった話でした
今回の事例がどこかのRailsプロダクトの参考になれば幸いです