feat: fail integration tests that run N+1 queries

imorland · imorland · commit 7da741fa68d5 · 2026-07-31T17:47:09.000+01:00
Every request sent through the integration TestCase is now inspected for
N+1 query patterns, and the test fails when it finds one. Extension
authors get the feedback while writing the feature rather than when a
forum grows: this session alone, one extension was issuing a query per
post on every page of every discussion, found only by hand-profiling a
live forum.

An N+1 is one query shape executed once per record. The detector groups
a request's query log by normalised SQL — IN lists collapsed, literals
replaced — and fails when a shape repeats past a threshold. Bindings are
counted separately rather than folded into the shape: the same SQL run
for four different users is not the same defect as one query per row,
and conflating them produces false positives (it fooled me on one
extension before this distinction existed).

On by default. A single legitimate shape can be exempted with
allowedRepeatedQueries(); a test case can override
detectsRepeatedQueries(); FLARUM_DETECT_REPEATED_QUERIES=0 disables it
for a whole run.

Verified against core's api suite: identical results with detection on
and off (353 tests, same pre-existing failures, no findings), and
against a real N+1 reintroduced in an extension, where it fails with
'10x (10 distinct bindings)' naming the offending query.
diff --git a/php-packages/testing/src/integration/RepeatedQueryDetector.php b/php-packages/testing/src/integration/RepeatedQueryDetector.php
@@ -0,0 +1,120 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Testing\integration;
+
+/**
+ * Spots N+1 query patterns in the queries a request ran.
+ *
+ * An N+1 is one query shape executed many times with different values — a
+ * relationship loaded per model, a permission check per row, a serializer
+ * callback hitting the database for each item. Grouping the query log by
+ * normalised SQL surfaces them: legitimate work uses a handful of distinct
+ * shapes, while an N+1 repeats one shape as many times as there are records.
+ *
+ * Bindings are counted, not folded into the shape. Two executions of the same
+ * SQL with the *same* bindings are usually memoisation the caller could have
+ * done, but they don't grow with the data; a shape repeated with *different*
+ * bindings is the real N+1 signal. Reporting them separately is what stops
+ * "the same SQL text appeared 4 times" from being mistaken for an N+1 when the
+ * queries were for four different users.
+ */
+class RepeatedQueryDetector
+{
+    /**
+     * @param array<array{query: string, bindings: array}> $queries
+     * @param int $threshold Repetitions of one shape before it is reported.
+     * @return array<array{sql: string, count: int, distinctBindings: int}>
+     */
+    public static function findRepeats(array $queries, int $threshold): array
+    {
+        $shapes = [];
+
+        foreach ($queries as $query) {
+            $sql = $query['query'] ?? '';
+
+            if ($sql === '' || ! self::isWorthCounting($sql)) {
+                continue;
+            }
+
+            $shape = self::normalise($sql);
+
+            if (! isset($shapes[$shape])) {
+                $shapes[$shape] = ['sql' => $sql, 'count' => 0, 'bindings' => []];
+            }
+
+            $shapes[$shape]['count']++;
+            $shapes[$shape]['bindings'][json_encode($query['bindings'] ?? [])] = true;
+        }
+
+        $repeats = [];
+
+        foreach ($shapes as $shape) {
+            if ($shape['count'] < $threshold) {
+                continue;
+            }
+
+            $repeats[] = [
+                'sql' => $shape['sql'],
+                'count' => $shape['count'],
+                'distinctBindings' => count($shape['bindings']),
+            ];
+        }
+
+        usort($repeats, fn ($a, $b) => $b['count'] <=> $a['count']);
+
+        return $repeats;
+    }
+
+    /**
+     * Reduce a query to its shape: values that vary between executions of the
+     * same code path are replaced, so `id in (1, 2, 3)` and `id in (4, 5)`
+     * count as one shape.
+     */
+    public static function normalise(string $sql): string
+    {
+        // Collapse IN lists (both placeholders and inlined values) first, so
+        // batched loads of different sizes are recognised as the same shape.
+        $sql = preg_replace('/\bin\s*\([^()]*\)/i', 'in (?)', $sql);
+
+        // Inlined numbers and quoted strings.
+        $sql = preg_replace('/\b\d+\b/', '?', $sql);
+        $sql = preg_replace("/'[^']*'/", '?', $sql);
+
+        return preg_replace('/\s+/', ' ', trim($sql));
+    }
+
+    /**
+     * Transactions, savepoints and the like are issued per test by the harness
+     * itself and say nothing about the code under test.
+     */
+    private static function isWorthCounting(string $sql): bool
+    {
+        return (bool) preg_match('/^\s*(select|insert|update|delete)\b/i', $sql);
+    }
+
+    /**
+     * @param array<array{sql: string, count: int, distinctBindings: int}> $repeats
+     */
+    public static function describe(array $repeats): string
+    {
+        $lines = [];
+
+        foreach ($repeats as $repeat) {
+            $lines[] = sprintf(
+                '  %dx (%d distinct bindings): %s',
+                $repeat['count'],
+                $repeat['distinctBindings'],
+                strlen($repeat['sql']) > 160 ? substr($repeat['sql'], 0, 160).'…' : $repeat['sql']
+            );
+        }
+
+        return implode("\n", $lines);
+    }
+}
diff --git a/php-packages/testing/src/integration/TestCase.php b/php-packages/testing/src/integration/TestCase.php
@@ -295,7 +295,110 @@ protected function rowsThroughFactory(string $modelClass, array $rows): array
      */
     protected function send(ServerRequestInterface $request): ResponseInterface
     {
-        return $this->server()->handle($request);
+        if (! $this->detectsRepeatedQueries()) {
+            return $this->server()->handle($request);
+        }
+
+        $database = $this->database();
+        $wasLogging = $database->logging();
+
+        $database->flushQueryLog();
+        $database->enableQueryLog();
+
+        try {
+            $response = $this->server()->handle($request);
+        } finally {
+            $queries = $database->getQueryLog();
+
+            if (! $wasLogging) {
+                $database->disableQueryLog();
+            }
+        }
+
+        $this->reportRepeatedQueries($request, $queries);
+
+        return $response;
+    }
+
+    /**
+     * Whether requests sent through this test case are inspected for N+1 query
+     * patterns, failing the test when one is found.
+     *
+     * On by default: an N+1 is a defect, and finding it while writing the
+     * feature is far cheaper than finding it in production. Override in a test
+     * case that legitimately needs it off:
+     *
+     *     protected function detectsRepeatedQueries(): bool
+     *     {
+     *         return false; // and say why
+     *     }
+     *
+     * Prefer {@see allowedRepeatedQueries()} to exempt one known query shape
+     * rather than disabling the check for the whole test case. Setting
+     * FLARUM_DETECT_REPEATED_QUERIES=0 turns it off for a whole run, which is
+     * useful when bisecting an unrelated failure.
+     */
+    protected function detectsRepeatedQueries(): bool
+    {
+        $env = getenv('FLARUM_DETECT_REPEATED_QUERIES');
+
+        return $env === false || $env === '' ? true : (bool) filter_var($env, FILTER_VALIDATE_BOOLEAN);
+    }
+
+    /**
+     * Repetitions of one query shape tolerated before a request is reported.
+     * Small fixed-size loops (the actor, a couple of authors) are normal; an
+     * N+1 grows with the fixture, so it clears any sensible threshold.
+     */
+    protected function repeatedQueryThreshold(): int
+    {
+        return (int) (getenv('FLARUM_REPEATED_QUERY_THRESHOLD') ?: 5);
+    }
+
+    /**
+     * Query shapes this test case knowingly repeats, as substrings of the
+     * normalised SQL — e.g. `'from `sessions`'`. A shape matching any of these
+     * does not fail the test.
+     *
+     * Preferable to turning the check off wholesale: the rest of the request
+     * stays covered. Say why each entry is legitimate.
+     *
+     * @return string[]
+     */
+    protected function allowedRepeatedQueries(): array
+    {
+        return [];
+    }
+
+    /**
+     * @param array<array{query: string, bindings: array}> $queries
+     */
+    private function reportRepeatedQueries(ServerRequestInterface $request, array $queries): void
+    {
+        $repeats = RepeatedQueryDetector::findRepeats($queries, $this->repeatedQueryThreshold());
+
+        foreach ($this->allowedRepeatedQueries() as $allowed) {
+            $repeats = array_values(array_filter(
+                $repeats,
+                fn (array $repeat) => ! str_contains(RepeatedQueryDetector::normalise($repeat['sql']), $allowed)
+            ));
+        }
+
+        if (! $repeats) {
+            return;
+        }
+
+        self::fail(sprintf(
+            "%s %s ran repeated queries — likely an N+1.\n\n%s\n\n"
+            ."A query shape repeated with different bindings is usually a relationship, permission check or\n"
+            ."serialized field resolved per model; load it for the whole page instead (eager loading, a\n"
+            ."batched relation, or a single grouped query). If the repetition is legitimate, list the shape\n"
+            ."in %s::allowedRepeatedQueries() with a comment saying why.",
+            $request->getMethod(),
+            (string) $request->getUri()->getPath(),
+            RepeatedQueryDetector::describe($repeats),
+            static::class
+        ));
     }
 
     /**
diff --git a/php-packages/testing/tests/tests/unit/RepeatedQueryDetectorTest.php b/php-packages/testing/tests/tests/unit/RepeatedQueryDetectorTest.php
@@ -0,0 +1,154 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Testing\Tests\unit;
+
+use Flarum\Testing\integration\RepeatedQueryDetector;
+use PHPUnit\Framework\Attributes\Test;
+use PHPUnit\Framework\TestCase;
+
+class RepeatedQueryDetectorTest extends TestCase
+{
+    /**
+     * @param array<array{0: string, 1: array}> $queries
+     */
+    private function log(array $queries): array
+    {
+        return array_map(fn ($q) => ['query' => $q[0], 'bindings' => $q[1]], $queries);
+    }
+
+    #[Test]
+    public function a_relationship_loaded_per_model_is_reported()
+    {
+        // The shape of a real N+1: one query per record, different binding
+        // each time. (Taken from a post listing loading each post's warnings.)
+        $queries = [];
+
+        for ($id = 100; $id < 110; $id++) {
+            $queries[] = ['select * from `warnings` where `warnings`.`post_id` = ?', [$id]];
+        }
+
+        $repeats = RepeatedQueryDetector::findRepeats($this->log($queries), 5);
+
+        $this->assertCount(1, $repeats);
+        $this->assertSame(10, $repeats[0]['count']);
+        $this->assertSame(10, $repeats[0]['distinctBindings']);
+    }
+
+    #[Test]
+    public function batched_loads_of_differing_sizes_are_one_shape_not_an_n_plus_one()
+    {
+        // Eager loading emits `in (...)` lists of whatever length; those are
+        // the fix for an N+1, not an instance of one.
+        $repeats = RepeatedQueryDetector::findRepeats($this->log([
+            ['select * from `users` where `users`.`id` in (1, 2, 3)', []],
+            ['select * from `users` where `users`.`id` in (4, 5)', []],
+        ]), 5);
+
+        $this->assertSame([], $repeats);
+    }
+
+    #[Test]
+    public function a_handful_of_distinct_queries_is_not_reported()
+    {
+        $repeats = RepeatedQueryDetector::findRepeats($this->log([
+            ['select * from `discussions` where `id` = ?', [1]],
+            ['select * from `posts` where `discussion_id` = ?', [1]],
+            ['select * from `users` where `id` in (?, ?)', [1, 2]],
+        ]), 5);
+
+        $this->assertSame([], $repeats);
+    }
+
+    #[Test]
+    public function repeats_below_the_threshold_are_left_alone()
+    {
+        // Small fixed-size loops are everywhere in a request (the actor, a
+        // couple of authors) and must not be flagged.
+        $queries = [];
+
+        for ($id = 1; $id <= 4; $id++) {
+            $queries[] = ['select * from `groups` where `user_id` = ?', [$id]];
+        }
+
+        $this->assertSame([], RepeatedQueryDetector::findRepeats($this->log($queries), 5));
+    }
+
+    #[Test]
+    public function the_same_query_with_the_same_bindings_is_distinguished_from_an_n_plus_one()
+    {
+        // A missed memoisation: the same rows fetched repeatedly. Worth
+        // reporting, but the binding count tells the reader it does NOT grow
+        // with the data — the distinction that stops false N+1 diagnoses.
+        $queries = array_fill(0, 6, ['select * from `settings` where `key` = ?', ['foo']]);
+
+        $repeats = RepeatedQueryDetector::findRepeats($this->log($queries), 5);
+
+        $this->assertCount(1, $repeats);
+        $this->assertSame(6, $repeats[0]['count']);
+        $this->assertSame(1, $repeats[0]['distinctBindings']);
+    }
+
+    #[Test]
+    public function harness_transactions_are_ignored()
+    {
+        $queries = array_fill(0, 10, ['SAVEPOINT trans1', []]);
+        $queries[] = ['BEGIN', []];
+
+        $this->assertSame([], RepeatedQueryDetector::findRepeats($this->log($queries), 5));
+    }
+
+    #[Test]
+    public function inlined_values_normalise_to_the_same_shape_as_placeholders()
+    {
+        // Some code paths inline values instead of binding them; they are the
+        // same shape and must count together.
+        $repeats = RepeatedQueryDetector::findRepeats($this->log([
+            ['select * from `posts` where `id` = 1', []],
+            ['select * from `posts` where `id` = 2', []],
+            ['select * from `posts` where `id` = ?', [3]],
+            ['select * from `posts` where `id` = ?', [4]],
+            ['select * from `posts` where `id` = ?', [5]],
+        ]), 5);
+
+        $this->assertCount(1, $repeats);
+        $this->assertSame(5, $repeats[0]['count']);
+    }
+
+    #[Test]
+    public function the_worst_offender_is_reported_first()
+    {
+        $queries = [];
+
+        for ($i = 0; $i < 6; $i++) {
+            $queries[] = ['select * from `flags` where `post_id` = ?', [$i]];
+        }
+        for ($i = 0; $i < 12; $i++) {
+            $queries[] = ['select * from `warnings` where `post_id` = ?', [$i]];
+        }
+
+        $repeats = RepeatedQueryDetector::findRepeats($this->log($queries), 5);
+
+        $this->assertCount(2, $repeats);
+        $this->assertStringContainsString('warnings', $repeats[0]['sql']);
+        $this->assertSame(12, $repeats[0]['count']);
+    }
+
+    #[Test]
+    public function the_description_names_counts_and_binding_diversity()
+    {
+        $description = RepeatedQueryDetector::describe([
+            ['sql' => 'select * from `warnings` where `post_id` = ?', 'count' => 10, 'distinctBindings' => 10],
+        ]);
+
+        $this->assertStringContainsString('10x', $description);
+        $this->assertStringContainsString('10 distinct bindings', $description);
+        $this->assertStringContainsString('warnings', $description);
+    }
+}
