🤺 Node.js + Expressの混沌を統治する 🤺

こんにちは！株式会社estieでEMをやっています、t-poyoです。
今回は、estieの創業以来走り続けてきたプロダクトのapiをどう改善しているかについて書きたいと思います。
当社は"estie"と"estie pro"という2つのサービスを作っていますが、今回は"estie"の開発にまつわるお話になります。

こんな方に読んでほしい

estieの開発チームが何をやっているのか知りたい方
node.jsでイチからプロダクトを作りたい方
apiのアーキテクチャに悩みつつも「クリーンアーキテクチャほどガチガチにやるのは…」な方

TL;DR

コールバック関数を利用してアプリケーション層をExpressから分離できる
分離した関数に対して複雑なモックを使わずテストを書ける

あらすじ

estieは、2020年2月にUI刷新をおこない、バージョンも2.0にメジャーアップデートしました。
その際、フロントエンド側はもちろんのこと、バックエンド側もapiをすべて書き直しリリースしました。

しかし当時はあまりにも人数と時間が足りない中の開発で、以下のような問題がありました。

フルタイムのエンジニアがおらず(当時t-poyoは業務委託で参加)、モデルやエンティティと呼ばれる概念を固めることができなかった
node.js + Express での関心分離に関してノウハウがなく、またweb上にも日本語記事が少なかった

特に後者は大きな問題で、結果、すべてのapiリソースで、「expressオブジェクトにすべての処理をベタ書きした関数を渡す」というような構造で書き通してしまいました。
つまり、HTTPレスポンス等に関心を持つインターフェイス層と、リクエストバリデーションやデータ整形をおこなうアプリケーション層が結合してしまったのです。

課題

上記のような状態では、様々な困難が発生することは皆さんの想像に難くないと思います。
中でも私たちが最も辟易したのは「テストが書きにくい」という問題です。

例えば「しかるべきタイミングでメールを飛ばす」「見せたくない情報を見せない」など、ビジネス上重要なロジックを保証するテストが本当に書きづらく、開発の障壁となっていました。
一部、supertestを用いてテストを書いていましたが、アサートでチェックできるのはexpressが吐くhttpレスポンスのみ。
内部のエラーハンドリングや、外部サービスにリクエストを投げるモジュールの動作確認、レスポンスに含まれるデータの型・正当性までチェックするのは至難の技でした。

Node.js のスペシャリストに指南を受ける

途方にくれるestie開発チームでしたが、有り難いご縁があり、光明を見出します。
ヤフー株式会社で「黒帯」として活躍されている伊藤さんをお招きし、改善方針をレクチャーいただきました。

黒帯って何？という方は下記リンクをご覧ください。
https://about.yahoo.co.jp/hr/article/550625/

その改善方針とは、「コールバック関数を利用したアプリケーション層の分離」です。

実際におこなったリファクタリング

それでは、実装例を見ていきましょう。
リクエストボディにuser idを受けて、userのインスタンスを返すapiリソースがあると過程し、リファクタリングしていきます。
バリデーションは joi を使用しています。
(joiはhapi/joiとして提供されていましたが、現在はプロジェクトの終了に伴い移行が推奨されています。)

router.get('/', async (req, res, next) => {
// idがnumber型であることを確認
  const {
    error: validationError,
  } = joi.object().keys({
    id: joi.number().required(),
  }).validate(req.body);

  if (validationError) {
    res.status(400).send({
      error: 'Bad Request',
    });
    return;
  }

// DBからuser情報を取得
  const user = await getUserInstanceById(req.body.id);

// userが見つからない場合は404を返す
  if (!user) {
    res.status(404).send({
      error: 'Not Found',
    });
    return;
  }

// 正常系は200で返す
  res.status(200).send(user);
});

module.exports = router;

以前のコードはこのように1つの関数の中にすべての要素を詰め込んでいました。

この書き方では複雑なオブジェクト操作はおこなっていませんが、
本来データの取得・整形に集中したいこの関数は、expressのrouterに依存しており、
更にHTTPステータスコードの指定にまで手を出してしまっています。

贅沢は味方、欲張りは時に良い結果をもたらしますが、ソフトウェアアーキテクチャにとっては大敵です。

handler関数の作成

それでは、estieのapiに巣食う欲張り関数を分解していきましょう。
まず、リクエストのみを引数に取り、アプリケーション層としての処理に集中する関数を作成します。

