feat(aggregations): cross-schema joins with literal-filter by rubenvdlinde · Pull Request #1479 · ConductionNL/openregister

rubenvdlinde · 2026-05-11T20:08:19Z

Summary

Adds from field support to x-openregister-aggregations enabling cross-schema aggregations (e.g. a Regulation schema counting rows in an Enrolment schema).
@self.<field> references in where clauses resolve against the parent object row, so each parent object gets its own scoped count.
Supports eq, ne, gt, gte, lt, lte, in operators in cross-schema where clauses via the existing filter engine.
Postgres-native fast path (SQL) is used for the target schema table; PHP-fallback applies when Postgres is unavailable. Both paths are capped at 10k rows (configurable via limit).
RBAC is double-gated: the parent schema's list permission runs in run(), and the target schema's list permission runs inside runCrossSchema() — callers cannot leak counts from schemas they cannot list.
Adds select/where aliases for metric/filter on intra-schema specs (backward-compatible).
AggregationAnnotationValidator now accepts cross-schema specs with lighter validation (field-existence checks are skipped for the target schema which is not available at annotation-save time).

Closes scholiq deps #7.

Test plan

AggregationAnnotationValidatorTest: 7 new cross-schema validator cases (empty from, bad metric, non-map where, field-existence skipped, select/where aliases)
CrossSchemaAggregationRunnerTest: 6 new runner unit tests (@self ref resolution, missing-field fail-closed, in operator, schema-not-found, register-not-found, RBAC gate on target schema)
All existing AggregationRunnerIntegrationTest intra-schema tests remain green (no behaviour change to existing path)
PHPCS clean on lib/ (0 errors, 0 warnings treated as errors)
PHPMD clean on changed files
Psalm — no errors on changed files

…holiq deps #7) Add `from` field support to `x-openregister-aggregations` enabling cross-schema aggregations. When a spec declares `from: <schema-slug>`, the engine queries the named schema's table rather than the parent schema's table. Key changes: - AggregationRunner: delegate to runCrossSchema() when `from` is present; supports @self.<field> parent-reference resolution in `where` clauses; applies eq/ne/gt/gte/ lt/lte/in operators via the existing applyFilter path; uses Postgres-native fast path for the target schema table; double-gates RBAC on both parent and target schemas; caches independently per parent-row field values. - AggregationRunner: adds `select`/`where` aliases for `metric`/`filter` (new DSL) on intra-schema specs too; adds optional `parentRow` param to run() for @self resolution. - AggregationAnnotationValidator: accepts cross-schema specs (`from` key) with lighter validation (field-existence skipped for target schema); supports `select`/`where` aliases on intra-schema specs. - Tests: 7 validator unit tests for cross-schema DSL + 6 runner unit tests covering @self ref resolution, in-operator filtering, schema-not-found, register-not-found, malformed-where, and RBAC gate on target schema.

github-actions · 2026-05-11T20:14:22Z

Quality Report — ConductionNL/openregister @ `e34e2fe`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		❌
stylelint		✅
composer			✅	✅ 153/153
npm			✅	✅ 598/598
PHPUnit					❌
Newman					⏭️
Playwright					⏭️

Quality workflow — 2026-05-11 20:14 UTC

Download the full PDF report from the workflow artifacts.

WilcoLouwerse · 2026-05-12T13:25:04Z

