- 開発
READ MORE
概要
原著者の許諾を得て翻訳・公開いたします。
- 英語記事: Rails’ CurrentAttributes considered harmful
- 公開日: 2017/06/22
- 著者: Ryan Bigg: Ryan Bigg氏はRailsのcontributorであると同時にCulture Amp社のメンバーであり、RubyやElixirでの開発を行っています。RailsガイドのドキュメントやMultitenancy with Rails など多くの執筆実績があり、現在はDeep Dive Railsという書籍を執筆中です（現時点で45%↓）。
leanpub.com/ddrより
RailsのCurrentAttributesは有害である（翻訳）
最近Rebecca Skinnerに教えてもらったこのcommitは、Railsアプリにいわゆる「グローバルステート（global state）」を効果的に導入するためのものです。
グローバルステートが一般によくない理由を述べる代わりに、Stack ExchangeのよくできたQ&Aのリンクをご紹介します。
ごく簡単に言えば、（グローバルステートがあると）プログラムのステートが予測不可能になります。
詳しくはこうです: 同じグローバル変数を共有しているオブジェクトが2つ（以上）あるとしましょう。ランダムさをもたらす不確定要素がモジュール内のどこにも使われていない前提条件においては、あるメソッドの実行前にシステムのステートが既知であれば、そのメソッドの出力は予測可能（したがってテスト可能）になります。
commitではスレッドローカルな変数も実装されていますが、このチョイスがよろしくない理由についてStack Overflowの回答から引用いたします。
- テストの難易度が上がる: スレッドローカルな変数をコードで使うと、そのコードの外で書くテストでもスレッドローカルな変数を設定し忘れないよう注意が必要
- スレッドローカルな変数を使うクラスは、オブジェクトが利用不可能になっているのではなくスレッドローカルな変数の内部にあることを認識できる必要がある: 通常、このような間接性（indirection）はデメテルの法則に反する
- スレッドローカルな変数をクリーンアップしないと、フレームワークでスレッドを再利用するときに問題が生じる可能性がある: スレッドローカルな変数が既に初期化され、変数の初期化で
||=呼び出しに依存するコードがコケる可能性がある
CurrentAttributesがデメテルの法則にも反していることについては本記事では言及しません。ある日から突然、Currentがアプリのあらゆる場所で利用できるようになることを問題にしたいと思います。よいコードとは、メソッドや関数で利用できるようにする方法をコードで明示する（explicit）ものです。しかるに、CurrentAttributes機能はよくないコードであり、モデルでCurrent.userが利用可能になるまでの流れが明らかになりません。ただひたすら「魔法のように」そうしてくれるというだけです。
私はいつもRailsの魔法で楽しんでいますし、これまでにも随分と助けられてきました。render @collectionやポリモーフィックルーティングなどお気に入りの魔法はいくつもありますが、こうした魔法が優れているのはスコープが限定されているからです。コレクションをレンダリングできるのはビューであることも、ポリモーフィックルーティングを使えるのはコントローラやモデルやヘルパーであることも、ちゃんと察しがつきます。
私がいくら魔法が好きだからと言っても、CurrentAttributesの魔法はあまりに強力すぎます。スレッドローカルなグローバルステートを導入すると、Currentに実際に値を設定するコードが覆い隠されてしまいますし、値の出どころも暗黙（implicit）になってしまうからです。
「それ、コントローラで設定されるから」という反論もあるでしょう。ではコントローラがない場合はどうなるのでしょう。バックグラウンドジョブでは、あるいはテストではどうなるのでしょう。確かに、どちらの場合についても以下のようにCurrentで値を指定できます。
def perform(user_id, post_id)
Current.user = User.find(user_id)
post = Post.find_by(post_id)
post.run_long_running_thing
# ジョブで実行するコードをここに書く
end
上のPost#run_long_running_thingは、Current.userにアクセスして現在のユーザーを取得するだけのシンプルなメソッドです。しかし、本当にそうなるかどうかはその場ではわかりません。Post#run_long_running_thing methodのことだけ考えれば、Current.userは最初の段階で設定されます。Current.userが他のどこかで設定されることぐらいはコードからもうかがえますが、この文脈ではどのコードで設定されるのかを突き止めるのは難しいでしょう。プロジェクトでCurrent.user =という文字列を検索しても、コントローラやジョブなど、変数を設定するあちこちのコードで見つかるでしょう。この文脈に合った本物のCurrent.user =は、果たしてその中のどれなのでしょう。
テストの場合も、Current.userに依存するコードがあればCurrent.userの設定が必要になるでしょう。以下のようなテスト例を考えてみましょう。
let(:user) { FactoryGirl.create(:user) }
before { Current.user = user }
it "時間のかかるテストを走らせる" do
post.run_long_running_thing
end
やはり、run_long_running_thingメソッドを見てもテストコードを見ても、Current.userが設定されるタイミングは明らかになりません。
私の見る限り、このCurrentオブジェクトのステートをテストのたびにリセットすると考えられる部分はCurrentAttributesのコードにも見当たらないようです。したがって、上のテストコード例のように、あるテストでの設定が「魔法のように」他のテストに漏れてしまいます。こうした挙動がコードベースに存在するのは恐ろしいことではないでしょうか。テストでCurrent.userがnilになることを期待している状況で、他のいずれかのテストで別の値が設定されてしまうことは十分ありえます。アプリでテストを500回まわしたとして、その中のどのテストのしわざなのでしょうか。首尾よく見つけられるかどうかは運次第です。
「設定より規約」に続く原則は「暗黙的プログラミングより明示的プログラミング」ではないか
訳注: Rails生みの親であるDHHは、エッセイなどでたびたび暗黙的なプログラミングを賞賛しています。
Railsは今も優れたフレームワークです。私のこうした主張に対してDHHが「いやなら使うな」といった調子で反駁するであろうことも察しがつきます。おそらく、少し前に私がコールバックのsuppressを元に戻そうとしたときのDHHの反応↓と同じように。
Railsの憲章には、有効なユースケースもあればよくない使い方もある新機能の導入時に、プログラマーを自滅から保護することについては明記されていない。
#25115のDHHコメントよりより
要するにDHHの説得は私には無理だということです。こうした点について私とDHHの意見は大きく食い違っています。
私は、今回のCurrentの件のようにRailsが暗黙的プログラミングの極北に向けてひた走ることは、大きな間違いだと考えます。こうした機能によってRailsのコードベースでたくさんのフラストレーションが生み出されます。今回のような状況であれば、Railsはあえて暗黙的プログラミングより明示的なプログラミングを選ぶべきです。Railsには既に十分な魔法が備わっているのですから、これ以上新しい魔法を編み出す必要はないはずです。
今回の機能は私が求めていたものではありません（DHHはある日この着想を得てそれをよしとし、そのまま実装したようです）し、もっとよいやり方が他にもあります。たとえば前述のジョブコードだったら、次のように値を明示的に渡す方がずっとよいのではないでしょうか。
def perform(user_id, post_id)
user = User.find(user_id)
post = Post.find_by(post_id)
post.run_long_running_thing(user)
# ジョブで実行するコードをここに書く
end
テストコードも、次のように明示的に書くほうがずっと有利です。
let(:user) { FactoryGirl.create(:user) }
it "時間のかかるテストを走らせる" do
post.run_long_running_thing(user)
end
どちらのコードも、userがどのようにしてrun_long_running_thingメソッドに到達するかがはっきりわかります。何しろ引数として渡しているのですから。
最後に、今回のプルリクのコードを明示的プログラミングに沿って書き直せることを簡単にお見せしたいと思います。
対決！ DHHのCurrentAttributesのコード vs 私の明示的プログラミングのコード
# app/models/current.rb
class Current < ActiveSupport::CurrentAttributes
attribute :account, :user
attribute :request_id, :user_agent, :ip_address
resets { Time.zone = nil }
def user=(user)
super
self.account = user.account
Time.zone = user.time_zone
end
end
# app/controllers/concerns/authentication.rb
module Authentication
extend ActiveSupport::Concern
included do
before_action :set_current_authenticated_user
end
private
def set_current_authenticated_user
Current.user = User.find(cookies.signed[:user_id])
end
end
# app/controllers/concerns/set_current_request_details.rb
module SetCurrentRequestDetails
extend ActiveSupport::Concern
included do
before_action do
Current.request_id = request.uuid
Current.user_agent = request.user_agent
Current.ip_address = request.ip
end
end
end
class ApplicationController < ActionController::Base
include Authentication
include SetCurrentRequestDetails
end
ApplicationControllerにメソッドを1つ追加するのにわざわざAuthenticationモジュールをincludeしているあたりがイマイチですが、ここでは目をつぶることにしましょう。
DHH流の実装ではbefore_actionでset_current_authenticated_userを設定しており、これによってCurrent.userがすべてのリクエストに設定されます。リクエストでcurrent_userをまったく参照しない場合も同様に設定されます。
よりよい実装は、current_userメソッドを呼ぶときにそのfindを評価することでしょう。この種のパターンは多くのRailsアプリで見かけると思います。
def current_user
@current_user ||= User.find(cookies.signed[:user_id])
end
実際この方法は、Deviseでcurrent_userメソッドを提供する方法に似ています。Deviseではcookies.signedの代わりにwardenを使いますが↓、実装はとても似通っています。
def current_#{mapping}
@current_#{mapping} ||= warden.authenticate(scope: :#{mapping})
end
では、コントローラにcurrent_userメソッドがあるとして、これをビューでも使いたくなった場合を考えてみましょう。たとえばログインしたユーザーに挨拶を表示するためにレイアウトに#{current_user.name}と書けるでしょうか。 次のヘルパーメソッドを書くだけでおしまいです。
def current_user
@current_user ||= User.find(cookies.signed[:user_id])
end
helper_method :current_user
このとおり、メソッドをコントローラでもヘルパーでもビューでも使えるようになりました。メソッドをわざわざ現在のスレッドのあらゆる場所で利用可能にする必要もありません。
続いて、DHHのコードの後半を見てみることにしましょう。
class MessagesController < ApplicationController
def create
Current.account.messages.create(message_params)
end
end
class Message < ApplicationRecord
belongs_to :creator, default: -> { Current.user }
after_create { |message| Event.create(record: message) }
end
class Event < ApplicationRecord
before_create do
self.request_id = Current.request_id
self.user_agent = Current.user_agent
self.ip_address = Current.ip_address
end
end
DHH流では、belongs_toのdefaultオプションを使って、このメッセージの作成者をCurrent.userに暗黙でリンクしています。私は、この書き方はMVCレイヤ抽象化に反していると信じています。Current.userは「魔法のように」モデルで使えるようになりますが、それだけです。そもそもモデルで最初に使えるようになるまでのコンテキストが完全に失われています。
Railsアプリで多用されるパターンではこのような書き方はしません。代わりに、メッセージを作成するときに作成者を明示的に指定します。
def create
@message = current_account.messages.create(message_params)
@message.creator = current_user
ここで、current_accountとcurrent_userがどちらも同じ程度に抽象化されていると仮定しましょう。コントローラであれば、作成者が代入される場所はこの場合明らかにコントローラになります。DHH流コードの場合、作成者が代入される場所がコントローラのコード自体であるかどうかは、コントローラのコードを見てもわかりません。
そればかりか、これには自身を「serviceオブジェクト」に抽象化する効果もあります（serviceオブジェクトはメッセージ作成の責任を持ちます）。さて、ここでMessageが作成されたら常にEventをログ出力したいとしましょう。ところが何ということでしょう、DHHのコードではafter_createコールバックで既にログを出力するではありませんか。
DHHのコードでは、アプリのどんな場所でMessageが作成されても常にafter_createコールバックが発生します。作成場所がコントローラならそれでよいかもしれませんが、たとえばデータベースロジックのテストや、メッセージが消えずに表示され続けることを要求するコードのテストはどうなるのでしょうか。メッセージ作成と同時にイベントが発生することに開発者が気づかなかったらどうなるのでしょうか。イベントを作成したときに、別のレコードの作成も行うロジックがそこに潜んでいたとしたらどうなるのでしょうか。
こうしたコールバックは、取り返しのつかないかたちでメッセージとイベントを暗黙に紐付けてしまいます。
このコードは、前述の「serviceオブジェクト」として抽象化する方がよいのではないでしょうか。
class CreateMessageWithCreator
def self.run(params, current_user)
message = current_account.messages.create(message_params)
message.creator = current_user
message.save
end
end
これで、このコードを次のようにコントローラで呼び出せるようになります。
def create
if CreateMessageWithCreator.run(message_params, current_user)
Event.create(record: record)
flash[:notice] = "メッセージ送信成功！"
redirect_to :index
else
flash[:alert] = "メッセージ送信失敗"
render :new
end
end
この方法なら、（特にこの場合）作成者に応じたメッセージを作成していることが誰からもわかるでしょう。必要であれば、作成者やイベントとは無関係なメッセージも自由に作成できます。
このように依存関係をコードではっきり示す方が、魔法のような抽象化よりもずっとずっと優れたソリューションだと私は思っています。
まとめ
Railsにグローバルステートを導入するアイデアはやはり悪手ではないでしょうか。私は、今回の変更が元に戻されることを強く、本当に心から強く願うものでありますが、DHH自ら手がけた変更であり、RailsフレームワークはDHHのものである以上、その見込みはほとんどなさそうです。DHHはその気になれば、自分の足を撃ち抜ける銃を売りさばけるほどの立場にいます。今回の変更が本質的に悪手である証拠が揃っているにもかかわらずDHHが導入を決行したことについては、とにかく残念の一言しかありません。DHHは長年に渡って経験を積んでいるのですし、この点についてもう少し理解を得られればと思いました。