Railsでエラー処理を共通化する

先日チャットを使った会話の中で、Railsでエラー処理をどこで書くかの議論をさせてもらいました。

私がふとした瞬間にエラー処理を全部コントローラ側に集約したらわかりやすくなるのではないかと、なんとなく思い込んでしまって以下のような投稿をしたのがきっかけです。

kaneko [10:35 AM]
@channel: Exceptionをどこで拾うかの議論

個人的な考え（RubyOnRailsにおいて）
Exceptionは全てコントローラで拾う
モデル層やサービス層ではExceptionを拾わず、エラーを発生させっぱなしにする
コントローラ側はExceptionを拾い共通処理を使ってログ出力やその他の適切な処理を行う

この考えにご意見をいただけたらと思います

すると早速以下の返信を頂きました。

yamama [10:59 AM]
んーと

yamama [10:59 AM]
深く考えたワケじゃないんだけど

yamama [11:01 AM]
たとえばとあるcontrollerでDBからselectした結果をJSONにして返すというmodelを使ってるとして

yamama [11:01 AM]
そのmodelで起こりそうな例外って例えばDBに接続できないとか、そんなIDないとか、JSONに変換できなかったとか、まぁ色々あると思うけど

yamama [11:02 AM]
controller側で必要なのって「データの取得に失敗しました」ってステータスだけだと思うのね。どうして失敗したのかはmodelだけが知ってればいいんじゃないかと。

yamama [11:03 AM]
なのでmodelからはreturn ‘{“hoge”: “moge”}’;的なJSONが返るか、return false;的に失敗したというステータスが返ればいいんじゃないのかなぁと。

要するに全てコントローラ側に丸投げするのではなく、各オブジェクトが自身のエラー内容を把握しているべき、エラー処理も自身が行う
ということでした。
たしかにそうだと思いました。納得しました。
なぜ私はこのような提案をしてしまったのだろうと、少し後悔した瞬間でした。

そこで私はこのように返答しました。

kaneko [11:05 AM]
@yamama: 実は今とある案件で発生した問題があって

kaneko [11:05 AM]
それが話の発端でした。
ご意見ありがとうございます :bow:

kaneko [11:05 AM]
発生した問題とは、モデルやサービス層の処理で以下のように実装されている部分があって、エラーを特定するのにえらい苦労したということです。

kaneko [11:06 AM]

def hozon
  hoge.save!
  return true
rescue => e
  return false
end

kaneko [11:08 AM]
このような処理でExceptionが発生した時に、return falseになっているだけでログ出力がされないので、どこでエラーになっているかわからず、しかも環境依存の問題などは本番環境などで発生してもログを追うことができないので特定が難しくなってしまいます。

kaneko [11:09 AM]
そこで、私がコントローラ側でExceptionを渡したら適切にエラーログを吐き出す共通処理をつくりました
その共通処理を使ってログ出力するところがモデルだったり、コントローラだったり、サービス層だったりしているので、Exceptionはコントローラ側で制御させるように決めて二度とreturn false問題を発生させないようにしたいと考えた次第です。

kaneko [11:10 AM]
しかしよく考えたら、エラーの詳細はそれぞれのオブジェクトが知っていれば良いと思います。
結局は適切にエラーログを出していないのが問題なので、例外処理したら必ずログを出力しようということを呼びかける、という結果に至りそうです。

要するに起きていた問題としては、例外が発生してもfalseをreturnするだけになっていたところがあって、
エラーが特定出来なかったということでした。
そのため、最低限ログ出力は入れようと呼びかければ良いということになりそうでした。

yamama [11:10 AM]
それこそ密結合になりすぎるんじゃないかなー。

yamama [11:11 AM]
rescueしてるってことはそこで何かが起こる可能性があることは知ってるわけだからそこでログ出力なりすればいいんじゃないかな。

