routerとserviceの責務

対象レビュー

単一ページのmdファイルエクスポート機能のサーバー側（ページデータへのアクセス部分）の実装 (PR external_link)

ここで言いたかったこと

答え自体は普通に考えればわかることだけど、一度冷静になってその処理は本当にそこに書くべきなのかを考える癖をつけるのが大事だと思いました。

議題

どこまでrouter (routes 配下のファイル)がやるべきでどっから service (/services 配下のファイル) がやるべきか（あくまでGROWI開発において）

実装物の処理の流れ

サーバーがフロントからの page の body 取得リクエストを受け取る
ログインチェック等を経て、　/router ディレクトリ内の page の body 取得用処理に飛ぶ
要求されているページの存在チェック
リクエスト送信ユーザーの要求されているページへのアクセス権限チェック
リクエストパラメータ内の revision id から revision を取得（revision id が指定されていなかったら同じくリクエストパラメータ内の page id から最新の revision を取得）
revision.body を取得し、適切なデータ形式に変換（当時はpdfエクスポート機能も予定していたため変換処理や送受信データの contents-type の調整などが必要だった）
変換を終えたデータをフロント側に送信

レビュー前

全て /routes 配下のファイルに書いていた

レビュー内容

レビューコメント external_link

page data, revision data へのアクセス権があるかどうか、そのエラー処理までは route でやる
revision を引数に、stream を返すところは service でやる
res に対しての処理は route でやる

このコメントに答えが出揃ってしまってはいるが、大事なのはrouter の役割を意識すること。

router がすべきなのは

リクエストを受け取ったら、そのリクエストの正当性をチェック
パラメータを処理しやすいように変換しておく
そのリクエストを達成できるような処理に案内する
リクエストにふさわしいレスポンスを返す

(あくまで個人的な考えです。また開発しているアプリによって変わってくると思います)

これを踏まえて今回の実装内容を見てみると

ページの存在チェックやアクセス権限チェックは一個目に該当するのでrouterでやるべき

routes/page.jsconst page = await Page.findByIdAndViewer(pageId, req.user);
const isPageExist = await Page.count({ _id: pageId }) > 0;
if (page == null) {
    const isPageExist = await Page.count({ _id: pageId }) > 0;
if (isPageExist) {
    // This page exists but req.user has not read permission
    return res.apiv3Err(new ErrorV3(`Haven't the right to see the page ${pageId}.`), 403);
}

リクエストパラメータの revision id から revision を取得しておくのもrouterでやる（ここは場合によってはserviceに任せても良さそう）
```
routes/page.jsconst revision = await Revision.findById(revisionIdForFind);
```
書くまでもないが、service の処理を呼び出すのも router がやる
```
routes/page.jsconst stream = exportService.getReadStreamFromRevision(revision);
```

フロントに返すデータの準備はservice がやるべき(ここでDBへのアクセスがない時は servise、ある時はrepositoryという分け方をする考え方もあるんですね。知りませんでした）

services/export.jsgetReadStreamFromRevision(revision) {
    const markdown = revision.body;

    const Readable = require('stream').Readable;
    const readable = new Readable();
    readable._read = () => {};
    readable.push(markdown);
    readable.push(null);

    return readable;
}

データをフロントに返す部分はrouterがやるべき

routes/page.jsres.set({
    'Content-Disposition': `attachment;filename*=UTF-8''${fileName}.${format}`,
});
return stream.pipe(res);

まとめ

このディレクトリ配下のファイルはどこまでの作業をすべきかを開発者同士の共通認識に沿って考慮しておくことが大事。今回は別に問題が生じたわけではないが、このように責務を明確に切り分けていない状態（ある権限チェックはrouterに書かれ、ある権限チェックはserviceに書かれているような状態）だとどこにどうゆう目的の処理が書かれているのか把握しづらく変更が大変なソースコードになってしまいそう。