Skip to content

Commit bd86a47

Browse files
mpdudempdude
committed
Fix collection filtering API for IN/NOT IN comparisons that require type conversions
This PR fixes the `Criteria` matching API for `IN` and `NIN` conditions with values that are arrays, by making sure that type information for the matched field is passed to the DBAL level correctly. Passing the right parameter type to DBAL is important to make sure parameter conversions are applied before matching at the database level. Memory-based collections (`ArrayCollection`s or initialized collection fields) would perform matching on the objects in memory where no type conversion to the database representation is required, giving correct results. But uninitialized collections that have their conditions evaluated at the database level need to convert parameter values to the database representation before performing the comparison. One extra challenge is that the DBAL type system does currently not support array-valued parameters for custom types. Only a [limited list of types](https://www.doctrine-project.org/projects/doctrine-dbal/en/4.2/reference/data-retrieval-and-manipulation.html#list-of-parameters-conversion) is supported. I discussed this with @morozov at the Doctrine Hackathon and came to the conclusion that it would be best to work around this limitation at the ORM level. Thus, this fix recognizes array-valued parameters and creates multiple placeholders (like `?, ?, ?`) for them, flattening out the arrays in the parameter list and repeating the type information for each one of them. Previous stalled attempt to fix this was in #11897.
1 parent a939dc2 commit bd86a47

File tree

11 files changed

+217
-43
lines changed

11 files changed

+217
-43
lines changed

‎phpstan-baseline.neon‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2946,12 +2946,6 @@ parameters:
29462946
count: 1
29472947
path: src/Persisters/Entity/BasicEntityPersister.php
29482948

2949-
-
2950-
message: '#^Strict comparison using \=\=\= between string and null will always evaluate to false\.$#'
2951-
identifier: identical.alwaysFalse
2952-
count: 1
2953-
path: src/Persisters/Entity/BasicEntityPersister.php
2954-
29552949
-
29562950
message: '#^Method Doctrine\\ORM\\Persisters\\Entity\\CachedPersisterContext\:\:__construct\(\) has parameter \$class with generic interface Doctrine\\Persistence\\Mapping\\ClassMetadata but does not specify its types\: T$#'
29572951
identifier: missingType.generics

‎src/Persisters/Collection/ManyToManyPersister.php‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -257,14 +257,14 @@ public function loadCriteria(PersistentCollection $collection, Criteria $criteri
257257

258258
if ($value === null && ($operator === Comparison::EQ || $operator === Comparison::NEQ)) {
259259
$whereClauses[] = sprintf('te.%s %s NULL', $field, $operator === Comparison::EQ ? 'IS' : 'IS NOT');
260-
} else {
261-
if ($operator === Comparison::IN) {
262-
$whereClauses[] = sprintf('te.%s IN (?)', $field);
263-
} elseif ($operator === Comparison::NIN) {
264-
$whereClauses[] = sprintf('te.%s NOT IN (?)', $field);
265-
} else {
266-
$whereClauses[] = sprintf('te.%s %s ?', $field, $operator);
260+
} elseif ($operator === Comparison::IN || $operator === Comparison::NIN) {
261+
$whereClauses[] = sprintf('te.%s %s (%s)', $field, $operator === Comparison::IN ? 'IN' : 'NOT IN', implode(', ', array_fill(0, count($value), '?')));
262+
foreach ($value as $item) {
263+
$params = array_merge($params, PersisterHelper::convertToParameterValue($item, $this->em));
264+
$paramTypes = array_merge($paramTypes, PersisterHelper::inferParameterTypes($name, $item, $targetClass, $this->em));
267265
}
266+
} else {
267+
$whereClauses[] = sprintf('te.%s %s ?', $field, $operator);
268268

269269
$params = array_merge($params, PersisterHelper::convertToParameterValue($value, $this->em));
270270
$paramTypes = array_merge($paramTypes, PersisterHelper::inferParameterTypes($name, $value, $targetClass, $this->em));

‎src/Persisters/Entity/BasicEntityPersister.php‎

Lines changed: 54 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,12 @@
3434
use LengthException;
3535

3636
use function array_combine;
37+
use function array_diff_key;
38+
use function array_fill;
39+
use function array_flip;
3740
use function array_keys;
3841
use function array_map;
3942
use function array_merge;
40-
use function array_search;
4143
use function array_unique;
4244
use function array_values;
4345
use function assert;
@@ -951,6 +953,20 @@ public function expandCriteriaParameters(Criteria $criteria)
951953
continue;
952954
}
953955

956+
if ($operator === Comparison::IN || $operator === Comparison::NIN) {
957+
if (! is_array($value)) {
958+
$value = [$value];
959+
}
960+
961+
$nonNullValues = array_diff_key($value, array_flip(array_keys($value, null, true)));
962+
foreach ($nonNullValues as $item) {
963+
$sqlParams = array_merge($sqlParams, PersisterHelper::convertToParameterValue($item, $this->em));
964+
$sqlTypes = array_merge($sqlTypes, PersisterHelper::inferParameterTypes($field, $item, $this->class, $this->em));
965+
}
966+
967+
continue;
968+
}
969+
954970
$sqlParams = array_merge($sqlParams, PersisterHelper::convertToParameterValue($value, $this->em));
955971
$sqlTypes = array_merge($sqlTypes, PersisterHelper::inferParameterTypes($field, $value, $this->class, $this->em));
956972
}
@@ -1689,6 +1705,8 @@ protected function getSelectConditionCriteriaSQL(Criteria $criteria)
16891705
*/
16901706
public function getSelectConditionStatementSQL($field, $value, $assoc = null, $comparison = null)
16911707
{
1708+
$comparison = $comparison ?? (is_array($value) ? Comparison::IN : Comparison::EQ);
1709+
16921710
$selectedColumns = [];
16931711
$columns = $this->getSelectConditionStatementColumnSQL($field, $assoc);
16941712

@@ -1708,46 +1726,45 @@ public function getSelectConditionStatementSQL($field, $value, $assoc = null, $c
17081726
$placeholder = $type->convertToDatabaseValueSQL($placeholder, $this->platform);
17091727
}
17101728

1711-
if ($comparison !== null) {
1712-
// special case null value handling
1713-
if (($comparison === Comparison::EQ || $comparison === Comparison::IS) && $value === null) {
1714-
$selectedColumns[] = $column . ' IS NULL';
1715-
1716-
continue;
1717-
}
1718-
1719-
if ($comparison === Comparison::NEQ && $value === null) {
1720-
$selectedColumns[] = $column . ' IS NOT NULL';
1721-
1722-
continue;
1723-
}
1724-
1725-
$selectedColumns[] = $column . ' ' . sprintf(self::$comparisonMap[$comparison], $placeholder);
1729+
// special case null value handling
1730+
if (($comparison === Comparison::EQ || $comparison === Comparison::IS) && $value === null) {
1731+
$selectedColumns[] = $column . ' IS NULL';
17261732

17271733
continue;
17281734
}
17291735

1730-
if (is_array($value)) {
1731-
$in = sprintf('%s IN (%s)', $column, $placeholder);
1736+
if ($comparison === Comparison::NEQ && $value === null) {
1737+
$selectedColumns[] = $column . ' IS NOT NULL';
17321738

1733-
if (array_search(null, $value, true) !== false) {
1734-
$selectedColumns[] = sprintf('(%s OR %s IS NULL)', $in, $column);
1739+
continue;
1740+
}
17351741

1736-
continue;
1742+
if ($comparison === Comparison::IN || $comparison === Comparison::NIN) {
1743+
if (! is_array($value)) {
1744+
$value = [$value];
17371745
}
17381746

1739-
$selectedColumns[] = $in;
1747+
$nullKeys = array_keys($value, null, true);
1748+
$nonNullValues = array_diff_key($value, array_flip($nullKeys));
17401749

1741-
continue;
1742-
}
1750+
$placeholders = implode(', ', array_fill(0, count($nonNullValues), $placeholder));
17431751

1744-
if ($value === null) {
1745-
$selectedColumns[] = sprintf('%s IS NULL', $column);
1752+
$in = $column . ' ' . sprintf(self::$comparisonMap[$comparison], $placeholders);
1753+
1754+
if ($nullKeys) {
1755+
if ($nonNullValues) {
1756+
$selectedColumns[] = sprintf('(%s OR %s IS NULL)', $in, $column);
1757+
} else {
1758+
$selectedColumns[] = $column . ' IS NULL';
1759+
}
1760+
} else {
1761+
$selectedColumns[] = $in;
1762+
}
17461763

17471764
continue;
17481765
}
17491766

1750-
$selectedColumns[] = sprintf('%s = %s', $column, $placeholder);
1767+
$selectedColumns[] = $column . ' ' . sprintf(self::$comparisonMap[$comparison], $placeholder);
17511768
}
17521769

17531770
return implode(' AND ', $selectedColumns);
@@ -1938,6 +1955,16 @@ public function expandParameters($criteria)
19381955
continue; // skip null values.
19391956
}
19401957

1958+
if (is_array($value)) {
1959+
$nonNullValues = array_diff_key($value, array_flip(array_keys($value, null, true)));
1960+
foreach ($nonNullValues as $item) {
1961+
$types = array_merge($types, PersisterHelper::inferParameterTypes($field, $item, $this->class, $this->em));
1962+
$params = array_merge($params, PersisterHelper::convertToParameterValue($item, $this->em));
1963+
}
1964+
1965+
continue;
1966+
}
1967+
19411968
$types = array_merge($types, PersisterHelper::inferParameterTypes($field, $value, $this->class, $this->em));
19421969
$params = array_merge($params, PersisterHelper::convertToParameterValue($value, $this->em));
19431970
}

‎src/Persisters/Entity/EntityPersister.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ public function getCountSQL($criteria = []);
7373
/**
7474
* Expands the parameters from the given criteria and use the correct binding types if found.
7575
*
76-
* @param string[] $criteria
76+
* @param array<string, mixed> $criteria
7777
*
7878
* @phpstan-return array{list<mixed>, list<int|string|null>}
7979
*/

‎tests/Tests/Models/ValueConversionType/InversedManyToManyEntity.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,12 @@ class InversedManyToManyEntity
2525
*/
2626
public $id1;
2727

28+
/**
29+
* @var string
30+
* @Column(type="rot13", length=255, nullable=true)
31+
*/
32+
public $field = null;
33+
2834
/**
2935
* @phpstan-var Collection<int, OwningManyToManyEntity>
3036
* @ManyToMany(targetEntity="OwningManyToManyEntity", mappedBy="associatedEntities")

‎tests/Tests/Models/ValueConversionType/InversedOneToManyEntity.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ class InversedOneToManyEntity
3333

3434
/**
3535
* @var string
36-
* @Column(type="string", name="some_property", length=255)
36+
* @Column(type="string", name="some_property", length=255, nullable=true)
3737
*/
3838
public $someProperty;
3939

‎tests/Tests/Models/ValueConversionType/OwningManyToManyEntity.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,12 @@ class OwningManyToManyEntity
2727
*/
2828
public $id2;
2929

30+
/**
31+
* @var string
32+
* @Column(type="rot13", length=255, nullable=true)
33+
*/
34+
public $field = null;
35+
3036
/**
3137
* @var Collection<int, InversedManyToManyEntity>
3238
* @ManyToMany(targetEntity="InversedManyToManyEntity", inversedBy="associatedEntities")

‎tests/Tests/Models/ValueConversionType/OwningManyToOneEntity.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,12 @@ class OwningManyToOneEntity
2424
*/
2525
public $id2;
2626

27+
/**
28+
* @var string
29+
* @Column(type="rot13", length=255, nullable=true)
30+
*/
31+
public $field = null;
32+
2733
/**
2834
* @var InversedOneToManyEntity
2935
* @ManyToOne(targetEntity="InversedOneToManyEntity", inversedBy="associatedEntities")

‎tests/Tests/ORM/Functional/ValueConversionType/ManyToManyCriteriaMatchingTest.php‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Doctrine\Tests\ORM\Functional\ValueConversionType;
6+
7+
use Doctrine\Common\Collections\Criteria;
8+
use Doctrine\Common\Collections\Expr\Comparison;
9+
use Doctrine\Tests\Models\ValueConversionType\InversedManyToManyEntity;
10+
use Doctrine\Tests\Models\ValueConversionType\OwningManyToManyEntity;
11+
use Doctrine\Tests\OrmFunctionalTestCase;
12+
use Generator;
13+
14+
class ManyToManyCriteriaMatchingTest extends OrmFunctionalTestCase
15+
{
16+
protected function setUp(): void
17+
{
18+
$this->useModelSet('vct_manytomany');
19+
20+
parent::setUp();
21+
}
22+
23+
/**
24+
* @dataProvider provideMatchingExpressions
25+
*/
26+
public function testCriteriaMatchingOnFieldInManyToManyTarget(Comparison $comparison): void
27+
{
28+
$associated = new InversedManyToManyEntity();
29+
$associated->id1 = 'associated';
30+
31+
$matching = new OwningManyToManyEntity();
32+
$matching->id2 = 'first';
33+
$matching->field = 'match this';
34+
$matching->associatedEntities->add($associated);
35+
36+
$nonMatching = new OwningManyToManyEntity();
37+
$nonMatching->id2 = 'second';
38+
$nonMatching->field = 'this is no match';
39+
$nonMatching->associatedEntities->add($associated);
40+
41+
$this->_em->persist($associated);
42+
$this->_em->persist($matching);
43+
$this->_em->persist($nonMatching);
44+
$this->_em->flush();
45+
$this->_em->clear();
46+
47+
$this->getQueryLog()->reset()->enable();
48+
49+
$associated = $this->_em->find(InversedManyToManyEntity::class, 'associated');
50+
self::assertFalse($associated->associatedEntities->isInitialized(), 'Pre-condition: lazy collection');
51+
52+
$result = $associated->associatedEntities->matching(Criteria::create()->where($comparison));
53+
54+
$l = $this->getQueryLog();
55+
56+
self::assertCount(1, $result);
57+
self::assertSame('first', $result[0]->id2);
58+
}
59+
60+
public function provideMatchingExpressions(): Generator
61+
{
62+
yield [Criteria::expr()->eq('field', 'match this')];
63+
yield [Criteria::expr()->in('field', ['match this'])];
64+
}
65+
}

‎tests/Tests/ORM/Functional/ValueConversionType/OneToManyCriteriaMatchingTest.php‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Doctrine\Tests\ORM\Functional\ValueConversionType;
6+
7+
use Doctrine\Common\Collections\Criteria;
8+
use Doctrine\Common\Collections\Expr\Comparison;
9+
use Doctrine\Tests\Models\ValueConversionType\InversedOneToManyEntity;
10+
use Doctrine\Tests\Models\ValueConversionType\OwningManyToOneEntity;
11+
use Doctrine\Tests\OrmFunctionalTestCase;
12+
use Generator;
13+
14+
class OneToManyCriteriaMatchingTest extends OrmFunctionalTestCase
15+
{
16+
protected function setUp(): void
17+
{
18+
$this->useModelSet('vct_onetomany');
19+
20+
parent::setUp();
21+
}
22+
23+
/**
24+
* @dataProvider provideMatchingExpressions
25+
*/
26+
public function testCriteriaMatchingOnFieldInOneToManyTarget(Comparison $comparison): void
27+
{
28+
$entityWithCollection = new InversedOneToManyEntity();
29+
$entityWithCollection->id1 = 'associated';
30+
31+
$matching = new OwningManyToOneEntity();
32+
$matching->id2 = 'first';
33+
$matching->field = 'match this';
34+
$matching->associatedEntity = $entityWithCollection;
35+
36+
$notMatching = new OwningManyToOneEntity();
37+
$notMatching->id2 = 'second';
38+
$notMatching->field = 'this is no match';
39+
$notMatching->associatedEntity = $entityWithCollection;
40+
41+
$this->_em->persist($entityWithCollection);
42+
$this->_em->persist($matching);
43+
$this->_em->persist($notMatching);
44+
$this->_em->flush();
45+
$this->_em->clear();
46+
47+
$this->getQueryLog()->reset()->enable();
48+
49+
$entityWithCollection = $this->_em->find(InversedOneToManyEntity::class, 'associated');
50+
self::assertFalse($entityWithCollection->associatedEntities->isInitialized(), 'Pre-condition: lazy collection');
51+
52+
$result = $entityWithCollection->associatedEntities->matching(Criteria::create()->where($comparison));
53+
54+
$l = $this->getQueryLog();
55+
56+
self::assertCount(1, $result);
57+
self::assertSame('first', $result[0]->id2);
58+
}
59+
60+
public function provideMatchingExpressions(): Generator
61+
{
62+
yield [Criteria::expr()->eq('field', 'match this')];
63+
yield [Criteria::expr()->in('field', ['match this'])];
64+
}
65+
}

0 commit comments

Comments
 (0)