Railsの`CurrentAttributes`は有害である（翻訳）

RailsのCurrentAttributesは有害である（翻訳）

本記事が日本語に翻訳されました。@hachi8833に感謝します！

最近Rebecca Skinnerに教えてもらったコミット（24a8644）は、Railsアプリにいわゆる「グローバルステート（global state）」を効果的に導入するためのものです。

グローバルステートが一般によくない理由を述べる代わりに、Stack ExchangeのよくできたQ&Aのリンクをご紹介します。

ごく簡単に言えば、（グローバルステートがあると）プログラムのステートが予測不可能になります。
詳しくはこうです: 同じグローバル変数を共有しているオブジェクトがいくつかあるとしましょう。ランダムさをもたらす不確定要素がモジュール内のどこにも使われていない前提条件においては、あるメソッドの実行前にシステムのステートが既知であれば、そのメソッドの出力は予測可能（したがってテスト可能）になります。

commitではスレッドローカルな変数も実装されています（24a8644）が、この決定がよろしくない理由について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が「いやなら使うな」といった調子で反駁するであろうことも察しがつきます。おそらく、少し前に私がコールバックのsuppressを元に戻そうとした（#25115）ときの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流の実装ではset_current_authenticated_userをbefore_actionで設定しており、これによって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メソッドをコントローラで利用できるようになりました。しかしこれをビューでも使いたくなった場合はどうでしょう？たとえばログインしたユーザーに挨拶を表示するためにレイアウトにHello, #{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流コードの場合、コントローラのコードを見ても、そのcreatorが代入されていることがすぐにはわかりません。

それだけではなく、これは自身を「Service Object」に抽象化するにも適しています（Service Objectはメッセージ作成を担当します）。さて、ここでMessageが作成されたら常にEventをログ出力したいとしましょう。ところがDHHのコードでは、after_createコールバックで既にログを出力してしまっています。

DHH流のコードでは、アプリのどんな場所でMessageが作成されても常にafter_createコールバックが発生します。作成場所がコントローラならそれでよいかもしれませんが、たとえばデータベースロジックをテストしたい場合や、メッセージが消えずに表示され続ける必要がある場合に、「メッセージ作成と同時にイベントが発生する」ことに開発者が気づかなかったらどうなるのでしょうか。イベントを作成したときに、別のレコードの作成も行うロジックがそこに潜んでいたとしたらどうなるのでしょうか。

こうしたコールバックは、取り返しのつかないかたちでメッセージとイベントを暗黙に紐付けてしまいます。

前述したように、このコードは「Service Object」として抽象化する方がよいのではないでしょうか。

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は長年に渡って経験を積んでいるのですし、この点についてもう少し理解を得られればと思いました。

関連記事

素のRailsは十分に豊かである（翻訳）

肥大化したActiveRecordモデルをリファクタリングする7つの方法（翻訳）