概要
原著者の許諾を得て翻訳・公開いたします。
- 英語記事: Traps on Rails - Overriding boolean methods in models - Karol Galanciak - Ruby on Rails and Ember.js consultant
- 原文公開日: 2017/11/26
- 著者: Karol Galanciak
Railsのワナ: モデルでbooleanメソッドをオーバーライドするな（翻訳）
ActiveRecordで実に有用な機能のひとつは、指定のテーブルの属性（attribute）リーダーやライターをすべてのカラムで自動的に定義してくれる点です。ただし、その中でboolean型のものについては、さらに?付きのエイリアスまで定義されるのです。このメソッドをオーバーライドして、指定された条件に合わせていくばくかの要求を追加するのが有用な場合もあるかもしれませんが、あまりよい考えとは言えないのではないでしょうか。
問題の分析
アプリの開発中、管理パネルからユーザーを有効にしたり無効にしたりできるとしましょう。しかしこのアプリは有償であり、アプリにアクセスしたいユーザーはサブスクリプションを購入する必要があります。このような状況で、そのユーザーが今有効であるかをチェックするのであれば、User#active?メソッドをオーバーライドして、サブスクリプションにまつわる要求をいくつか追加することは一応可能です。
# app/models/users.rb
class User < ApplicationRecord
def active?
super && valid_subscription?
end
def valid_subscription?
# サブスクリプションが有効かどうかをここでチェック
end
end
ここでは、ActiveRecordがbooleanカラムのエイリアスを定義して、元のカラム名の末尾に?を追加することを利用しています。つまり、activeというbooleanカラムがあれば、active?メソッドが定義されていてactiveと同じように動作するであろうことを期待しているわけです。
ほほう、ではユーザーが完全に有効かどうかをチェックするために、User#active?をあちこちで呼び出したとしましょう。次にユーザーをAPIで公開することを要求されたとしましょう。別に難しくありません。jsonapi-serializers gemを追加してJSONAPI互換のシリアライザを実装すればよいのですから。そして今度は、あるユーザーが有効かどうかをもAPIで公開する必要が生じたとしましょう。シリアライザは次のようになります。
# app/serializers/user.rb
class UserSerializer
include JSONAPI::Serializer
attribute :active
# 他にも属性がある
end
もうこれで仕事は終わった感ありますね。しかし実は、何ともいやらしいバグがここに隠れています。シリアライザはUser#active?ではなくUser#activeの値を返すのです！
どこがまずかったのか
主にまずかったのは、命名がいい加減だったことと、ドメイン概念を正しく導入していないことです。ActiveRecordはこの問題をむしろ後押ししています。activeというカラム名に応じてactive?というメソッドが定義されるので、ここで必要なのは下の機能をオーバーライドして条件を少し加えるだけで済みました。しかし、booleanメソッドをオーバーライドしてもいいことはありません。これが行われているということは、何らかの概念を見落としているか、オーバーライドがコードに埋もれて見えなくなっているかのどちらかが起きているに決まっています。
この問題の解決方法
方法のひとつは、単にこのドメインの概念を明確に示すことでしょう。User#active?メソッドはそのユーザーが有効かどうかをチェックしているのではなく、ユーザーがアプリにアクセスできるかどうかをチェックするものです。つまり、おそらくUser#can_access_application?の方がよりふさわしい名前です。
この機能が追加されると、それに関連して別の機能も追加されるというのはままある話です。たとえば「そのユーザーは有効だがアプリにはアクセスできない」とか、単にactiveフラグ自体をチェックしたいなどです。最終的なモデルは次のようになるでしょう。
# app/models/users.rb
class User < ApplicationRecord
def can_access_application?
active? && valid_subscription?
end
def cannot_access_application?
!can_access_application?
end
# 他のメソッド
end
シリアライザも更新しないといけませんね。
# app/serializers/user.rb
class UserSerializer
include JSONAPI::Serializer
attribute :active do
object.active
end
attribute :can_access_application do
object.can_access_application?
end
attribute :cannot_access_application do
object.cannot_access_application?
end
end
「こんな修正は不要だ」「このバグはアプリ開発者のせいだ」「モデルのメソッドがオーバーライドされていないかチェックしてシリアライザを調整しとけばよかったんだ」という反論があるかもしれません。それもある意味正しいでしょう。しかしこのコードがproductionにデプロイされたとしたら、この潜在的な問題をレビュアーが見落としたということになるのですから、この問題の特定が難しいとは思いませんか？ActiveRecordはあらゆるbooleanメソッドにエイリアスを追加するので、User#activeとUser#active?の結果は同じであると考えてしまうのも無理はないでしょう。
しかし実際には、私の修正は名前衝突のリスクを最小化したうえに柔軟性も高まり、さらにUser#active?とUser#can_access_application?の違いが素直に表現されています。修正前の実装では、?付きメソッドでこれをシンプルに実現するのは無理です。
まとめ
命名はコンピュータサイエンスにおける2大難問のひとつであり、たとえコード量が増える羽目になろうとも、常にドメイン概念を明確に表す名前を付けるのがよいと思います。何かが暗黙であるからといって、それが存在しないということは言い切れないからです。ActiveRecordモデルではこの点をいくら注意しても注意しすぎということはありません。このようなモデルでは永続性とドメイン概念がごっちゃになり、今回のようなケースでついつい地雷を踏んでしまいがちです。ActiveRecordが生成するbooleanメソッドはオーバーライドせず、何を行うのかがひと目でわかるよう適切な名前を付けましょう。