Failure teaches Success 限定公開の記事を編集できたのに、リファクタしたら編集できなくなった

実際のトラブルを架空のCMSに置き換えて説明します。

概要

投稿記事の公開状態は、3種類あります。下書き、公開、ゴミ箱です。

公開状態編集公開
下書き可能非公開
公開可能公開
ゴミ箱不可非公開

あるとき、いくつかの記事をログインユーザーだけに限定表示したいという要望があり、限定公開機能を実装することになりました。

公開状態編集公開
限定公開可能ログインユーザー限定公開

無事、限定公開機能を実装し、リリースしました。

後日、限定公開機能をリファクタリングしてデプロイしたところ、限定公開した記事を編集できなくなってしまいました。

技術的な要因は単純なものでした。

投稿記事の公開状態の定数です。


class Status
{
    public const DRAFT     = 0; // 下書き
    public const PUBLISHED = 1; // 公開
    public const TRASH     = 2; // ゴミ箱
    public const RESTRICT  = 3; // ログインユーザー限定公開
}Code language: plaintext (plaintext)

別の箇所で、編集可能かどうか判断するメソッドがあります。

class Foo
{
    public static function isEditable($status)
    {
        $editableStatuses = [
            Status::DRAFT,
            Status::PUBLISHED,
        ];

        $f = in_array($status, $editableStatuses);
        return $f;
    }
}Code language: plaintext (plaintext)

編集可能な公開状態リストに「限定公開」が入っていなかったので、限定公開の記事を編集できませんでした。

なぜ、リファクタ前は、限定公開記事を編集できていたのでしょうか。

リファクタ前は公開状態は3つしかなく、別のフラグで限定状態かどうかを判別していたからです。

class Status
{
    public const DRAFT     = 0; // 下書き
    public const PUBLISHED = 1; // 公開
    public const TRASH     = 2; // ゴミ箱
}Code language: plaintext (plaintext)

定数に「限定公開」を追加してリファクタリングしましたが、編集可能な公開状態リストに「限定公開」を追加するのを忘れていたんですね。

リファクタしなくてもよかったんじゃないの？

実際のプロジェクトでは、状態の定数を追加したことは妥当だったわ

実際のトラブルを架空のCMSに置き換えているから、大目に見てね

発生原因

１つめは、編集可能かどうかのメソッドに「限定公開」の追加が必要なことに気づかなかったことです。

        $editableStatuses = [
            Status::DRAFT,
            Status::PUBLISHED,
            Status::RESTRICT,   // コレに気づけなかった
        ];Code language: plaintext (plaintext)

実際にトラブルがあったプロジェクトで言うと、本人や第三者のコードレビューではなかなか気づけないと思うわ

２つめは、そのメソッドのユニットテストに「限定公開」の追加が必要なことに気づかなかったことです。

リファクタ後も、このユニットテストは合格するので、修正が必要なことに気づきませんでした。

        $this->assertTrue(Foo::isEditable(Status::DRAFT));
        $this->assertTrue(Foo::isEditable(Status::PUBLISHED));
        $this->assertFalse(Foo::isEditable(Status::TRASH));
        $this->assertTrue(Foo::isEditable(Status::RESTRICT));  // コレに気づけなかった
Code language: plaintext (plaintext)

それはそうだよね、１つめのメソッドの「限定公開」に気づいていれば、ユニットテストでも「限定公開」をテストしているよ

３つめは、限定公開の記事が編集についてのE2Eテストが必要なことに気づかなかったことです。

下書きや公開、ゴミ箱の記事を編集できるかのE2Eテストや、限定公開記事を公開するE2Eテストはあったのにです。

１つめ、２つめに気づけなかった原因は、公開状態の表、編集可能の表が２つに別れていて、編集可能の表に設定漏れがあってもわからないためです。

公開状態の表（Statusクラスの定数定義）

定数
DRAFT
PUBLISHED
TRASH

編集可能の表（FooクラスのisEditableメソッドの$editableStatuses配列）

定数
DRAFT
PUBLISHED

再発防止策

コードレビューをしっかりやります！

そうではなくて．．．

１つの表で管理する

表を２つに分けず、１つの表で管理することで、設定漏れはなくなります。

公開状態の表（Statusクラスの定数定義）

定数編集可
DRAFTtrue
PUBLISHEDtrue
TRASHfalse

PHPでは定数定義と、定数とフラグの配列の２つを用意しなければなりません。厳密に１つの表で管理することは難しいです。

２つの表の行数を同じにして、行数が同じかどうか確認する

isEditableメソッドを別のクラスに分けたほうがいい場合、編集可能の表に全ての定数を定義します。２つの表の行数が同じことを確認することで、設定漏れに気づくことができます。

公開状態の表（Statusクラスの定数定義）

定数
DRAFT
PUBLISHED
TRASH

編集可能の表（FooクラスのisEditableメソッドの$editableStatuses配列）

定数編集可
DRAFTtrue
PUBLISHEDtrue
TRASHfalse

定数を追加したらテストやメソッドが失敗する仕組み

