From 6c025c4949010117e8ca641ac8374b8dca358661 Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Tue, 20 Oct 2026 10:12:43 +0200 Subject: [PATCH] fix(review): #4580 union-path referenceType skip is tested and judged across every schema Threads: https://github.com/ConductionNL/openregister/pull/4580#discussion_r4322480908 https://github.com/ConductionNL/openregister/pull/4580#discussion_r4322480920 https://github.com/ConductionNL/openregister/pull/4580#discussion_r4322480927 - getTermsFacetUnion() skipped the object-name lookup when the FIRST table's schema declared a referenceType for the column, so a union whose later schema holds real object references lost those names. The new anyHoldsObjectReferences() keeps the lookup unless every schema of the union declares a referenceType. - testUnionIntegrationReferencesAreNotLookedUpAsObjects calls the union path with a referenceType property and expects no lookup; removing the skip turns it red. testUnionKeepsTheLookupWhenAnySchemaHoldsObjectReferences pins the mixed union; the first-schema-only check turns it red. - The @spec exclude text on holdsObjectReferences() now says a UUID-shaped integration id is labelled in full instead of its shortened form. Co-Authored-By: Claude Opus 5.5 --- lib/Db/MagicMapper/MagicFacetHandler.php | 28 ++++- .../Db/MagicFacetHandlerBucketLabelTest.php | 109 ++++++++++++++++++ 2 files changed, 135 insertions(+), 2 deletions(-) diff --git a/lib/Db/MagicMapper/MagicFacetHandler.php b/lib/Db/MagicMapper/MagicFacetHandler.php index 37bf6b6438..5f2e52cb57 100644 --- a/lib/Db/MagicMapper/MagicFacetHandler.php +++ b/lib/Db/MagicMapper/MagicFacetHandler.php @@ -750,7 +750,7 @@ private function getTermsFacetUnion( $labelMap = []; $firstConfig = reset($tableConfigs); if ($isMetadata === false - && $this->holdsObjectReferences(schema: $firstConfig['schema'], column: $field) === false + && $this->anyHoldsObjectReferences(tableConfigs: $tableConfigs, column: $field) === false ) { $uuidsToResolve = []; } @@ -1490,7 +1490,7 @@ private function getTermsFacet( * * @return bool False when the property declares a referenceType. * - * @spec exclude performance guard on facet label resolution; labels for object references are unchanged + * @spec exclude facet label performance guard; object-reference labels unchanged, a UUID-shaped integration id is labelled in full */ private function holdsObjectReferences(Schema $schema, string $column): bool { foreach ($schema->getProperties() ?? [] as $name => $property) { @@ -1505,6 +1505,30 @@ private function holdsObjectReferences(Schema $schema, string $column): bool { return true; }//end holdsObjectReferences() + /** + * Whether any table of a union facet can hold object references in a column. + * + * Two schemas in one union can share a column name with a different meaning, + * so the name lookup is skipped only when every schema declares a + * referenceType for it; one schema holding object references keeps it. + * + * @param array $tableConfigs The union's table configurations. + * @param string $column The column name as the facet queries it. + * + * @return bool False only when every schema declares a referenceType for the column. + * + * @spec exclude performance guard on facet label resolution, the multi-schema form of holdsObjectReferences() + */ + private function anyHoldsObjectReferences(array $tableConfigs, string $column): bool { + foreach ($tableConfigs as $config) { + if ($this->holdsObjectReferences(schema: $config['schema'], column: $column) === true) { + return true; + } + } + + return false; + }//end anyHoldsObjectReferences() + /** * Pick the label for one terms bucket. * diff --git a/tests/Unit/Db/MagicFacetHandlerBucketLabelTest.php b/tests/Unit/Db/MagicFacetHandlerBucketLabelTest.php index 2ee09a611f..21871b44a7 100644 --- a/tests/Unit/Db/MagicFacetHandlerBucketLabelTest.php +++ b/tests/Unit/Db/MagicFacetHandlerBucketLabelTest.php @@ -189,4 +189,113 @@ public function testIntegrationReferencesAreNotLookedUpAsObjects(): void { array_column($facet['buckets'], 'label', 'key') ); }//end testIntegrationReferencesAreNotLookedUpAsObjects() + + /** + * Run getTermsFacetUnion() over the given rows, one table per property set. + * + * @param array> $rows The union rows the database returns. + * @param CacheHandler $cacheHandler The name resolver. + * @param string $column The faceted column. + * @param array> $propertySets One schema's property definitions per table. + * + * @return array The facet result. + */ + private function unionFacetOver( + array $rows, + CacheHandler $cacheHandler, + string $column, + array $propertySets, + ): array { + $db = $this->createMock(IDBConnection::class); + $db->method('getDatabasePlatform')->willReturn($this->createMock(PostgreSQLPlatform::class)); + + $rows[] = false; + $db->method('prepare')->willReturnCallback( + function (string $sql) use ($column, $rows): IPreparedStatement { + $statement = $this->createMock(IPreparedStatement::class); + $statement->method('execute')->willReturn($this->createMock(IResult::class)); + if (str_contains($sql, 'information_schema') === true) { + $statement->method('fetch')->willReturnOnConsecutiveCalls(['col' => $column], false); + return $statement; + } + + $statement->method('fetch')->willReturnOnConsecutiveCalls(...$rows); + return $statement; + } + ); + + $handler = new MagicFacetHandler( + db: $db, + logger: $this->createMock(LoggerInterface::class), + cacheHandler: $cacheHandler + ); + + $tableConfigs = []; + foreach ($propertySets as $index => $properties) { + $schema = $this->createMock(Schema::class); + $schema->method('getProperties')->willReturn($properties); + $tableConfigs[] = [ + 'tableName' => 'openregister_table_1_'.($index + 2), + 'schema' => $schema, + 'register' => $this->createMock(Register::class), + ]; + } + + $method = new ReflectionMethod(MagicFacetHandler::class, 'getTermsFacetUnion'); + $method->setAccessible(true); + + return $method->invoke($handler, $tableConfigs, $column, [], $tableConfigs[0]['schema'], false); + }//end unionFacetOver() + + /** + * On the multi-schema union path a referenceType property is not looked up + * as object names either. + * + * @return void + */ + public function testUnionIntegrationReferencesAreNotLookedUpAsObjects(): void { + $cacheHandler = $this->createMock(CacheHandler::class); + $cacheHandler->expects($this->never())->method('getMultipleObjectNames'); + + $facet = $this->unionFacetOver( + [ + ['facet_value' => self::DANGLING_A, 'doc_count' => 2], + ['facet_value' => 'behandelaars', 'doc_count' => 4], + ], + $cacheHandler, + 'assigned_group', + [['assignedGroup' => ['type' => 'string', 'referenceType' => 'nextcloud-group']]] + ); + + $this->assertEquals( + [self::DANGLING_A => self::DANGLING_A, 'behandelaars' => 'behandelaars'], + array_column($facet['buckets'], 'label', 'key') + ); + }//end testUnionIntegrationReferencesAreNotLookedUpAsObjects() + + /** + * When one schema of the union holds object references in the column, the + * name lookup still runs, even if the first schema declares a referenceType. + * + * @return void + */ + public function testUnionKeepsTheLookupWhenAnySchemaHoldsObjectReferences(): void { + $cacheHandler = $this->createMock(CacheHandler::class); + $cacheHandler->expects($this->once()) + ->method('getMultipleObjectNames') + ->with([self::KNOWN]) + ->willReturn([self::KNOWN => 'Ontvangen']); + + $facet = $this->unionFacetOver( + [['facet_value' => self::KNOWN, 'doc_count' => 3]], + $cacheHandler, + 'assigned_group', + [ + ['assignedGroup' => ['type' => 'string', 'referenceType' => 'nextcloud-group']], + ['assignedGroup' => ['type' => 'string', 'format' => 'uuid', '$ref' => 'group']], + ] + ); + + $this->assertSame('Ontvangen', array_column($facet['buckets'], 'label', 'key')[self::KNOWN]); + }//end testUnionKeepsTheLookupWhenAnySchemaHoldsObjectReferences() }//end class