From 6591c9860f7db351147b1ae7b38cedcae7dfdc4a Mon Sep 17 00:00:00 2001 From: Jan Skrasek Date: Thu, 11 Apr 2024 15:14:50 +0200 Subject: [PATCH] fix & reimplement getLastInsertedId for various sequence names --- src/Drivers/PdoPgsql/PdoPgsqlDriver.php | 24 ++-- src/Drivers/Pgsql/PgsqlDriver.php | 11 +- src/IConnection.php | 15 +++ .../integration/connection.postgres.phpt | 6 +- tests/cases/unit/CachedPlatformTest.phpt | 126 ++++++++---------- 5 files changed, 95 insertions(+), 87 deletions(-) diff --git a/src/Drivers/PdoPgsql/PdoPgsqlDriver.php b/src/Drivers/PdoPgsql/PdoPgsqlDriver.php index 17f416d..eb73a79 100644 --- a/src/Drivers/PdoPgsql/PdoPgsqlDriver.php +++ b/src/Drivers/PdoPgsql/PdoPgsqlDriver.php @@ -76,18 +76,19 @@ public function createPlatform(IConnection $connection): IPlatform public function getLastInsertedId(string|Fqn|null $sequenceName = null): mixed { if ($sequenceName === null) { - throw new InvalidArgumentException('PgsqlDriver requires to pass sequence name for getLastInsertedId() method.'); + throw new InvalidArgumentException('PgsqlDriver requires passing a sequence name for getLastInsertedId() method.'); } $this->checkConnection(); assert($this->connection !== null); - $sequenceName = match (true) { - $sequenceName instanceof Fqn => $this->convertIdentifierToSql($sequenceName->schema) . '.' . - $this->convertIdentifierToSql($sequenceName->name), - default => $this->convertIdentifierToSql($sequenceName), - }; - $sql = 'SELECT CURRVAL(\'' . $sequenceName . '\')'; + if ($sequenceName instanceof Fqn) { + $sequenceName = $this->convertIdentifierToSql($sequenceName); + $sql = 'SELECT CURRVAL(\'' . $sequenceName . '\')'; + } else { + $sequenceName = $this->convertStringToSql($sequenceName); + $sql = "SELECT CURRVAL($sequenceName)"; + } return $this->loggedQuery($sql)->fetchField(); } @@ -117,12 +118,11 @@ protected function createResultAdapter(PDOStatement $statement): IResultAdapter protected function convertIdentifierToSql(string|Fqn $identifier): string { - $escaped = match (true) { - $identifier instanceof Fqn => str_replace('"', '""', $identifier->schema) . '.' - . str_replace('"', '""', $identifier->name), - default => str_replace('"', '""', $identifier), + return match (true) { + $identifier instanceof Fqn => '"' . str_replace('"', '""', $identifier->schema) . '"."' + . str_replace('"', '""', $identifier->name) . '"', + default => '"' . str_replace('"', '""', $identifier) . '"', }; - return '"' . $escaped . '"'; } diff --git a/src/Drivers/Pgsql/PgsqlDriver.php b/src/Drivers/Pgsql/PgsqlDriver.php index 811e991..dc1fc53 100644 --- a/src/Drivers/Pgsql/PgsqlDriver.php +++ b/src/Drivers/Pgsql/PgsqlDriver.php @@ -182,13 +182,18 @@ public function query(string $query): Result public function getLastInsertedId(string|Fqn|null $sequenceName = null): mixed { if ($sequenceName === null) { - throw new InvalidArgumentException('PgsqlDriver requires to pass sequence name for getLastInsertedId() method.'); + throw new InvalidArgumentException('PgsqlDriver requires passing a sequence name for getLastInsertedId() method.'); } $this->checkConnection(); assert($this->connection !== null); - $sequenceName = $this->convertIdentifierToSql($sequenceName); - $sql = 'SELECT CURRVAL(\'' . $sequenceName . '\')'; + if ($sequenceName instanceof Fqn) { + $sequenceName = $this->convertIdentifierToSql($sequenceName); + $sql = 'SELECT CURRVAL(\'' . $sequenceName . '\')'; + } else { + $sequenceName = $this->convertStringToSql($sequenceName); + $sql = "SELECT CURRVAL($sequenceName)"; + } return $this->loggedQuery($sql)->fetchField(); } diff --git a/src/IConnection.php b/src/IConnection.php index b0ce601..144c458 100644 --- a/src/IConnection.php +++ b/src/IConnection.php @@ -7,6 +7,9 @@ use Nextras\Dbal\Drivers\Exception\DriverException; use Nextras\Dbal\Drivers\Exception\QueryException; use Nextras\Dbal\Drivers\IDriver; +use Nextras\Dbal\Drivers\PdoPgsql\PdoPgsqlDriver; +use Nextras\Dbal\Drivers\Pgsql\PgsqlDriver; +use Nextras\Dbal\Platforms\Data\Column; use Nextras\Dbal\Platforms\Data\Fqn; use Nextras\Dbal\Platforms\IPlatform; use Nextras\Dbal\QueryBuilder\QueryBuilder; @@ -86,6 +89,18 @@ public function queryByQueryBuilder(QueryBuilder $queryBuilder): Result; /** * Returns last inserted ID. + * + * The sequence name's implementation depends on a particular database platform and driver. + * + * This method accepts the very same value obtained through platform reflection, e.g., through + * {@see IPlatform::getLastInsertedId} or alternatively through {@see IPlatform::getColumns()} and + * its {@see Column::$meta} property: `$column->meta['sequence']`. + * + * In case of {@see PgsqlDriver} or {@see PdoPgsqlDriver} the name is a string that may + * container double-quotes for handling cases sensitive names or names with special characters. + * I.e. `public."MySchemaName"` is a valid sequence name argument. Alternatively, you may pass a Fqn instance + * that will properly double-quote the schema and name. + * * @return int|string|null */ public function getLastInsertedId(string|Fqn|null $sequenceName = null); diff --git a/tests/cases/integration/connection.postgres.phpt b/tests/cases/integration/connection.postgres.phpt index 010e2e3..68c7ae8 100644 --- a/tests/cases/integration/connection.postgres.phpt +++ b/tests/cases/integration/connection.postgres.phpt @@ -33,11 +33,13 @@ class ConnectionPostgresTest extends IntegrationTestCase $this->connection->query('INSERT INTO publishers %values', ['name' => 'FOO']); Assert::same(2, $this->connection->getLastInsertedId('publishers_id_seq')); + Assert::same(2, $this->connection->getLastInsertedId('public.publishers_id_seq')); + Assert::same(2, $this->connection->getLastInsertedId('"public"."publishers_id_seq"')); Assert::same(2, $this->connection->getLastInsertedId(new Fqn(schema: 'public', name: 'publishers_id_seq'))); Assert::exception(function() { $this->connection->getLastInsertedId(); - }, InvalidArgumentException::class, 'PgsqlDriver requires to pass sequence name for getLastInsertedId() method.'); + }, InvalidArgumentException::class, 'PgsqlDriver requires passing a sequence name for getLastInsertedId() method.'); } @@ -47,7 +49,7 @@ class ConnectionPostgresTest extends IntegrationTestCase $this->connection->query('DROP SEQUENCE IF EXISTS %column', "MySequence"); $this->connection->query('CREATE SEQUENCE %column INCREMENT 5 START 10;', "MySequence"); $this->connection->query('SELECT NEXTVAL(\'%column\')', "MySequence"); - Assert::same(10, $this->connection->getLastInsertedId("MySequence")); + Assert::same(10, $this->connection->getLastInsertedId('"MySequence"')); } } diff --git a/tests/cases/unit/CachedPlatformTest.phpt b/tests/cases/unit/CachedPlatformTest.phpt index 9691873..8e180b3 100644 --- a/tests/cases/unit/CachedPlatformTest.phpt +++ b/tests/cases/unit/CachedPlatformTest.phpt @@ -6,11 +6,16 @@ namespace NextrasTests\Dbal; + use Mockery; use Mockery\MockInterface; use Nette\Caching\Cache; -use Nette\Caching\IStorage; +use Nette\Caching\Storages\FileStorage; use Nextras\Dbal\Bridges\NetteCaching\CachedPlatform; +use Nextras\Dbal\Platforms\Data\Column; +use Nextras\Dbal\Platforms\Data\ForeignKey; +use Nextras\Dbal\Platforms\Data\Fqn; +use Nextras\Dbal\Platforms\Data\Table; use Nextras\Dbal\Platforms\IPlatform; use Tester\Assert; @@ -20,103 +25,84 @@ require_once __DIR__ . '/../../bootstrap.php'; class CachedPlatformTest extends TestCase { - /** @var CachedPlatform */ - private $platform; - - /** @var IStorage|MockInterface */ - private $storageMock; + private CachedPlatform $cache; - /** @var IPlatform|MockInterface */ + /** @var MockInterface */ private $platformMock; - public function setUp() + public function setUp(): void { parent::setUp(); - $this->storageMock = Mockery::mock(IStorage::class); $this->platformMock = Mockery::mock(IPlatform::class); - $this->platform = new CachedPlatform($this->platformMock, new Cache($this->storageMock)); + $this->cache = new CachedPlatform($this->platformMock, new Cache(new FileStorage(TEMP_DIR))); } - public function testCachedColumn() + public function testColumns(): void { - $expectedCols = ['one', 'two']; - $this->storageMock->shouldReceive('read')->with(Mockery::type('string'))->once()->andReturn($expectedCols); - - $cols = $this->platform->getColumns('foo'); - Assert::same($expectedCols, $cols); - - $this->storageMock->shouldReceive('clean'); - $this->platform->clearCache(); + $column = new Column( + name: 'bar', + type: 'type', + size: 128, + default: null, + isPrimary: true, + isAutoincrement: false, + isUnsigned: false, + isNullable: true, + meta: ['sequence' => 'a.b'], + ); + + $this->platformMock->shouldReceive('getColumns')->with('foo', null)->once()->andReturn([clone $column]); + + Assert::equal([clone $column], $this->cache->getColumns('foo')); + Assert::equal([clone $column], $this->cache->getColumns('foo')); } - public function testQueryColumn() + public function testTables(): void { - $expectedCols = ['one', 'two']; - $this->storageMock->shouldReceive('read')->with(Mockery::type('string'))->once()->andReturnNull(); - $this->storageMock->shouldReceive('lock')->with(Mockery::type('string'))->once(); - $this->storageMock->shouldReceive('write')->with(Mockery::type('string'), $expectedCols, [])->once(); - $this->platformMock->shouldReceive('getColumns')->with('foo', null)->once()->andReturn($expectedCols); - - $cols = $this->platform->getColumns('foo'); - Assert::same($expectedCols, $cols); + $table = new Table( + fqnName: new Fqn('one', 'two'), + isView: false, + ); + $this->platformMock->shouldReceive('getTables')->once()->andReturn([clone $table]); + + Assert::equal([clone $table], $this->cache->getTables()); + Assert::equal([clone $table], $this->cache->getTables()); } - public function testQueryTables() + public function testForeignKeys(): void { - $expectedTables = ['one', 'two']; - $this->storageMock->shouldReceive('read')->with(Mockery::type('string'))->once()->andReturnNull(); - $this->storageMock->shouldReceive('lock')->with(Mockery::type('string'))->once(); - $this->storageMock->shouldReceive('write')->with(Mockery::type('string'), $expectedTables, [])->once(); - $this->platformMock->shouldReceive('getTables')->once()->andReturn($expectedTables); - - $cols = $this->platform->getTables(); - Assert::same($expectedTables, $cols); + $fk = new ForeignKey( + fqnName: new Fqn('one', 'two'), + column: 'col', + refTable: new Fqn('three', 'four'), + refColumn: 'refCol', + ); + $this->platformMock->shouldReceive('getForeignKeys')->with('foo', null)->once()->andReturn([clone $fk]); + + Assert::equal([clone $fk], $this->cache->getForeignKeys('foo')); + Assert::equal([clone $fk], $this->cache->getForeignKeys('foo')); } - public function testQueryFk() + public function testQueryPrimarySequence(): void { - $expectedFk = ['one', 'two']; - $this->storageMock->shouldReceive('read')->with(Mockery::type('string'))->once()->andReturnNull(); - $this->storageMock->shouldReceive('lock')->with(Mockery::type('string'))->once(); - $this->storageMock->shouldReceive('write')->with(Mockery::type('string'), $expectedFk, [])->once(); - $this->platformMock->shouldReceive('getForeignKeys')->with('foo', null)->once()->andReturn($expectedFk); - - $cols = $this->platform->getForeignKeys('foo'); - Assert::same($expectedFk, $cols); - } - - - public function testQueryPS() - { - $expectedPs = 'ps_name'; - $this->storageMock->shouldReceive('read')->with(Mockery::type('string'))->once()->andReturnNull(); - $this->storageMock->shouldReceive('lock')->with(Mockery::type('string'))->once(); - $this->storageMock->shouldReceive('write')->with(Mockery::type('string'), [$expectedPs], [])->once(); - $this->platformMock->shouldReceive('getPrimarySequenceName')->with('foo', null)->once()->andReturn($expectedPs); - - $cols = $this->platform->getPrimarySequenceName('foo'); - Assert::same($expectedPs, $cols); - - - $this->storageMock->shouldReceive('read')->with(Mockery::type('string'))->once()->andReturnNull(); - $this->storageMock->shouldReceive('lock')->with(Mockery::type('string'))->once(); - $this->storageMock->shouldReceive('write')->with(Mockery::type('string'), [null], [])->once(); - $this->platformMock->shouldReceive('getPrimarySequenceName')->with('foo', null)->once()->andReturn(null); + $this->platformMock->shouldReceive('getPrimarySequenceName')->with('foo', null)->once() + ->andReturn('ps_name'); - $cols = $this->platform->getPrimarySequenceName('foo'); - Assert::same(null, $cols); + Assert::equal('ps_name', $this->cache->getPrimarySequenceName('foo')); + Assert::equal('ps_name', $this->cache->getPrimarySequenceName('foo')); } - public function testName() + public function testName(): void { - $this->platformMock->shouldReceive('getName')->once()->andReturn('foo'); - Assert::same('foo', $this->platform->getName()); + $this->platformMock->shouldReceive('getName')->twice()->andReturn('foo'); + Assert::same('foo', $this->cache->getName()); + Assert::same('foo', $this->cache->getName()); // no cache } }