さきほどの「２つの表の行数を同じにして、行数が同じかどうか確認する」を実装します。

リフレクションで、定数の個数をカウントする

Statusクラスに、クラス定数の個数を返すメソッドを用意します。

ReflectionClass::getConstants を使います。クラス定数について、定数名と値の連想配列を取得します。配列の長さが、クラス定数の個数です。

class Status
{
    public const DRAFT     = 0; // 下書き
    public const PUBLISHED = 1; // 公開
    public const TRASH     = 2; // ゴミ箱

    public static function constCount(): int
    {
        $oClass = new ReflectionClass(__CLASS__);
        // print_r($oClass->getConstants());
        return count($oClass->getConstants());
    }
}Code language: plaintext (plaintext)

constCount内のprint_rの表示結果の例です。

Array
(
    [DRAFT] => 0
    [PUBLISHED] => 1
    [TRASH] => 2
)
Code language: plaintext (plaintext)

Enumクラスで、定数の個数をカウントする

myclabs/php-enum のEnumクラスを使います。

Enumクラスのkeysメソッドが定数名の配列を返すので、それをカウントします。

use MyCLabs\Enum\Enum;

class Status extends Enum
{
    public const DRAFT     = 0; // 下書き
    public const PUBLISHED = 1; // 公開
    public const TRASH     = 2; // ゴミ箱

    public static function constCount(): int
    {
        return count(self::keys());
    }
}
Code language: plaintext (plaintext)

定数が追加されたら失敗するテスト

Statusクラスに定数が追加されたら、isEditableメソッドのテストが失敗するようにしておきます。

isEditableメソッドのテストで、定数の個数を比較します。

class FooTest extends TestCase
{
    public function isEditable()
    {
        $this->assertEquals(3, Status::constCount(), 'Status定数に追加した？新しい定数のisEditableテストを書いた？');

        // Statusのすべての定数をテストすること
        $this->assertTrue(Foo::isEditable(Status::DRAFT));
        $this->assertTrue(Foo::isEditable(Status::PUBLISHED));
        $this->assertFalse(Foo::isEditable(Status::TRASH));
    }
}Code language: PHP (php)

StatusクラスにRESTRICT定数を追加します。

class Status
{
    ...
    public const RESTRICT  = 3; // ログインユーザー向け限定公開
    ...
}Code language: plaintext (plaintext)

Status::constCount()の返り値は4となり、さきほどのテストは失敗して、メッセージに

Status定数に追加した？新しい定数のisEditableテストを書いた？

と表示されます。新しい定数について、isEditableのテストを書かなければいけない、Foo::isEditableメソッドも修正する必要があるかもしれない、と気づくきっかけになります。

定数が追加されたら失敗するメソッド

もともとの$editableStatusesは、編集可能な定数だけを定義していましたが、

        $editableStatuses = [
            Status::DRAFT,
            Status::PUBLISHED,
        ];Code language: plaintext (plaintext)

$editableStatusesに全ての定数について、true/falseを定義するようにします。

class Foo
{
    public static function isEditable($status)
    {
        $editableStatuses = [
            Status::DRAFT      => true,
            Status::PUBLISHED  => true,
            Status::TRASH      => false,
        ];

        if (count(array_keys($editableStatuses)) !== Status::constCount()) {
            throw new Exception('Status定数に追加した？新しい定数を$editableStatusesに追加した？');
        }

        if (!array_key_exists($status, $editableStatuses)) {
            throw new Exception('Unknown status:' . $status);
        }

        return $editableStatuses[$status];
    }
}Code language: plaintext (plaintext)

Statusクラスに定数RESTRICTを追加して、$editableStatusesに定数RESTRICTを追加しないと、例外がスローされます。

定数とメソッドを同じクラスにまとめる

定数を定義しているクラスと、判別するメソッドのクラスのファイルは遠く離れていました。

Statusクラスの定数

class Status
{
    public const DRAFT     = 0; // 下書き
    public const PUBLISHED = 1; // 公開
    public const TRASH     = 2; // ゴミ箱
Code language: plaintext (plaintext)

判別するメソッドは、違うディレクトリ階層のクラスにありました。

class Foo
{
    public static function isEditable($status)
    {Code language: plaintext (plaintext)

メソッドをStatusクラスに移動します。

さきほどの「定数が追加されたら失敗するメソッド」も実装しておきます。

class Status
{
    public const DRAFT     = 0; // 下書き
    public const PUBLISHED = 1; // 公開
    public const TRASH     = 2; // ゴミ箱

    public static function isEditable($status)
    {Code language: plaintext (plaintext)

同じクラス内にisEditableメソッドがあることで、定数を追加したとき、チェックの仕組みに警告される前に、メソッドにも注意しやすくなります。

まとめ

ReflectionClass::getConstants を使って、クラス定数の個数を調べました。

myclabs/php-enum のEnumクラスを使って、クラス定数の個数を調べました。

クラス定数の個数をテストすることで、クラス定数が追加されたら失敗するメソッドやテストを作りました。

タイトルとURLをコピーしました