diff --git a/docs/book/sql/advanced.md b/docs/book/sql/advanced.md index 4540209d..1cdf55f8 100644 --- a/docs/book/sql/advanced.md +++ b/docs/book/sql/advanced.md @@ -46,7 +46,7 @@ CASE WHEN age > 18 THEN ADULT ELSE MINOR END ```php title="Array values in expressions" $expression = new Expression( 'id IN (?)', - [Argument::value([1, 2, 3, 4, 5])] + [Argument::values([1, 2, 3, 4, 5])] ); ``` @@ -56,6 +56,14 @@ Produces: id IN (?, ?, ?, ?, ?) ``` +The placeholder for an array (or `Argument\Values`) expands to one +placeholder per value, separated by commas, so write the surrounding +parentheses in the expression yourself. An empty array renders as `NULL`. +The placeholder must sit alone inside its parentheses: `id IN ?` and +`id IN (?, ?)` with an array throw +`PhpDb\Sql\Exception\InvalidArgumentException`, since they would render +invalid SQL. + ```php title="Nested expressions" $innerExpression = new Expression('COUNT(*)'); $outerExpression = new Expression( diff --git a/src/Sql/Exception/InvalidArgumentException.php b/src/Sql/Exception/InvalidArgumentException.php index 730e4c17..8066080c 100644 --- a/src/Sql/Exception/InvalidArgumentException.php +++ b/src/Sql/Exception/InvalidArgumentException.php @@ -72,6 +72,10 @@ class InvalidArgumentException extends Exception\InvalidArgumentException 'A PhpDb\Sql\Select instance cannot be provided' . ' with the merge flag'; + final public const string UNENCLOSED_VALUES_PLACEHOLDER = + 'An array or Values parameter in the expression "%s" must use a placeholder' + . ' wrapped in its own parentheses, such as IN (?)'; + final public const string UNKNOWN_ARGUMENT_TYPE = 'Unknown argument type'; final public const string UNKNOWN_COLUMN_KEY = 'The key %s was not found in this objects column list'; @@ -188,6 +192,11 @@ public static function forSelectWithMergeFlag(): self return new self(self::SELECT_WITH_MERGE_FLAG); } + public static function forUnenclosedValuesPlaceholder(string $expression): self + { + return new self(sprintf(self::UNENCLOSED_VALUES_PLACEHOLDER, $expression)); + } + public static function forUnknownArgumentType(): self { return new self(self::UNKNOWN_ARGUMENT_TYPE); diff --git a/src/Sql/Expression.php b/src/Sql/Expression.php index 4450c39b..2386bfb9 100644 --- a/src/Sql/Expression.php +++ b/src/Sql/Expression.php @@ -9,14 +9,18 @@ use PhpDb\Sql\Argument\Value; use PhpDb\Sql\Argument\Values; +use function array_shift; use function array_slice; use function array_unique; use function count; +use function explode; use function func_get_args; use function func_num_args; use function is_array; +use function preg_match; use function preg_match_all; use function str_replace; +use function substr; /** * @api @@ -66,6 +70,7 @@ public function getExpression(): string } /** + * @throws Exception\InvalidArgumentException * @throws Exception\RuntimeException * @inheritDoc */ @@ -83,13 +88,16 @@ public function getExpressionData(): array ]; } - // assign locally, escaping % signs - $count = 0; - $specification = str_replace(self::PLACEHOLDER, replace: '%s', subject: $specification, count: $count); + $segments = explode(self::PLACEHOLDER, $specification); + $specification = array_shift($segments); + + foreach ($segments as $index => $segment) { + $specification .= $this->placeholderFor($parameters[$index] ?? null, $specification, $segment) . $segment; + } // test number of replacements without considering same variable begin used many times first, which is // faster, if the test fails then resort to regex which are slow and used rarely - if ($count !== $parametersCount) { + if (count($segments) !== $parametersCount) { $matches = []; preg_match_all('/:\w*/', $specification, $matches); if (count(array_unique($matches[0] ?? [])) !== $parametersCount) { @@ -144,4 +152,22 @@ public function setParameters( return $this; } + + /** + * Values are bound one by one, so each needs its own placeholder inside the parentheses around the `?` + * + * @throws Exception\InvalidArgumentException + */ + private function placeholderFor(?ArgumentInterface $parameter, string $before, string $after): string + { + if (! $parameter instanceof Values) { + return '%s'; + } + + if (1 !== preg_match('/\(\s*$/', $before) || 1 !== preg_match('/^\s*\)/', $after)) { + throw Exception\InvalidArgumentException::forUnenclosedValuesPlaceholder($this->expression); + } + + return substr($parameter->getSpecification(), offset: 1, length: -1); + } } diff --git a/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php b/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php index 39ff8fd2..5a77eebf 100644 --- a/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php +++ b/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php @@ -38,6 +38,7 @@ #[CoversMethod(InvalidArgumentException::class, 'forNonStringValueKey')] #[CoversMethod(InvalidArgumentException::class, 'forReadOnlyConstructorState')] #[CoversMethod(InvalidArgumentException::class, 'forSelectWithMergeFlag')] +#[CoversMethod(InvalidArgumentException::class, 'forUnenclosedValuesPlaceholder')] #[CoversMethod(InvalidArgumentException::class, 'forUnknownArgumentType')] #[CoversMethod(InvalidArgumentException::class, 'forUnknownColumnKey')] #[CoversMethod(InvalidArgumentException::class, 'forValuesWithMergeFlag')] @@ -60,122 +61,127 @@ public static function foreignTableProvider(): array public static function namedConstructorProvider(): array { return [ - 'missing decimal digits' => [ + 'missing decimal digits' => [ 'forMissingDecimalDigits', ['price', 'DECIMAL'], InvalidArgumentException::MISSING_DECIMAL_DIGITS, ], - 'missing column length' => [ + 'missing column length' => [ 'forMissingColumnLength', ['name', 'VARCHAR'], InvalidArgumentException::MISSING_COLUMN_LENGTH, ], - 'invalid column length' => [ + 'invalid column length' => [ 'forInvalidColumnLength', ['age'], InvalidArgumentException::INVALID_COLUMN_LENGTH, ], - 'empty check expression' => [ + 'empty check expression' => [ 'forEmptyCheckExpression', [], InvalidArgumentException::EMPTY_CHECK_EXPRESSION, ], - 'already combined' => [ + 'already combined' => [ 'forAlreadyCombined', [], InvalidArgumentException::ALREADY_COMBINED, ], - 'empty expression' => [ + 'empty expression' => [ 'forEmptyExpression', [], InvalidArgumentException::EMPTY_EXPRESSION, ], - 'empty prefix' => [ + 'empty prefix' => [ 'forEmptyPrefix', [], InvalidArgumentException::EMPTY_PREFIX, ], - 'empty schema' => [ + 'empty schema' => [ 'forEmptySchema', [], InvalidArgumentException::EMPTY_SCHEMA, ], - 'empty separator' => [ + 'empty separator' => [ 'forEmptySeparator', [], InvalidArgumentException::EMPTY_SEPARATOR, ], - 'empty table' => [ + 'empty table' => [ 'forEmptyTable', [], InvalidArgumentException::EMPTY_TABLE, ], - 'foreign table' => [ + 'foreign table' => [ 'forForeignTable', ['users'], InvalidArgumentException::FOREIGN_TABLE, ], - 'invalid argument type' => [ + 'invalid argument type' => [ 'forInvalidArgumentType', [], InvalidArgumentException::INVALID_ARGUMENT_TYPE, ], - 'invalid from array' => [ + 'invalid from array' => [ 'forInvalidFromArray', [], InvalidArgumentException::INVALID_FROM_ARRAY, ], - 'invalid join name' => [ + 'invalid join name' => [ 'forInvalidJoinName', ['orders'], InvalidArgumentException::INVALID_JOIN_NAME, ], - 'invalid magic property' => [ + 'invalid magic property' => [ 'forInvalidMagicProperty', [], InvalidArgumentException::INVALID_MAGIC_PROPERTY, ], - 'invalid specification name' => [ + 'invalid specification name' => [ 'forInvalidSpecificationName', [], InvalidArgumentException::INVALID_SPECIFICATION_NAME, ], - 'missing values or select' => [ + 'missing values or select' => [ 'forMissingValuesOrSelect', [], InvalidArgumentException::MISSING_VALUES_OR_SELECT, ], - 'non numeric parameter' => [ + 'non numeric parameter' => [ 'forNonNumericParameter', ['PhpDb\\Sql\\Select::limit', 'string'], InvalidArgumentException::NON_NUMERIC_PARAMETER, ], - 'non string value key' => [ + 'non string value key' => [ 'forNonStringValueKey', [], InvalidArgumentException::NON_STRING_VALUE_KEY, ], - 'read only constructor state' => [ + 'read only constructor state' => [ 'forReadOnlyConstructorState', [], InvalidArgumentException::READ_ONLY_CONSTRUCTOR_STATE, ], - 'select with merge flag' => [ + 'select with merge flag' => [ 'forSelectWithMergeFlag', [], InvalidArgumentException::SELECT_WITH_MERGE_FLAG, ], - 'unknown argument type' => [ + 'unenclosed values placeholder' => [ + 'forUnenclosedValuesPlaceholder', + ['a IN ?'], + InvalidArgumentException::UNENCLOSED_VALUES_PLACEHOLDER, + ], + 'unknown argument type' => [ 'forUnknownArgumentType', [], InvalidArgumentException::UNKNOWN_ARGUMENT_TYPE, ], - 'unknown column key' => [ + 'unknown column key' => [ 'forUnknownColumnKey', ['email'], InvalidArgumentException::UNKNOWN_COLUMN_KEY, ], - 'values with merge flag' => [ + 'values with merge flag' => [ 'forValuesWithMergeFlag', [], InvalidArgumentException::VALUES_WITH_MERGE_FLAG, diff --git a/test/unit/Sql/ExpressionTest.php b/test/unit/Sql/ExpressionTest.php index 2237448d..5c019dba 100644 --- a/test/unit/Sql/ExpressionTest.php +++ b/test/unit/Sql/ExpressionTest.php @@ -18,6 +18,8 @@ use PHPUnit\Framework\TestCase; use TypeError; +use function sprintf; + /** * This is a unit testing test case. * A unit here is a method, there will be at least one test per method @@ -30,8 +32,25 @@ #[CoversMethod(Expression::class, 'setParameters')] #[CoversMethod(Expression::class, 'getParameters')] #[CoversMethod(Expression::class, 'getExpressionData')] +#[CoversMethod(Expression::class, 'placeholderFor')] final class ExpressionTest extends TestCase { + /** @return array, string}> */ + public static function arrayParameterSpecificationProvider(): array + { + return [ + 'array of three values' => ['a IN (?)', [[1, 2, 3]], 'a IN (%s, %s, %s)'], + 'empty array' => ['a IN (?)', [[]], 'a IN (NULL)'], + 'array between other parameters' => [ + '? IN (?) AND ? = ?', + [Argument::identifier('a'), [1, 2], Argument::identifier('b'), 4], + '%s IN (%s, %s) AND %s = %s', + ], + 'values argument' => ['a IN (?)', [Argument::values([1, 2])], 'a IN (%s, %s)'], + 'whitespace inside parentheses' => ['a IN ( ? )', [[1, 2]], 'a IN ( %s, %s )'], + ]; + } + /** @psalm-return array */ public static function falsyExpressionParametersProvider(): array { @@ -44,6 +63,19 @@ public static function falsyExpressionParametersProvider(): array ]; } + /** @return array}> */ + public static function unenclosedArrayParameterProvider(): array + { + return [ + 'array' => ['a IN ?', [[1, 2, 3]]], + 'empty array' => ['a IN ?', [[]]], + 'values argument' => ['a IN ?', [Argument::values([1, 2])]], + 'opening parenthesis only' => ['a IN (?', [[1, 2]]], + 'closing parenthesis only' => ['a IN ?)', [[1, 2]]], + 'parenthesis around more' => ['a IN (?, ?)', [[1, 2], 3]], + ]; + } + #[Test] #[DataProvider('falsyExpressionParametersProvider')] public function constructorWithFalsyValidParameters(mixed $falsyParameter): void @@ -113,6 +145,38 @@ public function getExpressionData(): void ); } + /** + * @param list $parameters + */ + #[Test] + #[Group('unit')] + #[DataProvider('arrayParameterSpecificationProvider')] + public function getExpressionDataGivesEachArrayValueItsOwnPlaceholder( + string $expression, + array $parameters, + string $expectedSpecification, + ): void { + $expressionData = (new Expression($expression, $parameters))->getExpressionData(); + + static::assertSame($expectedSpecification, $expressionData['spec']); + } + + /** + * @param list $parameters + */ + #[Test] + #[Group('unit')] + #[DataProvider('unenclosedArrayParameterProvider')] + public function getExpressionDataRejectsArrayPlaceholderOutsideParentheses( + string $expression, + array $parameters, + ): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage(sprintf(InvalidArgumentException::UNENCLOSED_VALUES_PLACEHOLDER, $expression)); + + (new Expression($expression, $parameters))->getExpressionData(); + } + #[Test] public function getExpressionDataThrowsExceptionWhenParameterCountMismatch(): void { diff --git a/test/unit/Sql/SelectTest.php b/test/unit/Sql/SelectTest.php index 0daf0ce8..7bf7f212 100644 --- a/test/unit/Sql/SelectTest.php +++ b/test/unit/Sql/SelectTest.php @@ -4,10 +4,12 @@ namespace PhpDbTest\Sql; +use Closure; use PhpDb\Adapter\Driver\DriverInterface; use PhpDb\Adapter\Driver\StatementInterface; use PhpDb\Adapter\ParameterContainer; use PhpDb\Adapter\Platform\Sql92; +use PhpDb\Adapter\StatementContainer; use PhpDb\Sql\Argument; use PhpDb\Sql\Argument\Identifier; use PhpDb\Sql\Argument\Value; @@ -83,6 +85,28 @@ final class SelectTest extends TestCase { use AdapterTestTrait; + /** + * @return array}, string, array}> + */ + public static function arrayExpressionParameterProvider(): array + { + return [ + 'prepared' => [ + static fn(self $test, Select $select): array => $test->prepareWithNamedParameters($select), + 'SELECT "t".* FROM "t" WHERE a IN (:where1, :where2, :where3) AND "b" = :where4', + ['where1' => 1, 'where2' => 2, 'where3' => 3, 'where4' => 4], + ], + 'unprepared' => [ + static fn(self $test, Select $select): array => [ + $select->getSqlString(new TrustingSql92Platform()), + [], + ], + 'SELECT "t".* FROM "t" WHERE a IN (\'1\', \'2\', \'3\') AND "b" = \'4\'', + [], + ], + ]; + } + /** * @psalm-return array} $render + * @param array $expectedParameters + */ + #[Test] + #[Group('unit')] + #[DataProvider('arrayExpressionParameterProvider')] + public function arrayExpressionParameterDoesNotShiftValuesIntoFollowingPredicates( + Closure $render, + string $expectedSql, + array $expectedParameters, + ): void { + $select = new Select('t'); + $select->where([new Predicate\Expression('a IN (?)', [[1, 2, 3]]), 'b' => 4]); + + static::assertSame([$expectedSql, $expectedParameters], $render($this, $select)); + } + #[Test] #[TestDox('unit test: Test join() exception with bad join')] public function badJoin(): void @@ -1918,4 +1960,20 @@ public function whereReturnsSameSelectObject(): void $select = new Select(); static::assertSame($select, $select->where('x = y')); } + + /** + * @return array{?string, array} + */ + private function prepareWithNamedParameters(Select $select): array + { + $driver = $this->createStub(DriverInterface::class); + $driver->method('formatParameterName')->willReturnCallback(static fn(string $name): string => ":{$name}"); + + $parameterContainer = new ParameterContainer(); + $statement = new StatementContainer(null, $parameterContainer); + + $select->prepareStatement($this->createMockAdapter($driver, new TrustingSql92Platform()), $statement); + + return [$statement->getSql(), $parameterContainer->getNamedArray()]; + } }