Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IBX-6634: Fixed relationship comparison #11

Merged
merged 4 commits into from
Dec 5, 2023
Merged

IBX-6634: Fixed relationship comparison #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a28128
IBX-6634: Fixed relationship comparison
Steveb-p Nov 22, 2023
05c878e
IBX-6634: Fixed relationship comparison
Steveb-p Nov 22, 2023
2ae1a4c
IBX-6634: Fixed inheritance breaking countBy
Steveb-p Nov 23, 2023
6a61338
IBX-6634: Fixed countBy for multiple joined tables
Steveb-p Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 58 additions & 39 deletions src/contracts/Gateway/AbstractDoctrineDatabase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,18 +125,15 @@ public function countAll(): int
public function countBy($criteria): int
{
$metadata = $this->getMetadata();
$qb = $this->connection->createQueryBuilder();
$qb = $this->createBaseQueryBuilder();
$this->applyInheritance($qb);

$identifierColumn = $metadata->getIdentifierColumn();
$tableAlias = $this->getTableAlias();
$qb->select($this->connection->getDatabasePlatform()->getCountExpression($tableAlias . '.' . $identifierColumn));
$qb->from($metadata->getTableName(), $tableAlias);
$platform = $this->connection->getDatabasePlatform();
$qb->select($platform->getCountExpression(sprintf('DISTINCT %s.%s', $tableAlias, $identifierColumn)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's \Doctrine\DBAL\Query\QueryBuilder::distinct but maybe not usable here given it's COUNT(DISTINCT ) not DISTINCT COUNT()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, due to the above it is not usable. I have actually checked and we have a similar code in our ibexa/core, and this syntax works for our supported databases.


$expr = $this->convertCriteriaToExpression($qb, $criteria);
if ($expr !== null) {
$qb->andWhere($expr);
}
$this->applyCriteria($qb, $criteria);

return (int)$qb->execute()->fetchOne();
}
Expand All @@ -162,39 +159,10 @@ public function findBy($criteria, ?array $orderBy = null, ?int $limit = null, in
$qb = $this->createBaseQueryBuilder();
$this->applyInheritance($qb);

foreach ($orderBy ?? [] as $column => $order) {
if (!$metadata->hasColumn($column) && !$metadata->isInheritedColumn($column)) {
$columns = $metadata->getColumns();
foreach ($metadata->getSubclasses() as $subMetadata) {
$columns = array_merge($columns, $subMetadata->getColumns());
}

throw new InvalidArgumentException(sprintf(
'"%s" does not exist in "%s", or is not available for ordering. Available columns are: "%s"',
$column,
$this->getTableName(),
implode('", "', $columns),
));
}
$this->applyOrderBy($qb, $orderBy);
$this->applyCriteria($qb, $criteria);
$this->applyLimits($qb, $limit, $offset);

if ($metadata->isInheritedColumn($column)) {
$subMetadata = $metadata->getInheritanceMetadataWithColumn($column);
assert($subMetadata !== null);
$qb->addOrderBy($subMetadata->getTableName() . '.' . $column, $order);
} else {
$qb->addOrderBy($this->getTableAlias() . '.' . $column, $order);
}
}

$expr = $this->convertCriteriaToExpression($qb, $criteria);
if ($expr !== null) {
$qb->andWhere($expr);
}

if ($limit !== null) {
$qb->setMaxResults($limit);
}
$qb->setFirstResult($offset);
$results = $qb->execute()->fetchAllAssociative();

return array_map([$metadata, 'convertToPHPValues'], $results);
Expand Down Expand Up @@ -427,4 +395,55 @@ private function buildCondition(QueryBuilder $qb, string $column, $value): strin

return $qb->expr()->eq($fullColumnName, $parameter);
}

/**
* @param array<string, string>|null $orderBy Map of column names to "ASC" or "DESC", that will be used in SORT query
*/
final protected function applyOrderBy(QueryBuilder $qb, ?array $orderBy = []): void
{
$metadata = $this->getMetadata();

foreach ($orderBy ?? [] as $column => $order) {
if (!$metadata->hasColumn($column) && !$metadata->isInheritedColumn($column)) {
$columns = $metadata->getColumns();
foreach ($metadata->getSubclasses() as $subMetadata) {
$columns = array_merge($columns, $subMetadata->getColumns());
}

throw new InvalidArgumentException(sprintf(
'"%s" does not exist in "%s", or is not available for ordering. Available columns are: "%s"',
$column,
$this->getTableName(),
implode('", "', $columns),
));
}

if ($metadata->isInheritedColumn($column)) {
$subMetadata = $metadata->getInheritanceMetadataWithColumn($column);
assert($subMetadata !== null);
$qb->addOrderBy($subMetadata->getTableName() . '.' . $column, $order);
} else {
$qb->addOrderBy($this->getTableAlias() . '.' . $column, $order);
}
}
}

/**
* @param \Doctrine\Common\Collections\Expr\Expression|array<string, \Doctrine\Common\Collections\Expr\Expression|scalar|array<scalar>|null> $criteria
*/
final protected function applyCriteria(QueryBuilder $qb, $criteria): void
{
$expr = $this->convertCriteriaToExpression($qb, $criteria);
if ($expr !== null) {
$qb->andWhere($expr);
}
}

final protected function applyLimits(QueryBuilder $qb, ?int $limit, int $offset): void
{
if ($limit !== null) {
$qb->setMaxResults($limit);
}
$qb->setFirstResult($offset);
}
}
14 changes: 7 additions & 7 deletions src/lib/Gateway/ExpressionVisitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,13 +252,13 @@ private function handleRelationshipComparison(string $column, Comparison $compar
$metadata,
$relationship->getForeignKeyColumn(),
$column,
$comparison->getValue(),
$comparison,
);
case $relationship->getJoinType() === DoctrineRelationship::JOIN_TYPE_JOINED:
return $this->handleJoinQuery(
$metadata,
$column,
$comparison->getValue(),
$comparison,
);
default:
throw new RuntimeMappingException(sprintf(
Expand Down Expand Up @@ -287,13 +287,13 @@ private function expr(): ExpressionBuilder
private function handleJoinQuery(
DoctrineSchemaMetadataInterface $relationshipMetadata,
string $field,
Value $value
Comparison $comparison
): string {
$tableName = $relationshipMetadata->getTableName();
$parameterName = $field . '_' . count($this->parameters);
$placeholder = ':' . $parameterName;

$value = $this->walkValue($value);
$value = $this->walkValue($comparison->getValue());
$type = $relationshipMetadata->getBindingTypeForColumn($field);
if (is_array($value)) {
$type += Connection::ARRAY_PARAM_OFFSET;
Expand All @@ -306,14 +306,14 @@ private function handleJoinQuery(
return $this->expr()->in($tableName . '.' . $field, $placeholder);
}

return $this->expr()->eq($tableName . '.' . $field, $placeholder);
return $this->expr()->comparison($tableName . '.' . $field, $comparison->getOperator(), $placeholder);
}

private function handleSubSelectQuery(
DoctrineSchemaMetadataInterface $relationshipMetadata,
string $foreignField,
string $field,
Value $value
Comparison $comparison
): string {
$tableName = $relationshipMetadata->getTableName();
$parameterName = $field . '_' . count($this->parameters);
Expand All @@ -329,7 +329,7 @@ private function handleSubSelectQuery(
),
);

$value = $this->walkValue($value);
$value = $this->walkValue($comparison->getValue());
$type = $relationshipMetadata->getBindingTypeForColumn($field);
if (is_array($value)) {
$type += Connection::ARRAY_PARAM_OFFSET;
Expand Down
36 changes: 29 additions & 7 deletions tests/integration/Gateway/ExpressionVisitorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,38 @@ public function testInvalidField(): void
$this->expressionVisitor->dispatch(new Comparison('non_existent_field', '=', 'bar'));
}

public function testTraversingRelationships(): void
/**
* @dataProvider provideForTraversingRelationships
*/
public function testTraversingRelationships(Comparison $expr, string $expectedResult): void
{
// Note: This assumes relationship tables are joined before being used.
$expr = new Comparison(
'relationship_1.relationship_2.relationship_2_foo',
'IN',
'bar',
);
$result = $this->expressionVisitor->dispatch($expr);
self::assertSame('relationship_2_table_name.relationship_2_foo = :relationship_2_foo_0', $result);
self::assertSame($expectedResult, $result);
}

/**
* @return iterable<array{\Doctrine\Common\Collections\Expr\Comparison, non-empty-string}>
*/
public static function provideForTraversingRelationships(): iterable
{
yield [
new Comparison(
'relationship_1.relationship_2.relationship_2_foo',
'IN',
'bar',
),
'relationship_2_table_name.relationship_2_foo IN :relationship_2_foo_0',
Comment on lines +67 to +72
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I don't follow there relationship between what you declare this relationship and what you get from it (pun intended), but I think we've already discussed this ages ago, so I'll just continue scrolling :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alongosz could you rephrase? I don't quite understand what part of the above is the issue? Is it that we are declaring relationships between tables?

];

yield [
new Comparison(
'relationship_1.relationship_2.relationship_2_foo',
'<',
'2023-11-22 10:00:00',
),
'relationship_2_table_name.relationship_2_foo < :relationship_2_foo_0',
];
}

public function testInvalidRelationshipTraversal(): void
Expand Down
Loading