From 60b6073643e8fdf33efeb98148632dbb75e4059d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Kurzeja?= Date: Fri, 9 Dec 2016 21:48:43 +0100 Subject: [PATCH 1/7] Fixes #6167 - nextval issue on master-slave PostgreSQL setup --- lib/Doctrine/ORM/Id/SequenceGenerator.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Id/SequenceGenerator.php b/lib/Doctrine/ORM/Id/SequenceGenerator.php index a27edae90..18d238499 100644 --- a/lib/Doctrine/ORM/Id/SequenceGenerator.php +++ b/lib/Doctrine/ORM/Id/SequenceGenerator.php @@ -76,7 +76,7 @@ class SequenceGenerator extends AbstractIdGenerator implements Serializable $conn = $em->getConnection(); $sql = $conn->getDatabasePlatform()->getSequenceNextValSQL($this->_sequenceName); - $this->_nextValue = (int) $conn->fetchColumn($sql); + $this->_nextValue = (int) $conn->query($sql)->fetchColumn(0); $this->_maxValue = $this->_nextValue + $this->_allocationSize; } From edffb4d44914761f2b9e0d17af942ad0b5d2b1d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Kurzeja?= Date: Mon, 12 Dec 2016 07:45:25 +0100 Subject: [PATCH 2/7] #6167 - fixed tests and added info why query is used in SequenceGenerator --- lib/Doctrine/ORM/Id/SequenceGenerator.php | 3 ++- tests/Doctrine/Tests/Mocks/ConnectionMock.php | 22 +++++++++++++++++++ .../Tests/ORM/Id/SequenceGeneratorTest.php | 6 ++--- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/lib/Doctrine/ORM/Id/SequenceGenerator.php b/lib/Doctrine/ORM/Id/SequenceGenerator.php index 18d238499..7db8e9083 100644 --- a/lib/Doctrine/ORM/Id/SequenceGenerator.php +++ b/lib/Doctrine/ORM/Id/SequenceGenerator.php @@ -76,7 +76,8 @@ class SequenceGenerator extends AbstractIdGenerator implements Serializable $conn = $em->getConnection(); $sql = $conn->getDatabasePlatform()->getSequenceNextValSQL($this->_sequenceName); - $this->_nextValue = (int) $conn->query($sql)->fetchColumn(0); + // Use query to force master in MasterSlaveConnection + $this->_nextValue = (int) $conn->query($sql)->fetchColumn(); $this->_maxValue = $this->_nextValue + $this->_allocationSize; } diff --git a/tests/Doctrine/Tests/Mocks/ConnectionMock.php b/tests/Doctrine/Tests/Mocks/ConnectionMock.php index 5a28ffa5a..11d08d480 100644 --- a/tests/Doctrine/Tests/Mocks/ConnectionMock.php +++ b/tests/Doctrine/Tests/Mocks/ConnectionMock.php @@ -2,6 +2,7 @@ namespace Doctrine\Tests\Mocks; use Doctrine\DBAL\Connection; +use Doctrine\DBAL\Driver\Statement; /** * Mock class for Connection. @@ -13,6 +14,11 @@ class ConnectionMock extends Connection */ private $_fetchOneResult; + /** + * @var Statement + */ + private $_queryResult; + /** * @var DatabasePlatformMock */ @@ -89,6 +95,14 @@ class ConnectionMock extends Connection return $this->_fetchOneResult; } + /** + * {@inheritdoc} + */ + public function query() + { + return $this->_queryResult; + } + /** * {@inheritdoc} */ @@ -132,6 +146,14 @@ class ConnectionMock extends Connection $this->_lastInsertId = $id; } + /** + * @param Statement $result + */ + public function setQueryResult($result) + { + $this->_queryResult = $result; + } + /** * @return array */ diff --git a/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php b/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php index 5d7f7a59b..c87547934 100644 --- a/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php +++ b/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php @@ -3,6 +3,7 @@ namespace Doctrine\Tests\ORM\Id; use Doctrine\ORM\Id\SequenceGenerator; +use Doctrine\Tests\Mocks\StatementArrayMock; use Doctrine\Tests\OrmTestCase; /** @@ -25,15 +26,14 @@ class SequenceGeneratorTest extends OrmTestCase { for ($i=0; $i < 42; ++$i) { if ($i % 10 == 0) { - $this->_em->getConnection()->setFetchOneResult((int)($i / 10) * 10); + $nextId = array(array((int)($i / 10) * 10)); + $this->_em->getConnection()->setQueryResult(new StatementArrayMock($nextId)); } $id = $this->_seqGen->generate($this->_em, null); $this->assertEquals($i, $id); $this->assertEquals((int)($i / 10) * 10 + 10, $this->_seqGen->getCurrentMaxValue()); $this->assertEquals($i + 1, $this->_seqGen->getNextValue()); } - - } } From 71b040c8497bcf5f60c2ad120d5a8283e4e41bdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Kurzeja?= Date: Mon, 12 Dec 2016 12:29:29 +0100 Subject: [PATCH 3/7] #6167 - tests - throw exception if wrong method used to get sequence nextval --- tests/Doctrine/Tests/Mocks/ConnectionMock.php | 19 +++++++++++++++++++ .../Tests/ORM/Id/SequenceGeneratorTest.php | 4 ++++ 2 files changed, 23 insertions(+) diff --git a/tests/Doctrine/Tests/Mocks/ConnectionMock.php b/tests/Doctrine/Tests/Mocks/ConnectionMock.php index 11d08d480..ba0be9ee4 100644 --- a/tests/Doctrine/Tests/Mocks/ConnectionMock.php +++ b/tests/Doctrine/Tests/Mocks/ConnectionMock.php @@ -14,6 +14,11 @@ class ConnectionMock extends Connection */ private $_fetchOneResult; + /** + * @var \Exception + */ + private $_fetchOneException; + /** * @var Statement */ @@ -92,6 +97,10 @@ class ConnectionMock extends Connection */ public function fetchColumn($statement, array $params = [], $colnum = 0, array $types = []) { + if ($this->_fetchOneException != null) { + throw $this->_fetchOneException; + } + return $this->_fetchOneResult; } @@ -126,6 +135,16 @@ class ConnectionMock extends Connection $this->_fetchOneResult = $fetchOneResult; } + /** + * @param \Exception $exception + * + * @return void + */ + public function setFetchOneException(\Exception $exception = null) + { + $this->_fetchOneException = $exception; + } + /** * @param \Doctrine\DBAL\Platforms\AbstractPlatform $platform * diff --git a/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php b/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php index c87547934..b972d0640 100644 --- a/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php +++ b/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php @@ -24,6 +24,10 @@ class SequenceGeneratorTest extends OrmTestCase public function testGeneration() { + $this->_em->getConnection()->setFetchOneException( + new \Exception('Fetch* method used. Query method should be used instead, as NEXTVAL should be run on a master server in master-slave setup.') + ); + for ($i=0; $i < 42; ++$i) { if ($i % 10 == 0) { $nextId = array(array((int)($i / 10) * 10)); From 571115cf18be0f63edb4868fe03f13f4cb8297e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Kurzeja?= Date: Mon, 12 Dec 2016 20:03:25 +0100 Subject: [PATCH 4/7] #6167 Code review updates, better readability --- tests/Doctrine/Tests/Mocks/ConnectionMock.php | 6 +++--- tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/Doctrine/Tests/Mocks/ConnectionMock.php b/tests/Doctrine/Tests/Mocks/ConnectionMock.php index ba0be9ee4..0b5685d2a 100644 --- a/tests/Doctrine/Tests/Mocks/ConnectionMock.php +++ b/tests/Doctrine/Tests/Mocks/ConnectionMock.php @@ -97,7 +97,7 @@ class ConnectionMock extends Connection */ public function fetchColumn($statement, array $params = [], $colnum = 0, array $types = []) { - if ($this->_fetchOneException != null) { + if (null !== $this->_fetchOneException) { throw $this->_fetchOneException; } @@ -136,7 +136,7 @@ class ConnectionMock extends Connection } /** - * @param \Exception $exception + * @param \Exception|null $exception * * @return void */ @@ -168,7 +168,7 @@ class ConnectionMock extends Connection /** * @param Statement $result */ - public function setQueryResult($result) + public function setQueryResult(Statement $result) { $this->_queryResult = $result; } diff --git a/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php b/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php index b972d0640..f7ec2ffc7 100644 --- a/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php +++ b/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php @@ -25,12 +25,12 @@ class SequenceGeneratorTest extends OrmTestCase public function testGeneration() { $this->_em->getConnection()->setFetchOneException( - new \Exception('Fetch* method used. Query method should be used instead, as NEXTVAL should be run on a master server in master-slave setup.') + new \RuntimeException('Fetch* method used. Query method should be used instead, as NEXTVAL should be run on a master server in master-slave setup.') ); for ($i=0; $i < 42; ++$i) { if ($i % 10 == 0) { - $nextId = array(array((int)($i / 10) * 10)); + $nextId = [[(int)($i / 10) * 10]]; $this->_em->getConnection()->setQueryResult(new StatementArrayMock($nextId)); } $id = $this->_seqGen->generate($this->_em, null); From d2be4a2b484073bceaf1cc540c80a2ca45a3c90a Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Wed, 21 Jun 2017 05:44:58 +0200 Subject: [PATCH 5/7] #6167 #6168 - clarifying on the reasoning why `query` is used instead of `fetchColumn` --- lib/Doctrine/ORM/Id/SequenceGenerator.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Id/SequenceGenerator.php b/lib/Doctrine/ORM/Id/SequenceGenerator.php index 7db8e9083..9d8e9eb75 100644 --- a/lib/Doctrine/ORM/Id/SequenceGenerator.php +++ b/lib/Doctrine/ORM/Id/SequenceGenerator.php @@ -76,7 +76,7 @@ class SequenceGenerator extends AbstractIdGenerator implements Serializable $conn = $em->getConnection(); $sql = $conn->getDatabasePlatform()->getSequenceNextValSQL($this->_sequenceName); - // Use query to force master in MasterSlaveConnection + // Using `query` to force usage of the master server in MasterSlaveConnection $this->_nextValue = (int) $conn->query($sql)->fetchColumn(); $this->_maxValue = $this->_nextValue + $this->_allocationSize; } From 462481ebbe1d57eff6bb39314704e788816f87f8 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Wed, 21 Jun 2017 05:47:19 +0200 Subject: [PATCH 6/7] #6167 #6168 - better connection mock documentation/docblocks/return-types --- tests/Doctrine/Tests/Mocks/ConnectionMock.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/Doctrine/Tests/Mocks/ConnectionMock.php b/tests/Doctrine/Tests/Mocks/ConnectionMock.php index 0b5685d2a..af8fc252e 100644 --- a/tests/Doctrine/Tests/Mocks/ConnectionMock.php +++ b/tests/Doctrine/Tests/Mocks/ConnectionMock.php @@ -1,6 +1,7 @@ _queryResult; } From a97c2659fc2e731bdc88070dce09dbc2cc03512b Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Wed, 21 Jun 2017 06:04:06 +0200 Subject: [PATCH 7/7] #6167 #6168 rewrote `SequenceGeneratorTest` for better readability and error messages --- .../Tests/ORM/Id/SequenceGeneratorTest.php | 58 ++++++++++++------- 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php b/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php index f7ec2ffc7..5e4247715 100644 --- a/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php +++ b/tests/Doctrine/Tests/ORM/Id/SequenceGeneratorTest.php @@ -2,41 +2,57 @@ namespace Doctrine\Tests\ORM\Id; +use Doctrine\ORM\EntityManager; use Doctrine\ORM\Id\SequenceGenerator; +use Doctrine\Tests\Mocks\ConnectionMock; use Doctrine\Tests\Mocks\StatementArrayMock; use Doctrine\Tests\OrmTestCase; -/** - * Description of SequenceGeneratorTest - * - * @author robo - */ class SequenceGeneratorTest extends OrmTestCase { - private $_em; - private $_seqGen; + /** + * @var EntityManager + */ + private $entityManager; - protected function setUp() + /** + * @var SequenceGenerator + */ + private $sequenceGenerator; + + /** + * @var ConnectionMock + */ + private $connection; + + protected function setUp() : void { - $this->_em = $this->_getTestEntityManager(); - $this->_seqGen = new SequenceGenerator('seq', 10); + parent::setUp(); + + $this->entityManager = $this->_getTestEntityManager(); + $this->sequenceGenerator = new SequenceGenerator('seq', 10); + $this->connection = $this->entityManager->getConnection(); + + self::assertInstanceOf(ConnectionMock::class, $this->connection); } - public function testGeneration() + public function testGeneration() : void { - $this->_em->getConnection()->setFetchOneException( - new \RuntimeException('Fetch* method used. Query method should be used instead, as NEXTVAL should be run on a master server in master-slave setup.') - ); + $this->connection->setFetchOneException(new \BadMethodCallException( + 'Fetch* method used. Query method should be used instead, ' + . 'as NEXTVAL should be run on a master server in master-slave setup.' + )); - for ($i=0; $i < 42; ++$i) { + for ($i = 0; $i < 42; ++$i) { if ($i % 10 == 0) { - $nextId = [[(int)($i / 10) * 10]]; - $this->_em->getConnection()->setQueryResult(new StatementArrayMock($nextId)); + $this->connection->setQueryResult(new StatementArrayMock([[(int)($i / 10) * 10]])); } - $id = $this->_seqGen->generate($this->_em, null); - $this->assertEquals($i, $id); - $this->assertEquals((int)($i / 10) * 10 + 10, $this->_seqGen->getCurrentMaxValue()); - $this->assertEquals($i + 1, $this->_seqGen->getNextValue()); + + $id = $this->sequenceGenerator->generate($this->entityManager, null); + + self::assertSame($i, $id); + self::assertSame((int)($i / 10) * 10 + 10, $this->sequenceGenerator->getCurrentMaxValue()); + self::assertSame($i + 1, $this->sequenceGenerator->getNextValue()); } } }