patorashのブログ

方向性はまだない

Rails6.0にアップグレードしたらActiveStorageでハマった件(追記あり)

最近は担当製品の関連アプリを改修しているのですが、Railsのバージョンを5.2系から6.0にアップグレードしました。そのときにActiveStorageでハマったので備忘録を残しておきます。なお、担当製品は既にRails6にしてあります。ただし、まだActiveStorage使ってない。その時の記事はこちら。

patorash.hatenablog.com

普通にRails6へのアップグレード

先ほどの記事を読みながらどういう対応をしたかを思い出して対応。一通り、手動で動かしてみたところ、問題なさそう。そして、RSpecを実行するとめちゃくちゃ落ちる!!びっくりするくらい落ちる!!

テストが落ちる原因を探る

テストが落ちた原因は、まぁRailsのバージョンを上げたのだから、バージョンの差異が原因でしょう。ActiveStorageが絡んでいるところが落ちていたので、調査しました。Qiitaに変更点がまとめられている記事がありました。ありがたい。

qiita.com

ここの中で「3. アップロードしたファイルがストレージに保存されるタイミングがsave時に変更」という見出しが…。

Rails5.2までは、インスタンスのattributeに代入した時点で、アップロードしたファイルがストレージに保存されていました。 Rails6.0では、saveしたタイミングでストレージに保存されるよう変更になっています。saveトランザクションがコミットされた後に、ストレージに保存されます。

おぉ…、まさにこれが原因でした…。

なぜsave後にアップロードでテストが落ちるのか?

理由は、バリデーションのためにActiveStorageにアップロードしたファイルの中身を参照して検証していたからです。

class Massage < ApplicationRecord
  has_many_attached :files

  validates :files, presence: true
  validate :check_files, if: -> { self.files.attached? }

  private

  def valid_files?
    self.files.all? do |file|
      file.blob.open do |f| # => Rails6だと、ここでopenできずに落ちる!
        # 検証する
      end
    end
  end

  def check_files
    self.errors.add(:files, :invalid) unless valid_files?
  end
end

ActiveStorage::Blobモデルが保存されていないため、ActiveStorage::FileNotFoundErrorで落ちてました。

アップロード前にファイルの内容を検証できるのか?

blobをopenしても落ちるので、何かいい方法はないものかと調べましたら、Railsのissueに有力な情報がありました。

github.com

attachment_changes['files'].attachablesを使え、とあります。(has_many_attachedならattachablesで、has_one_attachedならattachableになる)

pryを使いながら検証してみたところ、attachableの中身は、{ io: Fileオブジェクト, filename: 'ファイル名' }というHashでした。 そこで、以下のように修正。

def valid_files?
  self.attachment_changes['files'].attachables.pluck(:io).all? do |file|
    # 検証する
  end
end

これでRSpecを実行したところ…。modelのテストは通りました!修正成功です!

SystemSpecが落ちる…

しかし、テストを全部流してみたらSystemSpecが落ちました。手動で画面を操作してみたところ、確かに落ちます。しかも、先ほどの変更したところで…。 self.attachment_changes['files'].attachablesの中身がHashの配列ではなく、Stringの配列になっていました。なんでやねん…。

そもそも最初は手動テストで通ってのに、RSpecのテストが落ちるから修正したら、手動テストが通らなくなったので、もしかしたら画面からの場合はモデルの保存前でもActiveStorageにファイルがアップロードされてる?と考えてpryを仕込みながら調査したところ、ビンゴ。

2020-11-21 追記

もしかしたら画面からの場合はモデルの保存前でもActiveStorageにファイルがアップロードされてる?

これは、ActiveStorageのダイレクトアップロードしているからでした…。それなら当然ですね…。

画面からファイルをアップロードした場合

Messageモデルを保存していない状態で、blobオブジェクトの中身を見た結果。idがあるので、保存済みの証拠です。つまり、ActiveStorageにアプロード済み。(2020-11-21 追記:ダイレクトアップロードしていたからです!)

