アップロードされた画像からEXifデータを削除 #9298
アップロードされた画像からEXifデータを削除 #9298
Conversation
Walkthrough画像アップロード後に保存済み画像をMiniMagickで再読み込みしてEXIFメタデータを除去し、除去処理中に失敗した場合は画像を削除して422を返す Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller as API::ImageController
participant ImageModel as Image
participant Storage
participant MiniMagick
User->>Controller: POST /api/images (file)
Controller->>ImageModel: create(params)
ImageModel->>Storage: store original blob
Controller->>Controller: call strip_exif()
Controller->>Storage: read original blob
Storage-->>MiniMagick: image data
MiniMagick->>MiniMagick: strip metadata
MiniMagick-->>Controller: sanitized temp file
Controller->>ImageModel: reattach sanitized file (new filename)
alt strip_exif success
Controller-->>User: 201 Created + JSON
else strip_exif failure
Controller->>ImageModel: destroy
Controller-->>User: 422 Unprocessable Entity + error JSON
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分 注意点:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.81.7)app/controllers/api/image_controller.rbrubocop-minitest extension supports plugin, specify ... [truncated 255 characters] ... fig_loader_resolver.rb:76:in 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 |
matuaya
force-pushed
the
feature/strip-exif-from-image-in-markdown-editor
branch
from
1d3c65a to
31d0268
Compare
|
@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: 3
🧹 Nitpick comments (2)
test/integration/api/image_test.rb (1)
22-22: 保存された画像の取得方法が脆弱です。
Image.order(:created_at).lastを使用していますが、並行テストで他のテストが画像を作成すると、意図しない画像を取得する可能性があります。レスポンスボディから作成された画像のIDを取得する方が確実です。
以下の修正を検討してください：
post api_image_path(format: :json), params: { file: image_uploaded } + assert_response :created + response_json = JSON.parse(response.body) + saved_image = Image.find(response_json['id']) - saved_image = Image.order(:created_at).last processed_image = MiniMagick::Image.read(saved_image.image.download)app/controllers/api/image_controller.rb (1)
16-23: 画像処理ロジックのInteractorパターン検討を推奨します。現在の実装は動作しますが、コーディングガイドラインに従うと、外部リソース（アップロードファイル）を処理する場合はInteractorパターンの使用を検討することが推奨されています。
これにより、以下のメリットが得られます：
- コントローラーをスリムに保つ
- テストが容易になる
- エラーハンドリングをInteractor内でカプセル化できる
- 将来的に他の処理（画像リサイズなど）を追加する際に拡張しやすい
Based on coding guidelines
Interactorの実装例：
# app/interactors/strip_image_exif.rb class StripImageExif include Interactor def call original_image = context.image.image copied_image = MiniMagick::Image.read(original_image.download) copied_image.strip ext = File.extname(original_image.filename.to_s) timestamp = Time.current.to_i File.open(copied_image.path) do |file| original_image.attach(io: file, filename: "#{context.user_id}_#{timestamp}#{ext}") end rescue StandardError => e context.fail!(error: e.message) ensure copied_image&.destroy! end endコントローラーでの使用：
def create @image = Image.new(user: current_user, image: params[:file]) if @image.save result = StripImageExif.call(image: @image, user_id: current_user.id) if result.success? render :create, status: :created else @image.destroy render json: { error: result.error }, status: :unprocessable_entity end else render json: @image.errors, status: :unprocessable_entity end end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/image_controller.rb(1 hunks)test/integration/api/image_test.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
test/integration/api/image_test.rbapp/controllers/api/image_controller.rb
test/**/*
⚙️ CodeRabbit configuration file
test/**/*: # Test
- TestCase名は英語で書く。
- どうしても避けられない時以外にsystem testでsleepは使わない。
Unit Test
model, helper, decorator, view_componentについてはメソッドを追加した場合は必ず対応したUnit TestのTestCaseを1つは書く。
Files:
test/integration/api/image_test.rb
🔇 Additional comments (1)
test/integration/api/image_test.rb (1)
5-17: テストの構成が適切です。テスト名が英語で記述されており、テストケースの前提条件（Exifデータが存在すること）を事前に検証している点が良いです。コーディングガイドラインに従っています。
test/integration/api/image_test.rb
Outdated
| image_uploaded = fixture_file_upload(image_path, 'test.jpg') | ||
| post api_image_path(format: :json), params: { file: image_uploaded } |
There was a problem hiding this comment.
fixture_file_uploadの第2引数が不正です。
fixture_file_uploadの第2引数はコンテントタイプを指定すべきですが、ファイル名'test.jpg'が渡されています。正しくは'image/jpeg'を指定する必要があります。
また、アップロード後のレスポンスステータスを検証していないため、アップロードが成功したかどうかを確認できません。
以下の修正を適用してください：
- image_uploaded = fixture_file_upload(image_path, 'test.jpg')
+ image_uploaded = fixture_file_upload(image_path, 'image/jpeg')
post api_image_path(format: :json), params: { file: image_uploaded }
+ assert_response :created📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| image_uploaded = fixture_file_upload(image_path, 'test.jpg') | |
| post api_image_path(format: :json), params: { file: image_uploaded } | |
| image_uploaded = fixture_file_upload(image_path, 'image/jpeg') | |
| post api_image_path(format: :json), params: { file: image_uploaded } | |
| assert_response :created |
🤖 Prompt for AI Agents
In test/integration/api/image_test.rb around lines 19 to 20, fixture_file_upload
is passed an incorrect second argument (a filename) and the test doesn't assert
the upload response; change the second argument to the correct MIME type
'image/jpeg' when calling fixture_file_upload, and after the post request add an
assertion that the response status indicates success (e.g., assert_response
:success or assert_equal 201, response.status as appropriate for the endpoint)
to verify the upload succeeded.
There was a problem hiding this comment.
@matuaya さん、修正ありがとうございます！✨
修正内容を確認しました。fixture_file_uploadの第2引数が正しいコンテントタイプ（'image/jpeg'）に修正され、レスポンスステータスのアサーションも追加されていることが確認できました。これでテストがより堅牢になりましたね！
٩(◕‿◕｡)۶ 🐰✨
🧠 Learnings used
Learnt from: sekito1107
Repo: fjordllc/bootcamp PR: 9081
File: test/models/transcoder/movie_test.rb:0-0
Timestamp: 2025-08-26T07:31:10.235Z
Learning: Transcoder::Movie クラス（app/models/transcoder/movie.rb）では、Google::Cloud::Storage::FileVerificationError（Storage固有の例外）とGoogle::Cloud::Storage.new（メインクラス）の両方を使用しているため、テストファイルでは完全な google/cloud/storage gem の require が必要。google/cloud/errors gem だけでは不十分。
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
matuaya
force-pushed
the
feature/strip-exif-from-image-in-markdown-editor
branch
from
31d0268 to
870aad0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/controllers/api/image_controller.rb (1)
27-27: ファイルハンドルのリソースリークがあります。
File.openで開いたファイルハンドルが閉じられていません。大量のアップロードがあると「開きすぎたファイル」エラーが発生する可能性があります。以下のようにブロック形式を使用してファイルを自動的に閉じるようにしてください：
ext = File.extname(original_image.filename.to_s) - original_image.attach(io: File.open(copied_image.path), filename: "#{current_user.id}#{ext}") + File.open(copied_image.path) do |file| + original_image.attach(io: file, filename: "#{current_user.id}#{ext}") + end
🧹 Nitpick comments (2)
app/controllers/api/image_controller.rb (2)
7-13: エラーログの追加を検討してください。例外処理は適切に実装されていますが、デバッグのためにエラー情報をログに記録することを推奨します。
以下のようにログ記録を追加してください：
begin strip_exif render :create, status: :created - rescue StandardError + rescue StandardError => e + Rails.logger.error("Failed to strip EXIF data: #{e.message}\n#{e.backtrace.join("\n")}") @image.destroy render json: { error: '画像処理に失敗しました' }, status: :unprocessable_entity end
21-30: Interactorパターンの使用を検討してください。外部ライブラリ（MiniMagick）とのやりとりを含む処理のため、コーディングガイドラインに従ってInteractorパターンの使用を検討してください。
以下のようにInteractorを作成できます：
# app/interactors/strip_exif_from_image.rb class StripExifFromImage include Interactor def call original_image = context.image.image copied_image = MiniMagick::Image.read(original_image.download) copied_image.strip ext = File.extname(original_image.filename.to_s) File.open(copied_image.path) do |file| original_image.attach(io: file, filename: "#{context.user_id}#{ext}") end ensure copied_image&.destroy! end endコントローラーでの使用：
def create @image = Image.new(user: current_user, image: params[:file]) if @image.save result = StripExifFromImage.call(image: @image, user_id: current_user.id) if result.success? render :create, status: :created else @image.destroy render json: { error: '画像処理に失敗しました' }, status: :unprocessable_entity end else render json: @image.errors, status: :unprocessable_entity end endコーディングガイドラインに基づく
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/image_controller.rb(1 hunks)test/integration/api/image_test.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration/api/image_test.rb
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/controllers/api/image_controller.rb
🧠 Learnings (7)
📚 Learning: 2025-10-22T12:22:33.168Z
Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T12:22:33.168Z
Learning: CheckCallbacks内のメール通知・サイト内通知で例外が発生した場合、例外を握りつぶさず上位に伝搬させる。これにより通知失敗を検知し、運用上の混乱を防ぐ。CheckCallbacksはafter_create_commitで実行されるため、例外発生時にはCheckとステータス更新はすでにコミット済みであり、ユーザーにはエラーメッセージが表示されるが、DBには変更が残る状態になる。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-09-12T21:18:00.834Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-09-12T21:16:47.639Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-09-04T01:39:22.261Z
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-11-01T06:32:17.530Z
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9287
File: db/migrate/20250831044340_add_unique_index_to_buzzes_url_and_title.rb:1-6
Timestamp: 2025-11-01T06:32:17.530Z
Learning: Buzzモデルでは、仕様変更によりURLの重複は不可となった。URLには一意制約(unique index)が必要だが、タイトルの重複は許容される。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-08-24T16:19:17.661Z
Learnt from: sekito1107
Repo: fjordllc/bootcamp PR: 9081
File: app/models/transcoder/client.rb:98-102
Timestamp: 2025-08-24T16:19:17.661Z
Learning: In Ruby on Rails ActiveStorage code, when accessing the same attachment property multiple times within a method, it's better to use consistent syntax (e.g., always use `.blob.key` rather than mixing `.key` and `.blob.key`) for better readability and maintainability.
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-07-23T20:31:13.856Z
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 8977
File: app/controllers/reports_controller.rb:63-63
Timestamp: 2025-07-23T20:31:13.856Z
Learning: fjordllc/bootcampプロジェクトの`app/controllers/reports_controller.rb`において、`create`と`update`アクションは両方とも`report.save_uniquely`を使用し、同じ`:report_save`イベントと`'report.save'`イベントを発行する。これは両方とも本質的に「レポートの保存」操作であり、作成と更新を区別する必要がないためである。
Applied to files:
app/controllers/api/image_controller.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/image_controller.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/controllers/api/image_controller.rb
🧠 Learnings (7)
📚 Learning: 2025-10-22T12:22:33.168Z
Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T12:22:33.168Z
Learning: CheckCallbacks内のメール通知・サイト内通知で例外が発生した場合、例外を握りつぶさず上位に伝搬させる。これにより通知失敗を検知し、運用上の混乱を防ぐ。CheckCallbacksはafter_create_commitで実行されるため、例外発生時にはCheckとステータス更新はすでにコミット済みであり、ユーザーにはエラーメッセージが表示されるが、DBには変更が残る状態になる。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-09-12T21:18:00.834Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-09-12T21:16:47.639Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-09-04T01:39:22.261Z
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-11-01T06:32:17.530Z
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9287
File: db/migrate/20250831044340_add_unique_index_to_buzzes_url_and_title.rb:1-6
Timestamp: 2025-11-01T06:32:17.530Z
Learning: Buzzモデルでは、仕様変更によりURLの重複は不可となった。URLには一意制約(unique index)が必要だが、タイトルの重複は許容される。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-08-24T16:19:17.661Z
Learnt from: sekito1107
Repo: fjordllc/bootcamp PR: 9081
File: app/models/transcoder/client.rb:98-102
Timestamp: 2025-08-24T16:19:17.661Z
Learning: In Ruby on Rails ActiveStorage code, when accessing the same attachment property multiple times within a method, it's better to use consistent syntax (e.g., always use `.blob.key` rather than mixing `.key` and `.blob.key`) for better readability and maintainability.
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-07-23T20:31:13.856Z
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 8977
File: app/controllers/reports_controller.rb:63-63
Timestamp: 2025-07-23T20:31:13.856Z
Learning: fjordllc/bootcampプロジェクトの`app/controllers/reports_controller.rb`において、`create`と`update`アクションは両方とも`report.save_uniquely`を使用し、同じ`:report_save`イベントと`'report.save'`イベントを発行する。これは両方とも本質的に「レポートの保存」操作であり、作成と更新を区別する必要がないためである。
Applied to files:
app/controllers/api/image_controller.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
| begin | ||
| strip_exif | ||
| render :create, status: :created | ||
| rescue StandardError | ||
| @image.destroy | ||
| render json: { error: '画像処理に失敗しました' }, status: :unprocessable_entity | ||
| end |
There was a problem hiding this comment.
例外を記録せずに握りつぶしている問題があります。
現在の実装では、StandardErrorを捕捉して例外情報を記録せずに破棄しています。これには以下の問題があります：
- デバッグが困難: ログがないため、画像処理が失敗した原因を特定できません。
- 運用上の問題: 過去のレビューでの学習内容によると、通知処理などでは例外を握りつぶさず伝搬させることで、運用上の混乱を防ぐべきとされています。
- 過度に広い捕捉:
StandardErrorはプログラミングエラーまで含むため、予期しないバグを隠す可能性があります。
以下の修正を適用してください：
if @image.save
begin
strip_exif
render :create, status: :created
- rescue StandardError
+ rescue StandardError => e
+ Rails.logger.error("画像のEXIF削除処理に失敗しました: #{e.class} - #{e.message}")
+ Rails.logger.error(e.backtrace.join("\n"))
@image.destroy
render json: { error: '画像処理に失敗しました' }, status: :unprocessable_entity
end補足: より具体的なエラー（MiniMagick::Errorなど）のみを捕捉することも検討してください。
🤖 Prompt for AI Agents
In app/controllers/api/image_controller.rb around lines 7–13, the rescue
currently swallows StandardError without logging and may hide bugs; change it to
rescue only expected image-processing exceptions (e.g. MiniMagick::Error or
another specific class), log the exception and backtrace (Rails.logger.error or
logger.error) and send it to your error monitoring (e.g.
Sentry.capture_exception) before rendering the error response, and move
@image.destroy into an ensure block so cleanup always runs; if you intentionally
want fatal errors to bubble, re-raise after logging.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of the file to confirm the fix
cat app/controllers/api/image_controller.rb
Length of output: 991
@matuaya さん、修正ありがとうございます！
例外をキャプチャしてログに記録するようになったので、主要な問題は解決されました。これで画像処理が失敗した際に原因を特定できるようになります。
さらなる改善として以下も検討できますが、現時点の実装で十分機能します：
-
バックトレースの記録（デバッグをより容易にするため）:
Rails.logger.error("Failed to strip EXIF: #{e.message}\n#{e.backtrace.join("\n")}")
-
より具体的な例外クラスの捕捉（予期しないバグを隠さないため）:
rescue MiniMagick::Error => e
これらは必須ではありませんので、必要に応じて検討してください。
🧠 Learnings used
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 8977
File: app/controllers/reports_controller.rb:63-63
Timestamp: 2025-07-23T20:31:13.856Z
Learning: fjordllc/bootcampプロジェクトの`app/controllers/reports_controller.rb`において、`create`と`update`アクションは両方とも`report.save_uniquely`を使用し、同じ`:report_save`イベントと`'report.save'`イベントを発行する。これは両方とも本質的に「レポートの保存」操作であり、作成と更新を区別する必要がないためである。
Learnt from: sekito1107
Repo: fjordllc/bootcamp PR: 9023
File: app/interactors/process_transcoding_notification.rb:58-63
Timestamp: 2025-08-12T09:39:53.259Z
Learning: app/interactors/process_transcoding_notification.rbの`audio_missing_error?`メソッドは、AudioMissingエラーのみを検出することが意図されており、他の音声関連エラー（コーデック非対応、エンコード失敗など）は対象外。これは、AudioMissing以外のエラーに対してinclude_audio: falseでのリトライを行っても問題解決にならず、不適切なリトライが発生してしまうため。
Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T12:22:33.168Z
Learning: CheckCallbacks内のメール通知・サイト内通知で例外が発生した場合、例外を握りつぶさず上位に伝搬させる。これにより通知失敗を検知し、運用上の混乱を防ぐ。CheckCallbacksはafter_create_commitで実行されるため、例外発生時にはCheckとステータス更新はすでにコミット済みであり、ユーザーにはエラーメッセージが表示されるが、DBには変更が残る状態になる。
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| def strip_exif | ||
| original_image = @image.image | ||
| copied_image = MiniMagick::Image.read(original_image.download) | ||
| copied_image.strip | ||
|
|
||
| ext = File.extname(original_image.filename.to_s) | ||
| timestamp = Time.current.strftime('%Y%m%d%H%M%S') | ||
| original_image.attach(io: File.open(copied_image.path), filename: "#{current_user.id}_#{timestamp}#{ext}") | ||
| ensure | ||
| copied_image&.destroy! | ||
| end |
There was a problem hiding this comment.
ファイルハンドルのリークが発生します。
Line 28のFile.open(copied_image.path)がブロック形式ではないため、ファイルハンドルが明示的にクローズされません。ensure節はcopied_imageのみを破棄しており、開いたファイルハンドルは残ったままになります。
以下の修正を適用してください：
def strip_exif
original_image = @image.image
copied_image = MiniMagick::Image.read(original_image.download)
copied_image.strip
ext = File.extname(original_image.filename.to_s)
timestamp = Time.current.strftime('%Y%m%d%H%M%S')
- original_image.attach(io: File.open(copied_image.path), filename: "#{current_user.id}_#{timestamp}#{ext}")
+ File.open(copied_image.path) do |file|
+ original_image.attach(io: file, filename: "#{current_user.id}_#{timestamp}#{ext}")
+ end
ensure
copied_image&.destroy!
endその他の改善提案:
- タイムスタンプの粒度: 秒単位のタイムスタンプは、同時アップロードで衝突する可能性があります。ミリ秒を含めるか、
SecureRandom.hexを追加することを検討してください。
timestamp = Time.current.strftime('%Y%m%d%H%M%S%L') # ミリ秒を含める
# または
timestamp = "#{Time.current.to_i}_#{SecureRandom.hex(4)}"- Interactorパターンの検討: コーディングガイドラインによると、複数のActiveRecordモデル操作や外部API（MiniMagick）とのやりとりがある場合は、Interactorパターンの使用を検討すべきとされています。この処理は画像の保存、EXIF削除、再アタッチという複数の責務を持つため、
StripImageExifのようなInteractorに切り出すことで、コントローラーをよりシンプルに保てます。
Based on coding guidelines
🤖 Prompt for AI Agents
app/controllers/api/image_controller.rb lines 21-31:
File.open(copied_image.path) is called without a block causing a file-handle
leak; change to using File.open with a block (or ensure the returned IO is
closed in an ensure) when calling original_image.attach so the IO is closed
after attach, keep the existing copied_image&.destroy! in ensure, and optionally
strengthen the timestamp to include milliseconds or a SecureRandom suffix to
avoid filename collisions.
dfbf465 to
d236131
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/controllers/api/image_controller.rb (1)
10-11: バックトレースをログに記録してください。現在の実装では例外メッセージのみをログに記録していますが、バックトレースがないため本番環境でのデバッグが困難です。また、
StandardErrorの捕捉範囲が広すぎるため、予期しないプログラミングエラーまで握りつぶしてしまう可能性があります。以下の修正を適用してください：
- rescue StandardError => e - Rails.logger.error("Failed to strip EXIF: #{e.message}") + rescue MiniMagick::Error, ActiveStorage::FileNotFoundError => e + Rails.logger.error("Failed to strip EXIF: #{e.class} - #{e.message}") + Rails.logger.error(e.backtrace.join("\n")) @image.destroy render json: { error: '画像処理に失敗しました' }, status: :unprocessable_entity補足: より具体的な例外型（
MiniMagick::Errorなど）のみを捕捉することで、予期しないエラーを隠さずに済みます。
🧹 Nitpick comments (1)
app/controllers/api/image_controller.rb (1)
22-34: 実装が大幅に改善されました！過去のレビューフィードバックに対応し、以下の点が適切に実装されています：
- ブロック形式での
File.openによるファイルハンドルの適切なクローズ- ミリ秒を含むタイムスタンプによるファイル名衝突の回避
ensureブロックによる一時ファイルのクリーンアップ任意の改善提案: コーディングガイドラインによると、外部API（MiniMagick）とのやりとりがある場合はInteractorパターンの使用が推奨されています。現在の実装は動作しますが、将来的なメンテナンス性向上のため、
StripImageExifのようなInteractorクラスへの切り出しを検討してもよいかもしれません。# app/interactors/strip_image_exif.rb (例) class StripImageExif include Interactor def call original_image = context.image.image copied_image = MiniMagick::Image.read(original_image.download) copied_image.strip ext = File.extname(original_image.filename.to_s) timestamp = Time.current.strftime('%Y%m%d%H%M%S%L') File.open(copied_image.path) do |file| original_image.attach(io: file, filename: "#{context.user_id}_#{timestamp}#{ext}") end ensure copied_image&.destroy! end endただし、これは必須ではなく、現在のシンプルな実装で十分機能している場合はこのままでも問題ありません。
Based on coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/image_controller.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rb
⚙️ CodeRabbit configuration file
**/*.rb: # refactoring
- まずFat Controllerを避け、次にFat Modelを避ける。
- Serviceクラスの乱用を避ける。
- controller concernを作ろうとしたらPORO(Plain Old Ruby Object)やActiveRecordモデルでの実装で代替できないか検討する。
Rails Patterns
- ViewHelperにメソッドを実装する時にはまずDecoratorパターンを使うことを検討する。（active_decorator gemを導入しているのでそれを使う）
- 複雑なActiveRecordクエリがあり、再利用できそうな場合はQueryObjectパターンを検討する。（rails-patterns gemを導入しているのでそれのQuery機能を使う）
- Viewにpartialを作る場合はViewComponentを使うことを検討する。
- 複数のActiveRecordモデルを操作する１つの責務がある時や外部APIとやりとりする処理がある場合にはInteractorオブジェクトパターンを検討する。（interactor gemを導入しているのでそれを使う）
- 複数のInteractorを実行するような処理がある場合Organizerオブジェクトパターンを検討する。（interactor gemを導入しており、その中にOrganizerの機能があるので使う）
Files:
app/controllers/api/image_controller.rb
🧠 Learnings (11)
📓 Common learnings
Learnt from: sekito1107
Repo: fjordllc/bootcamp PR: 9081
File: test/models/transcoder/movie_test.rb:0-0
Timestamp: 2025-08-26T07:31:10.235Z
Learning: Transcoder::Movie クラス（app/models/transcoder/movie.rb）では、Google::Cloud::Storage::FileVerificationError（Storage固有の例外）とGoogle::Cloud::Storage.new（メインクラス）の両方を使用しているため、テストファイルでは完全な google/cloud/storage gem の require が必要。google/cloud/errors gem だけでは不十分。
📚 Learning: 2025-10-22T12:22:33.168Z
Learnt from: sjabcdefin
Repo: fjordllc/bootcamp PR: 9247
File: app/controllers/checks_controller.rb:0-0
Timestamp: 2025-10-22T12:22:33.168Z
Learning: CheckCallbacks内のメール通知・サイト内通知で例外が発生した場合、例外を握りつぶさず上位に伝搬させる。これにより通知失敗を検知し、運用上の混乱を防ぐ。CheckCallbacksはafter_create_commitで実行されるため、例外発生時にはCheckとステータス更新はすでにコミット済みであり、ユーザーにはエラーメッセージが表示されるが、DBには変更が残る状態になる。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-09-12T21:18:00.834Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/notifiers/discord_notifier.rb:131-135
Timestamp: 2025-09-12T21:18:00.834Z
Learning: Rails アップグレードPRにおいて、product_review_not_completed メソッドの webhook URL 設定の shared フォールバック追加も、設定システム全体の変更として別PRで対応すべきである。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-09-12T21:16:47.639Z
Learnt from: komagata
Repo: fjordllc/bootcamp PR: 9101
File: app/models/graduation_notifier.rb:23-28
Timestamp: 2025-09-12T21:16:47.639Z
Learning: Rails upgrade PRにおいて、configuration systemの変更やwebhook設定の改善提案も、アップグレードに直接関連しない場合は別PRで対応すべきである。PRのスコープ維持が重要。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-09-04T01:39:22.261Z
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: app/controllers/mentor/buzzes_controller.rb:0-0
Timestamp: 2025-09-04T01:39:22.261Z
Learning: fjordllc/bootcamp プロジェクトでは、更新処理の成功時の通知メッセージについて、I18n を使用せずに日本語文字列を直接記述する方針で一貫性を保っている。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-11-01T06:32:17.530Z
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9287
File: db/migrate/20250831044340_add_unique_index_to_buzzes_url_and_title.rb:1-6
Timestamp: 2025-11-01T06:32:17.530Z
Learning: Buzzモデルでは、仕様変更によりURLの重複は不可となった。URLには一意制約(unique index)が必要だが、タイトルの重複は許容される。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-08-24T16:19:17.661Z
Learnt from: sekito1107
Repo: fjordllc/bootcamp PR: 9081
File: app/models/transcoder/client.rb:98-102
Timestamp: 2025-08-24T16:19:17.661Z
Learning: In Ruby on Rails ActiveStorage code, when accessing the same attachment property multiple times within a method, it's better to use consistent syntax (e.g., always use `.blob.key` rather than mixing `.key` and `.blob.key`) for better readability and maintainability.
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-07-23T20:31:13.856Z
Learnt from: jun-kondo
Repo: fjordllc/bootcamp PR: 8977
File: app/controllers/reports_controller.rb:63-63
Timestamp: 2025-07-23T20:31:13.856Z
Learning: fjordllc/bootcampプロジェクトの`app/controllers/reports_controller.rb`において、`create`と`update`アクションは両方とも`report.save_uniquely`を使用し、同じ`:report_save`イベントと`'report.save'`イベントを発行する。これは両方とも本質的に「レポートの保存」操作であり、作成と更新を区別する必要がないためである。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-09-04T01:50:47.773Z
Learnt from: Miya096jp
Repo: fjordllc/bootcamp PR: 9102
File: db/migrate/20250831234516_remove_unique_index_from_buzzes_title.rb:1-5
Timestamp: 2025-09-04T01:50:47.773Z
Learning: fjordllc/bootcampプロジェクトでは、マイグレーションにおいてdisable_ddl_transaction!やalgorithm: :concurrentlyは使用せず、標準的なRailsのchangeメソッド内でremove_indexを実行する慣例がある。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-08-12T09:39:53.259Z
Learnt from: sekito1107
Repo: fjordllc/bootcamp PR: 9023
File: app/interactors/process_transcoding_notification.rb:58-63
Timestamp: 2025-08-12T09:39:53.259Z
Learning: app/interactors/process_transcoding_notification.rbの`audio_missing_error?`メソッドは、AudioMissingエラーのみを検出することが意図されており、他の音声関連エラー（コーデック非対応、エンコード失敗など）は対象外。これは、AudioMissing以外のエラーに対してinclude_audio: falseでのリトライを行っても問題解決にならず、不適切なリトライが発生してしまうため。
Applied to files:
app/controllers/api/image_controller.rb
📚 Learning: 2025-08-24T07:39:34.701Z
Learnt from: sekito1107
Repo: fjordllc/bootcamp PR: 9081
File: app/jobs/transcode_job.rb:11-24
Timestamp: 2025-08-24T07:39:34.701Z
Learning: In the bootcamp application's TranscodeJob, exceptions are not re-raised after logging/notification because the job runs via after_create_commit. The Movie creation has already succeeded, so re-raising would cause users to see a creation failure screen, degrading user experience. Instead, failures are logged and sent to Rollbar for monitoring only.
Applied to files:
app/controllers/api/image_controller.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
|
@jun-kondo |
|
@matuaya |
|
@jun-kondo |
jun-kondo
left a comment
jun-kondo
left a comment
There was a problem hiding this comment.
動作確認してexifが削除されていることを確認しました。
問題ないと思いますのでapprove致します。
fixture_file_uploadを使って画像処理の単体テストの書き方などとても参考になりました🙋♂️
|
@jun-kondo |
|
@okuramasafumi |
okuramasafumi
left a comment
okuramasafumi
left a comment
There was a problem hiding this comment.
提案
この処理はコントローラーではなくてモデルのコールバックに書いたほうがよいかと思いました。
理由
将来的に、画像をImageController以外で保存することがあるかもしれません。別のモデルを経由して作成されたりすることが特に考えられます。例えば、
some_model.images.build(image_params)このとき、モデルに処理を書いておけば処理の漏れが起こる可能性が低下します。
実装方針
基本的にはstrip_exifをモデルのプライベートメソッドに移動し、before_saveで呼ぶだけで大丈夫だと思います。
考慮事項
モデルのコールバック、特にbefore_saveを使うということはそれがそのモデルの「要件」であることを意味します。ここでは画像からEXIFデータを削除することが要件なのかを考えることが必要です。要件であるならば、EXIFデータの削除に失敗した場合はモデルの保存自体が失敗することが望ましいことになり、before_saveの使いどころとなります。
matuaya
force-pushed
the
feature/strip-exif-from-image-in-markdown-editor
branch
from
d236131 to
4fc90cd
Compare
|
@okuramasafumi |
|
@matuaya 返信が遅くなってしまってすみません！ |
Issue
概要
Markdownエディタにアップロードした画像にはEifFデータが残っており、情報漏洩のリスクがありました。
今回の変更でEXif情報が削除される処理を追加し、セキュリティを向上させました。
変更確認方法
feature/strip-exif-from-image-in-markdown-editorをローカルに取り込むforeman start -f Procfile.dev）補足
プロフィール画像をアップロードするときにExif情報を削除してリネームするようにした #4963を参考にしました。
Screenshot
アプリ自体にはUIの変化はないため、スクリーンショットは省略します。
Summary by CodeRabbit
新機能
テスト