Tech Racho エンジニアの「？」を「！」に。
  1. TOP
  2. 開発
  3. Railsの`CurrentAttributes`は有害である（翻訳）
  • 開発

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

概要

原著者の許諾を得て翻訳・公開いたします。


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

関連記事


この記事を書いた人

hachi8833

Twitter: @hachi8833、GitHub: @hachi8833 コボラー、ITコンサル、ローカライズ業界、Rails開発を経てTechRachoの編集・記事作成を担当。 これまでにRuby on Rails チュートリアル第2版の監修および半分程度を翻訳、Railsガイドの初期翻訳ではほぼすべてを翻訳。その後も折に触れて更新翻訳中。 かと思うと、正規表現の粋を尽くした日本語エラーチェックサービス enno.jpを運営。 実は最近Go言語が好きで、Goで書かれたRubyライクなGoby言語のメンテナーでもある。 仕事に関係ないすっとこブログ「あけてくれ」は2000年頃から多少の中断をはさんで継続、現在はnote.muに移転。Amazonウィッシュリスト: https://bit.ly/32aAmiI

hachi8833の書いた記事一覧へ
タグ

CONTACT

TechRachoでは、パートナーシップをご検討いただける方からの
ご連絡をお待ちしております。ぜひお気軽にご意見・ご相談ください。
CONTACT FORM