Quote ORDER BY names as identifiers where fragment quoting breaks them

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);

src/Sql/IdentifierPiece.php

@@ -0,0 +1,63 @@
+<?php
+
+declare(strict_types=1);
+
+namespace PhpDb\Sql;
+
+use PhpDb\Adapter\Platform\PlatformInterface;
+
+use function preg_match;
+use function strtolower;
+
+/**
+ * What a piece of an identifier string is, read between spaces, qualifier dots and `*`
+ *
+ * @internal
+ */
+enum IdentifierPiece
+{
+    /** A qualifier dot */
+    case Dot;
+
+    /** A `*` directly after a qualifier dot */
+    case QualifiedStar;
+
+    /** A `*` operator or an `AS` keyword, which fragment quoting leaves bare */
+    case Keyword;
+
+    /** A name fragment quoting renders as one identifier */
+    case Name;
+
+    /** A name fragment quoting would split into several quoted pieces */
+    case Whole;
+
+    /** An operator or expression syntax, which no identifier quoting renders as written */
+    case Unquotable;
+
+    /** A piece with no letter, digit or underscore, such as `-` or `=`, is an operator */
+    private const string OPERATOR = '/^[^\p{L}\p{N}_$]+$/u';
+
+    private const string EXPRESSION_CHARACTERS = '/[()=<>+\/%|!~^&;]/';
+
+    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,
+        };
+    }
+}

src/Sql/IdentifierQuoter.php

@@ -0,0 +1,94 @@
+<?php
+
+declare(strict_types=1);
+
+namespace PhpDb\Sql;
+
+use PhpDb\Adapter\Platform\PlatformInterface;
+
+use function array_map;
+use function count;
+use function explode;
+use function implode;
+use function preg_match;
+use function preg_split;
+use function trim;
+
+/**
+ * Quotes identifier strings, keeping fragment quoting wherever it renders them as written
+ *
+ * Fragment quoting quotes every word the platform splits out, which renders qualified names (`t.col`)
+ * and simple expressions (`price*qty`) correctly but breaks a name holding any other character
+ * (`x-y`, `my col`) into adjacent quoted pieces. Such a name is quoted whole instead, split only on
+ * the qualifier dot. A string holding other expression syntax, such as an operator or a function call,
+ * or mixing a whole-quoted name with `*` or `AS`, has no correct quoting, so it is rejected; pass it as
+ * an Expression instead.
+ *
+ * @internal
+ */
+final class IdentifierQuoter
+{
+    /**
+     * Word characters and qualifier dots, optionally ending in `.*`, which fragment quoting renders as
+     * written on every platform; a part that is the `AS` keyword is left to the full check
+     */
+    private const string PLAIN_NAME = '/^(?!as\b)\w+(?:\.(?!as\b)\w+)*(?:\.\*)?\z/i';
+
+    private const string ALIAS_KEYWORD = '/\s+as\s+/i';
+
+    /**
+     * @throws Exception\InvalidArgumentException When the name has an empty part, or holds expression
+     *     syntax that no identifier quoting renders as written.
+     */
+    public static function quote(string $name, PlatformInterface $platform): string
+    {
+        if (1 === preg_match(self::PLAIN_NAME, $name)) {
+            return $platform->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<string> $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)),
+            ),
+        );
+    }
+}

src/Sql/IdentifierShape.php

@@ -0,0 +1,80 @@
+<?php
+
+declare(strict_types=1);
+
+namespace PhpDb\Sql;
+
+use PhpDb\Adapter\Platform\PlatformInterface;
+
+use function preg_split;
+
+use const PREG_SPLIT_DELIM_CAPTURE;
+use const PREG_SPLIT_NO_EMPTY;
+
+/**
+ * How an identifier string must be quoted to render as written
+ *
+ * The string is read in pieces between spaces, qualifier dots and `*`, each classified by IdentifierPiece.
+ *
+ * @internal
+ */
+enum IdentifierShape
+{
+    /** Fragment quoting renders it as written */
+    case Fragment;
+
+    /** It holds a name that fragment quoting would split, so each qualifier part is quoted whole */
+    case Whole;
+
+    /** It holds expression syntax that no identifier quoting renders as written */
+    case Unquotable;
+
+    /** It is empty, or a qualifier dot leaves an empty part */
+    case EmptyPart;
+
+    /**
+     * The pattern must keep exactly one capturing group, so spaces, dots and `*` come back as pieces
+     * of their own
+     */
+    private const string PIECES = '/([ .*])/';
+
+    public static function of(string $name, PlatformInterface $platform): self
+    {
+        $whole    = false;
+        $keyword  = false;
+        $previous = null;
+        $spaced   = false;
+
+        /** @var list<string> $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,
+        };
+    }
+}

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;
@@ -37,6 +38,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<string, string|TableIdentifier|Select>
@@ -605,7 +608,7 @@ protected function processOrder(
             }
 
             $orders[] = [
-                $platform->quoteIdentifierInFragment($k),
+                IdentifierQuoter::quote($k, $platform),
                 strcasecmp(trim($v), self::ORDER_DESCENDING) === 0
                     ? self::ORDER_DESCENDING
                     : self::ORDER_ASCENDING,
@@ -753,6 +756,27 @@ protected function processWhere(
         ];
     }
 
+    /**
+     * Quote a select, join or group column string through IdentifierQuoter
+     *
+     * @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'] ?? '') . IdentifierQuoter::quoteColumn($column['column'], $platform);
+        }
+
+        return parent::resolveColumnValue($column, $platform, $driver, $parameterContainer, $namedParameterPrefix);
+    }
+
     /**
      * Copy each parameter with its type and maximum length, keeping their order
      */

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',
                 [],