kaneko [11:11 AM]
成功したらtrueや値を返して、失敗したらfalseやnilを返すのが良さそうですね　サービス層やモデルでは

kaneko [11:13 AM]
ご指摘のとおりだと思います
すっきりしてきました

ここで納得しようとしたら先輩に怒られました。

yamama [11:16 AM]
ダメだ！1つの意見だけに左右されちゃダメだ！！

という事で他の人の意見を待つことに

lanches-kaneko [11:18 AM]
少し他の人の意見を待ちます

takano [12:03 PM]
僕はコントローラのコードを極力シンプルにしたいのでサービス層でexceptionひろいます

lanches-kaneko [12:33 PM]
@lanches-takano:
確かに、コントローラは薄くしたいです

コントローラ側でなく、モデルやサービス層でexceptionを拾うという意見がでました
確かにコントローラのコードは薄くしたいと思いました。

蒼い惑星 [4:15 PM]
単に「Exception」を言っていますが、実は、想定外と想定内と二種類があると思います。

・想定外のExceptionは、 @kaneko さんの仰った通りに、ApplicationControllerで共通処理で拾ってログ出力したりして良いと思います。
・想定内のExceptionは、 @yamama さんの仰ったように、rescue している箇所は、ある異常なケースを想定して、それが発生する場合は、特別な処理（エラーではなく親切なメッセージを出すとか）をしたいわけなので、それを全部共通処理でやるのは無理と言いますか、良くないとも思います。

よって、まとめると
・想定外のException処理はApplicationControllerの共通処理に任せる
・ rescue をやりたいのであれば、ちゃんと共通のメソッドでエラー情報を出すように、開発メンバーが認識しておく
で、良いじゃないかと思います。

yamama [4:28 PM]
ふと思ったんですが「想定外のException」ってなんでしょうね。

yamama [4:29 PM]
Exceptionが上がりうる場所は（本来であれば）全部分かってないとならない、ような気がしなくもないです。

ここで想定外と想定内で分ける、という意見がでました。

lanches-kaneko [4:53 PM]
@蒼い惑星: 確かに、Exceptionによって分ける案もあるかと思います
しかし、 @yamama さんのいうとおり、全部想定して全て拾えるようにしておかないといけないとは思います。
おそらく @蒼い惑星さんの言っているのは想定内というよりは、想定できるけどエラーの種類まで特定しないような、その他のエラーみたいなものを言っているんだと思っています

蒼い惑星 [5:08 PM]
想定外のケースは、実装のバク系や、データベースのデータが不整合の状態だったり、と思いました。

ここは私の理解が思いっきり間違えていたという事故の部分です。

yamama [5:16 PM]
起こらないはずだけど（メソッド的に）Exceptionは発生する、的なところですかね。

蒼い惑星 [5:24 PM]
そうですね。
あるメソッドを実装して、あるインスタンス変数を使っているとして、そのインスタンス変数が別のメソッドに設定してもらう前提だったが、実は、認識していない経路から来るとその変数がnilなったり

yamama [7:10 PM]
そういうのは上の方で拾ってもいいですね、っていうか多分拾い忘れてシステムエラー出ますよねｗ

—– Today June 12th, 2015 —–

kaneko [8:18 AM]
@蒼い惑星: そうでしたか
となると、しっかりと各所でエラー処理を入れるという事になりそうですね

そのあと更に意見が出てきました。

上記の議論をふまえて、以下のようにログ出力をする共通処理を作って、それを使って例外を拾った時に適切にログ出力してもらうよう、呼びかけることになりました。
結果としては普通の事を丁寧にやろうという話だと思いますが、いろいろ議論できて良かったと思います。

lib/appname/logger.rb

module AppName
  class Logger
    def self.logging_exception(exception)
      Rails.logger.error exception.message
      Rails.logger.error exception.backtrace.join("\n")
    end
  end
end

最初はコントローラで実装していましたが、モデルなどからも呼ぶので名前空間作って実装しています。