Zenn
🧱

クエリをクラスとして扱い、色々安全側に倒してみる

2024/04/25に公開
PHP
tech

PHP(や、似ている言語)にてクエリを錬成(？)する際に、単にメソッドを増やしていくだけでなく、(PHPの場合は)DTO/Entityなクラスで分類・活用するといくつかのセキュリティ向上メリットがあるかもしれません、そのお話です。

(一部、概念コードになっているのでそのまま動作はしません)

注意として、ベストプラクティスを語っているわけではなくて、こういうテクもある、というお話です。

クエリの最終形態は大体文字列である

大体の場合、クエリはJsonだったりSQLだったり文字列になります。

今回は一旦ElasticSearchに対するクエリについて考えてみます。

ちなみにDDLはこんな感じ

一旦、Blogのエントリーを持つDDLを考えてみます。あまり良い設計でもないのですが、素朴なものです。

PUT /blog_entries
{
  "mappings": {
    "properties": {
      "title": {
        "type": "text",
        "analyzer": "standard"
      },
      "content": {
        "type": "text",
        "analyzer": "standard"
      },
      "author": {
        "type": "text",
        "index": "not_analyzed"
      },
      "tags": {
        "type": "keyword"
      },
      "published_at": {
        "type": "date",
        "format": "date_optional_time"
      },
      "created_at": {
        "type": "date",
        "format": "date_optional_time"
      },
      "updated_at": {
        "type": "date",
        "format": "date_optional_time"
      }
    }
  }
}

管理画面などで、Entryを全件引くクエリ

たとえばこういうクエリが考えられます。

{
  "sort": [
    {
      "created_at": {
        "order": "desc"
      }
    }
  ]
}

しかしブログのシステムなどでは 下書きや予約投稿の要件があるため。単純にこれでは公開用のトップページ画面ではこのようにならないかと思います。

公開エントリだけ引くクエリ

ここではpublished_atがはいっていない場合はまだ非公開(Draft)だとしてかんがえてみます。こんなクエリが考えられます

{
  "query": {
    "bool": {
      "must": [
        {
          "exists": {
            "field": "published_at"
          }
        }
      ]
    }
  },
  "sort": [
    {
      "published_at": {
        "order": "desc"
      }
    }
  ]
}

これをどう出し分けるか？

さて、クエリは二つできました。ではこのクエリをどうやって生成しましょう？

PHPなら当然(？)配列で組み立ててからJSONにするとおもいます。すごく素朴なクラスにするとこうなるかなと思います。

<?php

class GetEntriesQuery
{
    public function __construct(private bool $includePublishedAtNull = false)
    {
    }

    public function __toString(): string
    {
        $query = [
            'query' => [
                'bool' => [
                    'must' => []
                ]
            ],
            'sort' => [
                'published_at' => [
                    'order' => 'desc'
                ]
            ]
        ];

        if (!$this->includePublishedAtNull) {
            $query['query']['bool']['must'][] = [
                'exists' => [
                    'field' => 'published_at'
                ]
            ];
        }

        return json_encode($query, JSON_PRETTY_PRINT);
    }
}

ま、こんな感じですよね？

ifを滅ぼしたい

ただ、ifはやっぱり滅ぼしたいですよね。正しくやるならif文が一つ増えるたびにテストで考える次元が一つ増える事になりますので、排除しましょう。

クラスを割ってみました。

<?php

interface ESQueryInterface
{
    public function __toString(): string;
}

class GetTopPageEntriesQuery implements ESQueryInterface
{
    public function __toString(): string
    {
        $query = [
            'query' => [
                'bool' => [
                    'must' => [
                        [
                            'exists' => [
                                'field' => 'published_at'
                            ]
                        ]
                    ]
                ]
            ],
            'sort' => [
                'published_at' => [
                    'order' => 'desc'
                ]
            ]
        ];

        return json_encode($query);
    }
}

class GetMyPageEntriesQuery implements ESQueryInterface
{
    public function __toString(): string
    {
        $query = [
            'sort' => [
                'published_at' => [
                    'order' => 'desc'
                ]
            ]
        ];

        return json_encode($query);
    }
}

ElasticSearchのClientのWrapperなどにたたき込む時には共有したいので、ほぼ中身がないInterfaceをはさみつつ、クエリがことなるクラスを二つつくりました。

今回みたいなコードなら完全に静的なものなので、ifを削ったことでこのQuery自体のテストは不要かなと思います(コントローラ・アクションでテストできればよい)。

「これでifは消えたのか？」

「いや、これやってもコントローラーとかサービスとかにif相当があるんじゃない？」という話はあろうかとおもいます。

自論からいうと、消えた(消せる)とおもいます。

サービス全体からみれば、「/」アクセス時と「/mypage」アクセス時のif(？)はいずれにせよコントローラーやE2Eテストが作成され、そこで担保されるはずです。

よって、Queryのテストのifのパターンを色々見る必要が無くなります。

とはいえ、現実にはQueryにはページャなど動的な箇所が入るでしょうから、このような限定的なケースでしか「テスト不要！」とまではならないでしょうが。でも断然楽になります、型で保証できるからです。

DRYではないのでは？

クラスを二つにするということは、コピペが増える可能性があります。

