ちょっとだけ不正な値のPOSTに対して throw すべきか否か

Question

知りたいこと

以下の具体例において、php で throw すべきか否か・・ご意見いただけませんでしょうか

具体例

例えば「ライブラリに保存」とかの際に、「端末Aからすでに保存済みだが、端末BのHTML上ではまだ未保存であり、ゆえに保存処理はPOSTされうる」というケースがあるかと思います。

これは「不正なPOST」の一種かもしれませんけれど、「電話番号のフォームに住所が入力されたなどの不正なPOST」とは明らかにその性質が異なりますよね。
ちょっとだけ不正（JavaScript の検証を突破したわけではない不正さ、データベースに入ったらダメな値というほどではない不正さ）とでもいいましょうか。

ではこのようなちょっとだけ不正な値のPOSTに対して throw すべきか否か、というのを知りたいです。

現状の実装

現状では下記 throwIfExistsLibraryTag() メソッドで既存なら例外という処理を挟んでみたのですが、わざわざ throw すべきなのかな、と疑問に思いました。（尚、このメソッドは「ちょっとだけ不正」を意味する専用の class nonThreatIllegalException extends \Exception を throw します。）

final class LibraryTagController extends Controller
{
    // ライブラリにタグを追加
    final public function createLibraryTagAction(): void
    {
        $request = json_decode(file_get_contents('php://input'), true);

        $this->validator->setRules([
            'tagId' => [
                'function' => fn ($v) => $this->validator->isId($v),
            ],
        ]);

        $validatedRequest = $this->validator->validate($request)->getValidatedData();

        $libraryTagRowMapper = new LibraryTagRowMapper();

        $libraryTagRow = LibraryTagRow::newUsingArray([
            'libraryId' => (new LibraryRowMapper)->getUsersLibraryId($this->currentUser->getId()),
            'tagId' => $validatedRequest['tagId']
        ]);

        // 既存確認
        $libraryTagRowMapper->throwIfExistsLibraryTag($libraryTagRow);

        // `library_tags` テーブルに INSERT
        $libraryTagRow = $libraryTagRowMapper->insert($libraryTagRow);

        parent::render([
            'status' => 'succeeded'
        ]);
    }
}

改善案？

そしてもしわざわざ throw などしない方がいいとなると、
上記 throwIfExistsLibraryTag(), insert() のコンビ (既存なら throw する) でなく
下記 selectOrInsert(), wasInserted() のコンビ (既存なら 'exists' を返す) で良いですか？

final class LibraryTagController extends Controller
{
    // ライブラリにタグを追加
    final public function createLibraryTagAction(): void
    {
        $request = json_decode(file_get_contents('php://input'), true);

        $this->validator->setRules([
            'tagId' => [
                'function' => fn ($v) => $this->validator->isId($v),
            ],
        ]);

        $validatedRequest = $this->validator->validate($request)->getValidatedData();

        $libraryTagRowMapper = new LibraryTagRowMapper();

        $libraryTagRow = LibraryTagRow::newUsingArray([
            'libraryId' => (new LibraryRowMapper)->getUsersLibraryId($this->currentUser->getId()),
            'tagId' => $validatedRequest['tagId']
        ]);

        // `library_tags` テーブルに 既存でなければ INSERT
        $libraryTagRow = $libraryTagRowMapper->selectOrInsert($libraryTagRow, ['libraryId', 'tagId']);

        parent::render([
            'status' => $libraryTagRowMapper->wasInserted() ? 'succeeded' : 'exists',
        ]);
    }
}

求めるご意見

• throw すべき。だって▲▲だから。
• throw しない。だって■■だから。

のように、ご経験をふまえての理由も教えて頂けると幸いです。

よろしくお願い致します。

補足

HTTPステータスコードに 409 Conflict なるものがあると知りました。もしかしてこれがぴったりでしょうか？
だとすると当質問へのベストな回答は、以下のように「ConflictException という例外クラスを作った上でthrow すべき」という内容になりますでしょうか？

class HttpException extends \Exception
{
    protected int $statusCode;

    public function __construct(string $message, int $statusCode = 500)
    {
        parent::__construct($message);
        $this->statusCode = $statusCode;
    }

