From 2c910048e1e9f44e95dd45a610ae6477b29838f5 Mon Sep 17 00:00:00 2001 From: Simon Mundy Date: Tue, 13 Oct 2026 12:57:31 +1100 Subject: [PATCH 1/6] Quote Select, join and GROUP BY column names as identifiers Select resolved each column string through quoteIdentifierInFragment(), which splits on every non-word character, so a column such as x-y rendered as "x""-""y" while its alias rendered as "x-y". Column strings in the select list, join column lists and GROUP BY are now quoted as an identifier chain split only on the table-qualifier dot, with a * part left bare. A string holding an inline " AS " alias keeps fragment quoting, as before. --- src/Sql/Select.php | 46 ++++++++ test/unit/Sql/SelectColumnNameTest.php | 152 +++++++++++++++++++++++++ 2 files changed, 198 insertions(+) create mode 100644 test/unit/Sql/SelectColumnNameTest.php diff --git a/src/Sql/Select.php b/src/Sql/Select.php index 183750be9..619347d36 100644 --- a/src/Sql/Select.php +++ b/src/Sql/Select.php @@ -5,6 +5,7 @@ namespace PhpDb\Sql; use Closure; +use Override; use PhpDb\Adapter\Driver\DriverInterface; use PhpDb\Adapter\ParameterContainer; use PhpDb\Adapter\Platform\PlatformInterface; @@ -12,9 +13,11 @@ use function array_key_exists; use function array_key_first; +use function array_map; use function count; use function explode; use function gettype; +use function implode; use function is_array; use function is_int; use function is_numeric; @@ -36,6 +39,8 @@ * @property ?Having $having * @property ?Join $joins * @psalm-import-type Specification from AbstractSql + * @psalm-import-type ColumnReference from AbstractSql + * @psalm-import-type ColumnValue from AbstractSql * @psalm-import-type PredicateValue from Predicate\PredicateSet * @psalm-import-type JoinName from Join * @psalm-type TableReference = string|TableIdentifier|array @@ -748,6 +753,27 @@ protected function processWhere( ]; } + /** + * Quote a select, join or group column string as an identifier chain + * + * @param ColumnReference|ColumnValue $column + * @throws Exception\RuntimeException + */ + #[Override] + protected function resolveColumnValue( + Select|array|string|int|float|bool|ExpressionInterface|null $column, + PlatformInterface $platform, + ?DriverInterface $driver = null, + ?ParameterContainer $parameterContainer = null, + ?string $namedParameterPrefix = null, + ): string { + if (is_array($column) && ($column['isIdentifier'] ?? false) && is_string($column['column'])) { + return ($column['fromTable'] ?? '') . $this->quoteColumnIdentifier($column['column'], $platform); + } + + return parent::resolveColumnValue($column, $platform, $driver, $parameterContainer, $namedParameterPrefix); + } + private function getHaving(): Having { return $this->having ??= new Having(); @@ -770,6 +796,26 @@ private function prefixFromTable(?string $fromTable, PlatformInterface $platform : ''; } + /** + * Quote a column string as an identifier chain, splitting only on the table qualifier dot + * + * A `*` part is left bare. A string holding an inline ` AS ` alias keeps fragment quoting. + */ + private function quoteColumnIdentifier(string $column, PlatformInterface $platform): string + { + if (stripos($column, needle: ' as ') !== false) { + return $platform->quoteIdentifierInFragment($column); + } + + return implode( + $platform->getIdentifierSeparator(), + array_map( + static fn(string $part): string => self::SQL_STAR === $part ? $part : $platform->quoteIdentifier($part), + explode('.', $column), + ), + ); + } + /** * @param Select|TableReference|null $table * @return array{0: string|null, 1: string} diff --git a/test/unit/Sql/SelectColumnNameTest.php b/test/unit/Sql/SelectColumnNameTest.php new file mode 100644 index 000000000..2a8b45763 --- /dev/null +++ b/test/unit/Sql/SelectColumnNameTest.php @@ -0,0 +1,152 @@ + + */ + public static function groupColumnProvider(): array + { + return [ + 'plain' => ['bar', '"bar"'], + 'dotted' => ['t.bar', '"t"."bar"'], + 'hyphen' => ['x-y', '"x-y"'], + 'dotted hyphen' => ['t-1.x-y', '"t-1"."x-y"'], + 'space' => ['my col', '"my col"'], + 'quote character' => ['a"b', '"a\'b"'], + 'expression' => [new Expression('YEAR(created)'), 'YEAR(created)'], + ]; + } + + /** + * @return array, string}> + */ + public static function joinColumnProvider(): array + { + return [ + 'plain' => [['bar'], '"zac"."bar" AS "bar"'], + 'plain aliased' => [['b' => 'bar'], '"zac"."bar" AS "b"'], + 'dotted' => [['t.bar'], '"zac"."t"."bar" AS "t.bar"'], + 'star' => [['*'], '"zac".*'], + 'qualified star' => [['t.*'], '"zac"."t".* AS "t.*"'], + 'hyphen' => [['x-y'], '"zac"."x-y" AS "x-y"'], + 'hyphen aliased' => [['a' => 'x-y'], '"zac"."x-y" AS "a"'], + 'space' => [['my col'], '"zac"."my col" AS "my col"'], + 'space aliased' => [['a' => 'my col'], '"zac"."my col" AS "a"'], + 'quote character' => [['a"b'], '"zac"."a\'b" AS "a\'b"'], + 'quote character aliased' => [['a' => 'a"b'], '"zac"."a\'b" AS "a"'], + 'inline alias' => [['bar AS baz'], '"zac"."bar" AS "baz" AS "bar AS baz"'], + 'expression' => [[new Expression('COUNT(*)')], 'COUNT(*) AS Expression1'], + 'expression aliased' => [['c' => new Expression('COUNT(*)')], 'COUNT(*) AS "c"'], + ]; + } + + /** + * @return array, bool, string}> + */ + public static function selectColumnProvider(): array + { + return [ + 'plain' => [['bar'], true, 'SELECT "foo"."bar" AS "bar" FROM "foo"'], + 'plain aliased' => [['b' => 'bar'], true, 'SELECT "foo"."bar" AS "b" FROM "foo"'], + 'dotted' => [['t.bar'], true, 'SELECT "foo"."t"."bar" AS "t.bar" FROM "foo"'], + 'dotted without prefix' => [['t.bar'], false, 'SELECT "t"."bar" AS "t.bar" FROM "foo"'], + 'star' => [['*'], true, 'SELECT "foo".* FROM "foo"'], + 'star without prefix' => [['*'], false, 'SELECT * FROM "foo"'], + 'qualified star' => [['t.*'], true, 'SELECT "foo"."t".* AS "t.*" FROM "foo"'], + 'qualified star without prefix' => [['t.*'], false, 'SELECT "t".* AS "t.*" FROM "foo"'], + 'hyphen' => [['x-y'], true, 'SELECT "foo"."x-y" AS "x-y" FROM "foo"'], + 'hyphen aliased' => [['a' => 'x-y'], true, 'SELECT "foo"."x-y" AS "a" FROM "foo"'], + 'hyphen without prefix' => [['x-y'], false, 'SELECT "x-y" AS "x-y" FROM "foo"'], + 'space' => [['my col'], true, 'SELECT "foo"."my col" AS "my col" FROM "foo"'], + 'space aliased' => [['a' => 'my col'], true, 'SELECT "foo"."my col" AS "a" FROM "foo"'], + 'quote character' => [['a"b'], true, 'SELECT "foo"."a\'b" AS "a\'b" FROM "foo"'], + 'quote character aliased' => [['a' => 'a"b'], true, 'SELECT "foo"."a\'b" AS "a" FROM "foo"'], + 'dotted hyphen' => [['t-1.x-y'], false, 'SELECT "t-1"."x-y" AS "t-1.x-y" FROM "foo"'], + 'dotted space aliased' => [ + ['a' => 't 1.my col'], + false, + 'SELECT "t 1"."my col" AS "a" FROM "foo"', + ], + 'function call' => [['COUNT(id)'], false, 'SELECT "COUNT(id)" AS "COUNT(id)" FROM "foo"'], + 'arithmetic' => [ + ['price * qty'], + false, + 'SELECT "price * qty" AS "price * qty" FROM "foo"', + ], + 'inline alias' => [['bar AS baz'], true, 'SELECT "foo"."bar" AS "baz" FROM "foo"'], + 'inline alias lower case' => [['bar as baz'], true, 'SELECT "foo"."bar" as "baz" FROM "foo"'], + 'inline alias without prefix' => [['t.bar AS baz'], false, 'SELECT "t"."bar" AS "baz" FROM "foo"'], + 'expression' => [ + [new Expression('COUNT(id)')], + true, + 'SELECT COUNT(id) AS Expression1 FROM "foo"', + ], + 'expression aliased' => [ + ['c' => new Expression('COUNT(id)')], + true, + 'SELECT COUNT(id) AS "c" FROM "foo"', + ], + ]; + } + + #[Test] + #[DataProvider('groupColumnProvider')] + public function quotesGroupColumnStringsAsIdentifierChains(string|Expression $column, string $expected): void + { + $select = (new Select('foo'))->group($column); + + self::assertSame( + "SELECT \"foo\".* FROM \"foo\" GROUP BY {$expected}", + $select->getSqlString(new TrustingSql92Platform()), + ); + } + + /** + * @param array $columns + */ + #[Test] + #[DataProvider('joinColumnProvider')] + public function quotesJoinColumnStringsAsIdentifierChains(array $columns, string $expected): void + { + $select = (new Select('foo'))->columns([])->join('zac', 'm = n', $columns); + + self::assertSame( + "SELECT {$expected} FROM \"foo\" INNER JOIN \"zac\" ON \"m\" = \"n\"", + $select->getSqlString(new TrustingSql92Platform()), + ); + } + + /** + * @param array $columns + */ + #[Test] + #[DataProvider('selectColumnProvider')] + public function quotesSelectColumnStringsAsIdentifierChains( + array $columns, + bool $prefixColumnsWithTable, + string $expected, + ): void { + $select = (new Select('foo'))->columns($columns, $prefixColumnsWithTable); + + self::assertSame($expected, $select->getSqlString(new TrustingSql92Platform())); + } +} From 84996870976e6be9b6f7550b1101849c161e58ea Mon Sep 17 00:00:00 2001 From: Simon Mundy Date: Tue, 13 Oct 2026 17:02:17 +1100 Subject: [PATCH 2/6] Keep fragment quoting for column strings it already renders as valid SQL A column string now switches to identifier-chain quoting only when fragment quoting would render it broken: a piece between the safe delimiters (space, "." and "*") that the platform would split and quote in parts, as with x-y, a"b or COUNT(id), or two names separated by spaces alone, as with "my col". Strings such as "price * qty" or "a*b" render exactly as before. --- src/Sql/Select.php | 40 ++++++++- test/unit/Sql/SelectColumnNameTest.php | 115 +++++++++++++++---------- 2 files changed, 107 insertions(+), 48 deletions(-) diff --git a/src/Sql/Select.php b/src/Sql/Select.php index 619347d36..bd56e0540 100644 --- a/src/Sql/Select.php +++ b/src/Sql/Select.php @@ -32,6 +32,9 @@ use function strtoupper; use function trim; +use const PREG_SPLIT_DELIM_CAPTURE; +use const PREG_SPLIT_NO_EMPTY; + /** * @api * @@ -789,6 +792,38 @@ private function getWhere(): Where return $this->where ??= new Where(); } + /** + * Whether fragment quoting renders a column string as valid SQL + * + * It does when each piece between the safe delimiters (space, `.` and `*`) is quoted as one + * whole name and no two pieces are separated by spaces alone. + */ + private function isFragmentQuotable(string $column, PlatformInterface $platform): bool + { + $previousIsName = false; + $pieces = preg_split('/([ .*])/', $column, -1, PREG_SPLIT_DELIM_CAPTURE | PREG_SPLIT_NO_EMPTY); + foreach (false === $pieces ? [] : $pieces as $piece) { + if (' ' === $piece) { + continue; + } + + $isName = '.' !== $piece && self::SQL_STAR !== $piece; + if ( + $isName + && ( + $previousIsName + || $platform->quoteIdentifierInFragment($piece) !== $platform->quoteIdentifier($piece) + ) + ) { + return false; + } + + $previousIsName = $isName; + } + + return true; + } + private function prefixFromTable(?string $fromTable, PlatformInterface $platform): string { return $this->prefixColumnsWithTable && null !== $fromTable && '' !== $fromTable @@ -799,11 +834,12 @@ private function prefixFromTable(?string $fromTable, PlatformInterface $platform /** * Quote a column string as an identifier chain, splitting only on the table qualifier dot * - * A `*` part is left bare. A string holding an inline ` AS ` alias keeps fragment quoting. + * A `*` part is left bare. A string holding an inline ` AS ` alias, or one that fragment quoting + * already renders as valid SQL, keeps fragment quoting. */ private function quoteColumnIdentifier(string $column, PlatformInterface $platform): string { - if (stripos($column, needle: ' as ') !== false) { + if (stripos($column, needle: ' as ') !== false || $this->isFragmentQuotable($column, $platform)) { return $platform->quoteIdentifierInFragment($column); } diff --git a/test/unit/Sql/SelectColumnNameTest.php b/test/unit/Sql/SelectColumnNameTest.php index 2a8b45763..2daf4093e 100644 --- a/test/unit/Sql/SelectColumnNameTest.php +++ b/test/unit/Sql/SelectColumnNameTest.php @@ -18,6 +18,7 @@ #[CoversMethod(Select::class, 'processGroup')] #[CoversMethod(Select::class, 'resolveColumnValue')] #[CoversMethod(Select::class, 'quoteColumnIdentifier')] +#[CoversMethod(Select::class, 'isFragmentQuotable')] final class SelectColumnNameTest extends TestCase { /** @@ -26,13 +27,16 @@ final class SelectColumnNameTest extends TestCase public static function groupColumnProvider(): array { return [ - 'plain' => ['bar', '"bar"'], - 'dotted' => ['t.bar', '"t"."bar"'], - 'hyphen' => ['x-y', '"x-y"'], - 'dotted hyphen' => ['t-1.x-y', '"t-1"."x-y"'], - 'space' => ['my col', '"my col"'], - 'quote character' => ['a"b', '"a\'b"'], - 'expression' => [new Expression('YEAR(created)'), 'YEAR(created)'], + 'plain' => ['bar', '"bar"'], + 'dotted' => ['t.bar', '"t"."bar"'], + 'hyphen' => ['x-y', '"x-y"'], + 'dotted hyphen' => ['t-1.x-y', '"t-1"."x-y"'], + 'space' => ['my col', '"my col"'], + 'quote character' => ['a"b', '"a\'b"'], + 'function call' => ['YEAR(created)', '"YEAR(created)"'], + 'arithmetic' => ['a * b', '"a" * "b"'], + 'arithmetic without spaces' => ['a*b', '"a"*"b"'], + 'expression' => [new Expression('YEAR(created)'), 'YEAR(created)'], ]; } @@ -42,20 +46,22 @@ public static function groupColumnProvider(): array public static function joinColumnProvider(): array { return [ - 'plain' => [['bar'], '"zac"."bar" AS "bar"'], - 'plain aliased' => [['b' => 'bar'], '"zac"."bar" AS "b"'], - 'dotted' => [['t.bar'], '"zac"."t"."bar" AS "t.bar"'], - 'star' => [['*'], '"zac".*'], - 'qualified star' => [['t.*'], '"zac"."t".* AS "t.*"'], - 'hyphen' => [['x-y'], '"zac"."x-y" AS "x-y"'], - 'hyphen aliased' => [['a' => 'x-y'], '"zac"."x-y" AS "a"'], - 'space' => [['my col'], '"zac"."my col" AS "my col"'], - 'space aliased' => [['a' => 'my col'], '"zac"."my col" AS "a"'], - 'quote character' => [['a"b'], '"zac"."a\'b" AS "a\'b"'], - 'quote character aliased' => [['a' => 'a"b'], '"zac"."a\'b" AS "a"'], - 'inline alias' => [['bar AS baz'], '"zac"."bar" AS "baz" AS "bar AS baz"'], - 'expression' => [[new Expression('COUNT(*)')], 'COUNT(*) AS Expression1'], - 'expression aliased' => [['c' => new Expression('COUNT(*)')], 'COUNT(*) AS "c"'], + 'plain' => [['bar'], '"zac"."bar" AS "bar"'], + 'plain aliased' => [['b' => 'bar'], '"zac"."bar" AS "b"'], + 'dotted' => [['t.bar'], '"zac"."t"."bar" AS "t.bar"'], + 'star' => [['*'], '"zac".*'], + 'qualified star' => [['t.*'], '"zac"."t".* AS "t.*"'], + 'hyphen' => [['x-y'], '"zac"."x-y" AS "x-y"'], + 'hyphen aliased' => [['a' => 'x-y'], '"zac"."x-y" AS "a"'], + 'space' => [['my col'], '"zac"."my col" AS "my col"'], + 'space aliased' => [['a' => 'my col'], '"zac"."my col" AS "a"'], + 'quote character' => [['a"b'], '"zac"."a\'b" AS "a\'b"'], + 'quote character aliased' => [['a' => 'a"b'], '"zac"."a\'b" AS "a"'], + 'arithmetic' => [['price * qty'], '"zac"."price" * "qty" AS "price * qty"'], + 'arithmetic without spaces' => [['a*b'], '"zac"."a"*"b" AS "a*b"'], + 'inline alias' => [['bar AS baz'], '"zac"."bar" AS "baz" AS "bar AS baz"'], + 'expression' => [[new Expression('COUNT(*)')], 'COUNT(*) AS Expression1'], + 'expression aliased' => [['c' => new Expression('COUNT(*)')], 'COUNT(*) AS "c"'], ]; } @@ -65,42 +71,59 @@ public static function joinColumnProvider(): array public static function selectColumnProvider(): array { return [ - 'plain' => [['bar'], true, 'SELECT "foo"."bar" AS "bar" FROM "foo"'], - 'plain aliased' => [['b' => 'bar'], true, 'SELECT "foo"."bar" AS "b" FROM "foo"'], - 'dotted' => [['t.bar'], true, 'SELECT "foo"."t"."bar" AS "t.bar" FROM "foo"'], - 'dotted without prefix' => [['t.bar'], false, 'SELECT "t"."bar" AS "t.bar" FROM "foo"'], - 'star' => [['*'], true, 'SELECT "foo".* FROM "foo"'], - 'star without prefix' => [['*'], false, 'SELECT * FROM "foo"'], - 'qualified star' => [['t.*'], true, 'SELECT "foo"."t".* AS "t.*" FROM "foo"'], - 'qualified star without prefix' => [['t.*'], false, 'SELECT "t".* AS "t.*" FROM "foo"'], - 'hyphen' => [['x-y'], true, 'SELECT "foo"."x-y" AS "x-y" FROM "foo"'], - 'hyphen aliased' => [['a' => 'x-y'], true, 'SELECT "foo"."x-y" AS "a" FROM "foo"'], - 'hyphen without prefix' => [['x-y'], false, 'SELECT "x-y" AS "x-y" FROM "foo"'], - 'space' => [['my col'], true, 'SELECT "foo"."my col" AS "my col" FROM "foo"'], - 'space aliased' => [['a' => 'my col'], true, 'SELECT "foo"."my col" AS "a" FROM "foo"'], - 'quote character' => [['a"b'], true, 'SELECT "foo"."a\'b" AS "a\'b" FROM "foo"'], - 'quote character aliased' => [['a' => 'a"b'], true, 'SELECT "foo"."a\'b" AS "a" FROM "foo"'], - 'dotted hyphen' => [['t-1.x-y'], false, 'SELECT "t-1"."x-y" AS "t-1.x-y" FROM "foo"'], - 'dotted space aliased' => [ + 'plain' => [['bar'], true, 'SELECT "foo"."bar" AS "bar" FROM "foo"'], + 'plain aliased' => [['b' => 'bar'], true, 'SELECT "foo"."bar" AS "b" FROM "foo"'], + 'dotted' => [['t.bar'], true, 'SELECT "foo"."t"."bar" AS "t.bar" FROM "foo"'], + 'dotted without prefix' => [['t.bar'], false, 'SELECT "t"."bar" AS "t.bar" FROM "foo"'], + 'star' => [['*'], true, 'SELECT "foo".* FROM "foo"'], + 'star without prefix' => [['*'], false, 'SELECT * FROM "foo"'], + 'qualified star' => [['t.*'], true, 'SELECT "foo"."t".* AS "t.*" FROM "foo"'], + 'qualified star without prefix' => [['t.*'], false, 'SELECT "t".* AS "t.*" FROM "foo"'], + 'hyphen' => [['x-y'], true, 'SELECT "foo"."x-y" AS "x-y" FROM "foo"'], + 'hyphen aliased' => [['a' => 'x-y'], true, 'SELECT "foo"."x-y" AS "a" FROM "foo"'], + 'hyphen without prefix' => [['x-y'], false, 'SELECT "x-y" AS "x-y" FROM "foo"'], + 'space' => [['my col'], true, 'SELECT "foo"."my col" AS "my col" FROM "foo"'], + 'space aliased' => [['a' => 'my col'], true, 'SELECT "foo"."my col" AS "a" FROM "foo"'], + 'quote character' => [['a"b'], true, 'SELECT "foo"."a\'b" AS "a\'b" FROM "foo"'], + 'quote character aliased' => [['a' => 'a"b'], true, 'SELECT "foo"."a\'b" AS "a" FROM "foo"'], + 'dotted hyphen' => [['t-1.x-y'], false, 'SELECT "t-1"."x-y" AS "t-1.x-y" FROM "foo"'], + 'dotted space aliased' => [ ['a' => 't 1.my col'], false, 'SELECT "t 1"."my col" AS "a" FROM "foo"', ], - 'function call' => [['COUNT(id)'], false, 'SELECT "COUNT(id)" AS "COUNT(id)" FROM "foo"'], - 'arithmetic' => [ + 'function call' => [['COUNT(id)'], false, 'SELECT "COUNT(id)" AS "COUNT(id)" FROM "foo"'], + 'arithmetic' => [ ['price * qty'], + true, + 'SELECT "foo"."price" * "qty" AS "price * qty" FROM "foo"', + ], + 'arithmetic without prefix' => [ + ['price * qty'], + false, + 'SELECT "price" * "qty" AS "price * qty" FROM "foo"', + ], + 'arithmetic without spaces' => [['a*b'], false, 'SELECT "a"*"b" AS "a*b" FROM "foo"'], + 'arithmetic aliased' => [ + ['total' => 'price * qty'], + false, + 'SELECT "price" * "qty" AS "total" FROM "foo"', + ], + 'dotted arithmetic' => [ + ['t.price * t.qty'], false, - 'SELECT "price * qty" AS "price * qty" FROM "foo"', + 'SELECT "t"."price" * "t"."qty" AS "t.price * t.qty" FROM "foo"', ], - 'inline alias' => [['bar AS baz'], true, 'SELECT "foo"."bar" AS "baz" FROM "foo"'], - 'inline alias lower case' => [['bar as baz'], true, 'SELECT "foo"."bar" as "baz" FROM "foo"'], - 'inline alias without prefix' => [['t.bar AS baz'], false, 'SELECT "t"."bar" AS "baz" FROM "foo"'], - 'expression' => [ + 'arithmetic with hyphenated name' => [['x-y * 2'], false, 'SELECT "x-y * 2" AS "x-y * 2" FROM "foo"'], + 'inline alias' => [['bar AS baz'], true, 'SELECT "foo"."bar" AS "baz" FROM "foo"'], + 'inline alias lower case' => [['bar as baz'], true, 'SELECT "foo"."bar" as "baz" FROM "foo"'], + 'inline alias without prefix' => [['t.bar AS baz'], false, 'SELECT "t"."bar" AS "baz" FROM "foo"'], + 'expression' => [ [new Expression('COUNT(id)')], true, 'SELECT COUNT(id) AS Expression1 FROM "foo"', ], - 'expression aliased' => [ + 'expression aliased' => [ ['c' => new Expression('COUNT(id)')], true, 'SELECT COUNT(id) AS "c" FROM "foo"', From ea0bc14a524e2af722c9859398beaedea0f2ce4f Mon Sep 17 00:00:00 2001 From: Simon Mundy Date: Tue, 13 Oct 2026 17:04:16 +1100 Subject: [PATCH 3/6] Quote ORDER BY names as identifiers where fragment quoting breaks them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Select::processOrder() fragment-quoted each order name, so names such as x-y, my col, a"b or café rendered as several quoted pieces. Such names are now quoted whole as an identifier chain split only on '.'. Names whose parts between '*' and '.' already render as one identifier each, such as t.col and price*qty, render byte-for-byte as before. --- src/Sql/Select.php | 34 ++++- test/unit/Sql/SelectOrderNameTest.php | 142 ++++++++++++++++++ .../TestAsset/HyphenWordSql92Platform.php | 15 ++ 3 files changed, 190 insertions(+), 1 deletion(-) create mode 100644 test/unit/Sql/SelectOrderNameTest.php create mode 100644 test/unit/TestAsset/HyphenWordSql92Platform.php diff --git a/src/Sql/Select.php b/src/Sql/Select.php index 183750be9..7348393f1 100644 --- a/src/Sql/Select.php +++ b/src/Sql/Select.php @@ -12,9 +12,11 @@ use function array_key_exists; use function array_key_first; +use function array_map; use function count; use function explode; use function gettype; +use function implode; use function is_array; use function is_int; use function is_numeric; @@ -23,6 +25,7 @@ use function key; use function method_exists; use function preg_split; +use function str_replace; use function strcasecmp; use function stripos; use function strtolower; @@ -604,7 +607,7 @@ protected function processOrder( } $orders[] = [ - $platform->quoteIdentifierInFragment($k), + $this->quoteOrderIdentifier($k, $platform), strcasecmp(trim($v), self::ORDER_DESCENDING) === 0 ? self::ORDER_DESCENDING : self::ORDER_ASCENDING, @@ -770,6 +773,35 @@ private function prefixFromTable(?string $fromTable, PlatformInterface $platform : ''; } + /** + * Quote an order name, keeping fragment quoting only where it renders each part as a single identifier + * + * Fragment quoting splits on every non-word character, so a name such as `x-y` or `my col` would render + * as several quoted pieces. Such a name is quoted whole as an identifier chain instead, split only on + * the table qualifier dot. Names whose parts between `*` and `.` already quote whole, such as `t.col` + * or `price*qty`, render exactly as before. + */ + private function quoteOrderIdentifier(string $name, PlatformInterface $platform): string + { + foreach (explode('.', str_replace(self::SQL_STAR, replace: '.', subject: $name)) as $segment) { + $segment = trim($segment, characters: ' '); + $quoted = $platform->quoteIdentifierInFragment($segment); + if ($quoted !== $segment && $quoted !== $platform->quoteIdentifier($segment)) { + return implode( + $platform->getIdentifierSeparator(), + array_map( + static fn(string $part): string => self::SQL_STAR === $part + ? $part + : $platform->quoteIdentifier($part), + explode('.', $name), + ), + ); + } + } + + return $platform->quoteIdentifierInFragment($name); + } + /** * @param Select|TableReference|null $table * @return array{0: string|null, 1: string} diff --git a/test/unit/Sql/SelectOrderNameTest.php b/test/unit/Sql/SelectOrderNameTest.php new file mode 100644 index 000000000..d400142b0 --- /dev/null +++ b/test/unit/Sql/SelectOrderNameTest.php @@ -0,0 +1,142 @@ +|string, string}> + */ + public static function brokenOrderProvider(): array + { + return [ + 'hyphen' => ['x-y', '"x-y" ASC'], + 'hyphen descending' => ['x-y DESC', '"x-y" DESC'], + 'dotted hyphen' => ['t-1.x-y DESC', '"t-1"."x-y" DESC'], + 'quote character' => ['a"b DESC', '"a\'b" DESC'], + 'non-ascii' => ['café DESC', '"café" DESC'], + 'hyphen in comma list' => ['x-y DESC, b', '"x-y" DESC, "b" ASC'], + 'arithmetic with hyphen' => ['price*x-y DESC', '"price*x-y" DESC'], + 'hyphen array key' => [['x-y' => 'DESC'], '"x-y" DESC'], + 'space array key' => [['my col' => 'DESC'], '"my col" DESC'], + 'dotted space array key' => [['t.my col' => 'ASC'], '"t"."my col" ASC'], + 'quote character array key' => [['a"b' => 'DESC'], '"a\'b" DESC'], + 'non-ascii array key' => [['café' => 'DESC'], '"café" DESC'], + 'hyphen array list' => [['x-y DESC', 'b'], '"x-y" DESC, "b" ASC'], + 'hyphen qualified star' => ['x-y.* DESC', '"x-y".* DESC'], + ]; + } + + /** + * @return array + */ + public static function platformOrderProvider(): array + { + return [ + 'hyphen word character' => [new HyphenWordSql92Platform(), 'x-y DESC', '"x-y" DESC'], + 'hyphen word character arithmetic' => [ + new HyphenWordSql92Platform(), + 'price*x-y DESC', + '"price"*"x-y" DESC', + ], + 'hyphen word character with plus' => [new HyphenWordSql92Platform(), 'x+y DESC', '"x+y" DESC'], + 'identifier quoting off' => [ + new TestSql92Platform(quoteIdentifiers: false), + 'x-y DESC', + 'x-y DESC', + ], + 'identifier quoting off arithmetic' => [ + new TestSql92Platform(quoteIdentifiers: false), + 'price*qty DESC', + 'price*qty DESC', + ], + ]; + } + + /** + * @return array|string, string}> + */ + public static function validOrderProvider(): array + { + return [ + 'plain' => ['a', '"a" ASC'], + 'plain descending' => ['a DESC', '"a" DESC'], + 'lower case descending' => ['a desc', '"a" DESC'], + 'dotted' => ['t.col DESC', '"t"."col" DESC'], + 'comma list' => ['a DESC, b', '"a" DESC, "b" ASC'], + 'arithmetic' => ['price*qty DESC', '"price"*"qty" DESC'], + 'string with space' => ['my col DESC', '"my" ASC'], + 'array key' => [['a' => 'DESC'], '"a" DESC'], + 'array key unknown direction' => [['a' => 'sideways'], '"a" ASC'], + 'spaced arithmetic key' => [['price * qty' => 'DESC'], '"price" * "qty" DESC'], + 'dotted arithmetic key' => [['t.price*t.qty' => 'DESC'], '"t"."price"*"t"."qty" DESC'], + 'trailing space key' => [['a ' => 'DESC'], '"a" DESC'], + 'safe word part' => [['t.as' => 'DESC'], '"t".as DESC'], + 'array list' => [['a DESC', 'b'], '"a" DESC, "b" ASC'], + 'expression' => [new Expression('RAND()'), 'RAND()'], + 'expression in array' => [['a', new Expression('RAND()')], '"a" ASC, RAND()'], + ]; + } + + #[Test] + #[DataProvider('platformOrderProvider')] + public function followsThePlatformFragmentRulesForOrderNames( + PlatformInterface $platform, + string $order, + string $expected, + ): void { + $select = (new Select('foo'))->order($order); + + self::assertStringEndsWith(" ORDER BY {$expected}", $select->getSqlString($platform)); + } + + /** + * @param ExpressionInterface|array|string $order + */ + #[Test] + #[DataProvider('brokenOrderProvider')] + public function quotesOrderNameAsIdentifierChainWhenFragmentQuotingWouldBreakIt( + ExpressionInterface|array|string $order, + string $expected, + ): void { + self::assertSame("SELECT \"foo\".* FROM \"foo\" ORDER BY {$expected}", $this->render($order)); + } + + /** + * @param ExpressionInterface|array|string $order + */ + #[Test] + #[DataProvider('validOrderProvider')] + public function rendersValidOrderUnchanged(ExpressionInterface|array|string $order, string $expected): void + { + self::assertSame("SELECT \"foo\".* FROM \"foo\" ORDER BY {$expected}", $this->render($order)); + } + + /** + * @param ExpressionInterface|array|string $order + */ + private function render(ExpressionInterface|array|string $order): string + { + return (new Select('foo'))->order($order) + ->getSqlString(new TrustingSql92Platform()); + } +} diff --git a/test/unit/TestAsset/HyphenWordSql92Platform.php b/test/unit/TestAsset/HyphenWordSql92Platform.php new file mode 100644 index 000000000..224cbe48c --- /dev/null +++ b/test/unit/TestAsset/HyphenWordSql92Platform.php @@ -0,0 +1,15 @@ + Date: Wed, 4 Nov 2026 16:22:49 +1100 Subject: [PATCH 4/6] Quote identifier strings through one shared IdentifierQuoter Replace Select's own column-quoting helpers with IdentifierQuoter, which the ORDER BY and predicate fixes can share: - A plain name of word characters and qualifier dots takes a fast path straight to fragment quoting, as before. - Otherwise IdentifierShape reads the name in pieces. Fragment quoting is kept where it renders as written; a name it would split (x-y, my col, t.as) is quoted whole per qualifier part, with each part trimmed. - Expression syntax that no identifier quoting renders as written, such as COUNT(id), a - b or x-y * 2, throws InvalidArgumentException, as does an empty name or qualifier part. - An inline column alias such as 'x-y as z' renders "x-y" AS "z". TrustingHyphenWordPlatform becomes the shared hyphen-as-word test platform. --- .../Exception/InvalidArgumentException.php | 16 ++ src/Sql/IdentifierPiece.php | 63 ++++++ src/Sql/IdentifierQuoter.php | 94 +++++++++ src/Sql/IdentifierShape.php | 80 +++++++ src/Sql/Select.php | 62 +----- test/unit/Sql/IdentifierQuoterTest.php | 197 ++++++++++++++++++ test/unit/Sql/SelectColumnNameTest.php | 97 ++++++--- .../TestAsset/TrustingHyphenWordPlatform.php | 27 +++ 8 files changed, 544 insertions(+), 92 deletions(-) create mode 100644 src/Sql/IdentifierPiece.php create mode 100644 src/Sql/IdentifierQuoter.php create mode 100644 src/Sql/IdentifierShape.php create mode 100644 test/unit/Sql/IdentifierQuoterTest.php create mode 100644 test/unit/TestAsset/TrustingHyphenWordPlatform.php diff --git a/src/Sql/Exception/InvalidArgumentException.php b/src/Sql/Exception/InvalidArgumentException.php index 8066080c9..beec573d0 100644 --- a/src/Sql/Exception/InvalidArgumentException.php +++ b/src/Sql/Exception/InvalidArgumentException.php @@ -24,6 +24,8 @@ class InvalidArgumentException extends Exception\InvalidArgumentException final public const string EMPTY_EXPRESSION = 'Supplied expression must not be an empty string.'; + final public const string EMPTY_IDENTIFIER_PART = 'Identifier "%s" is empty or has an empty qualifier part'; + final public const string EMPTY_PREFIX = '$prefix must be a valid table prefix or null, empty string given'; final public const string EMPTY_SCHEMA = '$schema must be a valid schema name or null, empty string given'; @@ -80,6 +82,10 @@ class InvalidArgumentException extends Exception\InvalidArgumentException final public const string UNKNOWN_COLUMN_KEY = 'The key %s was not found in this objects column list'; + final public const string UNQUOTABLE_IDENTIFIER = + 'Identifier "%s" holds expression syntax that cannot be quoted as a name;' + . ' pass it as an Expression instead'; + final public const string VALUES_WITH_MERGE_FLAG = 'An array of values cannot be provided with the merge flag' . ' when a PhpDb\Sql\Select instance already exists as the value source'; @@ -99,6 +105,11 @@ public static function forEmptyExpression(): self return new self(self::EMPTY_EXPRESSION); } + public static function forEmptyIdentifierPart(string $identifier): self + { + return new self(sprintf(self::EMPTY_IDENTIFIER_PART, $identifier)); + } + public static function forEmptyPrefix(): self { return new self(self::EMPTY_PREFIX); @@ -207,6 +218,11 @@ public static function forUnknownColumnKey(string $name): self return new self(sprintf(self::UNKNOWN_COLUMN_KEY, $name)); } + public static function forUnquotableIdentifier(string $identifier): self + { + return new self(sprintf(self::UNQUOTABLE_IDENTIFIER, $identifier)); + } + public static function forValuesWithMergeFlag(): self { return new self(self::VALUES_WITH_MERGE_FLAG); diff --git a/src/Sql/IdentifierPiece.php b/src/Sql/IdentifierPiece.php new file mode 100644 index 000000000..b446d1a08 --- /dev/null +++ b/src/Sql/IdentifierPiece.php @@ -0,0 +1,63 @@ ++\/%|!~^&;]/'; + + public static function of(string $piece, ?self $previous, bool $spaced, PlatformInterface $platform): self + { + if ('.' === $piece) { + return self::Dot; + } + + if ('*' === $piece) { + return self::Dot === $previous ? self::QualifiedStar : self::Keyword; + } + + $isName = $platform->quoteIdentifierInFragment($piece) === $platform->quoteIdentifier($piece); + + return match (true) { + $spaced && 'as' === strtolower($piece) => self::Keyword, + 1 === preg_match(self::OPERATOR, $piece) + || (! $isName && 1 === preg_match(self::EXPRESSION_CHARACTERS, $piece)) + => self::Unquotable, + $isName => self::Name, + default => self::Whole, + }; + } +} diff --git a/src/Sql/IdentifierQuoter.php b/src/Sql/IdentifierQuoter.php new file mode 100644 index 000000000..98cfce3bc --- /dev/null +++ b/src/Sql/IdentifierQuoter.php @@ -0,0 +1,94 @@ +quoteIdentifierInFragment($name); + } + + return match (IdentifierShape::of($name, $platform)) { + IdentifierShape::Fragment => $platform->quoteIdentifierInFragment($name), + IdentifierShape::Whole => self::quoteChain($name, $platform), + IdentifierShape::Unquotable => throw Exception\InvalidArgumentException::forUnquotableIdentifier($name), + IdentifierShape::EmptyPart => throw Exception\InvalidArgumentException::forEmptyIdentifierPart($name), + }; + } + + /** + * Quote a column, quoting each side of an inline `AS` alias separately where the whole cannot be + * + * @throws Exception\InvalidArgumentException + */ + public static function quoteColumn(string $column, PlatformInterface $platform): string + { + if ( + 1 !== preg_match(self::PLAIN_NAME, $column) + && IdentifierShape::Unquotable === IdentifierShape::of($column, $platform) + ) { + /** @var list $parts */ + $parts = preg_split(self::ALIAS_KEYWORD, $column); + if (2 === count($parts)) { + /** @var array{string, string} $parts */ + [$name, $alias] = $parts; + + return self::quote($name, $platform) . ' AS ' . self::quote($alias, $platform); + } + } + + return self::quote($column, $platform); + } + + /** + * Quote each qualifier part whole, leaving a `*` part bare + */ + private static function quoteChain(string $name, PlatformInterface $platform): string + { + return implode( + $platform->getIdentifierSeparator(), + array_map( + static fn(string $part): string => '*' === $part ? $part : $platform->quoteIdentifier($part), + array_map(trim(...), explode('.', $name)), + ), + ); + } +} diff --git a/src/Sql/IdentifierShape.php b/src/Sql/IdentifierShape.php new file mode 100644 index 000000000..bb76cc076 --- /dev/null +++ b/src/Sql/IdentifierShape.php @@ -0,0 +1,80 @@ + $pieces */ + $pieces = preg_split(self::PIECES, $name, -1, PREG_SPLIT_DELIM_CAPTURE | PREG_SPLIT_NO_EMPTY); + foreach ($pieces as $piece) { + if (' ' === $piece) { + $spaced = true; + continue; + } + + $kind = IdentifierPiece::of($piece, $previous, $spaced, $platform); + if (IdentifierPiece::Unquotable === $kind) { + return self::Unquotable; + } + + if (IdentifierPiece::Dot === $kind && (null === $previous || IdentifierPiece::Dot === $previous)) { + return self::EmptyPart; + } + + $whole = + $whole + || IdentifierPiece::Whole === $kind + || (IdentifierPiece::Name === $kind && IdentifierPiece::Name === $previous && $spaced); + $keyword = $keyword || IdentifierPiece::Keyword === $kind; + $previous = $kind; + $spaced = false; + } + + return match (true) { + null === $previous, IdentifierPiece::Dot === $previous => self::EmptyPart, + $whole => $keyword ? self::Unquotable : self::Whole, + default => self::Fragment, + }; + } +} diff --git a/src/Sql/Select.php b/src/Sql/Select.php index bd56e0540..7efb21194 100644 --- a/src/Sql/Select.php +++ b/src/Sql/Select.php @@ -13,11 +13,9 @@ use function array_key_exists; use function array_key_first; -use function array_map; use function count; use function explode; use function gettype; -use function implode; use function is_array; use function is_int; use function is_numeric; @@ -32,9 +30,6 @@ use function strtoupper; use function trim; -use const PREG_SPLIT_DELIM_CAPTURE; -use const PREG_SPLIT_NO_EMPTY; - /** * @api * @@ -757,7 +752,7 @@ protected function processWhere( } /** - * Quote a select, join or group column string as an identifier chain + * Quote a select, join or group column string through IdentifierQuoter * * @param ColumnReference|ColumnValue $column * @throws Exception\RuntimeException @@ -771,7 +766,7 @@ protected function resolveColumnValue( ?string $namedParameterPrefix = null, ): string { if (is_array($column) && ($column['isIdentifier'] ?? false) && is_string($column['column'])) { - return ($column['fromTable'] ?? '') . $this->quoteColumnIdentifier($column['column'], $platform); + return ($column['fromTable'] ?? '') . IdentifierQuoter::quoteColumn($column['column'], $platform); } return parent::resolveColumnValue($column, $platform, $driver, $parameterContainer, $namedParameterPrefix); @@ -792,38 +787,6 @@ private function getWhere(): Where return $this->where ??= new Where(); } - /** - * Whether fragment quoting renders a column string as valid SQL - * - * It does when each piece between the safe delimiters (space, `.` and `*`) is quoted as one - * whole name and no two pieces are separated by spaces alone. - */ - private function isFragmentQuotable(string $column, PlatformInterface $platform): bool - { - $previousIsName = false; - $pieces = preg_split('/([ .*])/', $column, -1, PREG_SPLIT_DELIM_CAPTURE | PREG_SPLIT_NO_EMPTY); - foreach (false === $pieces ? [] : $pieces as $piece) { - if (' ' === $piece) { - continue; - } - - $isName = '.' !== $piece && self::SQL_STAR !== $piece; - if ( - $isName - && ( - $previousIsName - || $platform->quoteIdentifierInFragment($piece) !== $platform->quoteIdentifier($piece) - ) - ) { - return false; - } - - $previousIsName = $isName; - } - - return true; - } - private function prefixFromTable(?string $fromTable, PlatformInterface $platform): string { return $this->prefixColumnsWithTable && null !== $fromTable && '' !== $fromTable @@ -831,27 +794,6 @@ private function prefixFromTable(?string $fromTable, PlatformInterface $platform : ''; } - /** - * Quote a column string as an identifier chain, splitting only on the table qualifier dot - * - * A `*` part is left bare. A string holding an inline ` AS ` alias, or one that fragment quoting - * already renders as valid SQL, keeps fragment quoting. - */ - private function quoteColumnIdentifier(string $column, PlatformInterface $platform): string - { - if (stripos($column, needle: ' as ') !== false || $this->isFragmentQuotable($column, $platform)) { - return $platform->quoteIdentifierInFragment($column); - } - - return implode( - $platform->getIdentifierSeparator(), - array_map( - static fn(string $part): string => self::SQL_STAR === $part ? $part : $platform->quoteIdentifier($part), - explode('.', $column), - ), - ); - } - /** * @param Select|TableReference|null $table * @return array{0: string|null, 1: string} diff --git a/test/unit/Sql/IdentifierQuoterTest.php b/test/unit/Sql/IdentifierQuoterTest.php new file mode 100644 index 000000000..b5ed8f071 --- /dev/null +++ b/test/unit/Sql/IdentifierQuoterTest.php @@ -0,0 +1,197 @@ + */ + public static function emptyPartProvider(): array + { + return [ + 'empty' => [''], + 'spaces only' => [' '], + 'leading dot' => ['.a'], + 'trailing dot' => ['a.'], + 'double dot' => ['a..b'], + 'spaced dots' => ['x-y. .b'], + ]; + } + + /** @return array */ + public static function hyphenWordPlatformProvider(): array + { + return [ + 'hyphen' => ['x-y', '`x-y`'], + 'hyphen arithmetic' => ['a-b * c', '`a-b` * `c`'], + 'space' => ['my col', '`my col`'], + 'qualified space' => ['t.my col', '`t`.`my col`'], + ]; + } + + /** @return array */ + public static function quotedColumnProvider(): array + { + return [ + 'plain' => ['a', '"a"'], + 'fragment alias' => ['a as b', '"a" as "b"'], + 'hyphenated name alias' => ['x-y as z', '"x-y" AS "z"'], + 'qualified hyphen alias' => ['t.x-y AS my col', '"t"."x-y" AS "my col"'], + 'hyphen' => ['x-y', '"x-y"'], + ]; + } + + /** @return array */ + public static function quotedNameProvider(): array + { + return [ + 'plain' => ['a', '"a"'], + 'qualified' => ['t.a', '"t"."a"'], + 'schema qualified' => ['s.t.a', '"s"."t"."a"'], + 'star' => ['*', '*'], + 'qualified star' => ['t.*', '"t".*'], + 'multiplication' => ['price*qty', '"price"*"qty"'], + 'spaced multiplication' => ['price * qty', '"price" * "qty"'], + 'alias' => ['a as b', '"a" as "b"'], + 'trailing space' => ['a ', '"a" '], + 'hyphen' => ['x-y', '"x-y"'], + 'qualified hyphen' => ['t-1.x-y', '"t-1"."x-y"'], + 'hyphen then star' => ['x-y.*', '"x-y".*'], + 'space' => ['my col', '"my col"'], + 'qualified space' => ['t.my col', '"t"."my col"'], + 'padded hyphen' => [' x-y ', '"x-y"'], + 'padded qualified hyphen' => ['t. x-y', '"t"."x-y"'], + 'as after qualifier' => ['t.as', '"t"."as"'], + 'as alone' => ['as', '"as"'], + 'non-ascii letter' => ['café', '"café"'], + 'quote character' => ['a"b', '"a\'b"'], + ]; + } + + /** @return array */ + public static function unquotableColumnProvider(): array + { + return [ + 'function call with alias' => ['COUNT(id) as c'], + 'two aliases' => ['x-y as z as w'], + 'arithmetic' => ['x-y * 2'], + ]; + } + + /** @return array */ + public static function unquotableNameProvider(): array + { + return [ + 'function call' => ['COUNT(id)'], + 'function call of star' => ['COUNT(*)'], + 'spaced minus' => ['a - b'], + 'comparison' => ['a=b'], + 'hyphen times number' => ['x-y * 2'], + 'number times hyphen' => ['price*x-y'], + 'hyphenated name with alias' => ['x-y as z'], + ]; + } + + #[Test] + #[DataProvider('hyphenWordPlatformProvider')] + public function followsThePlatformsOwnWordCharacters(string $name, string $expected): void + { + static::assertSame($expected, IdentifierQuoter::quote($name, new TrustingHyphenWordPlatform())); + } + + #[Test] + public function leavesNamesUnquotedWhenThePlatformDoesNotQuoteIdentifiers(): void + { + $platform = new class extends Sql92 { + protected bool $quoteIdentifiers = false; + }; + + static::assertSame('x-y.my col', IdentifierQuoter::quote('x-y.my col', $platform)); + } + + #[Test] + #[DataProvider('quotedNameProvider')] + public function quotesANameSoItRendersAsWritten(string $name, string $expected): void + { + static::assertSame($expected, IdentifierQuoter::quote($name, new TrustingSql92Platform())); + } + + #[Test] + public function quotesAPlainNameByFragmentWithoutReadingItsPieces(): void + { + $fragment = static fn(string $name): string => "[{$name}]"; + $whole = static fn(string $name): string => "{{$name}}"; + $platform = $this->createStub(PlatformInterface::class); + $platform->method('quoteIdentifierInFragment')->willReturnCallback($fragment); + $platform->method('quoteIdentifier')->willReturnCallback($whole); + + static::assertSame('[t.a]', IdentifierQuoter::quote('t.a', $platform)); + } + + #[Test] + #[DataProvider('quotedColumnProvider')] + public function quotesEachSideOfAnInlineColumnAliasWhereTheWholeCannotBe(string $column, string $expected): void + { + static::assertSame($expected, IdentifierQuoter::quoteColumn($column, new TrustingSql92Platform())); + } + + #[Test] + #[DataProvider('unquotableColumnProvider')] + public function rejectsAColumnWhoseSidesCannotBeQuoted(string $column): void + { + $this->expectException(InvalidArgumentException::class); + + IdentifierQuoter::quoteColumn($column, new TrustingSql92Platform()); + } + + #[Test] + #[DataProvider('emptyPartProvider')] + public function rejectsANameWithAnEmptyPart(string $name): void + { + $this->expectExceptionObject(InvalidArgumentException::forEmptyIdentifierPart($name)); + + IdentifierQuoter::quote($name, new TrustingSql92Platform()); + } + + #[Test] + #[DataProvider('unquotableNameProvider')] + public function rejectsExpressionSyntaxThatNoQuotingRendersAsWritten(string $name): void + { + $this->expectExceptionObject(InvalidArgumentException::forUnquotableIdentifier($name)); + + IdentifierQuoter::quote($name, new TrustingSql92Platform()); + } + + #[Test] + public function separatesQualifierPartsWithThePlatformSeparator(): void + { + $platform = $this->createStub(PlatformInterface::class); + $platform->method('quoteIdentifierInFragment') + ->willReturnCallback( + static fn(string $name): string => "[{$name}]" === '[x-y]' ? '[x][-][y]' : "[{$name}]", + ); + $platform->method('quoteIdentifier')->willReturnCallback(static fn(string $name): string => "[{$name}]"); + $platform->method('getIdentifierSeparator')->willReturn('::'); + + static::assertSame('[t]::[x-y]', IdentifierQuoter::quote('t.x-y', $platform)); + } +} diff --git a/test/unit/Sql/SelectColumnNameTest.php b/test/unit/Sql/SelectColumnNameTest.php index 2daf4093e..f7f00b759 100644 --- a/test/unit/Sql/SelectColumnNameTest.php +++ b/test/unit/Sql/SelectColumnNameTest.php @@ -4,6 +4,7 @@ namespace PhpDbTest\Sql; +use PhpDb\Sql\Exception\InvalidArgumentException; use PhpDb\Sql\Expression; use PhpDb\Sql\Select; use PhpDbTest\TestAsset\TrustingSql92Platform; @@ -17,8 +18,6 @@ #[CoversMethod(Select::class, 'processSelect')] #[CoversMethod(Select::class, 'processGroup')] #[CoversMethod(Select::class, 'resolveColumnValue')] -#[CoversMethod(Select::class, 'quoteColumnIdentifier')] -#[CoversMethod(Select::class, 'isFragmentQuotable')] final class SelectColumnNameTest extends TestCase { /** @@ -33,7 +32,6 @@ public static function groupColumnProvider(): array 'dotted hyphen' => ['t-1.x-y', '"t-1"."x-y"'], 'space' => ['my col', '"my col"'], 'quote character' => ['a"b', '"a\'b"'], - 'function call' => ['YEAR(created)', '"YEAR(created)"'], 'arithmetic' => ['a * b', '"a" * "b"'], 'arithmetic without spaces' => ['a*b', '"a"*"b"'], 'expression' => [new Expression('YEAR(created)'), 'YEAR(created)'], @@ -71,59 +69,59 @@ public static function joinColumnProvider(): array public static function selectColumnProvider(): array { return [ - 'plain' => [['bar'], true, 'SELECT "foo"."bar" AS "bar" FROM "foo"'], - 'plain aliased' => [['b' => 'bar'], true, 'SELECT "foo"."bar" AS "b" FROM "foo"'], - 'dotted' => [['t.bar'], true, 'SELECT "foo"."t"."bar" AS "t.bar" FROM "foo"'], - 'dotted without prefix' => [['t.bar'], false, 'SELECT "t"."bar" AS "t.bar" FROM "foo"'], - 'star' => [['*'], true, 'SELECT "foo".* FROM "foo"'], - 'star without prefix' => [['*'], false, 'SELECT * FROM "foo"'], - 'qualified star' => [['t.*'], true, 'SELECT "foo"."t".* AS "t.*" FROM "foo"'], - 'qualified star without prefix' => [['t.*'], false, 'SELECT "t".* AS "t.*" FROM "foo"'], - 'hyphen' => [['x-y'], true, 'SELECT "foo"."x-y" AS "x-y" FROM "foo"'], - 'hyphen aliased' => [['a' => 'x-y'], true, 'SELECT "foo"."x-y" AS "a" FROM "foo"'], - 'hyphen without prefix' => [['x-y'], false, 'SELECT "x-y" AS "x-y" FROM "foo"'], - 'space' => [['my col'], true, 'SELECT "foo"."my col" AS "my col" FROM "foo"'], - 'space aliased' => [['a' => 'my col'], true, 'SELECT "foo"."my col" AS "a" FROM "foo"'], - 'quote character' => [['a"b'], true, 'SELECT "foo"."a\'b" AS "a\'b" FROM "foo"'], - 'quote character aliased' => [['a' => 'a"b'], true, 'SELECT "foo"."a\'b" AS "a" FROM "foo"'], - 'dotted hyphen' => [['t-1.x-y'], false, 'SELECT "t-1"."x-y" AS "t-1.x-y" FROM "foo"'], - 'dotted space aliased' => [ + 'plain' => [['bar'], true, 'SELECT "foo"."bar" AS "bar" FROM "foo"'], + 'plain aliased' => [['b' => 'bar'], true, 'SELECT "foo"."bar" AS "b" FROM "foo"'], + 'dotted' => [['t.bar'], true, 'SELECT "foo"."t"."bar" AS "t.bar" FROM "foo"'], + 'dotted without prefix' => [['t.bar'], false, 'SELECT "t"."bar" AS "t.bar" FROM "foo"'], + 'star' => [['*'], true, 'SELECT "foo".* FROM "foo"'], + 'star without prefix' => [['*'], false, 'SELECT * FROM "foo"'], + 'qualified star' => [['t.*'], true, 'SELECT "foo"."t".* AS "t.*" FROM "foo"'], + 'qualified star without prefix' => [['t.*'], false, 'SELECT "t".* AS "t.*" FROM "foo"'], + 'hyphen' => [['x-y'], true, 'SELECT "foo"."x-y" AS "x-y" FROM "foo"'], + 'hyphen aliased' => [['a' => 'x-y'], true, 'SELECT "foo"."x-y" AS "a" FROM "foo"'], + 'hyphen without prefix' => [['x-y'], false, 'SELECT "x-y" AS "x-y" FROM "foo"'], + 'space' => [['my col'], true, 'SELECT "foo"."my col" AS "my col" FROM "foo"'], + 'space aliased' => [['a' => 'my col'], true, 'SELECT "foo"."my col" AS "a" FROM "foo"'], + 'quote character' => [['a"b'], true, 'SELECT "foo"."a\'b" AS "a\'b" FROM "foo"'], + 'quote character aliased' => [['a' => 'a"b'], true, 'SELECT "foo"."a\'b" AS "a" FROM "foo"'], + 'dotted hyphen' => [['t-1.x-y'], false, 'SELECT "t-1"."x-y" AS "t-1.x-y" FROM "foo"'], + 'dotted space aliased' => [ ['a' => 't 1.my col'], false, 'SELECT "t 1"."my col" AS "a" FROM "foo"', ], - 'function call' => [['COUNT(id)'], false, 'SELECT "COUNT(id)" AS "COUNT(id)" FROM "foo"'], - 'arithmetic' => [ + 'hyphenated name with inline alias' => [['x-y as z'], false, 'SELECT "x-y" AS "z" FROM "foo"'], + 'padded hyphen' => [['x-y '], false, 'SELECT "x-y" AS "x-y " FROM "foo"'], + 'arithmetic' => [ ['price * qty'], true, 'SELECT "foo"."price" * "qty" AS "price * qty" FROM "foo"', ], - 'arithmetic without prefix' => [ + 'arithmetic without prefix' => [ ['price * qty'], false, 'SELECT "price" * "qty" AS "price * qty" FROM "foo"', ], - 'arithmetic without spaces' => [['a*b'], false, 'SELECT "a"*"b" AS "a*b" FROM "foo"'], - 'arithmetic aliased' => [ + 'arithmetic without spaces' => [['a*b'], false, 'SELECT "a"*"b" AS "a*b" FROM "foo"'], + 'arithmetic aliased' => [ ['total' => 'price * qty'], false, 'SELECT "price" * "qty" AS "total" FROM "foo"', ], - 'dotted arithmetic' => [ + 'dotted arithmetic' => [ ['t.price * t.qty'], false, 'SELECT "t"."price" * "t"."qty" AS "t.price * t.qty" FROM "foo"', ], - 'arithmetic with hyphenated name' => [['x-y * 2'], false, 'SELECT "x-y * 2" AS "x-y * 2" FROM "foo"'], - 'inline alias' => [['bar AS baz'], true, 'SELECT "foo"."bar" AS "baz" FROM "foo"'], - 'inline alias lower case' => [['bar as baz'], true, 'SELECT "foo"."bar" as "baz" FROM "foo"'], - 'inline alias without prefix' => [['t.bar AS baz'], false, 'SELECT "t"."bar" AS "baz" FROM "foo"'], - 'expression' => [ + 'inline alias' => [['bar AS baz'], true, 'SELECT "foo"."bar" AS "baz" FROM "foo"'], + 'inline alias lower case' => [['bar as baz'], true, 'SELECT "foo"."bar" as "baz" FROM "foo"'], + 'inline alias without prefix' => [['t.bar AS baz'], false, 'SELECT "t"."bar" AS "baz" FROM "foo"'], + 'expression' => [ [new Expression('COUNT(id)')], true, 'SELECT COUNT(id) AS Expression1 FROM "foo"', ], - 'expression aliased' => [ + 'expression aliased' => [ ['c' => new Expression('COUNT(id)')], true, 'SELECT COUNT(id) AS "c" FROM "foo"', @@ -131,6 +129,29 @@ public static function selectColumnProvider(): array ]; } + /** @return array */ + public static function unquotableColumnProvider(): array + { + return [ + 'select function call' => [ + static fn(Select $select): Select => $select->columns(['COUNT(id)']), + 'COUNT(id)', + ], + 'select arithmetic with hyphen' => [ + static fn(Select $select): Select => $select->columns(['x-y * 2']), + 'x-y * 2', + ], + 'group function call' => [ + static fn(Select $select): Select => $select->group('YEAR(created)'), + 'YEAR(created)', + ], + 'join function call' => [ + static fn(Select $select): Select => $select->join('zac', 'm = n', ['COUNT(id)']), + 'COUNT(id)', + ], + ]; + } + #[Test] #[DataProvider('groupColumnProvider')] public function quotesGroupColumnStringsAsIdentifierChains(string|Expression $column, string $expected): void @@ -172,4 +193,16 @@ public function quotesSelectColumnStringsAsIdentifierChains( self::assertSame($expected, $select->getSqlString(new TrustingSql92Platform())); } + + /** @param callable(Select): Select $build */ + #[Test] + #[DataProvider('unquotableColumnProvider')] + public function rejectsColumnStringsHoldingExpressionSyntax(callable $build, string $column): void + { + $select = $build(new Select('foo')); + + $this->expectExceptionObject(InvalidArgumentException::forUnquotableIdentifier($column)); + + $select->getSqlString(new TrustingSql92Platform()); + } } diff --git a/test/unit/TestAsset/TrustingHyphenWordPlatform.php b/test/unit/TestAsset/TrustingHyphenWordPlatform.php new file mode 100644 index 000000000..1e65219ed --- /dev/null +++ b/test/unit/TestAsset/TrustingHyphenWordPlatform.php @@ -0,0 +1,27 @@ +quoteTrustedValue($value); + } +} From 3b2c1402dda47d7f761abb1133f9d0ed1c92e9a3 Mon Sep 17 00:00:00 2001 From: Simon Mundy Date: Wed, 4 Nov 2026 16:24:41 +1100 Subject: [PATCH 5/6] Quote ORDER BY names through the shared IdentifierQuoter Replace quoteOrderIdentifier() with IdentifierQuoter::quote(), so ORDER BY names follow the same rule as columns: "t"."as" rather than the invalid "t".as, and arithmetic mixed with a hyphenated name, such as price*x-y, now throws instead of becoming one identifier. The tests use the shared TrustingHyphenWordPlatform, replacing HyphenWordSql92Platform. --- src/Sql/Select.php | 34 +-------------- test/unit/Sql/SelectOrderNameTest.php | 43 +++++++++++++++---- .../TestAsset/HyphenWordSql92Platform.php | 15 ------- 3 files changed, 36 insertions(+), 56 deletions(-) delete mode 100644 test/unit/TestAsset/HyphenWordSql92Platform.php diff --git a/src/Sql/Select.php b/src/Sql/Select.php index d7079628a..a8ffb0437 100644 --- a/src/Sql/Select.php +++ b/src/Sql/Select.php @@ -13,11 +13,9 @@ use function array_key_exists; use function array_key_first; -use function array_map; use function count; use function explode; use function gettype; -use function implode; use function is_array; use function is_int; use function is_numeric; @@ -26,7 +24,6 @@ use function key; use function method_exists; use function preg_split; -use function str_replace; use function strcasecmp; use function stripos; use function strtolower; @@ -610,7 +607,7 @@ protected function processOrder( } $orders[] = [ - $this->quoteOrderIdentifier($k, $platform), + IdentifierQuoter::quote($k, $platform), strcasecmp(trim($v), self::ORDER_DESCENDING) === 0 ? self::ORDER_DESCENDING : self::ORDER_ASCENDING, @@ -797,35 +794,6 @@ private function prefixFromTable(?string $fromTable, PlatformInterface $platform : ''; } - /** - * Quote an order name, keeping fragment quoting only where it renders each part as a single identifier - * - * Fragment quoting splits on every non-word character, so a name such as `x-y` or `my col` would render - * as several quoted pieces. Such a name is quoted whole as an identifier chain instead, split only on - * the table qualifier dot. Names whose parts between `*` and `.` already quote whole, such as `t.col` - * or `price*qty`, render exactly as before. - */ - private function quoteOrderIdentifier(string $name, PlatformInterface $platform): string - { - foreach (explode('.', str_replace(self::SQL_STAR, replace: '.', subject: $name)) as $segment) { - $segment = trim($segment, characters: ' '); - $quoted = $platform->quoteIdentifierInFragment($segment); - if ($quoted !== $segment && $quoted !== $platform->quoteIdentifier($segment)) { - return implode( - $platform->getIdentifierSeparator(), - array_map( - static fn(string $part): string => self::SQL_STAR === $part - ? $part - : $platform->quoteIdentifier($part), - explode('.', $name), - ), - ); - } - } - - return $platform->quoteIdentifierInFragment($name); - } - /** * @param Select|TableReference|null $table * @return array{0: string|null, 1: string} diff --git a/test/unit/Sql/SelectOrderNameTest.php b/test/unit/Sql/SelectOrderNameTest.php index d400142b0..1a9200eaa 100644 --- a/test/unit/Sql/SelectOrderNameTest.php +++ b/test/unit/Sql/SelectOrderNameTest.php @@ -5,11 +5,12 @@ namespace PhpDbTest\Sql; use PhpDb\Adapter\Platform\PlatformInterface; +use PhpDb\Sql\Exception\InvalidArgumentException; use PhpDb\Sql\Expression; use PhpDb\Sql\ExpressionInterface; use PhpDb\Sql\Select; -use PhpDbTest\TestAsset\HyphenWordSql92Platform; use PhpDbTest\TestAsset\TestSql92Platform; +use PhpDbTest\TestAsset\TrustingHyphenWordPlatform; use PhpDbTest\TestAsset\TrustingSql92Platform; use PHPUnit\Framework\Attributes\CoversMethod; use PHPUnit\Framework\Attributes\DataProvider; @@ -20,7 +21,6 @@ #[Group('unit')] #[CoversMethod(Select::class, 'order')] #[CoversMethod(Select::class, 'processOrder')] -#[CoversMethod(Select::class, 'quoteOrderIdentifier')] final class SelectOrderNameTest extends TestCase { /** @@ -35,7 +35,6 @@ public static function brokenOrderProvider(): array 'quote character' => ['a"b DESC', '"a\'b" DESC'], 'non-ascii' => ['café DESC', '"café" DESC'], 'hyphen in comma list' => ['x-y DESC, b', '"x-y" DESC, "b" ASC'], - 'arithmetic with hyphen' => ['price*x-y DESC', '"price*x-y" DESC'], 'hyphen array key' => [['x-y' => 'DESC'], '"x-y" DESC'], 'space array key' => [['my col' => 'DESC'], '"my col" DESC'], 'dotted space array key' => [['t.my col' => 'ASC'], '"t"."my col" ASC'], @@ -43,6 +42,7 @@ public static function brokenOrderProvider(): array 'non-ascii array key' => [['café' => 'DESC'], '"café" DESC'], 'hyphen array list' => [['x-y DESC', 'b'], '"x-y" DESC, "b" ASC'], 'hyphen qualified star' => ['x-y.* DESC', '"x-y".* DESC'], + 'as after qualifier' => [['t.as' => 'DESC'], '"t"."as" DESC'], ]; } @@ -52,13 +52,12 @@ public static function brokenOrderProvider(): array public static function platformOrderProvider(): array { return [ - 'hyphen word character' => [new HyphenWordSql92Platform(), 'x-y DESC', '"x-y" DESC'], + 'hyphen word character' => [new TrustingHyphenWordPlatform(), 'x-y DESC', '`x-y` DESC'], 'hyphen word character arithmetic' => [ - new HyphenWordSql92Platform(), + new TrustingHyphenWordPlatform(), 'price*x-y DESC', - '"price"*"x-y" DESC', + '`price`*`x-y` DESC', ], - 'hyphen word character with plus' => [new HyphenWordSql92Platform(), 'x+y DESC', '"x+y" DESC'], 'identifier quoting off' => [ new TestSql92Platform(quoteIdentifiers: false), 'x-y DESC', @@ -72,6 +71,18 @@ public static function platformOrderProvider(): array ]; } + /** + * @return array|string, string}> + */ + public static function unquotableOrderProvider(): array + { + return [ + 'arithmetic with hyphen' => [new TrustingSql92Platform(), 'price*x-y DESC', 'price*x-y'], + 'function call' => [new TrustingSql92Platform(), ['COUNT(id)' => 'DESC'], 'COUNT(id)'], + 'plus on hyphen platform' => [new TrustingHyphenWordPlatform(), 'x+y DESC', 'x+y'], + ]; + } + /** * @return array|string, string}> */ @@ -90,7 +101,6 @@ public static function validOrderProvider(): array 'spaced arithmetic key' => [['price * qty' => 'DESC'], '"price" * "qty" DESC'], 'dotted arithmetic key' => [['t.price*t.qty' => 'DESC'], '"t"."price"*"t"."qty" DESC'], 'trailing space key' => [['a ' => 'DESC'], '"a" DESC'], - 'safe word part' => [['t.as' => 'DESC'], '"t".as DESC'], 'array list' => [['a DESC', 'b'], '"a" DESC, "b" ASC'], 'expression' => [new Expression('RAND()'), 'RAND()'], 'expression in array' => [['a', new Expression('RAND()')], '"a" ASC, RAND()'], @@ -121,6 +131,23 @@ public function quotesOrderNameAsIdentifierChainWhenFragmentQuotingWouldBreakIt( self::assertSame("SELECT \"foo\".* FROM \"foo\" ORDER BY {$expected}", $this->render($order)); } + /** + * @param array|string $order + */ + #[Test] + #[DataProvider('unquotableOrderProvider')] + public function rejectsOrderNamesHoldingExpressionSyntax( + PlatformInterface $platform, + array|string $order, + string $name, + ): void { + $select = (new Select('foo'))->order($order); + + $this->expectExceptionObject(InvalidArgumentException::forUnquotableIdentifier($name)); + + $select->getSqlString($platform); + } + /** * @param ExpressionInterface|array|string $order */ diff --git a/test/unit/TestAsset/HyphenWordSql92Platform.php b/test/unit/TestAsset/HyphenWordSql92Platform.php deleted file mode 100644 index 224cbe48c..000000000 --- a/test/unit/TestAsset/HyphenWordSql92Platform.php +++ /dev/null @@ -1,15 +0,0 @@ - Date: Wed, 4 Nov 2026 17:04:51 +1100 Subject: [PATCH 6/6] Cover the identifier exception factories in InvalidArgumentExceptionTest The test restricts coverage to the factories it lists, so the new forEmptyIdentifierPart() and forUnquotableIdentifier() counted as uncovered and failed codecov/patch. --- .../Sql/Exception/InvalidArgumentExceptionTest.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php b/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php index 5a77eebfd..b4c921f6f 100644 --- a/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php +++ b/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php @@ -22,6 +22,7 @@ #[CoversMethod(InvalidArgumentException::class, 'forMissingColumnLength')] #[CoversMethod(InvalidArgumentException::class, 'forMissingDecimalDigits')] #[CoversMethod(InvalidArgumentException::class, 'forEmptyExpression')] +#[CoversMethod(InvalidArgumentException::class, 'forEmptyIdentifierPart')] #[CoversMethod(InvalidArgumentException::class, 'forEmptyPrefix')] #[CoversMethod(InvalidArgumentException::class, 'forEmptySchema')] #[CoversMethod(InvalidArgumentException::class, 'forEmptySeparator')] @@ -40,6 +41,7 @@ #[CoversMethod(InvalidArgumentException::class, 'forSelectWithMergeFlag')] #[CoversMethod(InvalidArgumentException::class, 'forUnenclosedValuesPlaceholder')] #[CoversMethod(InvalidArgumentException::class, 'forUnknownArgumentType')] +#[CoversMethod(InvalidArgumentException::class, 'forUnquotableIdentifier')] #[CoversMethod(InvalidArgumentException::class, 'forUnknownColumnKey')] #[CoversMethod(InvalidArgumentException::class, 'forValuesWithMergeFlag')] final class InvalidArgumentExceptionTest extends TestCase @@ -91,6 +93,11 @@ public static function namedConstructorProvider(): array [], InvalidArgumentException::EMPTY_EXPRESSION, ], + 'empty identifier part' => [ + 'forEmptyIdentifierPart', + ['a..b'], + InvalidArgumentException::EMPTY_IDENTIFIER_PART, + ], 'empty prefix' => [ 'forEmptyPrefix', [], @@ -181,6 +188,11 @@ public static function namedConstructorProvider(): array ['email'], InvalidArgumentException::UNKNOWN_COLUMN_KEY, ], + 'unquotable identifier' => [ + 'forUnquotableIdentifier', + ['COUNT(id)'], + InvalidArgumentException::UNQUOTABLE_IDENTIFIER, + ], 'values with merge flag' => [ 'forValuesWithMergeFlag', [],