diff --git a/docs/book/sql/select.md b/docs/book/sql/select.md index 2a667fef3..2f3823562 100644 --- a/docs/book/sql/select.md +++ b/docs/book/sql/select.md @@ -195,6 +195,12 @@ $select->limit(5); $select->offset(10); ``` +Both take a non-negative integer, as an `int` or a string of digits such as +`'10'`. A digit string is kept as given, so a count larger than `PHP_INT_MAX` +still works. Anything else, including negative numbers, fractions, exponents +and surrounding whitespace, throws +`PhpDb\Sql\Exception\InvalidArgumentException`. + ## group() The `group()` method specifies columns for GROUP BY clauses, diff --git a/src/Sql/AbstractSql.php b/src/Sql/AbstractSql.php index 25c6276e5..566a50385 100644 --- a/src/Sql/AbstractSql.php +++ b/src/Sql/AbstractSql.php @@ -16,6 +16,7 @@ use PhpDb\Sql\Argument\Value; use PhpDb\Sql\Argument\Values; use PhpDb\Sql\Platform\PlatformDecoratorInterface; +use Stringable; use function array_key_exists; use function array_key_first; @@ -27,6 +28,7 @@ use function preg_match; use function preg_replace; use function rtrim; +use function sprintf; use function str_replace; use function strtoupper; use function vsprintf; @@ -36,7 +38,11 @@ * * @psalm-type ParameterSpecification = array|null * @psalm-type Specification = string|array> - * @psalm-type SqlParameters = array>> + * @psalm-type SqlParameterValue = string|int|float|bool|Stringable|null + * @psalm-type SqlParameters = array< + * array-key, + * SqlParameterValue|array>, + * > * @psalm-type ColumnValue = Select|ExpressionInterface|string|int|float|bool|null * @psalm-type ColumnReference = array{ * column: ColumnValue, @@ -572,19 +578,19 @@ protected function resolveTable( } /** - * @param array> $values + * @param array> $values * @return list - * @throws Exception\RuntimeException When a value is a nested list rather than a string. + * @throws Exception\RuntimeException When a value is a nested list rather than a scalar. */ private function flattenParameterList(array $values): array { $strings = []; foreach ($values as $value) { - if (! is_string($value)) { + if (is_array($value)) { throw Exception\RuntimeException::forUnsupportedParameterCountOf(count($value)); } - $strings[] = $value; + $strings[] = $this->stringifyParameter($value); } return $strings; @@ -592,7 +598,7 @@ private function flattenParameterList(array $values): array /** * @param array $parameterSpecification - * @param array> $values + * @param array> $values * @throws Exception\RuntimeException When no format accepts the number of values. */ private function formatParameterList(array $parameterSpecification, array $values): string @@ -633,27 +639,42 @@ private function quoteJoinTableIdentifier(TableIdentifier $identifier, PlatformI /** * @param ParameterSpecification $parameterSpecification - * @param string|array> $parameter + * @param SqlParameterValue|array> $parameter * @throws Exception\RuntimeException */ - private function renderParameter(?array $parameterSpecification, string|array $parameter): string - { + private function renderParameter( + ?array $parameterSpecification, + string|int|float|bool|Stringable|array|null $parameter, + ): string { if (null === $parameterSpecification) { - return is_string($parameter) - ? $parameter - : throw Exception\RuntimeException::forUnsupportedParameterCountOf(count($parameter)); + return is_array($parameter) + ? throw Exception\RuntimeException::forUnsupportedParameterCountOf(count($parameter)) + : $this->stringifyParameter($parameter); } + $parameters = is_array($parameter) ? $parameter : [$parameter]; + $combinedBy = $parameterSpecification['combinedby'] ?? null; if (null === $combinedBy) { - return $this->formatParameterList($parameterSpecification, (array) $parameter); + return $this->formatParameterList($parameterSpecification, $parameters); } $rendered = []; - foreach ((array) $parameter as $values) { - $rendered[] = $this->formatParameterList($parameterSpecification, (array) $values); + foreach ($parameters as $values) { + $rendered[] = $this->formatParameterList( + $parameterSpecification, + is_array($values) ? $values : [$values], + ); } return implode($combinedBy, $rendered); } + + /** + * Converts a scalar clause parameter to the text vsprintf's %s conversion would produce. + */ + private function stringifyParameter(string|int|float|bool|Stringable|null $value): string + { + return is_string($value) ? $value : sprintf('%s', $value); + } } diff --git a/src/Sql/Exception/InvalidArgumentException.php b/src/Sql/Exception/InvalidArgumentException.php index 8066080c9..1c6a7465b 100644 --- a/src/Sql/Exception/InvalidArgumentException.php +++ b/src/Sql/Exception/InvalidArgumentException.php @@ -60,7 +60,7 @@ class InvalidArgumentException extends Exception\InvalidArgumentException final public const string MISSING_VALUES_OR_SELECT = 'values or select should be present'; - final public const string NON_NUMERIC_PARAMETER = '%s expects parameter to be numeric, "%s" given'; + final public const string NON_NEGATIVE_INTEGER_PARAMETER = '%s expects a non-negative integer, "%s" given'; final public const string NON_STRING_VALUE_KEY = 'set() expects a string for the value key'; @@ -172,9 +172,9 @@ public static function forMissingValuesOrSelect(): self return new self(self::MISSING_VALUES_OR_SELECT); } - public static function forNonNumericParameter(string $method, string $givenType): self + public static function forNonNegativeIntegerParameter(string $method, int|string $given): self { - return new self(sprintf(self::NON_NUMERIC_PARAMETER, $method, $givenType)); + return new self(sprintf(self::NON_NEGATIVE_INTEGER_PARAMETER, $method, $given)); } public static function forNonStringValueKey(): self diff --git a/src/Sql/Select.php b/src/Sql/Select.php index 183750be9..d272cbd62 100644 --- a/src/Sql/Select.php +++ b/src/Sql/Select.php @@ -14,14 +14,13 @@ use function array_key_first; use function count; use function explode; -use function gettype; use function is_array; use function is_int; -use function is_numeric; use function is_scalar; use function is_string; use function key; use function method_exists; +use function preg_match; use function preg_split; use function strcasecmp; use function stripos; @@ -36,6 +35,7 @@ * @property ?Having $having * @property ?Join $joins * @psalm-import-type Specification from AbstractSql + * @psalm-import-type SqlParameterValue from AbstractSql * @psalm-import-type PredicateValue from Predicate\PredicateSet * @psalm-import-type JoinName from Join * @psalm-type TableReference = string|TableIdentifier|array @@ -187,6 +187,16 @@ public function __construct(array|string|TableIdentifier|null $table = null) } } + /** + * Whether a row count is a non-negative integer, given as an int or as a string of digits + * + * A digit string is kept as given, so a count beyond PHP_INT_MAX is still accepted. + */ + private static function isNonNegativeInteger(int|string $value): bool + { + return is_int($value) ? $value >= 0 : 1 === preg_match('/^\d+\z/', $value); + } + /** * Specify columns from which to select * Possible valid states: @@ -335,8 +345,8 @@ public function join( */ public function limit(int|string $limit): static { - if (! is_numeric($limit)) { - throw Exception\InvalidArgumentException::forNonNumericParameter(__METHOD__, gettype($limit)); + if (! self::isNonNegativeInteger($limit)) { + throw Exception\InvalidArgumentException::forNonNegativeIntegerParameter(__METHOD__, $limit); } $this->limit = $limit; @@ -348,8 +358,8 @@ public function limit(int|string $limit): static */ public function offset(int|string $offset): static { - if (! is_numeric($offset)) { - throw Exception\InvalidArgumentException::forNonNumericParameter(__METHOD__, gettype($offset)); + if (! self::isNonNegativeInteger($offset)) { + throw Exception\InvalidArgumentException::forNonNegativeIntegerParameter(__METHOD__, $offset); } $this->offset = $offset; @@ -537,7 +547,11 @@ protected function processJoins( return $this->processJoin($this->joins, $platform, $driver, $parameterContainer); } - /** @return array{0: string}|null */ + /** + * Platform overrides may return the raw limit or offset unquoted; any scalar is rendered as vsprintf's %s would. + * + * @return array{0: SqlParameterValue}|null + */ protected function processLimit( PlatformInterface $platform, ?DriverInterface $driver = null, @@ -556,7 +570,11 @@ protected function processLimit( return [$platform->quoteValue((string) $this->limit)]; } - /** @return array{0: string}|null */ + /** + * Platform overrides may return the raw limit or offset unquoted; any scalar is rendered as vsprintf's %s would. + * + * @return array{0: SqlParameterValue}|null + */ protected function processOffset( PlatformInterface $platform, ?DriverInterface $driver = null, diff --git a/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php b/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php index 5a77eebfd..b88683d23 100644 --- a/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php +++ b/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php @@ -34,7 +34,7 @@ #[CoversMethod(InvalidArgumentException::class, 'forInvalidMagicProperty')] #[CoversMethod(InvalidArgumentException::class, 'forInvalidSpecificationName')] #[CoversMethod(InvalidArgumentException::class, 'forMissingValuesOrSelect')] -#[CoversMethod(InvalidArgumentException::class, 'forNonNumericParameter')] +#[CoversMethod(InvalidArgumentException::class, 'forNonNegativeIntegerParameter')] #[CoversMethod(InvalidArgumentException::class, 'forNonStringValueKey')] #[CoversMethod(InvalidArgumentException::class, 'forReadOnlyConstructorState')] #[CoversMethod(InvalidArgumentException::class, 'forSelectWithMergeFlag')] @@ -61,127 +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' => [ - 'forNonNumericParameter', - ['PhpDb\\Sql\\Select::limit', 'string'], - InvalidArgumentException::NON_NUMERIC_PARAMETER, + 'non negative integer parameter' => [ + 'forNonNegativeIntegerParameter', + ['PhpDb\\Sql\\Select::limit', '-1'], + InvalidArgumentException::NON_NEGATIVE_INTEGER_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, ], - 'unenclosed values placeholder' => [ + 'unenclosed values placeholder' => [ 'forUnenclosedValuesPlaceholder', ['a IN ?'], InvalidArgumentException::UNENCLOSED_VALUES_PLACEHOLDER, ], - 'unknown argument type' => [ + '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/SelectTest.php b/test/unit/Sql/SelectTest.php index 7bf7f2120..4cfb2b9bd 100644 --- a/test/unit/Sql/SelectTest.php +++ b/test/unit/Sql/SelectTest.php @@ -54,6 +54,7 @@ #[CoversMethod(Select::class, 'having')] #[CoversMethod(Select::class, 'order')] #[CoversMethod(Select::class, 'limit')] +#[CoversMethod(Select::class, 'isNonNegativeInteger')] #[CoversMethod(Select::class, 'offset')] #[CoversMethod(Select::class, 'combine')] #[CoversMethod(Select::class, 'reset')] @@ -83,8 +84,6 @@ #[CoversMethod(Select::class, 'getJoins')] final class SelectTest extends TestCase { - use AdapterTestTrait; - /** * @return array}, string, array}> */ @@ -107,6 +106,32 @@ public static function arrayExpressionParameterProvider(): array ]; } + /** @return array */ + public static function invalidRowCountProvider(): array + { + $values = [ + 'negative int' => -1, + 'negative string' => '-1', + 'fraction' => '1.5', + 'leading dot' => '.5', + 'exponent' => '1e3', + 'leading space' => ' 5', + 'trailing space' => '5 ', + 'trailing newline' => "5\n", + 'empty string' => '', + 'non-numeric string' => 'foobar', + ]; + + $cases = []; + foreach (['limit', 'offset'] as $method) { + foreach ($values as $label => $value) { + $cases["{$method}, {$label}"] = [$method, $value]; + } + } + + return $cases; + } + /** * @psalm-return array */ + public static function validRowCountProvider(): array + { + $values = [ + 'zero int' => 0, + 'zero string' => '0', + 'int' => 10, + 'digit string' => '10', + 'leading zeros' => '007', + 'beyond PHP_INT_MAX' => '10000000000000000000', + ]; + + $cases = []; + foreach (['limit', 'offset'] as $method) { + foreach ($values as $label => $value) { + $cases["{$method}, {$label}"] = [$method, $value]; + } + } + + return $cases; + } + /** * @param Closure(self, Select): array{?string, array} $render * @param array $expectedParameters @@ -1262,6 +1309,8 @@ public function join(): void static::assertSame('bar', $joins2->getJoins()[1]['name']); } + use AdapterTestTrait; + #[Test] public function joinAliasesUnkeyedExpressionColumn(): void { @@ -1297,16 +1346,6 @@ public function limit(): void static::assertSame(10, $select->getRawState(Select::LIMIT)); } - #[Test] - #[TestDox(': unit test: test limit() throws exception when invalid parameter passed')] - public function limitExceptionOnInvalidParameter(): void - { - $select = new Select(); - self::expectException(InvalidArgumentException::class); - self::expectExceptionMessage(Select::class . '::limit expects parameter to be numeric'); - $select->limit('foobar'); - } - #[Test] public function limitIsQuotedWhenContainerHasNoDriver(): void { @@ -1335,6 +1374,18 @@ public function magicGetJoinsReturnsJoinInstance(): void static::assertInstanceOf(Join::class, $select->joins); } + /** @param 'limit'|'offset' $method */ + #[Test] + #[DataProvider('validRowCountProvider')] + public function nonNegativeIntegerRowCountIsKeptAsGiven(string $method, int|string $value): void + { + static::assertSame( + $value, + (new Select())->{$method}($value) + ->getRawState($method), + ); + } + #[Test] public function numbersEachUnkeyedJoinExpression(): void { @@ -1370,16 +1421,6 @@ public function offset(): void static::assertSame(20, $select->getRawState(Select::OFFSET)); } - #[Test] - #[TestDox(': unit test: test offset() throws exception when invalid parameter passed')] - public function offsetExceptionOnInvalidParameter(): void - { - $select = new Select(); - self::expectException(InvalidArgumentException::class); - self::expectExceptionMessage(Select::class . '::offset expects parameter to be numeric'); - $select->offset('foobar'); - } - /** * @throws ReflectionException */ @@ -1726,6 +1767,18 @@ public function resetThrowsOnInvalidPart(): void static::assertSame($select, $result); } + /** @param 'limit'|'offset' $method */ + #[Test] + #[DataProvider('invalidRowCountProvider')] + public function rowCountThatIsNotANonNegativeIntegerIsRejected(string $method, int|string $value): void + { + $this->expectExceptionObject( + InvalidArgumentException::forNonNegativeIntegerParameter(Select::class . '::' . $method, $value), + ); + + (new Select())->{$method}($value); + } + #[Test] #[Group('Laminas-5192')] public function selectUsingTableIdentifierWithEmptyScheme(): void diff --git a/test/unit/Sql/UnpreparedScalarLimitOffsetTest.php b/test/unit/Sql/UnpreparedScalarLimitOffsetTest.php new file mode 100644 index 000000000..03b693fb8 --- /dev/null +++ b/test/unit/Sql/UnpreparedScalarLimitOffsetTest.php @@ -0,0 +1,119 @@ +|null>>}> + */ + public static function limitSpecificationProvider(): array + { + return [ + 'slot without parameter specification' => [['LIMIT %1$s' => [null]]], + 'slot with parameter specification' => [['LIMIT %1$s' => [[1 => '%1$s']]]], + 'slot combined by separator' => [['LIMIT %1$s' => [[1 => '%1$s', 'combinedby' => ', ']]]], + ]; + } + + /** + * @return array + */ + public static function scalarLimitOffsetProvider(): array + { + return [ + 'int limit' => [10, null, 'SELECT "t".* FROM "t" LIMIT 10'], + 'float limit' => [2.5, null, 'SELECT "t".* FROM "t" LIMIT 2.5'], + 'numeric string limit' => ['10', null, 'SELECT "t".* FROM "t" LIMIT 10'], + 'stringable limit' => [new ObjectToString('10'), null, 'SELECT "t".* FROM "t" LIMIT 10'], + 'int offset only' => [null, 5, 'SELECT "t".* FROM "t" LIMIT 18446744073709551615 OFFSET 5'], + 'float offset only' => [null, 5.0, 'SELECT "t".* FROM "t" LIMIT 18446744073709551615 OFFSET 5'], + 'int limit and int offset' => [10, 20, 'SELECT "t".* FROM "t" LIMIT 10 OFFSET 20'], + 'int limit and string offset' => [10, '20', 'SELECT "t".* FROM "t" LIMIT 10 OFFSET 20'], + ]; + } + + #[Test] + public function rendersFloatsWithTheSamePrecisionAsVsprintf(): void + { + $select = new UnpreparedScalarLimitSelect('t', 0.1 + 0.2); + + self::assertSame( + sprintf('SELECT "t".* FROM "t" LIMIT %s', 0.1 + 0.2), + $select->getSqlString(new TrustingSql92Platform()), + ); + } + + /** + * @param array|null>> $specification + */ + #[Test] + #[DataProvider('limitSpecificationProvider')] + public function rendersIntegerLimitThroughArraySpecifications(array $specification): void + { + $select = new UnpreparedScalarLimitSelect('t', 10, limitSpecification: $specification); + + self::assertSame('SELECT "t".* FROM "t" LIMIT 10', $select->getSqlString(new TrustingSql92Platform())); + } + + #[Test] + #[DataProvider('scalarLimitOffsetProvider')] + public function rendersScalarLimitAndOffsetReturnedUnquotedByProcessHooks( + int|float|string|Stringable|null $limit, + int|float|string|Stringable|null $offset, + string $expected, + ): void { + $select = new UnpreparedScalarLimitSelect('t', $limit, $offset); + + self::assertSame($expected, $select->getSqlString(new TrustingSql92Platform())); + } + + #[Test] + public function stillRejectsNestedListForSlotWithoutParameterSpecification(): void + { + $select = new UnpreparedScalarLimitSelect('t', ['1', '2'], limitSpecification: ['LIMIT %1$s' => [null]]); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage(sprintf(RuntimeException::UNSUPPORTED_PARAMETER_COUNT_OF, 2)); + + $select->getSqlString(new TrustingSql92Platform()); + } + + #[Test] + public function stillRejectsNestedListReturnedForStringSpecification(): void + { + $select = new UnpreparedScalarLimitSelect('t', ['1', '2']); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage(sprintf(RuntimeException::UNSUPPORTED_PARAMETER_COUNT_OF, 2)); + + $select->getSqlString(new TrustingSql92Platform()); + } +} diff --git a/test/unit/TestAsset/UnpreparedScalarLimitSelect.php b/test/unit/TestAsset/UnpreparedScalarLimitSelect.php new file mode 100644 index 000000000..9cb0087c1 --- /dev/null +++ b/test/unit/TestAsset/UnpreparedScalarLimitSelect.php @@ -0,0 +1,82 @@ +|null + */ +final class UnpreparedScalarLimitSelect extends Select +{ + /** + * @param RawValue $rawLimit + * @param RawValue $rawOffset + * @param array|null>>|null $limitSpecification + */ + public function __construct( + string $table, + private readonly int|float|string|Stringable|array|null $rawLimit, + private readonly int|float|string|Stringable|array|null $rawOffset = null, + private readonly ?array $limitSpecification = null, + ) { + parent::__construct($table); + + if (null !== $this->limitSpecification) { + $this->specifications[self::LIMIT] = $this->limitSpecification; + } + } + + #[Override] + protected function localizeVariables(): void + { + parent::localizeVariables(); + + if (null === $this->rawLimit && null !== $this->rawOffset) { + $this->specifications[self::LIMIT] = 'LIMIT 18446744073709551615'; + } + } + + /** @return array{0: RawValue}|null */ + #[Override] + protected function processLimit( + PlatformInterface $platform, + ?DriverInterface $driver = null, + ?ParameterContainer $parameterContainer = null, + ): ?array { + if (null === $this->rawLimit && null !== $this->rawOffset) { + return ['']; + } + + if (null === $this->rawLimit) { + return null; + } + + return [$this->rawLimit]; + } + + /** @return array{0: RawValue}|null */ + #[Override] + protected function processOffset( + PlatformInterface $platform, + ?DriverInterface $driver = null, + ?ParameterContainer $parameterContainer = null, + ): ?array { + if (null === $this->rawOffset) { + return null; + } + + return [$this->rawOffset]; + } +}