    public function getStatusCode(): int
    {
        return $this->statusCode;
    }
}

class ConflictException extends HttpException
{
    public function __construct(string $message = 'Conflict detected')
    {
        parent::__construct($message, 409);
    }
}

Answer 1

例外にすべきかどうかは、第三者には判定出来無さそうです。

端末Aからすでに保存済みだが、端末BのHTML上ではまだ未保存であり、ゆえに保存処理はPOSTされうる

「ある投稿に対してコメントを送信したら既にその投稿が（コメントを書いている間に）削除されている」
という様な場合と同じだと考えられませんか？
親子関係が厳密なモノとかなら「親投稿」が無くなった時点で不正な送信と判断するでしょうし、削除された投稿に対するコメントであると判る様にして受け入れるって場合もあるでしょう。

また、上書き（≒修正）を許す様なシステムなら後着が優先されるだけのお話でしょうし、でなければ例えマイクロ秒でも後着は拒否されるべきでしょう。
＿＿全て別物として受け入れるって事もありそうだし
＿＿（今回の様なタグ付けならば）既存であれば無視ってのもありそう

「不正なPOST」の一種かもしれませんけれど、「電話番号のフォームに住所が入力されたなどの不正なPOST」とは明らかにその性質が異なります

期待したデータ（どこからも未保存である事）ではないという時点で不正であって、性質云々は無関係では？
保存済みである状態で、再度保存しようとした際のリアクションに準ずるべきだと思います。
「ちょっとだけ」なんて中途半端な判定はあり得ないと思います。

HTTPステータスコードに 409 Conflict なるものがあると知りました。もしかしてこれがぴったりでしょうか？

そういうのは、相手（クライアント）に判断をゆだねる場合に使うモノでは？
＿＿現状が該当するものであるのかは良く解っていません
ウェブアプリ的なモノで、エラーだろうが何だろうが遷移等して何かを表示するなら 200以外はあり得ないようにも思いますし、API的なモノで、JavaScript等へのレスポンスとしてならあり得るのだろうと思います。

Comments

[munekun]

なるほどなるほどなるほど・・・いやぁ、なるほど。
こういうケースはこうすべきだという事例を列挙して頂いたおかげで、かなり見通しが立ちました。
ありがとうございます。

Answer 2

深い質問です。
結論から先に言うとこれはthrowの問題ではなくアーキテクチャの問題だと思いますが、throwに関しても順を追って記載します。

throwに関してはそれぞれやり方の好みがあります。
まあそもそもプログラミング自体無数のやり方があるので仕方ないと思いますが。

私の会社ではバリデーションでさえエラーをthrowさせませんし、私はこれに同意しています。
私の考えではそもそもエラーというのは、プログラムから発生される情報で、ユーザーに見せるべきものではないという考え方です。開発者だけエラーを知っていればよい。

その原則にのっとり、エントリーポイントではエラーを必ずキャッチし、logファイルに書き込みます。
これは何らかの開発上のミスで発生したエラーを特定しやすくする意図があり、バリデーションエラーのような意図的な状態をログに書き込んだところでなんの役にも立ちません。

エラーにはスタックトレースという開発に役立つ情報が含まれており、むやみにcatchしthrowするとスタックトレースの情報が失われ、実際にどこでエラーが起きたのか分からなくなります。

もちろん、この考え方を生かしつつもthrowする方法としてはエラークラスを分割する方法も考えられますが、うちではそこまで行っていません。

なおlogに書き込む情報としてはエラー以外に操作ログやトレーシングログなども考えられますから、
もしも、エラーが発生しないが不正な挙動が出る際に調査が必要というときはこちらも考慮できます。

例えば「ライブラリに保存」とかの際に、「端末Aからすでに保存済みだが、端末BのHTML上ではまだ未保存であり、ゆえに保存処理はPOSTされうる」というケースがあるかと思います。

質問のケースですと、エラーが発生した際の内容としては
エラーが発生する → ロールバックする　→　メッセージの表示や、適切なページの表示を行う
という流れになるでしょうから、これは一連の処理単位として考えられエラーは隠蔽されています。

