Rails: Active Recordのfindで怖い思いをした話（翻訳）

Rails: Active Recordのfindで怖い思いをした話（翻訳）

私は最近プロジェクトでこんなリファクタリングを行いました。identityに束縛されたコンテキストにドメインイベントをさらに追加することで、システム内のidentityに対して実行された特定のアクションから取得する監査ログを改善するというものです。手始めに、コマンドを消費する責務を持つServiceを抽出しました。当初は以下のようなものでした。

class UpdatePersonalSettings
  include Command
  attribute :email, String
  attribute :name, String
  attribute :identity_id, Integer
end

class IdentityService
  # ...

  def update_personal_settings(command)
    Identity.find(command.identity_id) do |identity|
      identity.email = command.email
      identity.name = command.name
      identity.save!
      publish(
        PersonalSettingsUpdated.strict(
          data: {
            identity_id: identity.id,
            email: identity.email,
            name: identity.name
          }
        )
      )
    end
  end

  # ...
end

一見すると何の問題もなさそうです。動作が意図通りであることを検証するテストもそこそこ書いたので、このコードをtest環境にデプロイしました。

すると、何かがおかしいことに気づきました。自分のテストアカウント名を更新しようとすると、emailフィールドでuniquenessバリデーションエラーが発生していたのです。

これは一体どういうことでしょう？デバッグを始めてみると、実際に更新されていたのはcommand.identity_idで指定したidentityではなく、1番目のidentityであったことが判明しました。テストスイートを見直してみましたが、どこにもおかしい点は見当たらず、名前とメールを更新するテストケースはパスしています。

だとすると、どこが問題なのでしょうか？
そこで、Active Recordのfindメソッドのソースコードを調べてみました¹。

# File activerecord/lib/active_record/core.rb, line 157
      def find(*ids) # :nodoc:
        # We don't have cache keys for this stuff yet
        return super unless ids.length == 1
        return super if block_given? ||
                        primary_key.nil? ||
                        scope_attributes? ||
                        columns_hash.include?(inheritance_column)

        id = ids.first

        return super if StatementCache.unsupported_value?(id)

        key = primary_key

        statement = cached_find_by_statement(key) { |params|
          where(key => params.bind).limit(1)
        }

        record = statement.execute([id], connection).first
        unless record
          raise RecordNotFound.new("Couldn't find #{name} with '#{primary_key}'=#{id}",
                                   name, primary_key, id)
        end
        record
      rescue ::RangeError
        raise RecordNotFound.new("Couldn't find #{name} with an out of range value for '#{primary_key}'",
                                 name, primary_key)
      end

ここで使われているsuperがとても気になったので、コンソールで以下のスニペットを実行してみました。

>> User.find(123) do |identity|
?>     puts identity
>>   end
  User Load (3.3ms)  SELECT `users`.* FROM `users`
#<User:0x00007faf2d800c30>
#<User:0x00007faf2d800af0>
#<User:0x00007faf2d8009b0>
#<User:0x00007faf2d800870>
#<User:0x00007faf2d800730>
#<User:0x00007faf2d8005f0>
#<User:0x00007faf2d8004b0>
#<User:0x00007faf2d800370>
#<User:0x00007faf2d800230>
#<User:0x00007faf2d8000f0>
=> nil

これでやっと何が起きているのかがわかりました。私のコードはDBテーブルの全レコードをひたすらイテレートし、渡したブロックをレコードごとに評価しようとしていたのです。

この振る舞いがtest環境の段階で即座にバリデーションで検出されたからよかったようなものの、このコードがもしproduction環境で実行されてuniquenessバリデーションが発生しなかったら、DBの全レコードがごっそり更新されてしまうという大変危険な事態になったかもしれません。

もうひとつ気づいたのは、私が書いたテストケースはこの問題を検出できるほど賢くなかったということです。テストではidentityを複数作成し、更新は少なくとも2つのidentityに対して行うべきです。

どうやって解決したかですか？方法は実に明白で、要は私がfind呼び出しにtapを足し忘れていただけだったのです。

class IdentityService
  # ...

  def update_personal_settings(command)
    Identity.find(command.identity_id).tap do |identity|
      identity.email = command.email
      identity.name = command.name
      identity.save!
      publish(
        PersonalSettingsUpdated.strict(
          data: {
            identity_id: identity.id,
            email: identity.email,
            name: identity.name
          }
        )
      )
    end
  end

  # ...
end

findに引数とブロックを両方渡すと、エラーなしで引数が無視されるので、これをバグとして報告するかどうか検討中です。少なくともこのような呼び出しでは、コードが意図通りに動作していないことにすぐ気付けるように、ブロックを渡すと引数が無視されるという警告を表示すべきではないかと考えています²。

関連記事

Rails: アプリケーションを静的解析で”防弾”する3つの便利ワザ（翻訳）

Rails: Active Recordモデルのスレッド安全性問題をインスタンス変数で解決する（翻訳）