Skip to content

Failing test: Criteria matching on fields with custom column types #11897

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

Closed
wants to merge 1 commit into from
Closed

Failing test: Criteria matching on fields with custom column types #11897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions tests/Tests/Models/ValueConversionType/InversedManyToManyEntity.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ class InversedManyToManyEntity
*/
public $id1;

/**
* @var string
* @Column(type="rot13", length=255, nullable=true)
*/
public $field = null;

/**
* @phpstan-var Collection<int, OwningManyToManyEntity>
* @ManyToMany(targetEntity="OwningManyToManyEntity", mappedBy="associatedEntities")
Expand Down
2 changes: 1 addition & 1 deletion tests/Tests/Models/ValueConversionType/InversedOneToManyEntity.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class InversedOneToManyEntity

/**
* @var string
* @Column(type="string", name="some_property", length=255)
* @Column(type="string", name="some_property", length=255, nullable=true)
*/
public $someProperty;

Expand Down
6 changes: 6 additions & 0 deletions tests/Tests/Models/ValueConversionType/OwningManyToManyEntity.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ class OwningManyToManyEntity
*/
public $id2;

/**
* @var string
* @Column(type="rot13", length=255, nullable=true)
*/
public $field = null;

/**
* @var Collection<int, InversedManyToManyEntity>
* @ManyToMany(targetEntity="InversedManyToManyEntity", inversedBy="associatedEntities")
Expand Down
6 changes: 6 additions & 0 deletions tests/Tests/Models/ValueConversionType/OwningManyToOneEntity.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ class OwningManyToOneEntity
*/
public $id2;

/**
* @var string
* @Column(type="rot13", length=255, nullable=true)
*/
public $field = null;

/**
* @var InversedOneToManyEntity
* @ManyToOne(targetEntity="InversedOneToManyEntity", inversedBy="associatedEntities")
Expand Down
65 changes: 65 additions & 0 deletions tests/Tests/ORM/Functional/ValueConversionType/ManyToManyCriteriaMatchingTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional\ValueConversionType;

use Doctrine\Common\Collections\Criteria;
use Doctrine\Common\Collections\Expr\Comparison;
use Doctrine\Tests\Models\ValueConversionType\InversedManyToManyEntity;
use Doctrine\Tests\Models\ValueConversionType\OwningManyToManyEntity;
use Doctrine\Tests\OrmFunctionalTestCase;
use Generator;

class ManyToManyCriteriaMatchingTest extends OrmFunctionalTestCase
{
protected function setUp(): void
{
$this->useModelSet('vct_manytomany');

parent::setUp();
}

/**
* @dataProvider provideMatchingExpressions
*/
public function testCriteriaMatchingOnFieldInManyToManyTarget(Comparison $comparison): void
{
$associated = new InversedManyToManyEntity();
$associated->id1 = 'associated';

$matching = new OwningManyToManyEntity();
$matching->id2 = 'first';
$matching->field = 'match this';
$matching->associatedEntities->add($associated);

$nonMatching = new OwningManyToManyEntity();
$nonMatching->id2 = 'second';
$nonMatching->field = 'this is no match';
$nonMatching->associatedEntities->add($associated);

$this->_em->persist($associated);
$this->_em->persist($matching);
$this->_em->persist($nonMatching);
$this->_em->flush();
$this->_em->clear();

$this->getQueryLog()->reset()->enable();

$associated = $this->_em->find(InversedManyToManyEntity::class, 'associated');
self::assertFalse($associated->associatedEntities->isInitialized(), 'Pre-condition: lazy collection');

$result = $associated->associatedEntities->matching(Criteria::create()->where($comparison));

$l = $this->getQueryLog();

self::assertCount(1, $result);
self::assertSame('first', $result[0]->id2);
}

public function provideMatchingExpressions(): Generator
{
yield [Criteria::expr()->eq('field', 'match this')];
yield [Criteria::expr()->in('field', ['match this'])];
Copy link
Member

@morozov morozov Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if it's related to custom types at all, but this test fails because the following code doesn't properly handle the IN operator:

orm/src/Persisters/Collection/ManyToManyPersister.php

Lines 257 to 263 in fd041fb

if ($value === null && ($operator === Comparison::EQ || $operator === Comparison::NEQ)) {
$whereClauses[] = sprintf('te.%s %s NULL', $field, $operator === Comparison::EQ ? 'IS' : 'IS NOT');
} else {
$whereClauses[] = sprintf('te.%s %s ?', $field, $operator);
$params[] = $value;
$paramTypes[] = PersisterHelper::getTypeOfField($name, $targetClass, $this->em)[0];
}

It should:

  1. Add parentheses to the SQL.
  2. Repeat the placeholder, $params[] and $paramTypes[] for each element of the value.

Copy link
Contributor

Choose a reason for hiding this comment

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

#11031 seems to be related to this bug.

}
}
65 changes: 65 additions & 0 deletions tests/Tests/ORM/Functional/ValueConversionType/OneToManyCriteriaMatchingTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional\ValueConversionType;

use Doctrine\Common\Collections\Criteria;
use Doctrine\Common\Collections\Expr\Comparison;
use Doctrine\Tests\Models\ValueConversionType\InversedOneToManyEntity;
use Doctrine\Tests\Models\ValueConversionType\OwningManyToOneEntity;
use Doctrine\Tests\OrmFunctionalTestCase;
use Generator;

class OneToManyCriteriaMatchingTest extends OrmFunctionalTestCase
{
protected function setUp(): void
{
$this->useModelSet('vct_onetomany');

parent::setUp();
}

/**
* @dataProvider provideMatchingExpressions
*/
public function testCriteriaMatchingOnFieldInOneToManyTarget(Comparison $comparison): void
{
$entityWithCollection = new InversedOneToManyEntity();
$entityWithCollection->id1 = 'associated';

$matching = new OwningManyToOneEntity();
$matching->id2 = 'first';
$matching->field = 'match this';
$matching->associatedEntity = $entityWithCollection;

$notMatching = new OwningManyToOneEntity();
$notMatching->id2 = 'second';
$notMatching->field = 'this is no match';
$notMatching->associatedEntity = $entityWithCollection;

$this->_em->persist($entityWithCollection);
$this->_em->persist($matching);
$this->_em->persist($notMatching);
$this->_em->flush();
$this->_em->clear();

$this->getQueryLog()->reset()->enable();

$entityWithCollection = $this->_em->find(InversedOneToManyEntity::class, 'associated');
self::assertFalse($entityWithCollection->associatedEntities->isInitialized(), 'Pre-condition: lazy collection');

$result = $entityWithCollection->associatedEntities->matching(Criteria::create()->where($comparison));

$l = $this->getQueryLog();

self::assertCount(1, $result);
self::assertSame('first', $result[0]->id2);
}

public function provideMatchingExpressions(): Generator
{
yield [Criteria::expr()->eq('field', 'match this')];
yield [Criteria::expr()->in('field', ['match this'])];
Copy link
Member

@morozov morozov Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

And this one fails because by the time when the following line is executed

orm/src/Persisters/Entity/BasicEntityPersister.php

Line 924 in a67f677

$stmt = $this->conn->executeQuery($query, $params, $types);

$types contains [ArrayParameterType::STRING, 'rot13'], so at this point, the information about the custom type is lost.

Right now, the DBAL only supports a fixed set of array types (see ArrayParameterType).

For this to work, we can try introducing the ArrayType class and have it eventually replace the ArrayParameterType enum. This class should accepts the element type in its constructor. For example, new ArrayType(ParameterType::INTEGER) would represent what's currently represented as ArrayParameterType::INTEGER, and new ArrayType('rot13') would represent an array of custom type.

Not sure what effort it would take and how doable this is within the current type system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, something like this was my assumption... that we currently cannot combine the array type hint with custom types. If I get you right, your suggestion is to have the ArrayType as a wrapper (not necessarily decorator) around other types.

Copy link
Member

@morozov morozov Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What's the difference between a wrapper and a decorator? I believe I want to see "typed array" as one of the standard types. It will (I guess) wrap (or be parameterized with) its value type. This way, we can build queries with the array values of any standard or custom type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The distinction between wrapper and decorator was not sharp enough. What I meant was that ArrayType could be wrapped around any other base type, but (to my understanding) need not implement the same interface as the base types themselves.

A textbook decorator – to my understanding – implements at least the same interface as the objects it wraps, it's transparent from the interface point of view.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, in this case I mean the decorator. It should wrap a type and be a type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let’s discuss this over at doctrine/dbal#6883

}
}
Loading