+            object: null
+        ) === false
+        ) {
+            throw new RuntimeException(


[BLOCKER] Cross-schema RBAC denial throws RuntimeException → HTTP 404 instead of 403

The cross-schema target-schema RBAC gate throws RuntimeException, which AggregationController catches and returns as HTTP 404. A caller with list permission on the parent schema but not the target receives a 404 — identical to a missing resource — instead of a 403. This is an information-disclosure vector: an attacker can enumerate which cross-schema targets exist by observing 404 vs. other errors. Fix: replace throw new RuntimeException(…'Forbidden…') with throw new NotAuthorizedException(…) to match the parent-schema gate behaviour.

WilcoLouwerse · 2026-05-12T13:25:06Z

[BLOCKER] SchemaMapper mock willReturnMap won't match — likely cause of CI PHPUnit failures on PHP 8.3 + 8.4

AggregationRunner::loadSchema() calls $this->schemaMapper->find($schemaRef) with one argument; PHPUnit's willReturnMap matches against explicitly-passed arguments only. Every test using willReturnMap([['regulation', [], null, true, true, $parentSchema], …]) will miss the match (the mock receives ['regulation'], not ['regulation', [], null, true, true]) and return null, causing every positive test to fail with an unexpected RuntimeException. Fix: switch to ->willReturnCallback(fn($ref) => match($ref) {…}) or use single-element map entries.

WilcoLouwerse · 2026-05-12T13:25:07Z

+        $registers = $this->registerMapper->findAll();
+        foreach ($registers as $register) {
+            $schemaIds = $register->getSchemas();
+            if (is_array($schemaIds) === true && in_array($schemaId, $schemaIds, false) === true) {


[BLOCKER] Loose in_array comparison in findRegisterForSchema — type mismatch silently misses schema

in_array($schemaId, $schemaIds, false) uses loose comparison. 0 == 'any-non-numeric-string' is true in PHP < 8.0 (still supported by NC stable32). Use in_array($schemaId, $schemaIds, true) and normalise the schemas list to int in setSchemas() to eliminate the ambiguity.

WilcoLouwerse · 2026-05-12T13:25:08Z

+     *
+     * @param Schema $schema The target schema to find a register for.
+     *
+     * @return Register|null The first matching register, or null.


[CONCERN] findRegisterForSchema fetches all registers on every cross-schema call — unguarded N+1

findRegisterForSchema() calls $this->registerMapper->findAll() without any limit or caching. On a tenant with hundreds of registers this is a full table scan per cross-schema aggregation request. Add a private array $registerCache = [] keyed on schema ID, or pass the already-loaded $parentRegister as a hint when the target schema is in the same register.

WilcoLouwerse · 2026-05-12T13:25:10Z

[CONCERN] No test exercises the Postgres-native path for cross-schema aggregation

All seven cross-schema runner tests force the PHP-fallback path. The tryNativeAggregation() branch (which builds a parameterised SQL query) is entirely untested at the unit level. Add at least one test that stubs getDatabasePlatform to return a PostgreSQL-named platform and verifies the backend: postgres key in the result.

WilcoLouwerse · 2026-05-12T13:25:12Z

[CONCERN] testMalformedWhereClauseCastToArrayDoesNotCrash assertion is too weak

The test passes a non-map where string and only asserts assertArrayHasKey('value', $result) — it would pass even if the code returned 1 (bug). Assert $this->assertSame(0, $result['value']) to capture the fail-closed behaviour.

WilcoLouwerse · 2026-05-12T13:25:14Z

[CONCERN] No test for empty result set from cross-schema aggregation

None of the tests verify behaviour when the target schema's table returns zero rows. For count the expected value is 0; for avg/min/max the expected value is null. Add a test where findAllInRegisterSchemaTable returns [].

WilcoLouwerse · 2026-05-12T13:25:15Z

+    ): array {
+        $fromRef = (string) ($spec['from'] ?? '');
+        if ($fromRef === '') {
+            throw new RuntimeException('Cross-schema aggregation spec is missing a non-empty `from` key.');


[CONCERN] runCrossSchema swallows Throwable from native path without logging

tryNativeAggregation() catches \Throwable and returns null to signal fallback. A misconfigured target table will silently degrade to the PHP-fallback path with no observable signal. Add a $this->logger?->debug(…) when the native path falls through for cross-schema calls.

WilcoLouwerse · 2026-05-12T13:25:16Z

+     *
+     * @return array<int, array{code: string, message: string}> Error list (empty = valid).
+     */
+    private function validateCrossSchemaSpec(string $name, array $spec): array


[CONCERN] validateCrossSchemaSpec does not validate @self reference syntax

validateCrossSchemaSpec accepts any string value in where, including malformed @self references like @self. or bare @self. A typo like @self.slugg passes validation silently and produces a zero-count result that looks correct. Add a pattern check: any string matching /^@self\./ should have a non-empty field name after the dot.

WilcoLouwerse

Review

🔴 Blockers (3)

Cross-schema RBAC denial throws RuntimeException → HTTP 404 instead of 403 — lib/Service/Aggregation/AggregationRunner.php:910
SchemaMapper mock willReturnMap won't match — likely cause of CI PHPUnit failures on PHP 8.3 + 8.4 — tests/Unit/Service/Aggregation/CrossSchemaAggregationRunnerTest.php:892
Loose in_array comparison in findRegisterForSchema — type mismatch silently misses schema — lib/Service/Aggregation/AggregationRunner.php:1108

🟡 Concerns (6)

findRegisterForSchema fetches all registers on every cross-schema call — unguarded N+1 — lib/Service/Aggregation/AggregationRunner.php:1095
No test exercises the Postgres-native path for cross-schema aggregation — tests/Unit/Service/Aggregation/CrossSchemaAggregationRunnerTest.php:874
testMalformedWhereClauseCastToArrayDoesNotCrash assertion is too weak — tests/Unit/Service/Aggregation/CrossSchemaAggregationRunnerTest.php:1146
No test for empty result set from cross-schema aggregation — tests/Unit/Service/Aggregation/CrossSchemaAggregationRunnerTest.php:874
runCrossSchema swallows Throwable from native path without logging — lib/Service/Aggregation/AggregationRunner.php:884
validateCrossSchemaSpec does not validate @self reference syntax — lib/Service/Aggregation/AggregationAnnotationValidator.php:205

🟢 Minor (1)

Three new PHPMD SuppressWarnings added — symptom of growing class complexity (lib/Service/Aggregation/AggregationRunner.php:145)
@SuppressWarnings(PHPMD.ExcessiveClassLength) at class level plus ExcessiveMethodLength, CyclomaticComplexity, NPathComplexity on runCrossSchema(). runCrossSchema() is 160+ lines with three distinct paths. Consider extracting a CrossSchemaAggregationPipeline class rather than suppressing the warnings.

Reviewed by WilcoLouwerse via automated batch review.

WilcoLouwerse · 2026-05-12T13:26:06Z

+            object: null
+        ) === false
+        ) {
+            throw new RuntimeException(


[BLOCKER] Cross-schema RBAC denial throws RuntimeException → HTTP 404 instead of 403

The cross-schema target-schema RBAC gate throws RuntimeException, which AggregationController catches and returns as HTTP 404. A caller with list permission on the parent schema but not the target receives a 404 — identical to a missing resource — instead of a 403. This is an information-disclosure vector: an attacker can enumerate which cross-schema targets exist by observing 404 vs. other errors. Fix: replace throw new RuntimeException(…'Forbidden…') with throw new NotAuthorizedException(…) to match the parent-schema gate behaviour.

WilcoLouwerse · 2026-05-12T13:26:16Z

[BLOCKER] SchemaMapper mock willReturnMap won't match — likely cause of CI PHPUnit failures on PHP 8.3 + 8.4

AggregationRunner::loadSchema() calls $this->schemaMapper->find($schemaRef) with one argument; PHPUnit's willReturnMap matches against explicitly-passed arguments only. Every test using willReturnMap([['regulation', [], null, true, true, $parentSchema], …]) will miss the match (the mock receives ['regulation'], not ['regulation', [], null, true, true]) and return null, causing every positive test to fail with an unexpected RuntimeException. Fix: switch to ->willReturnCallback(fn($ref) => match($ref) {…}) or use single-element map entries.

WilcoLouwerse · 2026-05-12T13:26:21Z

+        $registers = $this->registerMapper->findAll();
+        foreach ($registers as $register) {
+            $schemaIds = $register->getSchemas();
+            if (is_array($schemaIds) === true && in_array($schemaId, $schemaIds, false) === true) {


[BLOCKER] Loose in_array comparison in findRegisterForSchema — type mismatch silently misses schema

in_array($schemaId, $schemaIds, false) uses loose comparison. 0 == 'any-non-numeric-string' is true in PHP < 8.0 (still supported by NC stable32). Use in_array($schemaId, $schemaIds, true) and normalise the schemas list to int in setSchemas() to eliminate the ambiguity.

WilcoLouwerse · 2026-05-12T13:26:26Z

+     *
+     * @param Schema $schema The target schema to find a register for.
+     *
+     * @return Register|null The first matching register, or null.


[CONCERN] findRegisterForSchema fetches all registers on every cross-schema call — unguarded N+1

findRegisterForSchema() calls $this->registerMapper->findAll() without any limit or caching. On a tenant with hundreds of registers this is a full table scan per cross-schema aggregation request. Add a private array $registerCache = [] keyed on schema ID, or pass the already-loaded $parentRegister as a hint when the target schema is in the same register.

WilcoLouwerse · 2026-05-12T13:26:36Z

[CONCERN] No test exercises the Postgres-native path for cross-schema aggregation

All seven cross-schema runner tests force the PHP-fallback path. The tryNativeAggregation() branch (which builds a parameterised SQL query) is entirely untested at the unit level. Add at least one test that stubs getDatabasePlatform to return a PostgreSQL-named platform and verifies the backend: postgres key in the result.

WilcoLouwerse · 2026-05-12T13:26:45Z

[CONCERN] testMalformedWhereClauseCastToArrayDoesNotCrash assertion is too weak

The test passes a non-map where string and only asserts assertArrayHasKey('value', $result) — it would pass even if the code returned 1 (bug). Assert $this->assertSame(0, $result['value']) to capture the fail-closed behaviour.

WilcoLouwerse · 2026-05-12T13:26:55Z

[CONCERN] No test for empty result set from cross-schema aggregation

None of the tests verify behaviour when the target schema's table returns zero rows. For count the expected value is 0; for avg/min/max the expected value is null. Add a test where findAllInRegisterSchemaTable returns [].

WilcoLouwerse · 2026-05-12T13:27:00Z

+    ): array {
+        $fromRef = (string) ($spec['from'] ?? '');
+        if ($fromRef === '') {
+            throw new RuntimeException('Cross-schema aggregation spec is missing a non-empty `from` key.');


[CONCERN] runCrossSchema swallows Throwable from native path without logging

tryNativeAggregation() catches \Throwable and returns null to signal fallback. A misconfigured target table will silently degrade to the PHP-fallback path with no observable signal. Add a $this->logger?->debug(…) when the native path falls through for cross-schema calls.

WilcoLouwerse · 2026-05-12T13:27:06Z

+     *
+     * @return array<int, array{code: string, message: string}> Error list (empty = valid).
+     */
+    private function validateCrossSchemaSpec(string $name, array $spec): array


[CONCERN] validateCrossSchemaSpec does not validate @self reference syntax

validateCrossSchemaSpec accepts any string value in where, including malformed @self references like @self. or bare @self. A typo like @self.slugg passes validation silently and produces a zero-count result that looks correct. Add a pattern check: any string matching /^@self\./ should have a non-empty field name after the dot.

WilcoLouwerse

Review

🔴 Blockers (3)

Cross-schema RBAC denial throws RuntimeException → HTTP 404 instead of 403 — lib/Service/Aggregation/AggregationRunner.php:910
SchemaMapper mock willReturnMap won't match — likely cause of CI PHPUnit failures on PHP 8.3 + 8.4 — tests/Unit/Service/Aggregation/CrossSchemaAggregationRunnerTest.php:892
Loose in_array comparison in findRegisterForSchema — type mismatch silently misses schema — lib/Service/Aggregation/AggregationRunner.php:1108

🟡 Concerns (6)

findRegisterForSchema fetches all registers on every cross-schema call — unguarded N+1 — lib/Service/Aggregation/AggregationRunner.php:1095
No test exercises the Postgres-native path for cross-schema aggregation — tests/Unit/Service/Aggregation/CrossSchemaAggregationRunnerTest.php:874
testMalformedWhereClauseCastToArrayDoesNotCrash assertion is too weak — tests/Unit/Service/Aggregation/CrossSchemaAggregationRunnerTest.php:1146
No test for empty result set from cross-schema aggregation — tests/Unit/Service/Aggregation/CrossSchemaAggregationRunnerTest.php:874
runCrossSchema swallows Throwable from native path without logging — lib/Service/Aggregation/AggregationRunner.php:884
validateCrossSchemaSpec does not validate @self reference syntax — lib/Service/Aggregation/AggregationAnnotationValidator.php:205

🟢 Minor (1)

Three new PHPMD SuppressWarnings added — symptom of growing class complexity (lib/Service/Aggregation/AggregationRunner.php:145)
@SuppressWarnings(PHPMD.ExcessiveClassLength) at class level plus ExcessiveMethodLength, CyclomaticComplexity, NPathComplexity on runCrossSchema(). runCrossSchema() is 160+ lines with three distinct paths. Consider extracting a CrossSchemaAggregationPipeline class rather than suppressing the warnings.

Reviewed by WilcoLouwerse via automated batch review.

WilcoLouwerse

Review verdict

3 blockers: RBAC returns 404 instead of 403, PHPUnit mock mismatch, loose in_array. See inline comments above.

Reviewed by WilcoLouwerse — inline comments posted above.

rubenvdlinde requested review from WilcoLouwerse, bbrands02 and rjzondervan as code owners May 11, 2026 20:08

rubenvdlinde mentioned this pull request May 12, 2026

Consolidation: AI chat companion + 7 scholiq-deps features + 8 more clean/approved branches (PHPUnit green) #1496

Merged

WilcoLouwerse reviewed May 12, 2026

View reviewed changes

WilcoLouwerse requested changes May 12, 2026

View reviewed changes

WilcoLouwerse reviewed May 12, 2026

View reviewed changes

WilcoLouwerse requested changes May 12, 2026

View reviewed changes

WilcoLouwerse closed this pull request by merging all changes into development in f86e83f May 15, 2026

WilcoLouwerse deleted the feat/scholiq-deps/cross-schema-aggregations branch May 15, 2026 09:40

Conversation

rubenvdlinde commented May 11, 2026

Summary

Test plan

Uh oh!

github-actions Bot commented May 11, 2026

Quality Report — ConductionNL/openregister @ e34e2fe

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilcoLouwerse left a comment

Choose a reason for hiding this comment

Review

🔴 Blockers (3)

🟡 Concerns (6)

🟢 Minor (1)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WilcoLouwerse left a comment

Choose a reason for hiding this comment

Review

🔴 Blockers (3)

🟡 Concerns (6)

🟢 Minor (1)

Uh oh!

WilcoLouwerse left a comment

Choose a reason for hiding this comment

Review verdict

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Quality Report — ConductionNL/openregister @ `e34e2fe`