From 7608a1748c029b1e72f729c5dff6ef88dda5298a Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Sat, 3 Feb 2024 01:08:16 +0400 Subject: [PATCH 1/7] PostgresDriver: add support for multiple returning columns --- src/Driver/CompilerCache.php | 7 +++- src/Driver/Postgres/PostgresCompiler.php | 6 +-- src/Driver/Postgres/PostgresDriver.php | 2 +- .../Postgres/Query/PostgresInsertQuery.php | 37 +++++++++++-------- src/Query/InsertQuery.php | 3 +- src/Query/ReturningInterface.php | 9 +++++ 6 files changed, 42 insertions(+), 22 deletions(-) diff --git a/src/Driver/CompilerCache.php b/src/Driver/CompilerCache.php index 4340c0d7..0bd2356d 100644 --- a/src/Driver/CompilerCache.php +++ b/src/Driver/CompilerCache.php @@ -89,7 +89,12 @@ public function compile(QueryParameters $params, string $prefix, FragmentInterfa */ protected function hashInsertQuery(QueryParameters $params, array $tokens): string { - $hash = 'i_' . $tokens['table'] . implode('_', $tokens['columns']) . '_r' . ($tokens['return'] ?? ''); + $hash = \sprintf( + 'i_%s%s_r%s', + $tokens['table'], + \implode('_', $tokens['columns']), + \implode('_', (array)($tokens['return'] ?? [])) + ); foreach ($tokens['values'] as $value) { if ($value instanceof FragmentInterface) { if ($value instanceof Expression || $value instanceof Fragment) { diff --git a/src/Driver/Postgres/PostgresCompiler.php b/src/Driver/Postgres/PostgresCompiler.php index a0ae2295..85cb2a08 100644 --- a/src/Driver/Postgres/PostgresCompiler.php +++ b/src/Driver/Postgres/PostgresCompiler.php @@ -29,14 +29,14 @@ protected function insertQuery(QueryParameters $params, Quoter $q, array $tokens { $result = parent::insertQuery($params, $q, $tokens); - if ($tokens['return'] === null) { + if (empty($tokens['return'])) { return $result; } - return sprintf( + return \sprintf( '%s RETURNING %s', $result, - $this->quoteIdentifier($tokens['return']) + \implode(',', \array_map([$this, 'quoteIdentifier'], $tokens['return'])) ); } diff --git a/src/Driver/Postgres/PostgresDriver.php b/src/Driver/Postgres/PostgresDriver.php index 85c775d2..be60e157 100644 --- a/src/Driver/Postgres/PostgresDriver.php +++ b/src/Driver/Postgres/PostgresDriver.php @@ -98,7 +98,7 @@ public function shouldUseDefinedSchemas(): bool public function getPrimaryKey(string $prefix, string $table): ?string { $name = $prefix . $table; - if (array_key_exists($name, $this->primaryKeys)) { + if (\array_key_exists($name, $this->primaryKeys)) { return $this->primaryKeys[$name]; } diff --git a/src/Driver/Postgres/Query/PostgresInsertQuery.php b/src/Driver/Postgres/Query/PostgresInsertQuery.php index 7a0fd316..043385b4 100644 --- a/src/Driver/Postgres/Query/PostgresInsertQuery.php +++ b/src/Driver/Postgres/Query/PostgresInsertQuery.php @@ -20,6 +20,7 @@ use Cycle\Database\Query\InsertQuery; use Cycle\Database\Query\QueryInterface; use Cycle\Database\Query\QueryParameters; +use Cycle\Database\StatementInterface; use Throwable; /** @@ -30,8 +31,12 @@ class PostgresInsertQuery extends InsertQuery implements ReturningInterface /** @var PostgresDriver|null */ protected ?DriverInterface $driver = null; + /** @deprecated */ protected ?string $returning = null; + /** @var list */ + protected array $returningColumns = []; + public function withDriver(DriverInterface $driver, string $prefix = null): QueryInterface { $driver instanceof PostgresDriver or throw new BuilderException( @@ -48,13 +53,9 @@ public function returning(string|FragmentInterface ...$columns): self { $columns === [] and throw new BuilderException('RETURNING clause should contain at least 1 column.'); - if (count($columns) > 1) { - throw new BuilderException( - 'Postgres driver supports only single column returning at this moment.' - ); - } + $this->returning = \count($columns) === 1 ? \reset($columns) : null; - $this->returning = (string)$columns[0]; + $this->returningColumns = \array_values($columns); return $this; } @@ -69,6 +70,15 @@ public function run(): mixed $result = $this->driver->query($queryString, $params->getParameters()); try { + if ($this->returningColumns !== []) { + if (\count($this->returningColumns) === 1) { + return $result->fetchColumn(); + } + + return $result->fetch(StatementInterface::FETCH_ASSOC); + } + + // Return PK if no RETURNING clause is set if ($this->getPrimaryKey() !== null) { return $result->fetchColumn(); } @@ -83,7 +93,7 @@ public function getTokens(): array { return [ 'table' => $this->table, - 'return' => $this->getPrimaryKey(), + 'return' => $this->returningColumns, 'columns' => $this->columns, 'values' => $this->values, ]; @@ -91,15 +101,10 @@ public function getTokens(): array private function getPrimaryKey(): ?string { - $primaryKey = $this->returning; - if ($primaryKey === null && $this->driver !== null && $this->table !== null) { - try { - $primaryKey = $this->driver->getPrimaryKey($this->prefix, $this->table); - } catch (Throwable) { - return null; - } + try { + return $this->driver?->getPrimaryKey($this->prefix, $this->table); + } catch (Throwable) { + return null; } - - return $primaryKey; } } diff --git a/src/Query/InsertQuery.php b/src/Query/InsertQuery.php index dccf3f05..b548f820 100644 --- a/src/Query/InsertQuery.php +++ b/src/Query/InsertQuery.php @@ -112,8 +112,9 @@ public function values(mixed $rowsets): self /** * Run the query and return last insert id. + * Returns an assoc array of values if multiple columns were specified as returning columns. * - * @psalm-return int|non-empty-string|null + * @return array|int|non-empty-string|null */ public function run(): mixed { diff --git a/src/Query/ReturningInterface.php b/src/Query/ReturningInterface.php index 4203bb44..55b89906 100644 --- a/src/Query/ReturningInterface.php +++ b/src/Query/ReturningInterface.php @@ -4,12 +4,21 @@ namespace Cycle\Database\Query; +use Cycle\Database\Exception\BuilderException; use Cycle\Database\Injection\FragmentInterface; interface ReturningInterface extends QueryInterface { /** * Set returning column or expression. + * + * If set multiple columns and the driver supports it, then an insert result will be an array of values. + * If set one column and the driver supports it, then an insert result will be a single value, + * not an array of values. + * + * If set multiple columns and the driver does not support it, an exception will be thrown. + * + * @throws BuilderException */ public function returning(string|FragmentInterface ...$columns): self; } From 3ea8c7a128ee0e2087cfbf11cda9addf5c14e8ee Mon Sep 17 00:00:00 2001 From: Maxim Smakouz Date: Tue, 6 Feb 2024 13:30:43 +0200 Subject: [PATCH 2/7] Fix fragment returning, fix tokens --- src/Driver/Postgres/Query/PostgresInsertQuery.php | 4 ++-- .../Driver/Postgres/Query/InsertQueryTest.php | 11 ----------- 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/src/Driver/Postgres/Query/PostgresInsertQuery.php b/src/Driver/Postgres/Query/PostgresInsertQuery.php index 043385b4..a0d6b8e7 100644 --- a/src/Driver/Postgres/Query/PostgresInsertQuery.php +++ b/src/Driver/Postgres/Query/PostgresInsertQuery.php @@ -53,7 +53,7 @@ public function returning(string|FragmentInterface ...$columns): self { $columns === [] and throw new BuilderException('RETURNING clause should contain at least 1 column.'); - $this->returning = \count($columns) === 1 ? \reset($columns) : null; + $this->returning = \count($columns) === 1 ? (string) \reset($columns) : null; $this->returningColumns = \array_values($columns); @@ -93,7 +93,7 @@ public function getTokens(): array { return [ 'table' => $this->table, - 'return' => $this->returningColumns, + 'return' => $this->returningColumns !== [] ? $this->returningColumns : (array) $this->getPrimaryKey(), 'columns' => $this->columns, 'values' => $this->values, ]; diff --git a/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php b/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php index f7bb6738..10236615 100644 --- a/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php +++ b/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php @@ -115,17 +115,6 @@ public function testCustomReturningShouldContainColumns(): void ->returning(); } - public function testCustomReturningSupportsOnlySingleColumn(): void - { - $this->expectException(BuilderException::class); - $this->expectExceptionMessage('Postgres driver supports only single column returning at this moment.'); - - $this->database->insert()->into('table') - ->columns('name', 'balance') - ->values('Anton', 100) - ->returning('name', 'id'); - } - public function testInsertMicroseconds(): void { $schema = $this->schema( From 1f2adb66578b85feb6e66eae9a8f67bce53578e0 Mon Sep 17 00:00:00 2001 From: Maxim Smakouz Date: Tue, 6 Feb 2024 14:19:30 +0200 Subject: [PATCH 3/7] Add unit tests --- src/Driver/Postgres/PostgresCompiler.php | 8 ++- .../Postgres/Query/PostgresInsertQuery.php | 4 +- .../Driver/Postgres/Query/InsertQueryTest.php | 61 ++++++++++++++++++- 3 files changed, 68 insertions(+), 5 deletions(-) diff --git a/src/Driver/Postgres/PostgresCompiler.php b/src/Driver/Postgres/PostgresCompiler.php index 85cb2a08..e621f200 100644 --- a/src/Driver/Postgres/PostgresCompiler.php +++ b/src/Driver/Postgres/PostgresCompiler.php @@ -14,6 +14,7 @@ use Cycle\Database\Driver\CachingCompilerInterface; use Cycle\Database\Driver\Compiler; use Cycle\Database\Driver\Quoter; +use Cycle\Database\Injection\FragmentInterface; use Cycle\Database\Injection\Parameter; use Cycle\Database\Query\QueryParameters; @@ -36,7 +37,12 @@ protected function insertQuery(QueryParameters $params, Quoter $q, array $tokens return \sprintf( '%s RETURNING %s', $result, - \implode(',', \array_map([$this, 'quoteIdentifier'], $tokens['return'])) + \implode(',', \array_map( + fn (string|FragmentInterface|null $return) => $return instanceof FragmentInterface + ? (string) $return + : $this->quoteIdentifier($return), + $tokens['return'] + )) ); } diff --git a/src/Driver/Postgres/Query/PostgresInsertQuery.php b/src/Driver/Postgres/Query/PostgresInsertQuery.php index a0d6b8e7..2b72c1d6 100644 --- a/src/Driver/Postgres/Query/PostgresInsertQuery.php +++ b/src/Driver/Postgres/Query/PostgresInsertQuery.php @@ -32,7 +32,7 @@ class PostgresInsertQuery extends InsertQuery implements ReturningInterface protected ?DriverInterface $driver = null; /** @deprecated */ - protected ?string $returning = null; + protected string|FragmentInterface|null $returning = null; /** @var list */ protected array $returningColumns = []; @@ -53,7 +53,7 @@ public function returning(string|FragmentInterface ...$columns): self { $columns === [] and throw new BuilderException('RETURNING clause should contain at least 1 column.'); - $this->returning = \count($columns) === 1 ? (string) \reset($columns) : null; + $this->returning = \count($columns) === 1 ? \reset($columns) : null; $this->returningColumns = \array_values($columns); diff --git a/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php b/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php index 10236615..1d7a4e02 100644 --- a/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php +++ b/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php @@ -6,6 +6,7 @@ // phpcs:ignore use Cycle\Database\Driver\Postgres\Query\PostgresInsertQuery; +use Cycle\Database\Driver\Postgres\Schema\PostgresColumn; use Cycle\Database\Exception\BuilderException; use Cycle\Database\Injection\Fragment; use Cycle\Database\Tests\Functional\Driver\Common\Query\InsertQueryTest as CommonClass; @@ -91,19 +92,75 @@ public function testCustomReturning(): void ); } + public function testCustomMultipleReturning(): void + { + $insert = $this->database->insert()->into('table') + ->columns('name', 'balance') + ->values('Anton', 100) + ->returning('name', 'created_at'); + + $this->assertSameQuery( + 'INSERT INTO {table} ({name}, {balance}) VALUES (?, ?) RETURNING {name}, {created_at}', + $insert + ); + } + public function testCustomReturningWithFragment(): void { $insert = $this->database->insert()->into('table') ->columns('name', 'balance') ->values('Anton', 100) - ->returning(new Fragment('COUNT(name)')); + ->returning(new Fragment('"name" as "full_name"')); + + $this->assertSameQuery( + 'INSERT INTO {table} ({name}, {balance}) VALUES (?, ?) RETURNING {name} as {full_name}', + $insert + ); + } + + public function testCustomMultipleReturningWithFragment(): void + { + $insert = $this->database->insert()->into('table') + ->columns('name', 'balance') + ->values('Anton', 100) + ->returning('name', new Fragment('"created_at" as "date"')); $this->assertSameQuery( - 'INSERT INTO {table} ({name}, {balance}) VALUES (?, ?) RETURNING {COUNT(name)}', + 'INSERT INTO {table} ({name}, {balance}) VALUES (?, ?) RETURNING {name}, {created_at} as {date}', $insert ); } + public function testReturningValuesFromDatabase(): void + { + $schema = $this->schema('returning_values'); + $schema->primary('id'); + $schema->string('name'); + $schema->serial('sort'); + $schema->datetime('datetime', defaultValue: PostgresColumn::DATETIME_NOW); + $schema->save(); + + $returning = $this->database + ->insert('returning_values') + ->values(['name' => 'foo']) + ->returning( 'sort', 'datetime') + ->run(); + + $this->assertSame(1, $returning['sort']); + $this->assertIsString($returning['datetime']); + $this->assertNotFalse(\strtotime($returning['datetime'])); + + $returning = $this->database + ->insert('returning_values') + ->values(['name' => 'foo']) + ->returning('sort', new Fragment('"datetime" as "created_at"')) + ->run(); + + $this->assertSame(2, $returning['sort']); + $this->assertIsString($returning['created_at']); + $this->assertNotFalse(\strtotime($returning['created_at'])); + } + public function testCustomReturningShouldContainColumns(): void { $this->expectException(BuilderException::class); From 2d8db26399d1580a2cbc38c23b1121421588ee01 Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Tue, 6 Feb 2024 12:19:41 +0000 Subject: [PATCH 4/7] Apply fixes from StyleCI --- .../Functional/Driver/Postgres/Query/InsertQueryTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php b/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php index 1d7a4e02..14474934 100644 --- a/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php +++ b/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php @@ -143,7 +143,7 @@ public function testReturningValuesFromDatabase(): void $returning = $this->database ->insert('returning_values') ->values(['name' => 'foo']) - ->returning( 'sort', 'datetime') + ->returning('sort', 'datetime') ->run(); $this->assertSame(1, $returning['sort']); From 810f92b3584ac40c9df6c1f59a4e48fc461cf201 Mon Sep 17 00:00:00 2001 From: Maxim Smakouz Date: Tue, 6 Feb 2024 14:40:24 +0200 Subject: [PATCH 5/7] Add unit test for returning single value --- .../Driver/Postgres/Query/InsertQueryTest.php | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php b/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php index 14474934..6b0fa447 100644 --- a/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php +++ b/tests/Database/Functional/Driver/Postgres/Query/InsertQueryTest.php @@ -161,6 +161,31 @@ public function testReturningValuesFromDatabase(): void $this->assertNotFalse(\strtotime($returning['created_at'])); } + public function testReturningSingleValueFromDatabase(): void + { + $schema = $this->schema('returning_value'); + $schema->primary('id'); + $schema->string('name'); + $schema->serial('sort'); + $schema->save(); + + $returning = $this->database + ->insert('returning_value') + ->values(['name' => 'foo']) + ->returning('sort') + ->run(); + + $this->assertSame(1, $returning); + + $returning = $this->database + ->insert('returning_value') + ->values(['name' => 'foo']) + ->returning(new Fragment('"sort" as "number"')) + ->run(); + + $this->assertSame(2, $returning); + } + public function testCustomReturningShouldContainColumns(): void { $this->expectException(BuilderException::class); From e7c9c9db108dc45a870be2c2ced429744663bf84 Mon Sep 17 00:00:00 2001 From: Maxim Smakouz Date: Tue, 6 Feb 2024 15:35:37 +0200 Subject: [PATCH 6/7] Add unit test for hashInsertQuery with fragment returning --- .../Unit/Driver/CompilerCacheTest.php | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 tests/Database/Unit/Driver/CompilerCacheTest.php diff --git a/tests/Database/Unit/Driver/CompilerCacheTest.php b/tests/Database/Unit/Driver/CompilerCacheTest.php new file mode 100644 index 00000000..ddd9c7c5 --- /dev/null +++ b/tests/Database/Unit/Driver/CompilerCacheTest.php @@ -0,0 +1,31 @@ +setAccessible(true); + + $this->assertSame( + 'i_some_tablename_full_name_rname_"full_name" as "fullName"P?', + $ref->invoke($compiler, new QueryParameters(), [ + 'table' => 'some_table', + 'columns' => ['name', 'full_name'], + 'values' => ['Foo'], + 'return' => ['name', new Fragment('"full_name" as "fullName"')] + ]) + ); + } +} From 87182aeec2b5ca3ce710c4d87e5dd48d172d2aed Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Tue, 6 Feb 2024 13:35:47 +0000 Subject: [PATCH 7/7] Apply fixes from StyleCI --- tests/Database/Unit/Driver/CompilerCacheTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Database/Unit/Driver/CompilerCacheTest.php b/tests/Database/Unit/Driver/CompilerCacheTest.php index ddd9c7c5..638555d2 100644 --- a/tests/Database/Unit/Driver/CompilerCacheTest.php +++ b/tests/Database/Unit/Driver/CompilerCacheTest.php @@ -24,7 +24,7 @@ public function testHashInsertQueryWithReturningFragment(): void 'table' => 'some_table', 'columns' => ['name', 'full_name'], 'values' => ['Foo'], - 'return' => ['name', new Fragment('"full_name" as "fullName"')] + 'return' => ['name', new Fragment('"full_name" as "fullName"')], ]) ); }