From 61385f6bc42e92d6114698525a8b2bdc171c26ad Mon Sep 17 00:00:00 2001 From: Dzmitry Bannik Date: Tue, 22 Oct 2024 22:30:39 +0300 Subject: [PATCH 1/2] Is not correctly generated sql when changed/switched sqlFilter parameters CachedPersisterContext::$selectJoinSql should be clear or regenerated when sqlFilter changed The problem reproduce when in use fetch=EAGER and use additional sql filter on this property --- .../Entity/BasicEntityPersister.php | 6 +- .../ChangeFiltersTest.php | 142 ++++++++++++++++++ .../CompanySQLFilter.php | 26 ++++ .../Ticket/SwitchContextWithFilter/Order.php | 43 ++++++ .../Ticket/SwitchContextWithFilter/User.php | 35 +++++ 5 files changed, 251 insertions(+), 1 deletion(-) create mode 100644 tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/ChangeFiltersTest.php create mode 100644 tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/CompanySQLFilter.php create mode 100644 tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/Order.php create mode 100644 tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/User.php diff --git a/src/Persisters/Entity/BasicEntityPersister.php b/src/Persisters/Entity/BasicEntityPersister.php index 1bb54ed441..d5c0db66b4 100644 --- a/src/Persisters/Entity/BasicEntityPersister.php +++ b/src/Persisters/Entity/BasicEntityPersister.php @@ -173,6 +173,9 @@ class BasicEntityPersister implements EntityPersister private readonly CachedPersisterContext $limitsHandlingContext; private readonly CachedPersisterContext $noLimitsContext; + /** @var ?string */ + private $filterHash = null; + /** * Initializes a new BasicEntityPersister that uses the given EntityManager * and persists instances of the class described by the given ClassMetadata descriptor. @@ -1229,7 +1232,7 @@ final protected function getOrderBySQL(array $orderBy, string $baseTableAlias): */ protected function getSelectColumnsSQL(): string { - if ($this->currentPersisterContext->selectColumnListSql !== null) { + if ($this->currentPersisterContext->selectColumnListSql !== null && $this->filterHash === $this->em->getFilters()->getHash()) { return $this->currentPersisterContext->selectColumnListSql; } @@ -1339,6 +1342,7 @@ protected function getSelectColumnsSQL(): string } $this->currentPersisterContext->selectColumnListSql = implode(', ', $columnList); + $this->filterHash = $this->em->getFilters()->getHash(); return $this->currentPersisterContext->selectColumnListSql; } diff --git a/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/ChangeFiltersTest.php b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/ChangeFiltersTest.php new file mode 100644 index 0000000000..7ce97442b2 --- /dev/null +++ b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/ChangeFiltersTest.php @@ -0,0 +1,142 @@ +setUpEntitySchema([ + Order::class, + User::class, + ]); + } + + /** + * @return non-empty-array<"companyA"|"companyB", array{orderId: int, userId: int}> + */ + private function prepareData(): array + { + $user1 = new User(self::COMPANY_A); + $order1 = new Order($user1); + $user2 = new User(self::COMPANY_B); + $order2 = new Order($user2); + + $this->_em->persist($user1); + $this->_em->persist($order1); + $this->_em->persist($user2); + $this->_em->persist($order2); + $this->_em->flush(); + $this->_em->clear(); + + return [ + 'companyA' => ['orderId' => $order1->id, 'userId' => $user1->id], + 'companyB' => ['orderId' => $order2->id, 'userId' => $user2->id], + ]; + } + + public function testUseEnableDisableFilter(): void + { + $this->_em->getConfiguration()->addFilter(CompanySQLFilter::class, CompanySQLFilter::class); + $this->_em->getFilters()->enable(CompanySQLFilter::class)->setParameter('company', self::COMPANY_A); + + ['companyA' => $companyA, 'companyB' => $companyB] = $this->prepareData(); + + $order1 = $this->_em->find(Order::class, $companyA['orderId']); + + self::assertNotNull($order1->user, $this->generateMessage('Order1->User1 not found')); + self::assertEquals($companyA['userId'], $order1->user->id, $this->generateMessage('Order1->User1 != User1')); + + $this->_em->getFilters()->disable(CompanySQLFilter::class); + $this->_em->getFilters()->enable(CompanySQLFilter::class)->setParameter('company', self::COMPANY_B); + + $order2 = $this->_em->find(Order::class, $companyB['orderId']); + + self::assertNotNull($order2->user, $this->generateMessage('Order2->User2 not found')); + self::assertEquals($companyB['userId'], $order2->user->id, $this->generateMessage('Order2->User2 != User2')); + } + + public function testUseChangeFilterParameters(): void + { + $this->_em->getConfiguration()->addFilter(CompanySQLFilter::class, CompanySQLFilter::class); + $filter = $this->_em->getFilters()->enable(CompanySQLFilter::class); + + ['companyA' => $companyA, 'companyB' => $companyB] = $this->prepareData(); + + $filter->setParameter('company', self::COMPANY_A); + + $order1 = $this->_em->find(Order::class, $companyA['orderId']); + + self::assertNotNull($order1->user, $this->generateMessage('Order1->User1 not found')); + self::assertEquals($companyA['userId'], $order1->user->id, $this->generateMessage('Order1->User1 != User1')); + + $filter->setParameter('company', self::COMPANY_B); + + $order2 = $this->_em->find(Order::class, $companyB['orderId']); + + self::assertNotNull($order2->user, $this->generateMessage('Order2->User2 not found')); + self::assertEquals($companyB['userId'], $order2->user->id, $this->generateMessage('Order2->User2 != User2')); + } + + public function testUseQueryBuilder(): void + { + $this->_em->getConfiguration()->addFilter(CompanySQLFilter::class, CompanySQLFilter::class); + $filter = $this->_em->getFilters()->enable(CompanySQLFilter::class); + + ['companyA' => $companyA, 'companyB' => $companyB] = $this->prepareData(); + + $getOrderByIdCache = function (int $orderId): ?Order { + return $this->_em->createQueryBuilder() + ->select('orderMaster, user') + ->from(Order::class, 'orderMaster') + ->innerJoin('orderMaster.user', 'user') + ->where('orderMaster.id = :orderId') + ->setParameter('orderId', $orderId) + ->setCacheable(true) + ->getQuery() + ->setQueryCacheLifetime(10) + ->getOneOrNullResult(); + }; + + $filter->setParameter('company', self::COMPANY_A); + + $order = $getOrderByIdCache($companyB['orderId']); + self::assertNull($order); + + $order = $getOrderByIdCache($companyA['orderId']); + + self::assertInstanceOf(Order::class, $order); + self::assertInstanceOf(User::class, $order->user); + self::assertEquals($companyA['userId'], $order->user->id); + + $filter->setParameter('company', self::COMPANY_B); + + $order = $getOrderByIdCache($companyA['orderId']); + self::assertNull($order); + + $order = $getOrderByIdCache($companyB['orderId']); + + self::assertInstanceOf(Order::class, $order); + self::assertInstanceOf(User::class, $order->user); + self::assertEquals($companyB['userId'], $order->user->id); + } + + private function generateMessage(string $message): string + { + $log = $this->getLastLoggedQuery(); + + return sprintf("%s\nSQL: %s", $message, str_replace(['?'], (array) $log['params'], $log['sql'])); + } +} diff --git a/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/CompanySQLFilter.php b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/CompanySQLFilter.php new file mode 100644 index 0000000000..e65188334a --- /dev/null +++ b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/CompanySQLFilter.php @@ -0,0 +1,26 @@ +getName() === User::class) { + return sprintf('%s.%s = %s', $targetTableAlias, $targetEntity->fieldMappings['company']['fieldName'], $this->getParameter('company')); + } + + if ($targetEntity->getName() === Order::class) { + return sprintf('%s.%s = %s', $targetTableAlias, $targetEntity->fieldMappings['company']['fieldName'], $this->getParameter('company')); + } + + return ''; + } +} diff --git a/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/Order.php b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/Order.php new file mode 100644 index 0000000000..a6d86dca8a --- /dev/null +++ b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/Order.php @@ -0,0 +1,43 @@ +user = $user; + $this->company = $user->company; + } +} diff --git a/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/User.php b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/User.php new file mode 100644 index 0000000000..294bfdf87a --- /dev/null +++ b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/User.php @@ -0,0 +1,35 @@ +company = $company; + } +} From 69ef7f846a80c2dcd9c481042ab08f8ec5afcb1e Mon Sep 17 00:00:00 2001 From: Dzmitry Bannik Date: Wed, 23 Oct 2024 12:21:44 +0300 Subject: [PATCH 2/2] #11694 - update to 3.3.x --- .../Entity/BasicEntityPersister.php | 3 +- .../ChangeFiltersTest.php | 6 ++-- .../CompanySQLFilter.php | 6 ++-- .../Ticket/SwitchContextWithFilter/Order.php | 34 ++++++------------- .../Ticket/SwitchContextWithFilter/User.php | 26 +++++--------- 5 files changed, 24 insertions(+), 51 deletions(-) diff --git a/src/Persisters/Entity/BasicEntityPersister.php b/src/Persisters/Entity/BasicEntityPersister.php index d5c0db66b4..843cc0b21f 100644 --- a/src/Persisters/Entity/BasicEntityPersister.php +++ b/src/Persisters/Entity/BasicEntityPersister.php @@ -173,8 +173,7 @@ class BasicEntityPersister implements EntityPersister private readonly CachedPersisterContext $limitsHandlingContext; private readonly CachedPersisterContext $noLimitsContext; - /** @var ?string */ - private $filterHash = null; + private string|null $filterHash = null; /** * Initializes a new BasicEntityPersister that uses the given EntityManager diff --git a/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/ChangeFiltersTest.php b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/ChangeFiltersTest.php index 7ce97442b2..df48e013d1 100644 --- a/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/ChangeFiltersTest.php +++ b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/ChangeFiltersTest.php @@ -24,9 +24,7 @@ public function setUp(): void ]); } - /** - * @return non-empty-array<"companyA"|"companyB", array{orderId: int, userId: int}> - */ + /** @return non-empty-array<"companyA"|"companyB", array{orderId: int, userId: int}> */ private function prepareData(): array { $user1 = new User(self::COMPANY_A); @@ -97,7 +95,7 @@ public function testUseQueryBuilder(): void ['companyA' => $companyA, 'companyB' => $companyB] = $this->prepareData(); - $getOrderByIdCache = function (int $orderId): ?Order { + $getOrderByIdCache = function (int $orderId): Order|null { return $this->_em->createQueryBuilder() ->select('orderMaster, user') ->from(Order::class, 'orderMaster') diff --git a/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/CompanySQLFilter.php b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/CompanySQLFilter.php index e65188334a..32fe98ea2b 100644 --- a/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/CompanySQLFilter.php +++ b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/CompanySQLFilter.php @@ -11,14 +11,14 @@ class CompanySQLFilter extends SQLFilter { - public function addFilterConstraint(ClassMetadata $targetEntity, $targetTableAlias): string + public function addFilterConstraint(ClassMetadata $targetEntity, string $targetTableAlias): string { if ($targetEntity->getName() === User::class) { - return sprintf('%s.%s = %s', $targetTableAlias, $targetEntity->fieldMappings['company']['fieldName'], $this->getParameter('company')); + return sprintf('%s.%s = %s', $targetTableAlias, $targetEntity->fieldMappings['company']->fieldName, $this->getParameter('company')); } if ($targetEntity->getName() === Order::class) { - return sprintf('%s.%s = %s', $targetTableAlias, $targetEntity->fieldMappings['company']['fieldName'], $this->getParameter('company')); + return sprintf('%s.%s = %s', $targetTableAlias, $targetEntity->fieldMappings['company']->fieldName, $this->getParameter('company')); } return ''; diff --git a/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/Order.php b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/Order.php index a6d86dca8a..57bbfda480 100644 --- a/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/Order.php +++ b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/Order.php @@ -6,34 +6,20 @@ use Doctrine\ORM\Mapping as ORM; -/** - * @ORM\Entity - * @ORM\Table(name="Order_Master") - */ +#[ORM\Entity] +#[ORM\Table('Order_Master')] class Order { - /** - * @ORM\Id - * @ORM\Column(type="integer") - * @ORM\GeneratedValue(strategy="AUTO") - * - * @var int - */ - public $id; + #[ORM\Id] + #[ORM\Column(type: 'integer')] + #[ORM\GeneratedValue(strategy: 'AUTO')] + public int $id; - /** - * @ORM\Column(type="string") - * - * @var string - */ - public $company; + #[ORM\Column(type: 'string')] + public string $company; - /** - * @ORM\ManyToOne(targetEntity="User", fetch="EAGER") - * - * @var User - */ - public $user; + #[ORM\ManyToOne(targetEntity: User::class, fetch: 'EAGER')] + public User $user; public function __construct(User $user) { diff --git a/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/User.php b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/User.php index 294bfdf87a..6c3ceda127 100644 --- a/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/User.php +++ b/tests/Tests/ORM/Functional/Ticket/SwitchContextWithFilter/User.php @@ -6,27 +6,17 @@ use Doctrine\ORM\Mapping as ORM; -/** - * @ORM\Entity - * @ORM\Table(name="User_Master") - */ +#[ORM\Entity] +#[ORM\Table('User_Master')] class User { - /** - * @ORM\Id - * @ORM\Column(type="integer") - * @ORM\GeneratedValue(strategy="AUTO") - * - * @var int - */ - public $id; + #[ORM\Id] + #[ORM\Column(type: 'integer')] + #[ORM\GeneratedValue(strategy: 'AUTO')] + public int $id; - /** - * @ORM\Column(type="string") - * - * @var string - */ - public $company; + #[ORM\Column(type: 'string')] + public string $company; public function __construct(string $company) {