// 後々ミドルウェアでのエラーハンドリングに使用するためHandleErrorクラスを定義
class HandleError extends Error {
}
class ValidationError extends HandleError {
  constructor(message) {
    super(message);
    this.code = 400;
  }
}

const handler = async (req) => {
  // idがnumber型であることを確認
  const {
    error: validationError,
  } = joi.object().keys({
    id: joi.number().required(),
  }).validate(req.body);

    // バリデーション失敗したらカスタムエラーオブジェクトを投げる
    if (validationError) {
      throw new ValidationError(validationError.message);
    }

// DBからuser情報を取得
  const user = await getUserInstanceById(req.body.id);

  return user
};

module.exports = { handler };

このように書くことで、この関数単体でのテストが可能になります。
正常系はこのpromiseがresolveしたときに戻り値の中身をチェックすれば良いですし、
異常系は投げられたエラーのインスタンスをチェックすることでカバーできます。

wrapper関数の作成

そして、上記でexportしたhandlerをコールバック関数とする関数を、さらにexpressに引き渡します。

const { handler } = require('../上記の関数');
const wrap = fn => async (req, res, next) => {
  try {
    await fn(req);
    res.status(200).send(r);
  } catch (e) {
    next(e);
  }
};
module.exports = wrap(handler);

この関数で、resオブジェクトをhandlerから切り離せています。
また、エラー時のステータスコードもexpressの提供するnext関数が決めてくれるので、
アプリケーション層がHTTPレイヤーを気にする必要がなくなりました。

next関数内でのエラーハンドリング処理

next()に投げられたカスタムエラークラスをHTTPレスポンスに変換するためのミドルウェアを定義します。

const errorHandler = (err, req, res, next) => {
  if (err instanceof HandleError) {
    res.status(err.code).send(err.toString());
    return;
  }
  res.status(500).send({
    error: 'Server Error',
  });
};

app.use(errorHandler);

ステータスコードを指定する処理を集約しただけなのですが、
これだけでhandler()関数内の見通しがよくなります。

テストライティング

切り離したhandler関数の役割は非常にシンプルです。
その役割とは以下のようなものです。

リクエストオブジェクトを受け取る
正常系ならレスポンスに格納したいデータを返す
異常系ならエラーをスローする

なので、テストも非常にシンプルになります。
JESTを用いたテストコードを書いていきましょう。

const { handler } = require('./hoge');

// getUserInstanceById() はmockするかテストデータを用意

describe('handler', () => {
  it('id=1のリクエストに対して正しいデータを返す', async () => {
    const mockReq = { body: { id: 1 } };
    const r = await handler(mockReq);
    expect(r).toBe(correctData);
  });
  it('number型でないidを受けた時エラーを投げる', async () => {
    const mockReq = { body: { id: 'text' } };
    expect.assertions(1);
    try {
      await handler(mockReq);
    } catch (e) {
      expect(e instanceof NotFound).toEqual(true);
    }
  });
});

アサーションのTips

エラーをスローさせる処理のアサーションはいくつか方法があるのですが、
「意図に反してpromiseがresolveしてしまったとき、テストケースが失敗するように」気をつけて書く必要があります。
アサーションを数えることで、catch()節に入ったことを確認する上記の書き方がお勧めです。

JESTではdone()という関数も提供されていますが、
この関数は直接エラーを渡されない場合、タイムアウトによってテストケースの失敗を判定するため扱いが少々難しいです。

例えば上記のテストケースをdoneを使って書いてアサーションを数えない場合、

  it('number型でないidを受けた時エラーを投げる', async (done) => {
    const mockReq = { body: { id: 'text' } };
    return handler(mockReq).catch((e) => {
      expect(e instanceof NotFound).toEqual(true);
      done();
    });
  });

このように書くことになりますが、このテストケースが失敗する場合

タイムアウトの時間分待たされ、テストの実行時間が大幅に伸びる
エラーメッセージにタイムアウトのメッセージ(下記)も含まれ、原因特定に手間が増える

というデメリットがあります。

// done() がタイムアウトで失敗したときのエラーメッセージ
: Timeout - Async callback was not invoked within the 10000ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 10000ms timeout specified by jest.setTimeout.Error:

then() 節の中でdone()に直接エラーを渡してあげればタイムアウト分待つ必要はなくなりますが、
エラーメッセージを考えたりするのも結構な手間ですよね。