diff --git a/.github/workflows/backend-ci.yaml b/.github/workflows/backend-ci.yaml index 5568323..c852aca 100644 --- a/.github/workflows/backend-ci.yaml +++ b/.github/workflows/backend-ci.yaml @@ -139,7 +139,7 @@ jobs: run: composer run-script --timeout=600 test-integration env: SEARCH_ENGINE: legacy - DATABASE_URL: "pgsql://postgres:postgres@localhost:${{ job.services.postgres.ports[5432] }}/testdb?server_version=10" + DATABASE_URL: "pgsql://postgres:postgres@localhost:${{ job.services.postgres.ports[5432] }}/testdb?serverVersion=11" integration-tests-mysql: name: MySQL integration tests diff --git a/src/contracts/Gateway/AbstractDoctrineDatabase.php b/src/contracts/Gateway/AbstractDoctrineDatabase.php index c47e3ac..55fd8de 100644 --- a/src/contracts/Gateway/AbstractDoctrineDatabase.php +++ b/src/contracts/Gateway/AbstractDoctrineDatabase.php @@ -14,6 +14,7 @@ use Ibexa\CorePersistence\Gateway\ExpressionVisitor; use Ibexa\CorePersistence\Gateway\RelationshipTypeStrategyRegistry; use InvalidArgumentException; +use LogicException; /** * @internal @@ -61,19 +62,59 @@ public function getMetadata(): DoctrineSchemaMetadataInterface } /** + * Inserts a row and returns its identifier, taking it from $data when the caller supplied it + * and from the connection otherwise. + * * @param array $data * * @throws \Doctrine\DBAL\Exception */ protected function doInsert(array $data): int + { + $metadata = $this->getMetadata(); + $identifierColumns = $metadata->getIdentifierColumns(); + + if (count($identifierColumns) !== 1) { + throw new LogicException(sprintf( + '"%s" does not have a single identifier column to return. Use doInsertWithoutIdentity() instead.', + $metadata->getTableName(), + )); + } + + $this->executeInsert($data); + + $identifierColumn = $identifierColumns[0]; + + return isset($data[$identifierColumn]) + ? (int)$data[$identifierColumn] + : (int)$this->connection->lastInsertId(); + } + + /** + * Inserts a row into a table that has no single identifier column to return, such as one keyed + * by a composite primary key. + * + * @param array $data + * + * @throws \Doctrine\DBAL\Exception + */ + protected function doInsertWithoutIdentity(array $data): void + { + $this->executeInsert($data); + } + + /** + * @param array $data + * + * @throws \Doctrine\DBAL\Exception + */ + private function executeInsert(array $data): void { $metadata = $this->getMetadata(); $data = $metadata->convertToDatabaseValues($data); $types = $metadata->getBindingTypesForData($data); $this->connection->insert($metadata->getTableName(), $data, $types); - - return (int)$this->connection->lastInsertId(); } /** @@ -358,7 +399,7 @@ private function buildCondition(QueryBuilder $qb, string $column, $value): strin } elseif (is_array($value)) { $parameter = $qb->createPositionalParameter( $value, - $columnBinding + Connection::ARRAY_PARAM_OFFSET + $metadata->getArrayBindingTypeForColumn($column) ); $subquery->andWhere($qb->expr()->in($fullColumnName, $parameter)); @@ -390,7 +431,7 @@ private function buildCondition(QueryBuilder $qb, string $column, $value): strin if (is_array($value)) { $parameter = $qb->createPositionalParameter( $value, - $columnBinding + Connection::ARRAY_PARAM_OFFSET + $metadata->getArrayBindingTypeForColumn($column) ); return $qb->expr()->in($fullColumnName, $parameter); diff --git a/src/contracts/Gateway/ArrayParameterTypeConverter.php b/src/contracts/Gateway/ArrayParameterTypeConverter.php new file mode 100644 index 0000000..7c103db --- /dev/null +++ b/src/contracts/Gateway/ArrayParameterTypeConverter.php @@ -0,0 +1,37 @@ + ArrayParameterType::INTEGER, + ParameterType::ASCII => ArrayParameterType::ASCII, + ParameterType::BINARY => ArrayParameterType::BINARY, + ParameterType::STRING, + ParameterType::BOOLEAN, + ParameterType::LARGE_OBJECT, + ParameterType::NULL => ArrayParameterType::STRING, + }; + } +} diff --git a/src/contracts/Gateway/DoctrineSchemaMetadata.php b/src/contracts/Gateway/DoctrineSchemaMetadata.php index 09fe4a5..9c46df1 100644 --- a/src/contracts/Gateway/DoctrineSchemaMetadata.php +++ b/src/contracts/Gateway/DoctrineSchemaMetadata.php @@ -8,7 +8,9 @@ namespace Ibexa\Contracts\CorePersistence\Gateway; +use Doctrine\DBAL\ArrayParameterType; use Doctrine\DBAL\Connection; +use Doctrine\DBAL\ParameterType; use Doctrine\DBAL\Types\Type; use Ibexa\Contracts\CorePersistence\Exception\MappingException; use Ibexa\Contracts\CorePersistence\Exception\RuntimeMappingException; @@ -206,6 +208,11 @@ public function isInheritedColumn(string $column): bool return $this->getInheritanceMetadataWithColumn($column) !== null; } + public function getIdentifierColumns(): array + { + return $this->identifierColumns; + } + public function getIdentifierColumn(): string { if (count($this->identifierColumns) > 1) { @@ -250,7 +257,7 @@ public function convertToDatabaseValues(array $data): array /** * @param array $data * - * @return array + * @return array * * @throws \Doctrine\DBAL\Exception */ @@ -267,11 +274,21 @@ public function getBindingTypesForData(array $data): array /** * @throws \Doctrine\DBAL\Exception */ - public function getBindingTypeForColumn(string $columnName): int + public function getBindingTypeForColumn(string $columnName): ParameterType { return $this->getColumnType($columnName)->getBindingType(); } + /** + * @throws \Doctrine\DBAL\Exception + */ + public function getArrayBindingTypeForColumn(string $columnName): ArrayParameterType + { + return ArrayParameterTypeConverter::fromParameterType( + $this->getBindingTypeForColumn($columnName) + ); + } + public function setTranslationSchemaMetadata(TranslationDoctrineSchemaMetadataInterface $translationMetadata): void { $this->translationMetadata = $translationMetadata; diff --git a/src/contracts/Gateway/DoctrineSchemaMetadataInterface.php b/src/contracts/Gateway/DoctrineSchemaMetadataInterface.php index 18e4b1e..418770f 100644 --- a/src/contracts/Gateway/DoctrineSchemaMetadataInterface.php +++ b/src/contracts/Gateway/DoctrineSchemaMetadataInterface.php @@ -8,7 +8,9 @@ namespace Ibexa\Contracts\CorePersistence\Gateway; +use Doctrine\DBAL\ArrayParameterType; use Doctrine\DBAL\Connection; +use Doctrine\DBAL\ParameterType; use Doctrine\DBAL\Types\Type; /** @@ -84,7 +86,7 @@ public function convertToDatabaseValues(array $data): array; /** * @param array $data * - * @return array + * @return array * * @throws \Ibexa\Contracts\CorePersistence\Exception\RuntimeMappingExceptionInterface */ @@ -95,11 +97,18 @@ public function getBindingTypesForData(array $data): array; */ public function getIdentifierColumn(): string; + /** + * @return array + */ + public function getIdentifierColumns(): array; + /** * @throws \Doctrine\DBAL\Exception * @throws \Ibexa\Contracts\CorePersistence\Exception\RuntimeMappingExceptionInterface */ - public function getBindingTypeForColumn(string $columnName): int; + public function getBindingTypeForColumn(string $columnName): ParameterType; + + public function getArrayBindingTypeForColumn(string $columnName): ArrayParameterType; /** * @throws \Ibexa\Contracts\CorePersistence\Exception\MappingExceptionInterface diff --git a/src/lib/Gateway/ExpressionVisitor.php b/src/lib/Gateway/ExpressionVisitor.php index a9036f7..e731831 100644 --- a/src/lib/Gateway/ExpressionVisitor.php +++ b/src/lib/Gateway/ExpressionVisitor.php @@ -112,10 +112,9 @@ public function walkComparison(Comparison $comparison) $parameterName = $column . '_' . count($this->parameters); $placeholder = $this->getPlaceholder($parameterName); $value = $this->walkValue($comparison->getValue()); - $type = $this->schemaMetadata->getBindingTypeForColumn($column); - if (is_array($value)) { - $type += Connection::ARRAY_PARAM_OFFSET; - } + $type = is_array($value) + ? $this->schemaMetadata->getArrayBindingTypeForColumn($column) + : $this->schemaMetadata->getBindingTypeForColumn($column); if ($this->isInheritedColumn($column)) { $inheritanceMetadata = $this->schemaMetadata->getInheritanceMetadataWithColumn($column); @@ -288,11 +287,9 @@ private function handleJoinQuery( QueryBuilder $relationshipQuery ): string { $value = $this->walkValue($comparison->getValue()); - $type = $relationshipMetadata->getBindingTypeForColumn($field); - - if (is_array($value)) { - $type += Connection::ARRAY_PARAM_OFFSET; - } + $type = is_array($value) + ? $relationshipMetadata->getArrayBindingTypeForColumn($field) + : $relationshipMetadata->getBindingTypeForColumn($field); $parameter = new Parameter($parameterName, $value, $type); $placeholder = $this->getPlaceholder($parameterName); @@ -323,10 +320,9 @@ private function handleSubSelectQuery( QueryBuilder $relationshipQuery ): string { $value = $this->walkValue($comparison->getValue()); - $type = $relationshipMetadata->getBindingTypeForColumn($field); - if (is_array($value)) { - $type += Connection::ARRAY_PARAM_OFFSET; - } + $type = is_array($value) + ? $relationshipMetadata->getArrayBindingTypeForColumn($field) + : $relationshipMetadata->getBindingTypeForColumn($field); $this->parameters[] = new Parameter($parameterName, $value, $type); diff --git a/src/lib/Gateway/JoinedRelationshipTypeStrategy.php b/src/lib/Gateway/JoinedRelationshipTypeStrategy.php index 053fe07..b46d073 100644 --- a/src/lib/Gateway/JoinedRelationshipTypeStrategy.php +++ b/src/lib/Gateway/JoinedRelationshipTypeStrategy.php @@ -8,7 +8,9 @@ namespace Ibexa\CorePersistence\Gateway; +use Doctrine\DBAL\Query\Exception\NonUniqueAlias; use Doctrine\DBAL\Query\QueryBuilder; +use Doctrine\DBAL\Query\QueryException; use Ibexa\Contracts\CorePersistence\Gateway\DoctrineRelationshipInterface; /** @@ -23,19 +25,16 @@ public function handleRelationshipType( string $fromTable, string $toTable ): void { - if ($this->isTableAlreadyJoined($queryBuilder, $toTable)) { + $condition = (string)$queryBuilder->expr()->eq( + $fromTable . '.' . $relationship->getForeignKeyColumn(), + $toTable . '.' . $relationship->getRelatedClassIdColumn() + ); + + if ($this->isAliasAlreadyTaken($queryBuilder, $fromTable, $toTable, $condition)) { return; } - $queryBuilder->leftJoin( - $fromTable, - $toTable, - $toTable, - $queryBuilder->expr()->eq( - $fromTable . '.' . $relationship->getForeignKeyColumn(), - $toTable . '.' . $relationship->getRelatedClassIdColumn() - ) - ); + $queryBuilder->leftJoin($fromTable, $toTable, $toTable, $condition); } public function handleRelationshipTypeQuery( @@ -46,20 +45,30 @@ public function handleRelationshipTypeQuery( return $queryBuilder; } - private function isTableAlreadyJoined( + /** + * A gateway is free to join a relationship's table itself before handing the query over, so the + * only safe answer comes from the query builder rather than from what this strategy has joined. + * DBAL 4 exposes no accessor for the joins it holds, so the join is added to a copy and the copy + * is asked to build itself: an alias that is already taken is exactly what NonUniqueAlias reports. + */ + private function isAliasAlreadyTaken( QueryBuilder $queryBuilder, - string $tableToJoin + string $fromTable, + string $toTable, + string $condition ): bool { - $joinQueryPart = $queryBuilder->getQueryPart('join'); - - foreach ($joinQueryPart as $joins) { - foreach ($joins as $join) { - $joinAlias = $join['joinAlias'] ?? $join['joinTable']; + $probe = clone $queryBuilder; + $probe->leftJoin($fromTable, $toTable, $toTable, $condition); - if ($joinAlias === $tableToJoin) { - return true; - } - } + try { + $probe->getSQL(); + } catch (NonUniqueAlias) { + return true; + } catch (QueryException) { + // The query cannot be built for an unrelated reason - an unknown FROM alias, or no + // SELECT yet. Nothing is duplicated, so the join still has to be made, and the real + // query builder will raise the same problem on its own terms. + return false; } return false; diff --git a/src/lib/Gateway/Parameter.php b/src/lib/Gateway/Parameter.php index 0e3fe6e..fc197a4 100644 --- a/src/lib/Gateway/Parameter.php +++ b/src/lib/Gateway/Parameter.php @@ -8,6 +8,9 @@ namespace Ibexa\CorePersistence\Gateway; +use Doctrine\DBAL\ArrayParameterType; +use Doctrine\DBAL\ParameterType; + /** * @internal */ @@ -15,7 +18,7 @@ final class Parameter { private string $name; - private int $type; + private ArrayParameterType|ParameterType $type; /** @var mixed */ private $value; @@ -23,7 +26,7 @@ final class Parameter /** * @param mixed $value */ - public function __construct(string $name, $value, int $type) + public function __construct(string $name, $value, ArrayParameterType|ParameterType $type) { $this->name = $name; $this->value = $value; @@ -51,7 +54,7 @@ public function getValue() return $this->value; } - public function getType(): int + public function getType(): ArrayParameterType|ParameterType { return $this->type; } diff --git a/src/lib/Gateway/SubSelectRelationshipTypeStrategy.php b/src/lib/Gateway/SubSelectRelationshipTypeStrategy.php index 74981c0..63ec18b 100644 --- a/src/lib/Gateway/SubSelectRelationshipTypeStrategy.php +++ b/src/lib/Gateway/SubSelectRelationshipTypeStrategy.php @@ -8,7 +8,10 @@ namespace Ibexa\CorePersistence\Gateway; -use Doctrine\DBAL\Query\QueryBuilder; +use Doctrine\DBAL\Query\Exception\NonUniqueAlias; +use Doctrine\DBAL\Query\Exception\UnknownAlias; +use Doctrine\DBAL\Query\QueryBuilder; +use Doctrine\DBAL\Query\QueryException; use Ibexa\Contracts\CorePersistence\Gateway\DoctrineRelationshipInterface; use LogicException; @@ -17,6 +20,19 @@ */ final class SubSelectRelationshipTypeStrategy implements RelationshipTypeStrategyInterface { + private function isQueryInitialised(QueryBuilder $queryBuilder): bool + { + try { + $queryBuilder->getSQL(); + + return true; + } catch (UnknownAlias | NonUniqueAlias) { + return true; + } catch (QueryException) { + return false; + } + } + public function handleRelationshipType( QueryBuilder $queryBuilder, DoctrineRelationshipInterface $relationship, @@ -24,7 +40,7 @@ public function handleRelationshipType( string $fromTable, string $toTable ): void { - if (empty($queryBuilder->getQueryPart('select'))) { + if (!$this->isQueryInitialised($queryBuilder)) { $queryBuilder ->select($toTable . '.' . $relationship->getRelatedClassIdColumn()) ->from($toTable); @@ -48,7 +64,7 @@ public function handleRelationshipTypeQuery( string $fullColumnName, string $placeholder ): QueryBuilder { - if (empty($queryBuilder->getQueryPart('select'))) { + if (!$this->isQueryInitialised($queryBuilder)) { throw new LogicException( 'Query is not initialized.', ); diff --git a/tests/bundle/Gateway/ExpressionVisitorTest.php b/tests/bundle/Gateway/ExpressionVisitorTest.php index 845eb90..af71cba 100644 --- a/tests/bundle/Gateway/ExpressionVisitorTest.php +++ b/tests/bundle/Gateway/ExpressionVisitorTest.php @@ -10,7 +10,9 @@ use Doctrine\Common\Collections\Expr\Comparison; use Doctrine\Common\Collections\Expr\CompositeExpression; +use Doctrine\DBAL\ArrayParameterType; use Doctrine\DBAL\Connection; +use Doctrine\DBAL\ParameterType; use Doctrine\DBAL\Platforms\AbstractPlatform; use Doctrine\DBAL\Query\Expression\ExpressionBuilder; use Doctrine\DBAL\Query\QueryBuilder; @@ -45,7 +47,7 @@ protected function setUp(): void { $this->connection = $this->createMock(Connection::class); - $this->connection->method('getExpressionBuilder') + $this->connection->method('createExpressionBuilder') ->willReturn(new ExpressionBuilder($this->connection)); $platform = $this->getMockBuilder(AbstractPlatform::class) @@ -56,6 +58,14 @@ protected function setUp(): void $this->schemaMetadata = $this->createMock(DoctrineSchemaMetadataInterface::class); + $this->schemaMetadata + ->method('getBindingTypeForColumn') + ->willReturn(ParameterType::STRING); + + $this->schemaMetadata + ->method('getArrayBindingTypeForColumn') + ->willReturn(ArrayParameterType::STRING); + $this->registry = $this->createMock(DoctrineSchemaMetadataRegistryInterface::class); $this->registry->method('getMetadataForTable') ->with('table_name') @@ -83,7 +93,7 @@ public function testWalkComparison(): void new Parameter( 'field_0', 'value', - 0, + ParameterType::STRING, ), ], $this->expressionVisitor->getParameters()); } @@ -101,7 +111,7 @@ public function testLogicalNot(): void new Parameter( 'field_0', 'value', - 0, + ParameterType::STRING, ), ], $this->expressionVisitor->getParameters()); } @@ -126,12 +136,12 @@ public function testLogicalAnd(): void new Parameter( 'field_0', 'value', - 0, + ParameterType::STRING, ), new Parameter( 'field_2_1', 'value_2', - 0, + ParameterType::STRING, ), ], $this->expressionVisitor->getParameters()); } @@ -278,37 +288,37 @@ public static function provideForFieldFromInheritedRelationship(): iterable yield [ new Comparison('relationship_1.field', '=', 'value'), 'relationship_table_name.field = :field_0', - [new Parameter('field_0', 'value', 0)], + [new Parameter('field_0', 'value', ParameterType::STRING)], ]; yield [ new Comparison('relationship_1.field', 'IN', ['value', 'value_2']), 'relationship_table_name.field IN (:field_0)', - [new Parameter('field_0', ['value', 'value_2'], 100)], + [new Parameter('field_0', ['value', 'value_2'], ArrayParameterType::STRING)], ]; yield [ new Comparison('relationship_1.field', '=', ['value', 'value_2']), 'relationship_table_name.field IN (:field_0)', - [new Parameter('field_0', ['value', 'value_2'], 100)], + [new Parameter('field_0', ['value', 'value_2'], ArrayParameterType::STRING)], ]; yield [ new Comparison('relationship_1.field', 'STARTS_WITH', 'value'), 'relationship_table_name.field LIKE :field_0', - [new Parameter('field_0', 'value%', 0)], + [new Parameter('field_0', 'value%', ParameterType::STRING)], ]; yield [ new Comparison('relationship_1.field', 'ENDS_WITH', 'value'), 'relationship_table_name.field LIKE :field_0', - [new Parameter('field_0', '%value', 0)], + [new Parameter('field_0', '%value', ParameterType::STRING)], ]; yield [ new Comparison('relationship_1.field', 'CONTAINS', 'value'), 'relationship_table_name.field LIKE :field_0', - [new Parameter('field_0', '%value%', 0)], + [new Parameter('field_0', '%value%', ParameterType::STRING)], ]; } @@ -393,6 +403,14 @@ public function testFieldFromSubclass(): void ->with('inherited_field') ->willReturn($inheritanceMetadata); + $inheritanceMetadata + ->method('getBindingTypeForColumn') + ->willReturn(ParameterType::STRING); + + $inheritanceMetadata + ->method('getArrayBindingTypeForColumn') + ->willReturn(ArrayParameterType::STRING); + $inheritanceMetadata ->expects(self::once()) ->method('getTableName') @@ -424,6 +442,14 @@ private function createRelationshipSchemaMetadata(string $tableName = 'relations ->method('getIdentifierColumn') ->willReturn('id'); + $relationshipMetadata + ->method('getBindingTypeForColumn') + ->willReturn(ParameterType::STRING); + + $relationshipMetadata + ->method('getArrayBindingTypeForColumn') + ->willReturn(ArrayParameterType::STRING); + return $relationshipMetadata; } diff --git a/tests/lib/Gateway/BaseRelationshipTypeStrategyTestCase.php b/tests/lib/Gateway/BaseRelationshipTypeStrategyTestCase.php index 50afc19..53b90ff 100644 --- a/tests/lib/Gateway/BaseRelationshipTypeStrategyTestCase.php +++ b/tests/lib/Gateway/BaseRelationshipTypeStrategyTestCase.php @@ -25,7 +25,7 @@ protected function setUp(): void { $this->connection = $this->createMock(Connection::class); $this->connection - ->method('getExpressionBuilder') + ->method('createExpressionBuilder') ->willReturn(new ExpressionBuilder($this->connection)); $platform = $this->getMockBuilder(AbstractPlatform::class) diff --git a/tests/lib/Gateway/DoInsertIdentifierTest.php b/tests/lib/Gateway/DoInsertIdentifierTest.php new file mode 100644 index 0000000..c82fa76 --- /dev/null +++ b/tests/lib/Gateway/DoInsertIdentifierTest.php @@ -0,0 +1,78 @@ +connection = $this->createMock(Connection::class); + $this->connection->method('getDatabasePlatform')->willReturn( + $this->getMockBuilder(AbstractPlatform::class)->getMockForAbstractClass() + ); + } + + public function testReadsGeneratedIdentifierFromTheConnection(): void + { + $this->connection->expects(self::once())->method('insert'); + $this->connection->expects(self::once())->method('lastInsertId')->willReturn('42'); + + $gateway = $this->createGateway(['id']); + + self::assertSame(42, $gateway->insert(['name' => 'a'])); + } + + public function testReturnsTheCallerSuppliedIdentifierInstead(): void + { + $this->connection->expects(self::once())->method('insert'); + $this->connection->expects(self::never())->method('lastInsertId'); + + $gateway = $this->createGateway(['id']); + + self::assertSame(7, $gateway->insert(['id' => 7, 'name' => 'a'])); + } + + public function testRefusesACompositeKeyWithoutInsertingAnything(): void + { + $this->connection->expects(self::never())->method('insert'); + $this->connection->expects(self::never())->method('lastInsertId'); + + $gateway = $this->createGateway(['left_id', 'right_id']); + + $this->expectException(LogicException::class); + $this->expectExceptionMessage('"test_table" does not have a single identifier column to return'); + + $gateway->insert(['left_id' => 1, 'right_id' => 2]); + } + + /** + * @param array $identifierColumns + */ + private function createGateway(array $identifierColumns): IdentifierProbeGateway + { + return new IdentifierProbeGateway( + $this->connection, + $this->createMock(DoctrineSchemaMetadataRegistryInterface::class), + $identifierColumns, + ); + } +} diff --git a/tests/lib/Gateway/JoinedRelationshipTypeStrategyTest.php b/tests/lib/Gateway/JoinedRelationshipTypeStrategyTest.php index 6537e17..998662a 100644 --- a/tests/lib/Gateway/JoinedRelationshipTypeStrategyTest.php +++ b/tests/lib/Gateway/JoinedRelationshipTypeStrategyTest.php @@ -29,6 +29,7 @@ protected function setUp(): void public function testHandleRelationshipType(): void { $queryBuilder = new QueryBuilder($this->connection); + $queryBuilder->select('from_table.id')->from('from_table'); $this->strategy->handleRelationshipType( $queryBuilder, @@ -38,36 +39,83 @@ public function testHandleRelationshipType(): void 'to_table' ); - self::assertEmpty($queryBuilder->getQueryPart('select')); - self::assertEmpty($queryBuilder->getQueryPart('from')); - self::assertEmpty($queryBuilder->getQueryPart('where')); self::assertSame( - [ - 'from_table' => [ - [ - 'joinType' => 'left', - 'joinTable' => 'to_table', - 'joinAlias' => 'to_table', - 'joinCondition' => 'from_table.foreign_key_column = to_table.related_class_id_column', - ], - ], - ], - $queryBuilder->getQueryPart('join') + 'SELECT from_table.id FROM from_table' + . ' LEFT JOIN to_table to_table' + . ' ON from_table.foreign_key_column = to_table.related_class_id_column', + $queryBuilder->getSQL() ); } - public function testHandleRelationshipTypeQuery(): void + public function testHandleRelationshipTypeIsIdempotentForTheSameTable(): void { $queryBuilder = new QueryBuilder($this->connection); + $queryBuilder->select('from_table.id')->from('from_table'); + + $relationship = $this->createDoctrineRelationship(DoctrineRelationship::JOIN_TYPE_JOINED); + $this->strategy->handleRelationshipType($queryBuilder, $relationship, 'root_alias', 'from_table', 'to_table'); + $sqlAfterFirstJoin = $queryBuilder->getSQL(); + $this->strategy->handleRelationshipType($queryBuilder, $relationship, 'root_alias', 'from_table', 'to_table'); + + self::assertSame($sqlAfterFirstJoin, $queryBuilder->getSQL()); + } + + /** + * A gateway may have joined the relationship's table itself before criteria are converted, which + * is invisible to anything but the query builder. Joining it a second time makes the query + * unbuildable, so the alias has to be recognised as taken no matter who took it. + */ + public function testHandleRelationshipTypeSkipsATableTheCallerAlreadyJoined(): void + { + $queryBuilder = new QueryBuilder($this->connection); + $queryBuilder + ->select('from_table.id') + ->from('from_table') + ->leftJoin('from_table', 'to_table', 'to_table', 'from_table.id = to_table.from_table_id'); + + $sqlBeforeStrategy = $queryBuilder->getSQL(); + + $this->strategy->handleRelationshipType( + $queryBuilder, + $this->createDoctrineRelationship(DoctrineRelationship::JOIN_TYPE_JOINED), + 'root_alias', + 'from_table', + 'to_table' + ); + + self::assertSame($sqlBeforeStrategy, $queryBuilder->getSQL()); + } + + /** + * Each query gets its own builder, so a table joined into one must not be remembered as joined + * for the next. + */ + public function testHandleRelationshipTypeTracksEachQueryBuilderSeparately(): void + { + $relationship = $this->createDoctrineRelationship(DoctrineRelationship::JOIN_TYPE_JOINED); + + $first = new QueryBuilder($this->connection); + $first->select('from_table.id')->from('from_table'); + $this->strategy->handleRelationshipType($first, $relationship, 'root_alias', 'from_table', 'to_table'); + + $second = new QueryBuilder($this->connection); + $second->select('from_table.id')->from('from_table'); + $this->strategy->handleRelationshipType($second, $relationship, 'root_alias', 'from_table', 'to_table'); + + self::assertStringContainsString('LEFT JOIN to_table to_table', $second->getSQL()); + } + + public function testHandleRelationshipTypeQueryLeavesTheQueryUntouched(): void + { + $queryBuilder = new QueryBuilder($this->connection); + $queryBuilder->select('from_table.id')->from('from_table'); + $relationshipQuery = $this->strategy->handleRelationshipTypeQuery( $queryBuilder, 'to_table.related_class_id_column_0', ':related_class_id_column_0' ); - self::assertEmpty($relationshipQuery->getQueryPart('select')); - self::assertEmpty($relationshipQuery->getQueryPart('from')); - self::assertEmpty($relationshipQuery->getQueryPart('where')); - self::assertEmpty($relationshipQuery->getQueryPart('join')); + self::assertSame('SELECT from_table.id FROM from_table', $relationshipQuery->getSQL()); } } diff --git a/tests/lib/Gateway/SubSelectRelationshipTypeStrategyTest.php b/tests/lib/Gateway/SubSelectRelationshipTypeStrategyTest.php index 394aacf..073fd05 100644 --- a/tests/lib/Gateway/SubSelectRelationshipTypeStrategyTest.php +++ b/tests/lib/Gateway/SubSelectRelationshipTypeStrategyTest.php @@ -8,7 +8,6 @@ namespace Ibexa\Tests\CorePersistence\Gateway; -use Doctrine\DBAL\Query\Expression\CompositeExpression; use Doctrine\DBAL\Query\QueryBuilder; use Ibexa\Contracts\CorePersistence\Gateway\DoctrineRelationship; use Ibexa\CorePersistence\Gateway\SubSelectRelationshipTypeStrategy; @@ -28,54 +27,52 @@ protected function setUp(): void $this->strategy = new SubSelectRelationshipTypeStrategy(); } - public function testHandleRelationshipType(): void + public function testHandleRelationshipTypeInitialisesAnEmptyQuery(): void { $queryBuilder = new QueryBuilder($this->connection); $this->strategy->handleRelationshipType( $queryBuilder, $this->createDoctrineRelationship(DoctrineRelationship::JOIN_TYPE_SUB_SELECT), - 'root_alias', - 'from_table', + 'to_table', + 'to_table', 'to_table' ); self::assertSame( - ['to_table.related_class_id_column'], - $queryBuilder->getQueryPart('select') + 'SELECT to_table.related_class_id_column FROM to_table', + $queryBuilder->getSQL() ); - self::assertSame( - [ - [ - 'table' => 'to_table', - 'alias' => null, - ], - ], - $queryBuilder->getQueryPart('from') + } + + public function testHandleRelationshipTypeJoinsWhenFromTableIsNotTheRootAlias(): void + { + $queryBuilder = new QueryBuilder($this->connection); + $queryBuilder->select('from_table.id')->from('from_table'); + + $this->strategy->handleRelationshipType( + $queryBuilder, + $this->createDoctrineRelationship(DoctrineRelationship::JOIN_TYPE_SUB_SELECT), + 'root_alias', + 'from_table', + 'to_table' ); - self::assertEmpty($queryBuilder->getQueryPart('where')); + self::assertSame( - [ - 'from_table' => [ - [ - 'joinType' => 'inner', - 'joinTable' => 'to_table', - 'joinAlias' => 'to_table', - 'joinCondition' => 'from_table.foreign_key_column = to_table.related_class_id_column', - ], - ], - ], - $queryBuilder->getQueryPart('join') + 'SELECT from_table.id FROM from_table' + . ' INNER JOIN to_table to_table' + . ' ON from_table.foreign_key_column = to_table.related_class_id_column', + $queryBuilder->getSQL() ); } - public function testHandleRelationshipTypeQueryThrowsRuntimeMappingException(): void + public function testHandleRelationshipTypeQueryThrowsForAnUninitialisedQuery(): void { $this->expectException(LogicException::class); $this->expectExceptionMessage('Query is not initialized.'); $this->strategy->handleRelationshipTypeQuery( - $this->createMock(QueryBuilder::class), + new QueryBuilder($this->connection), 'alias.related_class_id_column', ':alias.related_class_id_column_0' ); @@ -95,28 +92,9 @@ public function testHandleRelationshipTypeQuery(): void ); self::assertSame( - ['test_alias.related_class_id_column'], - $relationshipQuery->getQueryPart('select') + 'SELECT test_alias.related_class_id_column FROM test_table test_alias' + . ' WHERE test_alias.related_class_id_column IN (:related_class_id_column_0)', + $relationshipQuery->getSQL() ); - self::assertSame( - [ - [ - 'table' => 'test_table', - 'alias' => 'test_alias', - ], - ], - $relationshipQuery->getQueryPart('from') - ); - self::assertEquals( - new CompositeExpression( - CompositeExpression::TYPE_AND, - [ - 'test_alias.related_class_id_column IN (:related_class_id_column_0)', - ] - ), - $relationshipQuery->getQueryPart('where') - ); - - self::assertEmpty($relationshipQuery->getQueryPart('join')); } } diff --git a/tests/lib/Stub/IdentifierProbeGateway.php b/tests/lib/Stub/IdentifierProbeGateway.php new file mode 100644 index 0000000..ec1067c --- /dev/null +++ b/tests/lib/Stub/IdentifierProbeGateway.php @@ -0,0 +1,79 @@ + + */ +final class IdentifierProbeGateway extends AbstractDoctrineDatabase +{ + /** @var array */ + private array $identifierColumns; + + /** + * @param array $identifierColumns + */ + public function __construct( + Connection $connection, + DoctrineSchemaMetadataRegistryInterface $registry, + array $identifierColumns + ) { + parent::__construct($connection, $registry); + + $this->identifierColumns = $identifierColumns; + } + + /** + * @param array $data + * + * @throws \Doctrine\DBAL\Exception + */ + public function insert(array $data): int + { + return $this->doInsert($data); + } + + protected function getTableName(): string + { + return 'test_table'; + } + + protected function getTableAlias(): string + { + return 'test_alias'; + } + + protected function buildMetadata(): DoctrineSchemaMetadataInterface + { + return new DoctrineSchemaMetadata( + $this->connection, + null, + $this->getTableName(), + [ + 'id' => Types::INTEGER, + 'left_id' => Types::INTEGER, + 'right_id' => Types::INTEGER, + 'name' => Types::STRING, + ], + $this->identifierColumns, + ); + } +}