画像からExifが削除されないバグの修正#9907
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEXIF除去処理をモデルのafter_commitからAPI::ImageController#create内で明示的にstrip_exif!を呼ぶ方式へ移行。Imageモデルにstrip_exif!と画像添付必須検証が追加され、テストは保存後URLとblob.signed_idの照合を追加。 ChangesEXIF strip refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/exif_stripper.rb`:
- Around line 3-13: Add a unit test for ExifStripper.call: create
test/models/exif_stripper_test.rb with at least one happy-path test that builds
a fake uploaded_file (e.g., fixture or Tempfile with known image bytes and
stubbed original_filename/content_type), calls ExifStripper.call(uploaded_file),
and asserts the returned hash has an :io responding to read (or to_blob
equivalence), filename equals uploaded_file.original_filename.to_s, and
content_type equals uploaded_file.content_type; ensure the test cleans up temp
files and uses MiniTest assertions consistent with the existing test suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a39b98d4-83ea-4db6-b78a-543432b534b5
📒 Files selected for processing (4)
app/controllers/api/image_controller.rbapp/models/exif_stripper.rbapp/models/image.rbtest/integration/api/image_test.rb
💤 Files with no reviewable changes (1)
- app/models/image.rb
🚀 Review AppURL: https://bootcamp-pr-9907-fvlfu45apq-an.a.run.app
|
matuaya
force-pushed
the
bug/fix-exif-not-removed
branch
3 times, most recently
from
983d229 to
a0a5b37
Compare
|
@komagata |
| @image = Image.new(user: current_user) | ||
| processed_image = ExifStripper.call(params[:file]) | ||
| @image.image.attach(processed_image) |
There was a problem hiding this comment.
| @image = Image.new(user: current_user) | |
| processed_image = ExifStripper.call(params[:file]) | |
| @image.image.attach(processed_image) | |
| @image = Image.new(user: current_user, image: ExifStripper.call(params[:file])) |
こういう感じでいいかも？
| @@ -0,0 +1,14 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| class ExifStripper | |||
There was a problem hiding this comment.
ExifStripper という名前だと「Exifを削除する処理」（動詞）に注目した名前になっていて、手続き的な印象があります。
クラス名は原則名詞なので名詞に注目した名前がいいと思います。
そういう意味ではImageクラスは既にそうなっているのでそちらに処理があるほうが自然かなと思いました。
There was a problem hiding this comment.
アドバイスいただきありがとうございます！
サービスクラスを削除し、モデルに処理を移動しました。
matuaya
force-pushed
the
bug/fix-exif-not-removed
branch
from
a0a5b37 to
b6103c8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/models/image.rb (1)
7-18: エラーハンドリングの追加を検討してください（任意）。
MiniMagick::Image.readは非画像ファイルや破損ファイルを渡された場合にMiniMagick::Invalidを送出する可能性があります。現状はコントローラで rescue されていないため、不正なファイルがアップロードされると 500 応答になります。ユーザー入力を直接処理するパスなので、例外を捕捉してerrors.addでバリデーションエラーに変換するか、コントローラ側で rescue するとより堅牢になります。また、
uploaded_fileがnilや想定外のオブジェクト（tempfile/original_filename/content_typeを持たない）だった場合もNoMethodErrorになるため、事前ガードがあるとより安全です。♻️ 提案例
def strip_exif(uploaded_file) + return if uploaded_file.blank? + original_image = MiniMagick::Image.read(uploaded_file.tempfile) original_image.strip blob = { io: StringIO.new(original_image.to_blob), filename: uploaded_file.original_filename.to_s, content_type: uploaded_file.content_type } image.attach(blob) + rescue MiniMagick::Invalid => e + errors.add(:image, :invalid, message: "画像の処理に失敗しました: #{e.message}") end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/image.rb` around lines 7 - 18, strip_exifでMiniMagick::Image.readに渡された不正な入力がそのまま例外になるため、uploaded_fileがnilまたは予期しないオブジェクトかを先にガードし、MiniMagick::Image.readをbegin/rescueで囲んでMiniMagick::Invalid（およびNoMethodError/TypeError）を捕捉してerrors.add(:image, "invalid image")のようにモデルのバリデーションエラーに変換するかfalseを返してimage.attachを呼ばないようにしてください（メソッド名: strip_exif、例外: MiniMagick::Invalid、参照先: uploaded_file.original_filename / tempfile / content_type、エラー報告: errors.add / image.attachをスキップ）。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/models/image.rb`:
- Around line 7-18:
strip_exifでMiniMagick::Image.readに渡された不正な入力がそのまま例外になるため、uploaded_fileがnilまたは予期しないオブジェクトかを先にガードし、MiniMagick::Image.readをbegin/rescueで囲んでMiniMagick::Invalid（およびNoMethodError/TypeError）を捕捉してerrors.add(:image,
"invalid
image")のようにモデルのバリデーションエラーに変換するかfalseを返してimage.attachを呼ばないようにしてください（メソッド名:
strip_exif、例外: MiniMagick::Invalid、参照先: uploaded_file.original_filename /
tempfile / content_type、エラー報告: errors.add / image.attachをスキップ）。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cafe8d7d-bc64-48fc-be89-7b46a0ab0099
📒 Files selected for processing (3)
app/controllers/api/image_controller.rbapp/models/image.rbtest/integration/api/image_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- test/integration/api/image_test.rb
- app/controllers/api/image_controller.rb
|
@komagata |
| def create | ||
| @image = Image.new(user: current_user, image: params[:file]) | ||
| @image = Image.new(user: current_user) | ||
| @image.strip_exif(params[:file]) |
There was a problem hiding this comment.
主語（名詞）.述語（動詞）
のように読める必要があるともうのですが、imageの情報が引数にもあってわかりづらくなっていると思います。
@image = Image.new(user: current_user, file: params[:file])
@image.strip_exif! # 事故破壊的メソッドなので!をつけるみたいな感じの方が自然かなと思います。
There was a problem hiding this comment.
アドバイスをいただきありがとうございます！
アップロードされたファイルを保存前にモデル内でアクセスする方法が見つけられずにいるのですが、
@image = Image.new(user: current_user, file: params[:file])というのは、fileという仮の属性を通してparams[:file]をモデルでアクセスできるようにするということでしょうか？
教えていただけると嬉しいです🙇♀️
There was a problem hiding this comment.
@matuaya 上記はイメージをお伝えするための仮のコードです。
モデル内でアクセスする方法
僕がちょっと意味がわかってないかもですが、モデル内でアクセスできる必要はないと思います。controllerでアクセスできればそれをmodelに渡せばいいはずですので。
controllerにはhttp的に必ずデータが渡っているはずなのでどうやってruby/railsで取得するかについては調べてみてください。
There was a problem hiding this comment.
アドバイスいただきありがとうございます！
attr_accessorを使用して、アップロードデータを取得するようにしました。
調べたところ、ActiveStorageのattachment_changesを使用するとsave前の画像を取得できることもわかったのですが、"undocumented internal API" と説明されているのを見て、もしかしたら使用を避けた方がよいかもしれないと思い、attr_accessorを使ってデータを取得するようにしました。
もし認識が違っていましたらご指摘いただけると嬉しいです🙇♀️
There was a problem hiding this comment.
@matuaya attr_accessorは外部から使えるアクセサを定義するものなのでクラス内の処理には関係がないことだと思います。
There was a problem hiding this comment.
@komagata
色々調べて試行錯誤しましたが、@image = Image.new(user: current_user, image: params[:file]) とした後で、params[:file]をモデルから扱う方法が分かりませんでした。
blobへはアクセスできますが、tempfileやdownloadといったメソッドではデータへのアクセスできず、attachment_changes以外でアップロードされたファイルの取得方法がわかりません。
すでにこれ以上にないくらいアドバイスを頂いていて申し訳ないのですが、教えていただけますと嬉しいです🙇♀️
There was a problem hiding this comment.
@matuaya すみません、ちょっとどの点がわからないのかわかっていないかもです。
@image = Image.new(user: current_user, image: params[:file])
コンストラクタに渡しているのであれば、それをクラスの中で使えばいいと思うのですが、どの部分がわからないでしょうか？
お手数ですが、もう少し詳細に言葉を尽くして説明していただけると非常にありがたいです。
There was a problem hiding this comment.
@komagata
説明が足りず申し訳ありません🙇♀️
@image = Image.new(user: current_user, image: params[:file])ですが、モデル内でself.imageはActiveStorage::Attached::Oneオブジェクトだと分かりました。
Minimagickでファイルの処理をするにはファイルパスが必要だと理解しているのですが、 ActiveStorage::Attached::Oneからファイルパスを得られる手段が分かりませんでした。
それならActiveStorage::Blobオブジェクトだったら可能なのではないかと思いimage.attachment.blob.downloadを試してみましたが、ActiveStorage::FileNotFoundErrorとなってしましました。
image: params[:file]と渡した後に、モデルのself.imageを使ってその後どのActiveStorageオブジェクトやメソッドから画像のデータやファイルパスを取得できるのか分からずにいます。
全く見当違いのところを見ているのかもしれないですが、同じ場所でずっとつっかえています🙇♀️
There was a problem hiding this comment.
@komagata
会議でもアドバイスをくださりありがとうございます！
それにもかかわらずあまり理解できていません🙇♀️
tempfileを作る方法と、ファイルパスではなくデータ自体をMinimagickに渡すやり方を説明していただいたと思うのですが、具体的な方法が思いつかず悩んでいます。お教えいただけると嬉しいです🙇♀️
あと、
params[:file]は引数として渡しているだけなので、モデル内でもモデル外でも同じものだと思うのですが、どういう意味でおっしゃっていますか？
完全な理解不足ではあると思うのですが、self.image = params[:file]だと考えていました。だけれどself.image.tempfileのようなことはできなかったので直接的には必要な情報は得られないのだと考えていました。
matuaya
force-pushed
the
bug/fix-exif-not-removed
branch
from
b6103c8 to
c8c142d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/api/image_controller.rb`:
- Around line 5-6: リクエストに file がないときに Line 6 の `@image.strip_exif`! が例外を投げて 422
分岐に到達できないため、コントローラの先頭で params[:file] の存在をガードし、なければ早期に 422 を返すようにしてください；具体的には
action 内で params[:file].present? をチェックして不在なら render json: { error: ... },
status: :unprocessable_entity を返し、存在する場合のみ Image.new(user: current_user, file:
params[:file]) と `@image.strip_exif`! を実行するように流れを分岐してください（関連シンボル: params[:file],
Image.new, `@image.strip_exif`!, current_user）。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 79ce7d96-da56-440e-b84e-be4d9211d538
📒 Files selected for processing (3)
app/controllers/api/image_controller.rbapp/models/image.rbtest/integration/api/image_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration/api/image_test.rb
|
@komagata |
2 participants
Issue
概要
こちらのPR（アップロードされた画像からExifデータを削除#9298）にて画像からExifデータを削除する機能を追加していましたが、本番環境では削除処理が正常に動作していなかったため本PRで修正対応しました。
変更確認方法
bug/fix-exif-not-removedをローカルに取り込むアップロードされた画像からExifデータを削除#9298の確認方法と同じです。
Summary by CodeRabbit
Flakewatch Report
Review App