[2.x] feat: fail integration tests that run N+1 queries (and fix the first one it found) (#4871)

* feat: fail integration tests that run N+1 queries

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.

* Apply fixes from StyleCI

* perf: eager load nested includes on the flags endpoint

The flags index declares post.discussion and post.user as default
includes, but never eager loaded what those nested resources need. Core's
DiscussionResource eager loads the actor's discussion state on its own
endpoints; that doesn't carry over when a discussion is included by
another resource. So every flag on the moderation page read
discussion_user on its own, and the flag authors' groups were re-fetched
per flag.

Caught by the N+1 detection added in this branch, on its first CI run.

* feat: warn, rather than fail, when repeated queries don't scale

Running the detector across the bundled extensions turned up 22 findings
beyond the flags N+1, and they were all the same shape: a query repeated
5-10 times for only 1-4 distinct values. That is not an N+1 — five
queries for two users stays five queries whether the forum has two users
or two million. Failing on it would have meant rewriting formatter and
write-path code for no scaling benefit, so the threshold was the thing
that was wrong.

The two cases are now distinguished by the data already being collected.
Roughly as many distinct bindings as executions means one query per
record: that fails, because the work grows with the forum. A handful of
values repeated is wasteful but bounded: that raises a PHP warning, which
PHPUnit attributes to the test without failing the run.

mentions (19 findings) and subscriptions (1) now pass; the flags N+1
still fails when its fix is reverted.

* feat: surface query findings on the pull request, not just in logs

The warning tier was invisible in three separate ways. The trigger_error
call was prefixed with @, so PHPUnit never saw it at all — nothing
appeared even with --display-warnings. Only one of eighteen phpunit
configs in this repo set displayDetailsOnTestsThatTriggerWarnings, so
even a working warning printed no detail. And a developer who relies on
CI rather than local runs would have to read the middle of a job log to
find either.

So: the @ is gone, every integration config displays warning details, and
the detector appends findings to FLARUM_REPEATED_QUERY_LOG when it is
set. The reusable backend workflow points that at a temp file and turns
it into GitHub annotations — errors for N+1s, warnings for non-scaling
repetition — plus a table in the run summary. Annotations attach to the
pull request, which is where the audience that most needs them is
looking.

* feat: tell the developer how to see warning detail

Without displayDetailsOnTestsThatTriggerWarnings PHPUnit prints only a
count — 'Warnings: 11' — which is visible but not actionable. The first
warning of a run now carries the pointer to --display-warnings and the
config setting.

Once per run, not per finding: tests run with processIsolation, so each
test is a separate process and a static flag cannot track 'first'. A
marker file keyed to the project and the hour serves as the shared
signal.

---------

Co-authored-by: StyleCI Bot <bot@styleci.io>

Author: imorland

.github/workflows/REUSABLE_backend.yml

@@ -287,12 +287,43 @@ jobs:
           fi
         working-directory: ${{ inputs.backend_directory }}
         env:
+          FLARUM_REPEATED_QUERY_LOG: ${{ runner.temp }}/repeated-queries.jsonl
           DB_HOST: 127.0.0.1
           DB_PORT: ${{ (matrix.driver == 'mysql' && job.services.mysql.ports['3306']) || (matrix.driver == 'mariadb' && job.services.mariadb.ports['3306']) || (matrix.driver == 'pgsql' && job.services.postgres.ports['5432']) }}
           DB_PREFIX: ${{ matrix.prefix }}
           DB_DRIVER: ${{ matrix.driver }}
           COMPOSER_PROCESS_TIMEOUT: 600
 
+      # Surface the query findings on the pull request itself. A developer who
+      # only ever looks at the checks page would otherwise never see the
+      # warnings PHPUnit prints mid-log. Runs even when the tests fail, since a
+      # failing N+1 is exactly what we want to point at.
+      - name: Report repeated queries
+        if: ${{ !cancelled() }}
+        run: |
+          log="${{ runner.temp }}/repeated-queries.jsonl"
+
+          [ -s "$log" ] || exit 0
+
+          # One annotation per distinct finding; identical findings recur across
+          # tests, so collapse them first.
+          sort -u "$log" | while read -r finding; do
+            level=$(echo "$finding" | jq -r 'if .scaling then "error" else "warning" end')
+            title=$(echo "$finding" | jq -r 'if .scaling then "N+1 query" else "Repeated query" end')
+            echo "$finding" | jq -r --arg level "$level" --arg title "$title" \
+              '"::\($level) title=\($title)::\(.where) — \(.count) executions, \(.distinctBindings) distinct: \(.sql)"'
+          done
+
+          {
+            echo "### Query findings"
+            echo
+            echo "| | Where | Executions | Distinct | Query |"
+            echo "|---|---|---|---|---|"
+            sort -u "$log" | jq -r '"| \(if .scaling then "🛑 N+1" else "⚠️ repeated" end) | `\(.where)` | \(.count) | \(.distinctBindings) | `\(.sql | .[0:120])` |"'
+            echo
+            echo "🛑 grows with your data — fix it. ⚠️ repeats a few values — consider memoising."
+          } >> "$GITHUB_STEP_SUMMARY"
+
   phpstan:
     runs-on: ${{ inputs.runner_type }}
 

extensions/akismet/tests/phpunit.integration.xml

@@ -5,6 +5,7 @@
     backupGlobals="false"
     cacheDirectory=".phpunit.cache"
     backupStaticProperties="false"
+    displayDetailsOnTestsThatTriggerWarnings="true"
     colors="true"
     processIsolation="true"
     stopOnFailure="false"

extensions/approval/tests/phpunit.integration.xml

@@ -5,6 +5,7 @@
     backupGlobals="false"
     cacheDirectory=".phpunit.cache"
     backupStaticProperties="false"
+    displayDetailsOnTestsThatTriggerWarnings="true"
     colors="true"
     processIsolation="true"
     stopOnFailure="false"

extensions/audit/tests/phpunit.integration.xml

@@ -5,6 +5,7 @@
     backupGlobals="false"
     cacheDirectory=".phpunit.cache"
     backupStaticProperties="false"
+    displayDetailsOnTestsThatTriggerWarnings="true"
     colors="true"
     processIsolation="true"
     stopOnFailure="false"

extensions/flags/src/Api/Resource/FlagResource.php

@@ -96,6 +96,16 @@ public function endpoints(): array
             Endpoint\Index::make()
                 ->authenticated()
                 ->defaultInclude(['user', 'post', 'post.user', 'post.discussion'])
+                // The included discussions and users are serialized like any
+                // others, so they need the relations their own resources
+                // eager load: the actor's discussion state, and group
+                // memberships for permission checks. Without this each
+                // flag's discussion reads `discussion_user` on its own.
+                ->eagerLoad([
+                    'post.discussion.state',
+                    'post.user.groups',
+                    'user.groups',
+                ])
                 ->defaultSort('-createdAt')
                 ->paginate()
                 ->after(function (FlarumContext $context, $data) {

extensions/flags/tests/phpunit.integration.xml

@@ -5,6 +5,7 @@
     backupGlobals="false"
     cacheDirectory=".phpunit.cache"
     backupStaticProperties="false"
+    displayDetailsOnTestsThatTriggerWarnings="true"
     colors="true"
     processIsolation="true"
     stopOnFailure="false"

extensions/gdpr/tests/phpunit.integration.xml

@@ -5,6 +5,7 @@
     backupGlobals="false"
     cacheDirectory=".phpunit.cache"
     backupStaticProperties="false"
+    displayDetailsOnTestsThatTriggerWarnings="true"
     colors="true"
     processIsolation="true"
     stopOnFailure="false"

extensions/likes/tests/phpunit.integration.xml

@@ -5,6 +5,7 @@
     backupGlobals="false"
     cacheDirectory=".phpunit.cache"
     backupStaticProperties="false"
+    displayDetailsOnTestsThatTriggerWarnings="true"
     colors="true"
     processIsolation="true"
     stopOnFailure="false"

extensions/lock/tests/phpunit.integration.xml

@@ -5,6 +5,7 @@
     backupGlobals="false"
     cacheDirectory=".phpunit.cache"
     backupStaticProperties="false"
+    displayDetailsOnTestsThatTriggerWarnings="true"
     colors="true"
     processIsolation="true"
     stopOnFailure="false"

extensions/mentions/tests/phpunit.integration.xml

@@ -5,6 +5,7 @@
     backupGlobals="false"
     cacheDirectory=".phpunit.cache"
     backupStaticProperties="false"
+    displayDetailsOnTestsThatTriggerWarnings="true"
     colors="true"
     processIsolation="true"
     stopOnFailure="false"