そう考えた場合、これはエラーをthrowするかどうか？という話ではなく、本質的にはアーキテクチャの問題かと思われます。

アーキテクチャとは、処理の順序を抽象化し、クラスに分割する考えた方です。
例えば、処理のフェーズとして
バリデーションを行う　→　保存処理を行う
というフェーズをこのプロジェクトでは一貫して行う。というように決めます。
これはバリデーションクラスでバリデーションを行い、保存クラスで保存を行うという考えで、
単体テストもしくは結合テストを可能とし、不正な入力をした際に適切な処理が行われているのかを確認可能となります。

その場合、保存処理を行うクラス内で
エラー発生　→　ロールバック　→　メッセージの表示、適切なページの表示
をすると、適切なページの表示という保存処理に関係のないロジックが含まれることになり、
ページの表示単体でのテスト等できなくなりますし、一貫した開発ができるとは言えません。
この場合、
保存処理を行うクラスがエラーを発行することになります。

エラー発生　→　ロールバック　→　エラー発生したことを伝えるエラーを発生　→　コントローラでcatch →　メッセージの表示、適切なページの表示

この処理フローでいうと、質問文が「保存処理がエラーを発生するべきか」という質問だとすると、答えはYesになります。
※これはバリデーションロジックにも同じことが言えます。

これは要するに抽象化の考えで、
保存クラスで保存処理を行った際にエラーが発生したら、私のクラスでは責任をとれないので、呼び出し元でキャッチして、あなた好みの後処理を書いてくださいね。
という話になります。

エラーをキャッチしないのはただのバグです。

もしあなたが、開発者向けにAPIを作っているのであれば、409エラーを返すのは有効なアプローチでしょう。

しかし、一般ユーザー向けに作っているのであれば、無意味ですし、バグです。
一般ユーザーにそれなりのシステム開発の知識を求め、復旧作業に貢献してもらいたいなら話は別でしょうが、そんなシステムは見たことがありません。

Answer 3

※既に「解決済み」となっていますが……

よく「例外を使うべきか否か」の指標として、 「それが例外的な事象であれば使い、そうでなければ使うべきではない」 と言われます。つまり、プログラムの実行に際して「普通に起こり得る事」に対しては例外を使うべきではないという意見ですね。

例えば、データをDBに登録するプログラムが在ったとします。このプログラムは、DBが正常に動作している事を前提として動作している為、「DBと通信が出来ない」という状況が生じるとしたら、それは前提が崩れている状態である為、例外的事象だと言えます。DBに接続しようとしたり、クエリを投げたりするメソッドは例外を投げるのが妥当という話になるでしょう。

他方、データを検索する処理を考えてみます。例えば、Teratailのページのヘッダ部分に在る「キーワードで検索」の処理の結果として、検索結果が無い（0件だった）というのは例外的事象でしょうか？そうではありませんね。ここで入力したキーワードがヒットしないなんて、幾らでも起こりえます。よって、このケースでは「例外以外の表現を採るべき」という話となります。

---

では、質問に在るケースではどうでしょうか。

例えば「ライブラリに保存」とかの際に、「端末Aからすでに保存済みだが、端末BのHTML上ではまだ未保存であり、ゆえに保存処理はPOSTされうる」というケースがあるかと思います。

この場合、「そもそも、そのデータに対する複数端末からの操作が、利用方法として想定されている物か？」で結論が異なってきそうです。例えば、Googleスプレッドシート上のデータは、複数人数での同時編集は想定された使用法です。それと同等の仕様であれば、これは例外的事象とは言えないでしょう。一方、Facebookで自身の投稿内容を後から編集する場合、これは例外的事象と捉える事が可能です。仮にその操作が結果的に可能だったとしても、想定される利用法上は、複数端末上で同時に行う必要が無い物ですので。

その「一寸だけ不正な」状況は、想定されている利用法の上では、どのような立場となる物でしょうか？それを追求すれば「例外とすべきか否か」の結論は得られると思われます。

Comments

[munekun]

なるほど。将来ずっと使えそうな指針ですね！ありがとうございます。Evernoteのようなものを想定しているので、「例外とすべきか否か」は否っぽいです。