[1] pry(#<Message>)> self.files.first.blob
=> #<ActiveStorage::Blob:0x00007fb56b9479b0
 id: 320,
 key: "************************",
 filename: "test.csv",
 content_type: "text/csv",
 metadata: {},
 byte_size: 1452,
 checksum: "**************************",
 created_at: Fri, 20 Nov 2020 13:45:17 JST +09:00>
RSpecでファイルをattachした場合

テストでは、attachメソッドで付けてます。

message = FactoryBot.build(:message)
file = File.open(Rails.root.join('spec', 'files', 'test.csv'), 'r')
message.files.attach(io: file, filename: File.basename(file))

pryで中身を見た結果は、以下の通り。idnilのため、ActiveStorageにはまだアップロードされていません。

[1] pry(#<Message>)> self.files.first.blob
=> #<ActiveStorage::Blob:0x00007fdcfd305d68
 id: nil,
 key: nil,
 filename: "test.csv",
 content_type: "text/csv",
 metadata: {"identified"=>true},
 byte_size: 600,
 checksum: "**************************",
 created_at: nil>

導かれる仮説は…

2020-11-21 追記

ダイレクトアップロードしていたからだったので、この仮説は間違っていました。すみません。

  • 画面を経由せずにActiveStorageにアップロードする場合は、モデルの保存が成功したタイミングでアップロード。
  • 画面を経由してActiveStorageにアップロードする場合は、モデルの保存をする以前にアップロード。(これが間違い。ダイレクトアップロードしてたから)

となると、画面を経由しているケースを想定した処理と、経由していないケースを想定した処理を書かないといけません…。ここにすごく違和感を感じる…。

修正

とりあえず、コードは以下のようになりました。

class Massage < ApplicationRecord
  has_many_attached :files

  validates :files, presence: true
  validate :check_files, if: -> { self.files.attached? }

  private

  def valid_files?
    if uploaded? # => アップロード時は今まで同様の処理
      self.files.all? do |file|
        file.blob.open do |f|
          # 検証する
        end
      end
    else
      # テストにてattachで添付時はこちら
      self.attachment_changes['files'].attachables.pluck(:io).all? do |file|
        # 検証する
      end
    end
  end

  def check_files
    self.errors.add(:files, :invalid) unless valid_files?
  end

  def uploaded?
    self.files.all? { |file| file.blob.persisted? }
  end
end

これでテストは全て通るようになりました。やった!!!

不満点

違和感を感じると書いていますが、その違和感の正体は、「これだとモデルのユニットテストでActiveStorageにファイルが保存されているケースを検証できない」という点です。attachだと保存されないから…。画面からのアップロードでも、ActiveStorageのアップロード前の状態で、デフォルトで一時ファイルへのパス情報をどっかに保存していたらいいのに…。ダイレクトアップロードでなければ一時ファイルに保存されていることを確認したので、私が間違えていました。

何か他にいい方法をご存知の方がいらっしゃいましたらコメントでもツイッターでもいいので教えてください!!

2020-11-21 追記

画面からアップロードした際に先にクラウドにアップロードされていた原因はダイレクトアップロードをしていたためでした。先にコード読めよって話ですけど、自分が作ったところじゃなかったのでテストが落ちる原因にばかり考えが巡ってしまってActiveStorageのダイレクトアップロードの存在を忘れてました。このブログを書いた後に、急にピーン!ときました。

ダイレクトアップロードではない場合、当然ながらモデルが保存されるまではアップロードされず、一時ファイルに置かれていました。attachableの内容もHashではなく、ActionDispatch::Http::UploadedFileになっていたので、これに合わせて修正すればよさそうです。

>> self.attachment_changes['files'].attachables.first
=> #<ActionDispatch::Http::UploadedFile:0x00007f9d4a54ef18
 @tempfile=#<Tempfile:/var/folders/7m/9fg43zzx58lb4715wm1dkj00yj5swh/T/RackMultipart20201121-93972-xgi34u.csv>,
 @original_filename="test.csv",
 @content_type="text/csv",
 @headers="Content-Disposition: form-data; name=\"message[files][]\"; filename=\"test.csv\"\r\nContent-Type: text/csv\r\n">

となると………

  • ダイレクトアップロードの場合のバリデーション
  • ダイレクトアップロードじゃない場合のバリデーション
  • attachメソッドを使った場合のバリデーション

の3パターンを考えないといけないわけか…。うっ、頭が…。

(これはあくまでファイルの中身をチェックして保存したい場合の話なので、普通に画像をアップロードするだけ、とかだとこんな面倒な話にはなりませんのでActiveStorageが怖いわけではありません)

2020-11-26 追記

ダイレクトアップロードじゃないケースは実装上、ないので、そこはテストで放置して、一応attachメソッドも考慮したテストを追加して対応しました。 FactoryBotのコードは、こう。

FactoryBot.define do
  factory :message do
    sequence(:content) { |n| "メッセージ#{n}" }
  end

  trait :with_direct_upload_file do
    after :build do |message|
      valid_file = File.open(Rails.root.join('spec', 'files', 'valid_file.csv'), 'r')
      blob = ActiveStorage::Blob.create_and_upload!(
          io: valid_file,
          filename: File.basename(valid_file),
          identify: false
      )
      message.files.build(blob: blob)
    end
  end

  trait :with_attach_file do
    after :build do |message|
      valid_file = File.open(Rails.root.join('spec', 'files', 'valid_file.csv'), 'r')
      message.files.attach(io: valid_file, filename: File.basename(valid_file))
    end
  end
end

そして、RSpecはこう。

RSpec.describe Message, type: :model do
  context 'ダイレクトアップロードの場合' do
    subject { FactoryBot.build(:message, :with_direct_upload_file) }
    it '保存できること' do
      # 検証する
    end
  end

  context 'attachの場合' do
    subject { FactoryBot.build(:message, :with_attach_file) }
    it '保存できること' do
      # 検証する
    end
  end
end

とりあえず、これでモデルのテストでもダイレクトアップロードを考慮したテストを行うことができました。