Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Postgres positional parameters to Parser #6634

Open
wants to merge 1 commit into
base: 4.3.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/Driver/PgSQL/ConvertParameters.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

use function count;
use function implode;
use function ltrim;
use function str_starts_with;

final class ConvertParameters implements Visitor
{
Expand All @@ -19,6 +21,14 @@

public function acceptPositionalParameter(string $sql): void
{
if (str_starts_with($sql, '$')) {
$position = (int) ltrim($sql, '$');
$this->parameterMap[$position] = $position;
$this->buffer[] = $sql;

Check warning on line 27 in src/Driver/PgSQL/ConvertParameters.php

View check run for this annotation

Codecov / codecov/patch

src/Driver/PgSQL/ConvertParameters.php#L24-L27

Added lines #L24 - L27 were not covered by tests

return;

Check warning on line 29 in src/Driver/PgSQL/ConvertParameters.php

View check run for this annotation

Codecov / codecov/patch

src/Driver/PgSQL/ConvertParameters.php#L29

Added line #L29 was not covered by tests
}

$position = count($this->parameterMap) + 1;
$this->parameterMap[$position] = $position;
$this->buffer[] = '$' . $position;
Expand Down
4 changes: 2 additions & 2 deletions src/SQL/Parser.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,13 @@
*/
final class Parser
{
private const SPECIAL_CHARS = ':\?\'"`\\[\\-\\/';
private const SPECIAL_CHARS = ':\?\'"`\\[\\-\\/$';

private const BACKTICK_IDENTIFIER = '`[^`]*`';
private const BRACKET_IDENTIFIER = '(?<!\b(?i:ARRAY))\[(?:[^\]])*\]';
private const MULTICHAR = ':{2,}';
private const NAMED_PARAMETER = ':[a-zA-Z0-9_]+';
private const POSITIONAL_PARAMETER = '(?<!\\?)\\?(?!\\?)';
private const POSITIONAL_PARAMETER = '((?<!\\?)\\?(?!\\?)|\\$\d+)';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to extract the new pattern into a separate constant?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. Two arguments:

  • This is Postgres' way of specifying positional parameters. So the naming makes a lot of sense.
  • Adding an additional pattern would incur a performance penalty and complicate the code (having to add one more handler, which in turn just calls $visitor->acceptPositionalParameter($sql);)

private const ONE_LINE_COMMENT = '--[^\r\n]*';
private const MULTI_LINE_COMMENT = '/\*([^*]+|\*+[^/*])*\**\*/';
private const SPECIAL = '[' . self::SPECIAL_CHARS . ']';
Expand Down
42 changes: 42 additions & 0 deletions tests/Functional/SQL/PostgresNativePositionalParametersTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Tests\Functional\SQL;

use Doctrine\DBAL\ParameterType;
use Doctrine\DBAL\Schema\Table;
use Doctrine\DBAL\Tests\FunctionalTestCase;
use Doctrine\DBAL\Tests\TestUtil;
use Doctrine\DBAL\Types\Types;

final class PostgresNativePositionalParametersTest extends FunctionalTestCase
{
public function testPostgresNativePositionalParameters(): void
{
if (! TestUtil::isDriverOneOf('pgsql')) {
self::markTestSkipped('This test requires the pgsql driver.');
}
Comment on lines +17 to +19
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the PDO driver? Does it support this style of positional parameters? The behavior of the two drivers should be as similar as possible.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested with the pdo_pgsql driver and it does not seem to support this style of positional parameters. Only the native pgsql driver supports this native postgresql functionality


$table = new Table('dummy_table');
$table->addColumn('a_number', Types::SMALLINT);
$table->addColumn('a_number_2', Types::SMALLINT);
$table->addColumn('b_number', Types::SMALLINT);
$table->addColumn('c_number', Types::SMALLINT);
$table->addColumn('a_number_3', Types::SMALLINT);
$this->dropAndCreateTable($table);
$this->connection->executeStatement(
'INSERT INTO dummy_table (a_number, a_number_2, b_number, c_number, a_number_3)' .
' VALUES ($1, $1, $2, $3, $1)',
[1, 2, 3],
[ParameterType::INTEGER, ParameterType::INTEGER, ParameterType::INTEGER],
);
$result = $this->connection->executeQuery('SELECT * FROM dummy_table')->fetchAllAssociative();
self::assertCount(1, $result);
self::assertEquals(1, $result[0]['a_number']);
self::assertEquals(1, $result[0]['a_number_2']);
self::assertEquals(2, $result[0]['b_number']);
self::assertEquals(3, $result[0]['c_number']);
self::assertEquals(1, $result[0]['a_number_3']);
}
}
85 changes: 85 additions & 0 deletions tests/SQL/ParserTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,61 +45,121 @@ private static function getStatementsWithParameters(): iterable
'SELECT {?}',
];

yield [
'SELECT $1',
'SELECT {$1}',
];

yield [
'SELECT * FROM Foo WHERE bar IN (?, ?, ?)',
'SELECT * FROM Foo WHERE bar IN ({?}, {?}, {?})',
];

yield [
'SELECT * FROM Foo WHERE bar IN ($1, $2, $1)',
'SELECT * FROM Foo WHERE bar IN ({$1}, {$2}, {$1})',
];

yield [
'SELECT ? FROM ?',
'SELECT {?} FROM {?}',
];

yield [
'SELECT $1 FROM $2',
'SELECT {$1} FROM {$2}',
];

yield [
'SELECT "?" FROM foo WHERE bar = ?',
'SELECT "?" FROM foo WHERE bar = {?}',
];

yield [
'SELECT "$1" FROM foo WHERE bar = $1',
'SELECT "$1" FROM foo WHERE bar = {$1}',
];

yield [
"SELECT '?' FROM foo WHERE bar = ?",
"SELECT '?' FROM foo WHERE bar = {?}",
];

yield [
"SELECT '$1' FROM foo WHERE bar = $1",
"SELECT '$1' FROM foo WHERE bar = {\$1}",
];

yield [
'SELECT `?` FROM foo WHERE bar = ?',
'SELECT `?` FROM foo WHERE bar = {?}',
];

yield [
'SELECT `$1` FROM foo WHERE bar = $1',
'SELECT `$1` FROM foo WHERE bar = {$1}',
];

yield [
'SELECT [?] FROM foo WHERE bar = ?',
'SELECT [?] FROM foo WHERE bar = {?}',
];

yield [
'SELECT [$1] FROM foo WHERE bar = $1',
'SELECT [$1] FROM foo WHERE bar = {$1}',
];

yield [
'SELECT * FROM foo WHERE jsonb_exists_any(foo.bar, ARRAY[?])',
'SELECT * FROM foo WHERE jsonb_exists_any(foo.bar, ARRAY[{?}])',
];

yield [
'SELECT * FROM foo WHERE jsonb_exists_any(foo.bar, ARRAY[$1])',
'SELECT * FROM foo WHERE jsonb_exists_any(foo.bar, ARRAY[{$1}])',
];

yield [
"SELECT 'Doctrine\DBAL?' FROM foo WHERE bar = ?",
"SELECT 'Doctrine\DBAL?' FROM foo WHERE bar = {?}",
];

yield [
"SELECT 'Doctrine\DBAL$1' FROM foo WHERE bar = $1",
"SELECT 'Doctrine\DBAL$1' FROM foo WHERE bar = {\$1}",
];

yield [
'SELECT "Doctrine\DBAL?" FROM foo WHERE bar = ?',
'SELECT "Doctrine\DBAL?" FROM foo WHERE bar = {?}',
];

yield [
'SELECT "Doctrine\DBAL$1" FROM foo WHERE bar = $1',
'SELECT "Doctrine\DBAL$1" FROM foo WHERE bar = {$1}',
];

yield [
'SELECT `Doctrine\DBAL?` FROM foo WHERE bar = ?',
'SELECT `Doctrine\DBAL?` FROM foo WHERE bar = {?}',
];

yield [
'SELECT `Doctrine\DBAL$1` FROM foo WHERE bar = $1',
'SELECT `Doctrine\DBAL$1` FROM foo WHERE bar = {$1}',
];

yield [
'SELECT [Doctrine\DBAL?] FROM foo WHERE bar = ?',
'SELECT [Doctrine\DBAL?] FROM foo WHERE bar = {?}',
];

yield [
'SELECT [Doctrine\DBAL?] FROM foo WHERE bar = $1',
'SELECT [Doctrine\DBAL?] FROM foo WHERE bar = {$1}',
];

yield [
'SELECT :foo FROM :bar',
'SELECT {:foo} FROM {:bar}',
Expand Down Expand Up @@ -293,6 +353,31 @@ private static function getStatementsWithParameters(): iterable
,
];

yield 'Postgres placeholders inside comments' => [
<<<'SQL'
/*
* test placeholder $1
*/
SELECT dummy as "dummy$1"
FROM DUAL
WHERE '$1' = '$1'
-- AND dummy <> $1
AND dummy = $1
SQL
,
<<<'SQL'
/*
* test placeholder $1
*/
SELECT dummy as "dummy$1"
FROM DUAL
WHERE '$1' = '$1'
-- AND dummy <> $1
AND dummy = {$1}
SQL
,
];

yield 'Escaped question' => [
<<<'SQL'
SELECT '{"a":null}'::jsonb ?? :key
Expand Down