作業ログバグ修正 2019/08/31

※この記事に登場するソースコードは僕個人が開発しているプロジェクトのものです

ざっくり仕様説明

ユーザーが複数の画像をドラッグアンドドロップでアップロードできて、ユーザーは別のユーザーに自分がアップロードした画像を送ることができる。みたいな機能があります。

ターゲットが少し特殊なサービスで基本的に画像は一眼レフで撮影されるもので、一枚あたり5MB以上のサイズになります。

そのままではでかすぎてまともにページが表示できなくなってしまうので、サムネイル用、プレビュー用に加工したものをバックグラウンドで処理して、出来上がったものから順番にウェブソケでブラウザにイベントを送って、ブラウザは一枚あたり3KB程度まで小さくなったサムネイル画像を表示します。

このとき加工が完了するまでの数秒間は「圧縮処理中」のような仮の画像を表示しておくことになります。

バグの内容

加工処理が完了したはずの画像を別のユーザーに送ったときに、受け取った側のユーザーが確認すると仮置き画像が表示されている

直す

まずは原因を見つける

Imageモデル（imagesテーブル）にthumbnail_urlというフィールドを持っていて、

これがnullの場合は仮置き画像のパスを返すメソッドを持っています。

  def thumbnail
    thumbnail_url || '仮置き画像のパス'
  end

DBを見てみると、送信側の画像では正しくthumbnail_urlが入っていましたが、受け取ったImageのthumbnail_urlがnullになっていることがわかりました。

ImageTransferServiceというサービスクラスが別のユーザーにImageを紐づけているのでそこを見てみる。

    # それっぽい場所
    images.each do |image|
      copied = folder.images.create(
        name: image.name,
      )
      ActiveStorage::Attachment.create(
        name: :file,
        record: copied,
        blob: image.file.blob,
      )
    end

あ、thumbnail_urlをコピーしてない。これっぽい。

単純にUserとImageと多対多にすればこの手のバグは回避しやすいんですが、そうしていないのは、仕様的に共有ではなく送信だからです（受信側で画像を削除したときに送信側でも消えてしまうようなことになるのは困る）

～～～～～～～～～～～～～～～～～～
ちなみにthumbnail_urlはパフォーマンス改善のために最近追加したやつで、そのときに考慮が漏れていたらしい。もともとはjbuilderで

image.file.variant(resize_to_fit: [100, 100], auto_orient: true, strip: true, quality: 70).processed

みたいな処理を呼んでいたんですが、つまり一度にimagesが（ページネーションあるので）25個あるとして、すでにすべての画像の加工処理が完了していたとしてもページアクセス時に25回processedが呼ばれることになります

速くバグ修正したいので説明ははしょりますがこれのせいでたった一人のアクセスがPumaのスレッドを瞬間的にすべて占有してしまう問題がおきていました。サムネイルのURL情報をこれを呼ばずに取得できるようにDBに移すことでスレッド奪いまくり問題を解決しました。詳しくはRubyのスレッド（特にGIL）とIO待ちについて調べてみてくださいm(,,)m
～～～～～～～～～～～～～～～～

これだけが原因かはわからないけど少なくとも原因のひとつであることには間違いないので修正します

テスト書く

fixtures/image.ymlにthumbnail_urlを追加

正常系については既存のテストでimage.nameをチェックしていたところがあるので、サムネイルも確認するように追加する

before

  # Folder has_many :images になっています
  assert_equal('foo1.png', folder.images[0].name)
  assert_equal('foo2.png', folder.images[1].name)
  assert_equal('foo3.png', folder.images[2].name)

after

  assert_equal('foo1.png', folder.images[0].name)
+ assert_equal('foo1_thumbnail.png', folder.images[0].thumbnail_url)
  assert_equal('foo2.png', folder.images[1].name)
+ assert_equal('foo2_thumbnail.png', folder.images[1].thumbnail_url)
  assert_equal('foo3.png', folder.images[2].name)
+ assert_equal('foo3_thumbnail.png', folder.images[2].thumbnail_url)

そういえば最近MiniTestつかってます。使い始めて3か月くらいだと思いますがいまはまだRSpecのほうが好きです。

で、走らすとちゃんと落ちる

Failure:
ImageTransferServiceTest#test_transfer_images_to_non_existing_email [/home/kenta-s/Repositories/oak/test/services/image_transfer_service_test.rb:57]:
Expected: "foo1_thumbnail.png"
  Actual: nil

コード本体修正

    images.each do |image|
      copied = folder.images.create(
        name: image.name,
+       thumbnail_url: image.thumbnail_url,
      )
      ActiveStorage::Attachment.create(
        name: :file,
        record: copied,
        blob: image.file.blob,
      )
    end

Run options: -n test_transfer_images_to_non_existing_email --seed 1429

# Running:

.

Finished in 0.779493s, 1.2829 runs/s, 23.0919 assertions/s.
1 runs, 18 assertions, 0 failures, 0 errors, 0 skips

通ったのでほかのテストも全部同じように修正する

異常系

thumbnail_urlがnilの場合は送信できないようにしたい。ということで異常系のテストを書く

テスト書く

+ test "images without thumbnail_url cannot be sent" do
+  # 省略
+  service = ImageTransferService.new(
+    sender: sender,
+    images: [
+      image_with_thumbnail_1,
+      image_with_thumbnail_2,
+      image_without_thumbnail_1,
+     ,
+    recipient_username_or_email: 'user@example.com',
+  )
+
+  service.call
+
+  # 省略
+
+  assert_equal(2, folder.images.count)
+
+  assert_equal('foo1.png', folder.images[0].name)
+  assert_equal('foo1_thumbnail.png', folder.images[0].thumbnail_url)
+  assert_equal('foo2.png', folder.images[1].name)
+  assert_equal('foo2_thumbnail.png', folder.images[1].thumbnail_url)
+ end

Failure:
ImageTransferServiceTest#test_images_without_thumbnail_url_cannot_be_sent [/home/kenta-s/Repositories/oak/test/services/image_transfer_service_test.rb:176]:
Expected: 2
  Actual: 3

ちゃんと落ちる。

コード本体修正

    images.each do |image|
+     next if image.thumbnail_url.nil?
      copied = folder.images.create(
        name: image.name,
        thumbnail_url: image.thumbnail_url,
      )
      ActiveStorage::Attachment.create(
        name: :file,
        record: copied,
        blob: image.file.blob,
      )
    end

Run options: -n test_images_without_thumbnail_url_cannot_be_sent --seed 51849

# Running:

.

Finished in 0.536750s, 1.8631 runs/s, 11.1784 assertions/s.
1 runs, 6 assertions, 0 failures, 0 errors, 0 skips

通った。のでブラウザで確認...したら直ってたのでmasterにマージしてデプロイ

画面のスクショとかなくてごめんちょ。

記事書きながら修正するの結構大変でした（汗