実際このクエリは1ファイルでよいところ、3ファイルにしています、これは悪なのか？
まず、修正の手間は増えるかなと思います。まあそりゃそうですよね、変更箇所が一箇所で済む筈が数ヶ所修正必要になります。そうなると障害の発生可能性が増えるともいえます。

ただし、これまた自論ですが、バランス的にはこれで良いかなと思っています。なぜなら後述しますが、障害が安全側に倒れやすいと言うことです。

命名できる

以下のようなコントローラのコードがあるとして、あからさまにおかしい箇所を見つけやすくなります。

<?php

class TopPageController{
  public static function run(){
  	$res = ESQuery(new GetMyPageEntriesQuery); <== おかしいじゃない？？
  	render($res);
  }
}

前提として、QueryはQueryなので、ElasticSearchのClientにつっこめるようにはしたいだろうと思います。

(実際にはRepo層をつくって、そこでさらに…という事になるでしょうが、結局概念上は同じで、それがGetTopPageEntriesRepoやそれをたたくServiceになるだけでしょう)

すると、ElasticSearchのClientにQueryの引数としては、どちらもESのQueryなので「静的解析では」ただしいかを担保できません。

ここらへんが人間の目が限度かなと思います。

さすがにそれをかいちゃうのはコピペが過ぎるし、レビューで指摘できないのはLGTMが過ぎる。

型でassertできる

さらにいえば、やはりデータは色々なメソッド間を旅します。そうなると型があると差し止める事ができます。

(あるいは明示で assert() を入れても良いですが)

たとえば以下のようにクラスをラベリングすると

class GetMyPageEntriesQuery implements InpublicESQueryInterface
{
}

class GetTopPageEntriesQuery implements PublicESQueryInterface
{
}

Repo層やService層があるならば、あるいはESのClientのnew時にScopeをいれて、実行前にAssertを入れるというのもありえるとおもっていて、うまい感じに型の制限を練り込む事ができそうです。

<?php

class ESClient
{
  private function realQuery(ESQueryInterface $query){
  	// ESにクエリする
  }
}

class PublicESClient extend ESClient
{
  public function query(PublicESQueryInterface $query){
  	// ESにクエリする
  	$this->realQuery($query)
  }
}

class TopPageController{
  public function __constructor(protected PublicESClient $esc){
  }

  public static function run(){
  	$res = $this->esc->query(new GetTopPageEntriesQuery);
  	//...
  }
}

とかすることが可能になります。

こういう「認可みたいなの」をうまくつかえれば、ifを書かなくても制約をかけることができます。

欠点としては不自由なことですね、でも不自由と安全は大体トレードオフかなと思います。(諸説あります)

あとは、メソッドを作ることもない場合でも、もっとカジュアルにassertで制限をかけることも可能です
(assertは設定によってはDisableにできますが、まあDisableにしてる所もないだろう…)

$query = new TopPageQuery;
assert($query instanceof PublicESQueryInterface);
$res = ESQuery($query);

同様にレスポンスもあつかうことができる

Queryを押さえるのと同時にレスポンスにも同様のラベリングをすることができます。

このあたりは「配列を捨てよう！」というトークで以前話したので駆け足ですが、結局プロパティは両方で同じです、以下のリストになるでしょう。

<?php

class Entry
{
    public function __construct(
        public string $title,
        public string $content,
        public string $author,
        public array $tags,
        public DateTimeInterface $createdAt,
        public DateTimeInterface $updatedAt,
        public ?DateTimeInterface $publishedAt = null,
    ) {
    }
}

class PublicEntry extends Entry
{
}

class PrivateEntry extends Entry
{
}

全く同じ形をしていますが、「ひっぱってきたEntryListが(公開して)安全なのか？」ということをRepoのレスポンスでつかいわけることで、コードを奥深くまでよみこまなくても判断することができます。

(このあたり、動的言語でもDuck typeではないPHPの強みかもしれませんね)

あるいは、「PublicにはCreated_atやUpdated_atは不要である」というケースもあるかもしれませんね、そこで分離することもできます(このあたりは前述のとおり、過去トークではなしました)

※ https://speakerdeck.com/uzulla/throw-away-all-php-array-now

「めんどくない？」

あらためてですが、面倒です。というかコピペで量産すると型が食い違うのでエラーがおきやすくなります。若干知恵の輪じみることもあります。

ただ、それは止めてくれているのだから素直にエラーはなおすべきだし、そもそもそれがテストで担保できてないのはマズいだろうということで私はOKとしています。

あるいはテストがなかったとしても、静的解析をつかっていれば、実行前に指摘もされるとおもいます。

まとめ

  • 簡単便利にするためにフルアクセスを許可しがち
    • 「やはり、自動的にテーブルの全部のカラムを盲目的にだせるORMは危険！！」(過激派の意見)
  • DRYを裏切ることで(？？？)、間違えにくいコードにしていくことが可能
  • とはいえ、「安全なクエリ」と名付けた「危険なクエリ」をつくれば突破は可能
    • でも、自身や、レビュアーが重点的に注目すべき箇所は制限されて便利
  • でも、これって実は結構設計がむずかしい。「これはよさそう！」と飛びつくと、大抵ファイルがつぎはぎになる。
    • まずは小さいAPIの小さいFeatureでやって練習してからのトライをお勧めします(小さくスタートすることは可能な筈です)

Discussion