続・ActiveSupport::Callbackを使うのをやめろ

ActiveSupport::Callbackを使うのをやめろという話をしたがどうにもスッキリしない部分が残ってしまっていたのがKyoto.rbで話すテーマにしたらシュッと解決できたので共有。

• Kyoto.rb Meetup 20191116

前回からのモヤッと部分

• ActiveRecord::Callbackを使わないほうがいいという結論に達したが本当にそうなのか？
  • だとしたらもっと多くの人が議論してRailsフレームワークから除外しているのではないか？
  • コールバックが本当に悪いのか？悪いのは実装側の都合なだけでは？
• プロダクトやチームのレギュレーションのようなもので縛ることが果たして本当に正しいのか？
  • それは対処療法としての対応でなのでは？
  • railsはMVCフレームワークなのでコールバックがどうしても浮いたものになってしまうがその結果安易になんちゃらServiceにしてしまうのが正しいのか？
  • レビューでコールバック使ってたらリジェクトするというのは1つあるかもしれないがその理由がきちんと説明できるのか？

というような部分で前回の書いたコールバックを使わない方針で大筋は間違っていない(やろうとすることに対する副作用が大きすぎる)と思うもののコールバック自体が絶対に駄目なのか？という点でモヤッと部分が残ってしまったことをKyoto.rbでこれが知りたいというテーマとしてあげさせてもらったら自分の中で腹落ちすることができた。

コールバックで成し遂げたいこと

• Modelに関するなにかを起因として漏れなくアクションが実行されることを担保したい

なにはともあれコールバックってどんなものがあるの？ということで一覧を確認。
ActiveRecordのコールバック早見表 | Rails

矛盾するようだがコールバック関連で問題になるのはこの 漏れなく実行されること が問題になりやすい。
例えばテストのときには実行されたくない、バッチ処理のときには実行されたくないなど。

コールバックで許せるものとしてあがったのはコールバックが連鎖しないこと、実行されるアクションが単独で完結することという条件があることが話していてわかった。

つまりコールバックを使ってもOKなものとしては

• バリデーション
• ログ出力
• メールやチャットへの通知

の3つはコールバックで実装されていてもよいのではないか？という結論に達した。
他にもあるかもしれないがパッと思いつきはしなかったのでひとまず一例ということでこれらはOKと判断した。

個人的にメールやチャットなどの通知系はOKとするか微妙なところだが、少なくとも問題が起こったとしても書き換えが容易なのでここではOKとして扱っている。
データ作ってメール送信する実装をService Object相当なファイルに実装してデータ作成している箇所の実装をそのService Objectで実装しているメソッド呼ぶようにすればいいだけなので書き換え箇所もそこまで多くならないはず。

そういえば、associate　は例外的に連鎖するけどこれは許容するべきという話になった。
Service Objectとしても実装できなくはないがコールバックでもいいんじゃない？元々それを期待して関連性を定義しているのだから、ということでこれだけ少し特殊な扱いになっているかも。

最終結論

なので用途を絞った運用上であればコールバックを使うことはやぶさかではないという結論に達した。
レギュレーションとしてコールバック駄目、絶対！とするにしても「これは単独で完結しないのでコールバックで実装すべきでない」や「これをしてしまうとA→B→Cとコールバックが連鎖する可能性があるので実装すべきでない」という自分が「何故駄目なのか？」を自信をもって言える気がする程度には納得できた。

一方で再認識できた点としてはコールバックで実行されるのはどのような条件でも必ず行われるものに限るということ。
テストでは行いたくない、バッチ処理では行われたくないという条件が発生しそうであればそれはコールバックでは実装してはならない。
コールバックを実装するときはどのようなパターンのとき実行されてほしくないかをきちんと議論する必要がある。

さらなる追記

このエントリを書いたあとでFactoryBotのコールバックが実装されていて「おまえ！！！！」となった。
テストデータをシュッと作るために存在するはずがそのせいでテストデータに強い副作用が生まれてしまっていた。
associateは例外と書いたがテストデータに関してはさまざまな状態を考慮したいのでこの場合は少し違うのかなと思う。

最終的に↓の状態から変更を行った。本来はtraitなどを用いて状態を明示的にするべきだと思うのだけど今回はそこまでする必要性がない&そもそも使用箇所が多いところだったので暫定対応として実装を行った。
元のコードが悪いといえば悪いのだけどまあすでに書かれてコミットされている状態でその話をしても意味ないので私はこうしましたよという感じです。

# 元のコード
factory :shop do
  name: 'お店の名前'
  after(:create) do |shop|
    FactoryBot.create :staff, shop: shop
  end
end

# 変更したコード
factory :shop do
  name: 'お店の名前'
  association :staff
end

教訓

FactoryBotでコールバック使われてたら別の方法で書き直せないか？あるいは本当にそのコールバックが漏れなく実行されなければいけないかを検討するほうがよい。
モデルだけじゃなくてテストデータの作成でも気をつけましょうね。