From cddda7d44eb5ee687502d9de0bad06005e1946f6 Mon Sep 17 00:00:00 2001 From: Marco Perberschlager Date: Mon, 30 Mar 2026 18:58:54 +0200 Subject: [PATCH 1/7] [Improvement] Refactor IndexQueueRepository to use named column aliases and simplify deleteQueueEntries - Refactor generateSelectQuery to accept associative columnAliases array (alias => column) instead of positional selects, producing named SQL aliases - Refactor getValuesFromSqlResult to extract values by column alias name instead of fragile positional index access - Update all EnqueueService callers and type adapters to use new associative alias format - Simplify deleteQueueEntries to use WHERE id IN (...) leveraging the primary key - Remove unused quoteParameters method - Add unit tests for generateSelectQuery and enqueueBySelectQuery --- src/Repository/IndexQueueRepository.php | 106 ++---- .../SearchIndex/IndexQueue/EnqueueService.php | 75 +++-- .../ElementTypeAdapter/AssetTypeAdapter.php | 12 +- .../DataObjectTypeAdapter.php | 24 +- .../DocumentTypeAdapter.php | 12 +- .../Functional/SearchIndex/IndexQueueTest.php | 11 +- .../Repository/IndexQueueRepositoryTest.php | 304 ++++++++++++++++++ 7 files changed, 405 insertions(+), 139 deletions(-) create mode 100644 tests/Unit/Repository/IndexQueueRepositoryTest.php diff --git a/src/Repository/IndexQueueRepository.php b/src/Repository/IndexQueueRepository.php index 59d2c547..51feaae0 100644 --- a/src/Repository/IndexQueueRepository.php +++ b/src/Repository/IndexQueueRepository.php @@ -124,27 +124,19 @@ public function deleteQueueEntries(array $entries): void { $chunks = array_chunk($entries, self::BATCH_SIZE); foreach ($chunks as $chunk) { - $condition = []; - - /** @var IndexQueue $entry */ - foreach ($chunk as $entry) { - $condition[] = sprintf( - '(%s, %s)', - $this->connection->quote((string)$entry->getId()), - $this->connection->quote($entry->getOperationTime()) - ); - } - - $condition = sprintf('(%s, %s) IN (%s) ORDER BY %s ASC LIMIT %s', - $this->connection->quoteIdentifier('id'), - $this->connection->quoteIdentifier('operationTime'), - implode(',', $condition), - $this->connection->quoteIdentifier('id'), - self::BATCH_SIZE + $ids = array_map( + fn (IndexQueue $entry) => $this->connection->quote((string)$entry->getId()), + $chunk ); - //delete handled entry from queue table - $this->connection->executeQuery('DELETE FROM ' . IndexQueue::TABLE . ' WHERE ' . $condition); + $this->connection->executeQuery( + sprintf( + 'DELETE FROM %s WHERE %s IN (%s)', + $this->connection->quoteIdentifier(IndexQueue::TABLE), + $this->connection->quoteIdentifier('id'), + implode(',', $ids) + ) + ); } } @@ -161,24 +153,29 @@ public function denormalizeDatabaseEntry(array $entry): IndexQueue return $this->denormalizer->denormalize($entry, IndexQueue::class); } + /** + * @param array $columnAliases Associative array mapping alias names to SQL expressions + * @param array $params Query parameters for setParameters() + * @param array $whereParameters Column names for WHERE clauses + */ public function generateSelectQuery( string $tableName, - array $fields, - string $idField = 'id', + array $columnAliases, array $params = [], array $whereParameters = [] ): DBALQueryBuilder { - $fields = $this->quoteParameters($fields); - array_unshift($fields, $idField); + $selectExpressions = []; + foreach ($columnAliases as $alias => $expression) { + $selectExpressions[] = $expression . ' AS ' . $alias; + } $qb = $this->connection->createQueryBuilder() - ->addSelect(...$fields) + ->addSelect(...$selectExpressions) ->from($tableName); $this->addWhereStatements($qb, $whereParameters); if (!empty($params)) { - $params = $this->quoteParameters($params); $qb->setParameters($params); } @@ -315,20 +312,6 @@ private function createQueryBuilder(string $alias): QueryBuilder ->createQueryBuilder($alias); } - private function quoteParameters(array $parameters): array - { - return array_map( - function ($parameter) { - if (is_string($parameter)) { - return $this->connection->quote($parameter); - } - - return $parameter; - }, - $parameters - ); - } - private function addWhereStatements(DBALQueryBuilder $queryBuilder, array $whereParameters): DBALQueryBuilder { foreach ($whereParameters as $operator => $parameter) { @@ -412,10 +395,10 @@ private function insertFromChunk( return $this->connection->executeStatement($insertSql, $insertParams); } - /* - * @TODO: Refactor to avoid that all values have to be in a specific order. Either use aliases or pass the keys as parameters. - * Both things can be considered breaking changes. - */ + /** + * Extracts values from SQL result rows that use named column aliases. + * Expected aliases: elementId, elementType, elementIndexName, operation, operationTime. + */ private function getValuesFromSqlResult(array $result): array { if (empty($result)) { @@ -423,40 +406,13 @@ private function getValuesFromSqlResult(array $result): array } $firstRow = $result[0]; - $keys = array_keys($firstRow); - $columnCount = count($keys); - - $ids = array_column($result, $keys[0]); - $elementType = $firstRow[$keys[1]]; - - $elementIndexName = match ($elementType) { - ElementType::ASSET->value, ElementType::DOCUMENT->value => $elementType, - ElementType::DATA_OBJECT->value => $firstRow['className'] ?? - $firstRow[ - $keys[2] - ] ?? - null, - default => null, - }; - - $operation = $firstRow[ - $keys[ - $columnCount > 5 ? 3 : 2 - ] - ]; - - $operationTime = (int)$firstRow[ - $keys[ - $columnCount > 5 ? 4 : 3 - ] - ]; return [ - $ids, - $elementType, - $operation, - $operationTime, - $elementIndexName, + array_column($result, 'elementId'), + $firstRow['elementType'], + $firstRow['operation'], + (int)$firstRow['operationTime'], + $firstRow['elementIndexName'], ]; } } diff --git a/src/Service/SearchIndex/IndexQueue/EnqueueService.php b/src/Service/SearchIndex/IndexQueue/EnqueueService.php index 40a94669..67563209 100644 --- a/src/Service/SearchIndex/IndexQueue/EnqueueService.php +++ b/src/Service/SearchIndex/IndexQueue/EnqueueService.php @@ -49,8 +49,9 @@ public function enqueueByTag(Tag $tag): EnqueueService { $tagCondition = $this->indexQueueRepository->generateSelectQuery( 'tags_assignment', - [], - 'cid', + [ + 'elementId' => 'cid', + ], [], ['ctype', IndexQueueRepository::AND_OPERATOR => 'tagid'] ); @@ -59,13 +60,13 @@ public function enqueueByTag(Tag $tag): EnqueueService $assetQuery = $this->indexQueueRepository->generateSelectQuery( 'assets', [ - ElementType::ASSET->value, - IndexName::ASSET->value, - IndexQueueOperation::UPDATE->value, - (string)$this->timeService->getCurrentMillisecondTimestamp(), - '0', + 'elementId' => 'id', + 'elementType' => "'" . ElementType::ASSET->value . "'", + 'elementIndexName' => "'" . IndexName::ASSET->value . "'", + 'operation' => "'" . IndexQueueOperation::UPDATE->value . "'", + 'operationTime' => "'" . (string)$this->timeService->getCurrentMillisecondTimestamp() . "'", + 'dispatched' => '0', ], - 'id', ['ctype' => ElementType::ASSET->value, 'tagid' => $tag->getId()], ); $assetQuery->where($assetQuery->expr()->in('id', $tagCondition->getSQL())); @@ -75,12 +76,13 @@ public function enqueueByTag(Tag $tag): EnqueueService $dataObjectQuery = $this->indexQueueRepository->generateSelectQuery( 'objects', [ - ElementType::DATA_OBJECT->value, - IndexQueueOperation::UPDATE->value, - (string)$this->timeService->getCurrentMillisecondTimestamp(), - '0', + 'elementId' => 'id', + 'elementType' => "'" . ElementType::DATA_OBJECT->value . "'", + 'elementIndexName' => 'className', + 'operation' => "'" . IndexQueueOperation::UPDATE->value . "'", + 'operationTime' => "'" . (string)$this->timeService->getCurrentMillisecondTimestamp() . "'", + 'dispatched' => '0', ], - 'id, className', ['ctype' => ElementType::DATA_OBJECT->value, 'tagid' => $tag->getId()], ); $dataObjectQuery->where($dataObjectQuery->expr()->in('id', $tagCondition->getSQL())); @@ -98,13 +100,13 @@ public function enqueueByClassDefinition(ClassDefinition $classDefinition): Enqu $selectQuery = $this->indexQueueRepository->generateSelectQuery( $dataObjectTableName, [ - ElementType::DATA_OBJECT->value, - $classDefinition->getName(), - IndexQueueOperation::UPDATE->value, - (string)$this->timeService->getCurrentMillisecondTimestamp(), - '0', - ], - 'oo_id' + 'elementId' => 'oo_id', + 'elementType' => "'" . ElementType::DATA_OBJECT->value . "'", + 'elementIndexName' => "'" . $classDefinition->getName() . "'", + 'operation' => "'" . IndexQueueOperation::UPDATE->value . "'", + 'operationTime' => "'" . (string)$this->timeService->getCurrentMillisecondTimestamp() . "'", + 'dispatched' => '0', + ] ); $this->indexQueueRepository->enqueueBySelectQuery( $selectQuery @@ -119,11 +121,12 @@ public function enqueueDataObjectFolders(): EnqueueServiceInterface $selectQuery = $this->indexQueueRepository->generateSelectQuery( 'objects', [ - ElementType::DATA_OBJECT->value, - IndexName::DATA_OBJECT_FOLDER->value, - IndexQueueOperation::UPDATE->value, - (string)$this->timeService->getCurrentMillisecondTimestamp(), - '0', + 'elementId' => 'id', + 'elementType' => "'" . ElementType::DATA_OBJECT->value . "'", + 'elementIndexName' => "'" . IndexName::DATA_OBJECT_FOLDER->value . "'", + 'operation' => "'" . IndexQueueOperation::UPDATE->value . "'", + 'operationTime' => "'" . (string)$this->timeService->getCurrentMillisecondTimestamp() . "'", + 'dispatched' => '0', ], )->where('type = "folder"'); $this->indexQueueRepository->enqueueBySelectQuery($selectQuery); @@ -145,11 +148,12 @@ public function enqueueAssets(): EnqueueService $selectQuery = $this->indexQueueRepository->generateSelectQuery( 'assets', [ - ElementType::ASSET->value, - IndexName::ASSET->value, - IndexQueueOperation::UPDATE->value, - (string)$this->timeService->getCurrentMillisecondTimestamp(), - '0', + 'elementId' => 'id', + 'elementType' => "'" . ElementType::ASSET->value . "'", + 'elementIndexName' => "'" . IndexName::ASSET->value . "'", + 'operation' => "'" . IndexQueueOperation::UPDATE->value . "'", + 'operationTime' => "'" . (string)$this->timeService->getCurrentMillisecondTimestamp() . "'", + 'dispatched' => '0', ] ); $this->indexQueueRepository->enqueueBySelectQuery($selectQuery); @@ -171,11 +175,12 @@ public function enqueueDocuments(): EnqueueService $selectQuery = $this->indexQueueRepository->generateSelectQuery( 'documents', [ - ElementType::DOCUMENT->value, - IndexName::DOCUMENT->value, - IndexQueueOperation::UPDATE->value, - (string)$this->timeService->getCurrentMillisecondTimestamp(), - '0', + 'elementId' => 'id', + 'elementType' => "'" . ElementType::DOCUMENT->value . "'", + 'elementIndexName' => "'" . IndexName::DOCUMENT->value . "'", + 'operation' => "'" . IndexQueueOperation::UPDATE->value . "'", + 'operationTime' => "'" . (string)$this->timeService->getCurrentMillisecondTimestamp() . "'", + 'dispatched' => '0', ] ); $this->indexQueueRepository->enqueueBySelectQuery($selectQuery); diff --git a/src/Service/SearchIndex/IndexService/ElementTypeAdapter/AssetTypeAdapter.php b/src/Service/SearchIndex/IndexService/ElementTypeAdapter/AssetTypeAdapter.php index 8e490c35..f190a602 100644 --- a/src/Service/SearchIndex/IndexService/ElementTypeAdapter/AssetTypeAdapter.php +++ b/src/Service/SearchIndex/IndexService/ElementTypeAdapter/AssetTypeAdapter.php @@ -85,12 +85,12 @@ public function getRelatedItemsOnUpdateQuery( ): QueryBuilder { $selects = [ - (string)$element->getId(), - "'" . ElementType::ASSET->value . "'", - "'" . IndexName::ASSET->value . "'", - "'$operation'", - "'$operationTime'", - '0', + (string)$element->getId() . ' AS elementId', + "'" . ElementType::ASSET->value . "' AS elementType", + "'" . IndexName::ASSET->value . "' AS elementIndexName", + "'$operation' AS operation", + "'$operationTime' AS operationTime", + '0 AS dispatched', ]; return $this->dbConnection->createQueryBuilder() diff --git a/src/Service/SearchIndex/IndexService/ElementTypeAdapter/DataObjectTypeAdapter.php b/src/Service/SearchIndex/IndexService/ElementTypeAdapter/DataObjectTypeAdapter.php index 47e3a757..0e6928a9 100644 --- a/src/Service/SearchIndex/IndexService/ElementTypeAdapter/DataObjectTypeAdapter.php +++ b/src/Service/SearchIndex/IndexService/ElementTypeAdapter/DataObjectTypeAdapter.php @@ -115,12 +115,12 @@ public function getRelatedItemsOnUpdateQuery( } $selects = [ - 'id', - "'" . ElementType::DATA_OBJECT->value . "'", - 'className', - "'$operation'", - "'$operationTime'", - '0', + 'id AS elementId', + "'" . ElementType::DATA_OBJECT->value . "' AS elementType", + 'className AS elementIndexName', + "'$operation' AS operation", + "'$operationTime' AS operationTime", + '0 AS dispatched', ]; $select = $this->dbConnection->createQueryBuilder() @@ -188,12 +188,12 @@ private function getSelectParameters( int $operationTime ): array { return [ - (string)$element->getId(), - "'" . ElementType::DATA_OBJECT->value . "'", - $this->getIndexName($element, $operation), - "'$operation'", - "'$operationTime'", - '0', + (string)$element->getId() . ' AS elementId', + "'" . ElementType::DATA_OBJECT->value . "' AS elementType", + $this->getIndexName($element, $operation) . ' AS elementIndexName', + "'$operation' AS operation", + "'$operationTime' AS operationTime", + '0 AS dispatched', ]; } diff --git a/src/Service/SearchIndex/IndexService/ElementTypeAdapter/DocumentTypeAdapter.php b/src/Service/SearchIndex/IndexService/ElementTypeAdapter/DocumentTypeAdapter.php index 942b0793..b59d7f48 100644 --- a/src/Service/SearchIndex/IndexService/ElementTypeAdapter/DocumentTypeAdapter.php +++ b/src/Service/SearchIndex/IndexService/ElementTypeAdapter/DocumentTypeAdapter.php @@ -84,12 +84,12 @@ public function getRelatedItemsOnUpdateQuery( bool $includeElement = false ): QueryBuilder { $selects = [ - (string)$element->getId(), - "'" . ElementType::DOCUMENT->value . "'", - "'" . IndexName::DOCUMENT->value . "'", - "'$operation'", - "'$operationTime'", - '0', + (string)$element->getId() . ' AS elementId', + "'" . ElementType::DOCUMENT->value . "' AS elementType", + "'" . IndexName::DOCUMENT->value . "' AS elementIndexName", + "'$operation' AS operation", + "'$operationTime' AS operationTime", + '0 AS dispatched', ]; return $this->dbConnection->createQueryBuilder() diff --git a/tests/Functional/SearchIndex/IndexQueueTest.php b/tests/Functional/SearchIndex/IndexQueueTest.php index be894153..e33a6589 100644 --- a/tests/Functional/SearchIndex/IndexQueueTest.php +++ b/tests/Functional/SearchIndex/IndexQueueTest.php @@ -92,11 +92,12 @@ public function testIndexQueueRepository(): void $indexQueueRepository->enqueueBySelectQuery( $indexQueueRepository->generateSelectQuery('assets', [ - ElementType::ASSET->value, - IndexName::ASSET->value, - IndexQueueOperation::UPDATE->value, - '1234', - '0', + 'elementId' => 'id', + 'elementType' => "'" . ElementType::ASSET->value . "'", + 'elementIndexName' => "'" . IndexName::ASSET->value . "'", + 'operation' => "'" . IndexQueueOperation::UPDATE->value . "'", + 'operationTime' => "'1234'", + 'dispatched' => '0', ]) ); $this->assertEquals( diff --git a/tests/Unit/Repository/IndexQueueRepositoryTest.php b/tests/Unit/Repository/IndexQueueRepositoryTest.php new file mode 100644 index 00000000..952c28bd --- /dev/null +++ b/tests/Unit/Repository/IndexQueueRepositoryTest.php @@ -0,0 +1,304 @@ +realConnection = DriverManager::getConnection([ + 'driver' => 'pdo_sqlite', + 'memory' => true, + ]); + } + + public function testGenerateSelectQueryProducesNamedAliases(): void + { + $repository = $this->createRepository($this->realConnection); + + $qb = $repository->generateSelectQuery('assets', [ + 'elementId' => 'id', + 'elementType' => "'asset'", + 'elementIndexName' => "'asset'", + 'operation' => "'update'", + 'operationTime' => "'1234'", + 'dispatched' => '0', + ]); + + $sql = $qb->getSQL(); + + $this->assertStringContainsString('id AS elementId', $sql); + $this->assertStringContainsString("'asset' AS elementType", $sql); + $this->assertStringContainsString("'asset' AS elementIndexName", $sql); + $this->assertStringContainsString("'update' AS operation", $sql); + $this->assertStringContainsString("'1234' AS operationTime", $sql); + $this->assertStringContainsString('0 AS dispatched', $sql); + $this->assertStringContainsString('FROM assets', $sql); + } + + public function testGenerateSelectQueryWithColumnReference(): void + { + $repository = $this->createRepository($this->realConnection); + + $qb = $repository->generateSelectQuery('objects', [ + 'elementId' => 'id', + 'elementType' => "'data_object'", + 'elementIndexName' => 'className', + 'operation' => "'update'", + 'operationTime' => "'5678'", + 'dispatched' => '0', + ]); + + $sql = $qb->getSQL(); + + $this->assertStringContainsString('className AS elementIndexName', $sql); + } + + public function testGenerateSelectQueryWithCustomIdColumn(): void + { + $repository = $this->createRepository($this->realConnection); + + $qb = $repository->generateSelectQuery('object_1', [ + 'elementId' => 'oo_id', + 'elementType' => "'data_object'", + 'elementIndexName' => "'MyClass'", + 'operation' => "'update'", + 'operationTime' => "'9999'", + 'dispatched' => '0', + ]); + + $sql = $qb->getSQL(); + + $this->assertStringContainsString('oo_id AS elementId', $sql); + $this->assertStringContainsString('FROM object_1', $sql); + } + + public function testGenerateSelectQueryWithWhereParameters(): void + { + $repository = $this->createRepository($this->realConnection); + + $qb = $repository->generateSelectQuery( + 'tags_assignment', + [ + 'elementId' => 'cid', + ], + [], + ['ctype', IndexQueueRepository::AND_OPERATOR => 'tagid'] + ); + + $sql = $qb->getSQL(); + + $this->assertStringContainsString('cid AS elementId', $sql); + $this->assertStringContainsString('ctype = :ctype', $sql); + $this->assertStringContainsString('tagid = :tagid', $sql); + } + + public function testGenerateSelectQueryWithParams(): void + { + $repository = $this->createRepository($this->realConnection); + + $qb = $repository->generateSelectQuery( + 'assets', + [ + 'elementId' => 'id', + 'elementType' => "'asset'", + 'elementIndexName' => "'asset'", + 'operation' => "'update'", + 'operationTime' => "'1234'", + 'dispatched' => '0', + ], + ['ctype' => 'asset', 'tagid' => 42], + ); + + $params = $qb->getParameters(); + + $this->assertSame('asset', $params['ctype']); + $this->assertSame(42, $params['tagid']); + } + + public function testGenerateSelectQueryWithSubsetOfAliases(): void + { + $repository = $this->createRepository($this->realConnection); + + $qb = $repository->generateSelectQuery('tags_assignment', [ + 'elementId' => 'cid', + ]); + + $sql = $qb->getSQL(); + + $this->assertStringContainsString('cid AS elementId', $sql); + $this->assertStringNotContainsString('elementType', $sql); + $this->assertStringNotContainsString('elementIndexName', $sql); + } + + public function testEnqueueBySelectQueryWithEmptyResult(): void + { + $connection = $this->makeEmpty(Connection::class, [ + 'fetchAllAssociative' => [], + ]); + + $repository = $this->createRepository($connection); + + $qb = $this->makeEmpty(\Doctrine\DBAL\Query\QueryBuilder::class, [ + 'getSQL' => 'SELECT 1', + 'getParameters' => [], + ]); + + // Should not throw -- empty result is a no-op + $repository->enqueueBySelectQuery($qb); + + // If we get here without exception, the test passes + $this->assertTrue(true); + } + + public function testEnqueueBySelectQueryExtractsNamedColumns(): void + { + $insertStatements = []; + + $connection = $this->makeEmpty(Connection::class, [ + 'fetchAllAssociative' => [ + [ + 'elementId' => '10', + 'elementType' => 'asset', + 'elementIndexName' => 'asset', + 'operation' => 'update', + 'operationTime' => '1711800000000', + 'dispatched' => '0', + ], + [ + 'elementId' => '20', + 'elementType' => 'asset', + 'elementIndexName' => 'asset', + 'operation' => 'update', + 'operationTime' => '1711800000000', + 'dispatched' => '0', + ], + ], + 'beginTransaction' => null, + 'commit' => null, + 'quoteIdentifier' => function (string $identifier) { + return '`' . $identifier . '`'; + }, + 'executeStatement' => function (string $sql, array $params) use (&$insertStatements) { + $insertStatements[] = ['sql' => $sql, 'params' => $params]; + + return count($params) > 0 ? 2 : 0; + }, + ]); + + $repository = $this->createRepository($connection); + + $qb = $this->makeEmpty(\Doctrine\DBAL\Query\QueryBuilder::class, [ + 'getSQL' => 'SELECT 1', + 'getParameters' => [], + ]); + + $repository->enqueueBySelectQuery($qb); + + $this->assertNotEmpty($insertStatements, 'Expected at least one INSERT statement'); + + $insertSql = $insertStatements[0]['sql']; + $insertParams = $insertStatements[0]['params']; + + // Verify the INSERT uses the correct column names + $this->assertStringContainsString('`elementId`', $insertSql); + $this->assertStringContainsString('`elementType`', $insertSql); + $this->assertStringContainsString('`elementIndexName`', $insertSql); + $this->assertStringContainsString('`operation`', $insertSql); + $this->assertStringContainsString('`operationTime`', $insertSql); + + // Verify both element IDs are included in the params + $this->assertContains('10', $insertParams); + $this->assertContains('20', $insertParams); + + // Verify the element type is passed correctly + $this->assertContains('asset', $insertParams); + + // Verify the operation is passed correctly + $this->assertContains('update', $insertParams); + } + + public function testEnqueueBySelectQueryExtractsDataObjectNamedColumns(): void + { + $insertStatements = []; + + $connection = $this->makeEmpty(Connection::class, [ + 'fetchAllAssociative' => [ + [ + 'elementId' => '100', + 'elementType' => 'data_object', + 'elementIndexName' => 'Product', + 'operation' => 'update', + 'operationTime' => '1711800000000', + 'dispatched' => '0', + ], + ], + 'beginTransaction' => null, + 'commit' => null, + 'quoteIdentifier' => function (string $identifier) { + return '`' . $identifier . '`'; + }, + 'executeStatement' => function (string $sql, array $params) use (&$insertStatements) { + $insertStatements[] = ['sql' => $sql, 'params' => $params]; + + return 1; + }, + ]); + + $repository = $this->createRepository($connection); + + $qb = $this->makeEmpty(\Doctrine\DBAL\Query\QueryBuilder::class, [ + 'getSQL' => 'SELECT 1', + 'getParameters' => [], + ]); + + $repository->enqueueBySelectQuery($qb); + + $this->assertNotEmpty($insertStatements); + + $insertParams = $insertStatements[0]['params']; + + // Verify the element ID is extracted correctly + $this->assertContains('100', $insertParams); + + // Verify the elementIndexName (class name) is extracted correctly + $this->assertContains('Product', $insertParams); + + // Verify element type + $this->assertContains('data_object', $insertParams); + } + + private function createRepository(Connection $connection): IndexQueueRepository + { + return new IndexQueueRepository( + $this->makeEmpty(EntityManagerInterface::class), + $this->makeEmpty(TimeServiceInterface::class), + $connection, + $this->makeEmpty(DenormalizerInterface::class) + ); + } +} From 39af4d448f172466269d4a5c89cf13e15766e687 Mon Sep 17 00:00:00 2001 From: Marco Perberschlager Date: Tue, 31 Mar 2026 08:59:59 +0200 Subject: [PATCH 2/7] Fix deleteQueueEntries race condition: restore operationTime guard The simplified WHERE id IN (...) introduced a race condition: if an element is re-queued (via ON DUPLICATE KEY UPDATE) while being processed, the operationTime on the existing row changes but the id stays the same. The id-only DELETE would accidentally remove the re-queued entry, losing the pending update. Restore the original WHERE (id, operationTime) IN (...) tuple matching. This acts as an optimistic-concurrency guard: if operationTime changed since the entry was fetched, the DELETE is a no-op and the re-queued entry survives for the next processing cycle. Add unit tests documenting this race condition protection. --- src/Repository/IndexQueueRepository.php | 15 +++- .../Repository/IndexQueueRepositoryTest.php | 83 +++++++++++++++++++ 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/src/Repository/IndexQueueRepository.php b/src/Repository/IndexQueueRepository.php index 51feaae0..1632b405 100644 --- a/src/Repository/IndexQueueRepository.php +++ b/src/Repository/IndexQueueRepository.php @@ -124,17 +124,24 @@ public function deleteQueueEntries(array $entries): void { $chunks = array_chunk($entries, self::BATCH_SIZE); foreach ($chunks as $chunk) { - $ids = array_map( - fn (IndexQueue $entry) => $this->connection->quote((string)$entry->getId()), + $tuples = array_map( + fn (IndexQueue $entry) => sprintf( + '(%s, %s)', + $this->connection->quote((string)$entry->getId()), + $this->connection->quote($entry->getOperationTime()) + ), $chunk ); $this->connection->executeQuery( sprintf( - 'DELETE FROM %s WHERE %s IN (%s)', + 'DELETE FROM %s WHERE (%s, %s) IN (%s) ORDER BY %s ASC LIMIT %d', $this->connection->quoteIdentifier(IndexQueue::TABLE), $this->connection->quoteIdentifier('id'), - implode(',', $ids) + $this->connection->quoteIdentifier('operationTime'), + implode(',', $tuples), + $this->connection->quoteIdentifier('id'), + self::BATCH_SIZE ) ); } diff --git a/tests/Unit/Repository/IndexQueueRepositoryTest.php b/tests/Unit/Repository/IndexQueueRepositoryTest.php index 952c28bd..b249da54 100644 --- a/tests/Unit/Repository/IndexQueueRepositoryTest.php +++ b/tests/Unit/Repository/IndexQueueRepositoryTest.php @@ -17,6 +17,7 @@ use Doctrine\DBAL\Connection; use Doctrine\DBAL\DriverManager; use Doctrine\ORM\EntityManagerInterface; +use Pimcore\Bundle\GenericDataIndexBundle\Entity\IndexQueue; use Pimcore\Bundle\GenericDataIndexBundle\Repository\IndexQueueRepository; use Pimcore\Bundle\GenericDataIndexBundle\Service\TimeServiceInterface; use Symfony\Component\Serializer\Normalizer\DenormalizerInterface; @@ -292,6 +293,88 @@ public function testEnqueueBySelectQueryExtractsDataObjectNamedColumns(): void $this->assertContains('data_object', $insertParams); } + public function testDeleteQueueEntriesMatchesOnIdAndOperationTime(): void + { + $executedQueries = []; + + $connection = $this->makeEmpty(Connection::class, [ + 'quote' => function (string $value) { + return "'" . $value . "'"; + }, + 'quoteIdentifier' => function (string $identifier) { + return '`' . $identifier . '`'; + }, + 'executeQuery' => function (string $sql) use (&$executedQueries) { + $executedQueries[] = $sql; + + return $this->makeEmpty(\Doctrine\DBAL\Result::class); + }, + ]); + + $repository = $this->createRepository($connection); + + $entry1 = new IndexQueue(); + $entry1->setId(42); + $entry1->setOperationTime('1711800000000'); + + $entry2 = new IndexQueue(); + $entry2->setId(99); + $entry2->setOperationTime('1711800001000'); + + $repository->deleteQueueEntries([$entry1, $entry2]); + + $this->assertCount(1, $executedQueries); + $sql = $executedQueries[0]; + + // Must match on BOTH id AND operationTime to prevent race condition + $this->assertStringContainsString('(`id`, `operationTime`) IN', $sql); + $this->assertStringContainsString("('42', '1711800000000')", $sql); + $this->assertStringContainsString("('99', '1711800001000')", $sql); + } + + public function testDeleteQueueEntriesDoesNotDeleteRequeuedEntries(): void + { + // This test documents WHY the operationTime guard exists: + // If an element is re-queued (e.g., saved again) while being processed, + // the ON DUPLICATE KEY UPDATE changes the operationTime on the existing row. + // The DELETE must NOT match the re-queued entry because the operationTime + // no longer matches what was fetched during processing. + + $executedQueries = []; + + $connection = $this->makeEmpty(Connection::class, [ + 'quote' => function (string $value) { + return "'" . $value . "'"; + }, + 'quoteIdentifier' => function (string $identifier) { + return '`' . $identifier . '`'; + }, + 'executeQuery' => function (string $sql) use (&$executedQueries) { + $executedQueries[] = $sql; + + return $this->makeEmpty(\Doctrine\DBAL\Result::class); + }, + ]); + + $repository = $this->createRepository($connection); + + // Simulate an entry that was fetched with operationTime=1000 + // but has since been re-queued with operationTime=2000 + $entry = new IndexQueue(); + $entry->setId(42); + $entry->setOperationTime('1000'); // original time when fetched + + $repository->deleteQueueEntries([$entry]); + + $sql = $executedQueries[0]; + + // The DELETE uses the ORIGINAL operationTime ('1000'), not the new one ('2000'). + // In the database, the row now has operationTime=2000, so the tuple (42, '1000') + // will NOT match, and the re-queued entry will survive. + $this->assertStringContainsString("('42', '1000')", $sql); + $this->assertStringNotContainsString('WHERE `id` IN', $sql, 'Must not use id-only matching'); + } + private function createRepository(Connection $connection): IndexQueueRepository { return new IndexQueueRepository( From fc416f7b0841f3550a1b284f5a3bcad736b0acc7 Mon Sep 17 00:00:00 2001 From: Marco Perberschlager Date: Tue, 31 Mar 2026 09:22:53 +0200 Subject: [PATCH 3/7] chore: Upgrade notes --- doc/01_Installation/02_Upgrade.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/01_Installation/02_Upgrade.md b/doc/01_Installation/02_Upgrade.md index 87e2e845..dda47cf0 100644 --- a/doc/01_Installation/02_Upgrade.md +++ b/doc/01_Installation/02_Upgrade.md @@ -8,6 +8,8 @@ Following steps are necessary during updating to newer versions. - The messenger transport DSN is now configurable via the `%pimcore.messenger.transport_dsn_prefix%` container parameter instead of being hardcoded to `doctrine://default`. This allows the installer to wire the transport DSN from environment variables (e.g. `PIMCORE_MESSENGER_TRANSPORT_DSN_PREFIX`). - Added support to `PHP` `8.5`. - Removed support to `PHP` `8.3` and Symfony `v6`. +- [Indexing] Refactored `IndexQueueRepository::generateSelectQuery()` to use named column aliases instead of positional field arrays. The method now accepts an associative `$columnAliases` array (`alias => expression`) instead of the previous positional `$fields` array with a separate `$idField` parameter. All enqueue methods in `EnqueueService` and the element type adapters (`AssetTypeAdapter`, `DocumentTypeAdapter`, `DataObjectTypeAdapter`) have been updated accordingly. The `quoteParameters()` helper method has been removed. +- [Indexing] Refactored `IndexQueueRepository::getValuesFromSqlResult()` to extract values by named column alias (`elementId`, `elementType`, `elementIndexName`, `operation`, `operationTime`) instead of fragile positional index access. ## Upgrade to 2.2.0 - [Indexing] Added `id` column as new primary key to `generic_data_index_queue`. Please make sure to execute migrations. From e7092a12cb1981e070cee94c900dc666fd697edc Mon Sep 17 00:00:00 2001 From: Marco Perberschlager Date: Tue, 31 Mar 2026 09:31:20 +0200 Subject: [PATCH 4/7] Fix PHPDoc type for $whereParameters to reflect actual usage --- src/Repository/IndexQueueRepository.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Repository/IndexQueueRepository.php b/src/Repository/IndexQueueRepository.php index 1632b405..f0c7199e 100644 --- a/src/Repository/IndexQueueRepository.php +++ b/src/Repository/IndexQueueRepository.php @@ -163,7 +163,8 @@ public function denormalizeDatabaseEntry(array $entry): IndexQueue /** * @param array $columnAliases Associative array mapping alias names to SQL expressions * @param array $params Query parameters for setParameters() - * @param array $whereParameters Column names for WHERE clauses + * @param array $whereParameters Column names for WHERE clauses; keys may be numeric + * or operator constants (AND_OPERATOR, OR_OPERATOR) */ public function generateSelectQuery( string $tableName, From b1f27570bb9afbb33ed763034cdc191f064d2d23 Mon Sep 17 00:00:00 2001 From: Marco Perberschlager Date: Tue, 31 Mar 2026 09:35:58 +0200 Subject: [PATCH 5/7] Validate columnAliases keys are strings and update upgrade notes Add InvalidArgumentException when generateSelectQuery() receives numeric keys in $columnAliases, catching misuse from callers migrating from the old positional array API. Update upgrade notes: mark generateSelectQuery() change as BC break, remove private getValuesFromSqlResult() entry (not relevant for upgrades). --- doc/01_Installation/02_Upgrade.md | 3 +-- src/Repository/IndexQueueRepository.php | 9 +++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/doc/01_Installation/02_Upgrade.md b/doc/01_Installation/02_Upgrade.md index dda47cf0..42d0faaa 100644 --- a/doc/01_Installation/02_Upgrade.md +++ b/doc/01_Installation/02_Upgrade.md @@ -8,8 +8,7 @@ Following steps are necessary during updating to newer versions. - The messenger transport DSN is now configurable via the `%pimcore.messenger.transport_dsn_prefix%` container parameter instead of being hardcoded to `doctrine://default`. This allows the installer to wire the transport DSN from environment variables (e.g. `PIMCORE_MESSENGER_TRANSPORT_DSN_PREFIX`). - Added support to `PHP` `8.5`. - Removed support to `PHP` `8.3` and Symfony `v6`. -- [Indexing] Refactored `IndexQueueRepository::generateSelectQuery()` to use named column aliases instead of positional field arrays. The method now accepts an associative `$columnAliases` array (`alias => expression`) instead of the previous positional `$fields` array with a separate `$idField` parameter. All enqueue methods in `EnqueueService` and the element type adapters (`AssetTypeAdapter`, `DocumentTypeAdapter`, `DataObjectTypeAdapter`) have been updated accordingly. The `quoteParameters()` helper method has been removed. -- [Indexing] Refactored `IndexQueueRepository::getValuesFromSqlResult()` to extract values by named column alias (`elementId`, `elementType`, `elementIndexName`, `operation`, `operationTime`) instead of fragile positional index access. +- [Indexing] **BC Break**: `IndexQueueRepository::generateSelectQuery()` signature changed. The method now accepts an associative `$columnAliases` array (`alias => expression`) instead of the previous positional `$fields` array with a separate `$idField` parameter. Passing a numerically-indexed array will throw an `InvalidArgumentException`. All enqueue methods in `EnqueueService` and the element type adapters (`AssetTypeAdapter`, `DocumentTypeAdapter`, `DataObjectTypeAdapter`) have been updated accordingly. The `quoteParameters()` helper method has been removed. ## Upgrade to 2.2.0 - [Indexing] Added `id` column as new primary key to `generic_data_index_queue`. Please make sure to execute migrations. diff --git a/src/Repository/IndexQueueRepository.php b/src/Repository/IndexQueueRepository.php index f0c7199e..cc80d6d1 100644 --- a/src/Repository/IndexQueueRepository.php +++ b/src/Repository/IndexQueueRepository.php @@ -174,6 +174,15 @@ public function generateSelectQuery( ): DBALQueryBuilder { $selectExpressions = []; foreach ($columnAliases as $alias => $expression) { + if (is_int($alias)) { + throw new \InvalidArgumentException( + sprintf( + 'Column aliases must be an associative array with string keys (alias => expression), got numeric key %d for expression "%s".', + $alias, + $expression + ) + ); + } $selectExpressions[] = $expression . ' AS ' . $alias; } From 9e6bbd544c67641d631e9ca6988eadb4aba520f4 Mon Sep 17 00:00:00 2001 From: Marco Perberschlager Date: Tue, 31 Mar 2026 09:42:11 +0200 Subject: [PATCH 6/7] Add PHPStan ignore for runtime type guard on is_int() check --- src/Repository/IndexQueueRepository.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Repository/IndexQueueRepository.php b/src/Repository/IndexQueueRepository.php index cc80d6d1..3997798a 100644 --- a/src/Repository/IndexQueueRepository.php +++ b/src/Repository/IndexQueueRepository.php @@ -174,7 +174,7 @@ public function generateSelectQuery( ): DBALQueryBuilder { $selectExpressions = []; foreach ($columnAliases as $alias => $expression) { - if (is_int($alias)) { + if (is_int($alias)) { // @phpstan-ignore function.impossibleType (runtime guard for callers without static analysis) throw new \InvalidArgumentException( sprintf( 'Column aliases must be an associative array with string keys (alias => expression), got numeric key %d for expression "%s".', From f0783b4a1c9dfa29bc2ee6475bafe0763e56c439 Mon Sep 17 00:00:00 2001 From: Marco Perberschlager Date: Tue, 31 Mar 2026 09:44:42 +0200 Subject: [PATCH 7/7] Fix line length violations (S103) in generateSelectQuery validation --- src/Repository/IndexQueueRepository.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Repository/IndexQueueRepository.php b/src/Repository/IndexQueueRepository.php index 3997798a..f77cf4bd 100644 --- a/src/Repository/IndexQueueRepository.php +++ b/src/Repository/IndexQueueRepository.php @@ -174,10 +174,12 @@ public function generateSelectQuery( ): DBALQueryBuilder { $selectExpressions = []; foreach ($columnAliases as $alias => $expression) { - if (is_int($alias)) { // @phpstan-ignore function.impossibleType (runtime guard for callers without static analysis) + /** @phpstan-ignore function.impossibleType (runtime guard for callers without static analysis) */ + if (is_int($alias)) { throw new \InvalidArgumentException( sprintf( - 'Column aliases must be an associative array with string keys (alias => expression), got numeric key %d for expression "%s".', + 'Column aliases must be an associative array with string keys ' + . '(alias => expression), got numeric key %d for expression "%s".', $alias, $expression )