Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: プリセットマネージャーのエラーハンドリングを改善 #1489

Merged
merged 2 commits into from
Nov 25, 2024

Conversation

sabonerune
Copy link
Contributor

@sabonerune sabonerune commented Nov 22, 2024

内容

PresetManagerでFileNotFoundErrorを捕捉するように書かれていますが通常はまず起こらない上にそれ以外のOSErrorを捕捉しないので修正します。

それに伴いエラーメッセージも変更します。

関連 Issue

その他

個人的な意見ですがInternal Server Errorはエラーの詳細をクライアントに伝えるべきではないような気がします。
例外の内容コンソールに表示した方がいいような気がします。

try:
presets = preset_manager.load_presets()
except PresetInputError as err:
raise HTTPException(status_code=422, detail=str(err))
except PresetInternalError as err:
raise HTTPException(status_code=500, detail=str(err))

-        except PresetInternalError as err:
-            raise HTTPException(status_code=500, detail=str(err))
+        except PresetInternalError:
+            print_exc()
+            raise HTTPException(status_code=500, detail="Internal Server Error")

@sabonerune sabonerune requested a review from a team as a code owner November 22, 2024 17:08
@sabonerune sabonerune requested review from Hiroshiba and removed request for a team November 22, 2024 17:08
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRありがとうございます!!!

通常はまず起こらない上にそれ以外のOSErrorを捕捉しないので修正します。

たしか存在しないディレクトリに書き込もうとしたときにFileNotFoundErrorになりますね!
これが起こり得ないのかどうか判断が難しいですね・・・・・。
プリセットのパスを変えられるので起こりえそう・・・・・?

ただまあもしこのエラーをキャッチしなくても、エラーメッセージがプリセットの設定ファイルが見つかりませんからNo such file or directory: '/プリセットのパス'に変わるだけで、何が原因かは推察できるからエラーハンドリングの目的(開発者が何が原因か推察できる)は達成できそう・・・?
いやでもちょっとだけ難度上がりますね。。。まーーーあってもなくても良さそうではあるかな・・・!!


あ、from errですが、これめちゃめちゃ良いですね!!!!!!!!
ただ適用するなら可能なら全raiseに適用したい気持ちがあります。
もしよければこのPRではただのraiseにしていただいて、別PRで全部from errにしてみませんか・・・!!
けっこう大変なのですが、もしよければ!


個人的な意見ですがInternal Server Errorはエラーの詳細をクライアントに伝えるべきではないような気がします。
例外の内容コンソールに表示した方がいいような気がします。

こちらは完全に仰るとおりだと思います。
今って出るようになってるんでしたっけ、ぜひ改善したいです。
お手数おかけしますが、一旦issueだけ作っていただいても良いでしょうか 🙇
影響範囲小さそうであればいきなりPRでも大丈夫です!!

@Hiroshiba Hiroshiba requested a review from Copilot November 23, 2024 08:08

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no suggestions.

@sabonerune
Copy link
Contributor Author

sabonerune commented Nov 24, 2024

たしか存在しないディレクトリに書き込もうとしたときにFileNotFoundErrorになりますね!
これが起こり得ないのかどうか判断が難しいですね・・・・・。

プリセットマネージャーの初期化時にプリセットを作成しているので通常は起動に失敗すると思います。
発生するとしたら起動中にファイルを操作くらいでしょうか?

もしよければこのPRではただのraiseにしていただいて、別PRで全部from errにしてみませんか・・・!!

from errは一度削除しました。

今って出るようになってるんでしたっけ、ぜひ改善したいです。

プリセットマネージャーの例外はコンソールには出ませんね。
レスポンスのdetailのみに例外のメッセージが入る感じです。
未捕捉の例外はコンソールに表示されレスポンスには含まれないようになっています。
それ以外の例外は確認していないですね。

@Hiroshiba
Copy link
Member

プリセットマネージャーの初期化時にプリセットを作成しているので通常は起動に失敗すると思います。
発生するとしたら起動中にファイルを操作くらいでしょうか?

そうなんですよね、発生しなくはないけれども、まあでも発生しないだろうみたいな・・・。

そもそもエラーハンドリングについて考えたんですが、重大なエラーだったらハンドリングしてあげて、そうでもないエラーはまあ何が起こったかわかるぐらいにしておけば良さそうに思いました。
file not foundエラーはそのままにしておいても何が起こったかわかるし、あと発生確率も少ないでしょうしで、消して良さそうに思いました!

プリセットマネージャーの例外はコンソールには出ませんね。
レスポンスのdetailのみに例外のメッセージが入る感じです。
未捕捉の例外はコンソールに表示されレスポンスには含まれないようになっています。
それ以外の例外は確認していないですね。

なるほどです!
プリセットの例外はコンソールにのみログを出力する形にするのが良さそうですね!
まあ影響範囲が大きくなってしまうの、もし後で気が向いたらプルリクエストいただければと!!

別のところで同じようなことが起こっていたら、そちらも一緒に変更していただけると・・・!
あまり興味なければそのままでも大丈夫です! 🙏

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

マージします!

@Hiroshiba Hiroshiba merged commit 54d4c9b into VOICEVOX:master Nov 25, 2024
4 checks passed
@sabonerune sabonerune deleted the fix/preset-error-handle branch November 25, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants