Orm: Paginator breaks when DQL query is re-used with different identifier parameters that require conversion

[Ocramius]

Bug Report

| Q | A
|------------ | ------
| BC Break | no
| Version | 2.6.4

Summary

7820 and #7821 fix pagination issues happening when the paginator is relying on identifiers that require custom DBAL type conversion.

The type conversion works, but since it has been performed inside the WhereInWalker (to keep things a bit simpler), it is only performed when the query is in Doctrine\ORM\Query::STATE_DIRTY.

If the parameters are changed on a Doctrine\ORM\Query, the query doesn't change parser state, and therefore the WhereInWalker is completely skipped.

I will try to write a small reproducer and fix ASAP, hoping to not cause too big performance regressions in the ORM. I think it may come down to forcing query parsing to be repeated.

Current behavior

$id = generateCustomId();
$a  = new A($id);

$this->em->persist($a);
$this->em->flush();

$query = $this->em->createQuery('SELECT a FROM ' . A::class);

$paginator = new Paginator($query);

$query->setFirstResult(1);

Assert::count(0, \iterator_to_array($paginator));

$query->setFirstResult(0);

Assert::same([$a], \iterator_to_array($paginator));

Expected behavior

The last assertion in the test above fails: this is an unfortunate case of mutable state shared inside Doctrine\ORM\Query.

[lcobucci]

@Ocramius this is an interesting one (which I didn't manage to reproduce in the way you described)...

As far as I've seen the Paginator doesn't influence the original query state because it clones it before applying the changes.

However, things get a bit weird when using (and changing) a query builder instead of a query object. This happens because QueryBuilder#getQuery() always returns a new query object:

<?php

declare(strict_types=1);

namespace Doctrine\Tests\ORM\Functional\Ticket;

use Doctrine\Common\Collections\Criteria;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\StringType;
use Doctrine\DBAL\Types\Type;
use Doctrine\ORM\Tools\Pagination\Paginator;
use Doctrine\Tests\OrmFunctionalTestCase;
use function is_string;
use function iterator_to_array;

/**
 * @group GH7837
 */
final class GH7837Test extends OrmFunctionalTestCase
{
    private const SONG = [
        'What is this song all about?',
        'Can\'t figure any lyrics out',
        'How do the words to it go?',
        'I wish you\'d tell me, I don\'t know',
        'Don\'t know, don\'t know, don\'t know, I don\'t know!',
        'Don\'t know, don\'t know, don\'t know...',
    ];

    protected function setUp() : void
    {
        parent::setUp();

        if (! Type::hasType(GH7837LineTextType::class)) {
            Type::addType(GH7837LineTextType::class, GH7837LineTextType::class);
        }

        $this->setUpEntitySchema([GH7837Line::class, GH7837LineAuthor::class]);

        foreach (self::SONG as $index => $line) {
            $this->_em->persist(new GH7837Line(GH7837LineText::fromText($line), $index));
        }

        $this->_em->flush();
    }

    /** @test */
    public function paginatorWithQueryBuilder() : void // fails
    {
        $query = $this->_em->getRepository(GH7837Line::class)
            ->createQueryBuilder('l')
            ->orderBy('l.lineNumber', Criteria::ASC);

        $paginator = new Paginator($query);

        $query->setFirstResult(6);

        self::assertCount(0, iterator_to_array($paginator));

        $query->setFirstResult(5);

        $result = iterator_to_array($paginator);
        self::assertCount(1, $result);
        self::assertSame(self::SONG[5], $result[0]->toString());
    }

    /** @test */
    public function paginatorWithQueryFromQueryBuilder() : void // passes
    {
        $query = $this->_em->getRepository(GH7837Line::class)
            ->createQueryBuilder('l')
            ->orderBy('l.lineNumber', Criteria::ASC)
            ->getQuery();

        $paginator = new Paginator($query);

        $query->setFirstResult(6);

        self::assertCount(0, iterator_to_array($paginator));

        $query->setFirstResult(5);

        $result = iterator_to_array($paginator);
        self::assertCount(1, $result);
        self::assertSame(self::SONG[5], $result[0]->toString());
    }

    /** @test */
    public function paginatorWithDQL() : void // passes
    {
        $query = $this->_em->createQuery('SELECT l FROM ' . GH7837Line::class . ' l ORDER BY l.lineNumber ASC');

        $paginator = new Paginator($query);

        $query->setFirstResult(6);

        self::assertCount(0, iterator_to_array($paginator));

        $query->setFirstResult(5);

        $result = iterator_to_array($paginator);
        self::assertCount(1, $result);
        self::assertSame(self::SONG[5], $result[0]->toString());
    }
}

/** @Entity */
class GH7837LineAuthor
{
    /**
     * @Id
     * @Column(type="integer")
     *
     * @var int
     */
    public $id;

    /**
     * @Column
     *
     * @var string
     */
    public $name;

    /**
     * @ManyToOne(targetEntity=GH7837Line::class)
     * @JoinColumn(referencedColumnName="text")
     *
     * @var GH7837Line
     */
    public $line;

    public function __construct(int $id, string $name, GH7837Line $line)
    {
        $this->id = $id;
        $this->name = $name;
        $this->line = $line;
    }
}

/** @Entity */
class GH7837Line
{
    /**
     * @var GH7837LineText
     * @Id()
     * @Column(type="Doctrine\Tests\ORM\Functional\Ticket\GH7837LineTextType")
     */
    private $text;

    /**
     * @var int
     * @Column(type="integer")
     */
    private $lineNumber;

    public function __construct(GH7837LineText $text, int $index)
    {
        $this->text       = $text;
        $this->lineNumber = $index;
    }

    public function toString() : string
    {
        return $this->text->getText();
    }
}

final class GH7837LineText
{
    /** @var string */
    private $text;

    private function __construct(string $text)
    {
        $this->text = $text;
    }

    public static function fromText(string $text) : self
    {
        return new self($text);
    }

    public function getText() : string
    {
        return $this->text;
    }

    public function __toString() : string
    {
        return 'Line: ' . $this->text;
    }
}

final class GH7837LineTextType extends StringType
{
    public function convertToPHPValue($value, AbstractPlatform $platform)
    {
        $text = parent::convertToPHPValue($value, $platform);

        if (! is_string($text)) {
            return $text;
        }

        return GH7837LineText::fromText($text);
    }

    public function convertToDatabaseValue($value, AbstractPlatform $platform)
    {
        if (! $value instanceof GH7837LineText) {
            return parent::convertToDatabaseValue($value, $platform);
        }

        return parent::convertToDatabaseValue($value->getText(), $platform);
    }

    /** {@inheritdoc} */
    public function getName() : string
    {
        return self::class;
    }
}

[Ocramius]

I will try to restrict this to a more specific use-case 👌

On Wed, Oct 2, 2019, 00:55 Luís Cobucci [e-mail] wrote:

@Ocramius https://github.com/Ocramius this is an interesting one (which
I didn't manage to reproduce in the way you described)...

As far as I've seen the Paginator doesn't influence the original query
state because it clones it before applying the changes.

However, things get a bit weird when using (and changing) a query builder
instead of a query object. This happens because QueryBuilder#getQuery()
always returns a new query object:

* * @group GH7837 */final class GH7837Test extends OrmFunctionalTestCase{ private const SONG = [ 'What is this song all about?', 'Can\'t figure any lyrics out', 'How do the words to it go?', 'I wish you\'d tell me, I don\'t know', 'Don\'t know, don\'t know, don\'t know, I don\'t know!', 'Don\'t know, don\'t know, don\'t know...', ]; protected function setUp() : void { parent::setUp(); if (! Type::hasType(GH7837LineTextType::class)) { Type::addType(GH7837LineTextType::class, GH7837LineTextType::class); } $this->setUpEntitySchema([GH7837Line::class, GH7837LineAuthor::class]); foreach (self::SONG as $index => $line) { $this->_em->persist(new GH7837Line(GH7837LineText::fromText($line), $index)); } $this->_em->flush(); } /* @test / public function paginatorWithQueryBuilder() : void // fails { $query = $this->_em->getRepository(GH7837Line::class) ->createQueryBuilder('l') ->orderBy('l.lineNumber', Criteria::ASC); $paginator = new Paginator($query); $query->setFirstResult(6); self::assertCount(0, iterator_to_array($paginator)); $query->setFirstResult(5); $result = iterator_to_array($paginator); self::assertCount(1, $result); self::assertSame(self::SONG[5], $result[0]->toString()); } /* @test / public function paginatorWithQueryFromQueryBuilder() : void // passes { $query = $this->_em->getRepository(GH7837Line::class) ->createQueryBuilder('l') ->orderBy('l.lineNumber', Criteria::ASC) ->getQuery(); $paginator = new Paginator($query); $query->setFirstResult(6); self::assertCount(0, iterator_to_array($paginator)); $query->setFirstResult(5); $result = iterator_to_array($paginator); self::assertCount(1, $result); self::assertSame(self::SONG[5], $result[0]->toString()); } /* @test / public function paginatorWithDQL() : void // passes { $query = $this->_em->createQuery('SELECT l FROM ' . GH7837Line::class . ' l ORDER BY l.lineNumber ASC'); $paginator = new Paginator($query); $query->setFirstResult(6); self::assertCount(0, iterator_to_array($paginator)); $query->setFirstResult(5); $result = iterator_to_array($paginator); self::assertCount(1, $result); self::assertSame(self::SONG[5], $result[0]->toString()); }}/* @Entity /class GH7837LineAuthor{ /* * @Id * @Column(type="integer") * * @var int / public $id; /* * @Column * * @var string / public $name; /* * @ManyToOne(targetEntity=GH7837Line::class) * @JoinColumn(referencedColumnName="text") * * @var GH7837Line / public $line; public function __construct(int $id, string $name, GH7837Line $line) { $this->id = $id; $this->name = $name; $this->line = $line; }}/* @Entity /class GH7837Line{ /* * @var GH7837LineText * @Id() * @Column(type="Doctrine\Tests\ORM\Functional\Ticket\GH7837LineTextType") / private $text; /* * @var int * @Column(type="integer") / private $lineNumber; public function __construct(GH7837LineText $text, int $index) { $this->text = $text; $this->lineNumber = $index; } public function toString() : string { return $this->text->getText(); }}final class GH7837LineText{ /* @var string / private $text; private function __construct(string $text) { $this->text = $text; } public static function fromText(string $text) : self { return new self($text); } public function getText() : string { return $this->text; } public function __toString() : string { return 'Line: ' . $this->text; }}final class GH7837LineTextType extends StringType{ public function convertToPHPValue($value, AbstractPlatform $platform) { $text = parent::convertToPHPValue($value, $platform); if (! is_string($text)) { return $text; } return GH7837LineText::fromText($text); } public function convertToDatabaseValue($value, AbstractPlatform $platform) { if (! $value instanceof GH7837LineText) { return parent::convertToDatabaseValue($value, $platform); } return parent::convertToDatabaseValue($value->getText(), $platform); } /* {@inheritdoc} */ public function getName() : string { return self::class; }}

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/doctrine/orm/issues/7837?email_source=notifications&email_token=AABFVEAQ3TLQJI54BLDJ7E3QMPIOXA5CNFSM4I3JPGWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEADAKVI#issuecomment-537265493,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABFVEGNSZTGAPIBQTKPKALQMPIOXANCNFSM4I3JPGWA
.

[ofriedrich]

Maybe this is interesting too: https://github.com/KnpLabs/KnpPaginatorBundle/issues/578

[Ocramius]

Ok, the bug is related to caching: the WhereInWalker is only hit if the query is not present in the query cache.

If there's a cache hit for the parser result, then the walker is not reached, and parameters aren't converted.

I'll write up a test case tonight.

[akorz]

I'm using ramsey/uuid and after my latest dependecy update I hit that error. I tried to add your fix in my vendor code and run my tests. No luck. the same errors :(

[Ocramius]

Have you tried the related patch?

[akorz]

Do you mean this https://github.com/doctrine/orm/pull/7865/commits/023e94661a1956215a6ddf17cb1b8908274b0d01 ? Yes, I added that line in my code and problem is still there :(

[Ocramius]

@akorz can you try distilling that into a test case then, please? I think you hit a different scenario...

[akorz]

@Ocramius let me figure out how to do it

[akorz]

@Ocramius I tried to adapt existed test for enities with custom id. Here is my test https://github.com/akorz/orm/commit/a6ce54330eb69afdc66c8dbc03b7c795262a2804

And the result error is a different what I get on real project, but probably it's somehow releted

There was 1 error:

1) Doctrine\Tests\ORM\Functional\PaginationCustomTypeIdTest::testQueryWalkerIsKept
Exception: [PHPUnit\Framework\Error\Notice] Trying to get property 'id' of non-object

With queries:
198. SQL: 'SELECT DISTINCT c0_.id AS id_0 FROM cms_users_customid c0_ WHERE c0_.name = 'Name1'' Params: 
197. SQL: '"COMMIT"' Params: 
196. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
195. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
194. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
193. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
192. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
191. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
190. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
189. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
188. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
187. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
186. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
185. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
184. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
183. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
182. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
181. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
180. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
179. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
178. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
177. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
176. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
175. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject
174. SQL: 'INSERT INTO cms_users_groups_customid (user_id, group_id) VALUES (?, ?)' Params: Doctrine\Tests\DbalTypes\CustomIdObject, Doctrine\Tests\DbalTypes\CustomIdObject

Trace:
/app/tests/Doctrine/Tests/DbalTypes/CustomIdObjectType.php:18
/app/vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:1499
/app/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php:181
/app/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php:182
/app/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php:119
/app/lib/Doctrine/ORM/Query/TreeWalkerChain.php:113
/app/lib/Doctrine/ORM/Query/Parser.php:389
/app/lib/Doctrine/ORM/Query.php:286
/app/lib/Doctrine/ORM/Query.php:298
/app/lib/Doctrine/ORM/AbstractQuery.php:968
/app/lib/Doctrine/ORM/AbstractQuery.php:923
/app/lib/Doctrine/ORM/AbstractQuery.php:726
/app/lib/Doctrine/ORM/Tools/Pagination/Paginator.php:170
/app/tests/Doctrine/Tests/ORM/Functional/PaginationCustomTypeIdTest.php:641
/app/vendor/phpunit/phpunit/src/Framework/TestCase.php:1154
/app/vendor/phpunit/phpunit/src/Framework/TestCase.php:842
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:693
/app/vendor/phpunit/phpunit/src/Framework/TestCase.php:796
/app/vendor/phpunit/phpunit/src/Framework/TestSuite.php:746
/app/vendor/phpunit/phpunit/src/Framework/TestSuite.php:746
/app/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:652
/app/vendor/phpunit/phpunit/src/TextUI/Command.php:206
/app/vendor/phpunit/phpunit/src/TextUI/Command.php:162
/app/vendor/phpunit/phpunit/phpunit:61


/app/tests/Doctrine/Tests/OrmFunctionalTestCase.php:842

Caused by
PHPUnit\Framework\Error\Notice: Trying to get property 'id' of non-object

/app/tests/Doctrine/Tests/DbalTypes/CustomIdObjectType.php:18
/app/vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:1499
/app/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php:181
/app/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php:182
/app/lib/Doctrine/ORM/Tools/Pagination/WhereInWalker.php:119
/app/lib/Doctrine/ORM/Query/TreeWalkerChain.php:113
/app/lib/Doctrine/ORM/Query/Parser.php:389
/app/lib/Doctrine/ORM/Query.php:286
/app/lib/Doctrine/ORM/Query.php:298
/app/lib/Doctrine/ORM/AbstractQuery.php:968
/app/lib/Doctrine/ORM/AbstractQuery.php:923
/app/lib/Doctrine/ORM/AbstractQuery.php:726
/app/lib/Doctrine/ORM/Tools/Pagination/Paginator.php:170
/app/tests/Doctrine/Tests/ORM/Functional/PaginationCustomTypeIdTest.php:641

--

[Ocramius]

That diff is gigantic: can you reduce it to just the failure?

[lcobucci]

And also try to keep the entities in the same file as the test :+1:

[akorz]

@lcobucci What do you mean? I made it in the same way how did pagination test for entities with autogenerated id

[akorz]

@Ocramius I got that in

    public function testQueryWalkerIsKept()
    {
        $dql = 'SELECT u FROM Doctrine\Tests\Models\CMSCustomId\CmsUser u';
        $query = $this->_em->createQuery($dql);
        $query->setHint(Query::HINT_CUSTOM_TREE_WALKERS, [CustomPaginationTestTreeWalker::class]);

        $paginator = new Paginator($query, true);
        $paginator->setUseOutputWalkers(false);
        $this->assertCount(1, $paginator->getIterator());
        $this->assertEquals(1, $paginator->count());
    }

I guess bug came from #7820
Pay attention that it test uses Doctrine\Tests\Models\CMSCustomId\CmsUser

Where
class CmsUser { /** * @Id * @Column(type="CustomIdObject") * @GeneratedValue(strategy="NONE") */ public $id;

[akorz]

I tried to simple delete that code
https://github.com/doctrine/orm/pull/7821/files#diff-1a0c7b79092b175a78f64ed6c44c8c8cR113

My test passed.

So, looks like it's a reason for https://github.com/doctrine/orm/pull/7821#issuecomment-541089381 and why https://github.com/doctrine/orm/pull/7865 did not help me

[akorz]

@Ocramius had you get a chance to check it out?

[Ocramius]

@akorz can you open a PR with the failing test? Also, what if https://github.com/doctrine/orm/pull/7865 is factored in?

[lcobucci]

@lcobucci What do you mean? I made it in the same way how did pagination test for entities with autogenerated id

@akorz the link you sent has entities and test in separate files. For the sake of isolation and easier reproducibility we're asking to always create new entities in the same file as the test.

[akorz]

@Ocramius sorry for delay. Pull is here https://github.com/doctrine/orm/pull/7886 I hope I made it correct

@lcobucci I see. Of course I can do it, but I though that models in tests/Doctrine/Tests/Models/CMS were created with some intention behind. Thats why I did it similar.

[lcobucci]

Handled by https://github.com/doctrine/orm/pull/7865