From a08a5cf8cc8c372f7975f72f394f90423b827000 Mon Sep 17 00:00:00 2001 From: Simon Mundy Date: Tue, 13 Oct 2026 12:32:45 +1100 Subject: [PATCH 1/2] Reject setSpecification() on Sql\Expression instead of ignoring it Expression inherits setSpecification() from AbstractExpression, but getExpressionData() only renders $this->expression, so the override was accepted and silently dropped. Throw a RuntimeException pointing to setExpression() so the caller cannot get different SQL than they asked for. Predicates that honour a custom specification are unchanged. --- docs/book/sql/where-having.md | 7 +++ src/Sql/Exception/RuntimeException.php | 9 ++++ src/Sql/Expression.php | 12 +++++ .../Sql/Exception/RuntimeExceptionTest.php | 7 +++ test/unit/Sql/ExpressionTest.php | 17 ++++++ .../Sql/Predicate/CustomSpecificationTest.php | 54 +++++++++++++++++++ test/unit/Sql/Predicate/ExpressionTest.php | 19 +++++++ 7 files changed, 125 insertions(+) create mode 100644 test/unit/Sql/Predicate/CustomSpecificationTest.php diff --git a/docs/book/sql/where-having.md b/docs/book/sql/where-having.md index 31573e4f0..d77d15ecf 100644 --- a/docs/book/sql/where-having.md +++ b/docs/book/sql/where-having.md @@ -394,6 +394,13 @@ class Expression implements } ``` +> **Note:** Unlike `Operator`, `Like`, `Between`, `In` and `IsNull`, +> `Expression` does not accept a specification override. It always renders +> the string given to its constructor or `setExpression()`, so calling +> `setSpecification()` on an `Expression` (or `Predicate\Expression`) throws +> `PhpDb\Sql\Exception\RuntimeException`. Call `setExpression()` to change +> the SQL it renders. + Expression parameters can be supplied in multiple ways: ```php title="Using Expression with various parameter types" diff --git a/src/Sql/Exception/RuntimeException.php b/src/Sql/Exception/RuntimeException.php index 51ecffc7e..846c73472 100644 --- a/src/Sql/Exception/RuntimeException.php +++ b/src/Sql/Exception/RuntimeException.php @@ -34,6 +34,10 @@ class RuntimeException extends Exception\RuntimeException 'A number of parameters (%d) was found' . ' that is not supported by this specification'; + final public const string UNSUPPORTED_SPECIFICATION_OVERRIDE = + '%s renders its expression string and does not accept' + . ' a specification override; use setExpression() instead'; + public static function forReplacementMismatch(): self { return new self(self::REPLACEMENT_MISMATCH); @@ -68,4 +72,9 @@ public static function forUnsupportedParameterCountOf(int $count): self { return new self(sprintf(self::UNSUPPORTED_PARAMETER_COUNT_OF, $count)); } + + public static function forUnsupportedSpecificationOverride(string $class): self + { + return new self(sprintf(self::UNSUPPORTED_SPECIFICATION_OVERRIDE, $class)); + } } diff --git a/src/Sql/Expression.php b/src/Sql/Expression.php index 4450c39bb..f44c916d8 100644 --- a/src/Sql/Expression.php +++ b/src/Sql/Expression.php @@ -144,4 +144,16 @@ public function setParameters( return $this; } + + /** + * Expression renders the string given to setExpression(), so a + * specification override would be silently ignored. Use setExpression(). + * + * @throws Exception\RuntimeException + */ + #[Override] + public function setSpecification(string $specification): static + { + throw Exception\RuntimeException::forUnsupportedSpecificationOverride(static::class); + } } diff --git a/test/unit/Sql/Exception/RuntimeExceptionTest.php b/test/unit/Sql/Exception/RuntimeExceptionTest.php index 57f965b28..88729babc 100644 --- a/test/unit/Sql/Exception/RuntimeExceptionTest.php +++ b/test/unit/Sql/Exception/RuntimeExceptionTest.php @@ -6,6 +6,7 @@ use PhpDb\Exception\ExceptionInterface; use PhpDb\Sql\Exception\RuntimeException; +use PhpDb\Sql\Expression; use PhpDb\Sql\SqlInterface; use PHPUnit\Framework\Attributes\CoversMethod; use PHPUnit\Framework\Attributes\DataProvider; @@ -23,6 +24,7 @@ #[CoversMethod(RuntimeException::class, 'forSubjectNotSqlInterface')] #[CoversMethod(RuntimeException::class, 'forUnsupportedParameterCount')] #[CoversMethod(RuntimeException::class, 'forUnsupportedParameterCountOf')] +#[CoversMethod(RuntimeException::class, 'forUnsupportedSpecificationOverride')] final class RuntimeExceptionTest extends TestCase { /** @return array, string}> */ @@ -64,6 +66,11 @@ public static function namedConstructorProvider(): array [2], RuntimeException::UNSUPPORTED_PARAMETER_COUNT_OF, ], + 'unsupported specification override' => [ + 'forUnsupportedSpecificationOverride', + [Expression::class], + RuntimeException::UNSUPPORTED_SPECIFICATION_OVERRIDE, + ], ]; } diff --git a/test/unit/Sql/ExpressionTest.php b/test/unit/Sql/ExpressionTest.php index 2237448dc..d454cdc61 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,6 +32,8 @@ #[CoversMethod(Expression::class, 'setParameters')] #[CoversMethod(Expression::class, 'getParameters')] #[CoversMethod(Expression::class, 'getExpressionData')] +#[CoversMethod(Expression::class, 'setSpecification')] +#[Group('unit')] final class ExpressionTest extends TestCase { /** @psalm-return array */ @@ -262,4 +266,17 @@ public function setParametersWrapsArrayInValuesArgument(): void static::assertCount(2, $data['values']); static::assertInstanceOf(Argument\Values::class, $data['values'][1]); } + + #[Test] + public function setSpecificationIsRejectedInFavourOfSetExpression(): void + { + $expression = new Expression('LOWER(?) = ?', ['x', 'y']); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage( + sprintf(RuntimeException::UNSUPPORTED_SPECIFICATION_OVERRIDE, Expression::class), + ); + + $expression->setSpecification('IGNORED %s %s'); + } } diff --git a/test/unit/Sql/Predicate/CustomSpecificationTest.php b/test/unit/Sql/Predicate/CustomSpecificationTest.php new file mode 100644 index 000000000..bef7fa0ff --- /dev/null +++ b/test/unit/Sql/Predicate/CustomSpecificationTest.php @@ -0,0 +1,54 @@ + */ + public static function predicateProvider(): array + { + return [ + 'operator' => [new Operator('a', Operator::OPERATOR_EQUAL_TO, 1), '%1$s <=> %2$s'], + 'like' => [new Like('a', 'b%'), '%1$s SOUNDS LIKE %2$s'], + 'between' => [new Between('a', 1, 2), '%1$s BETWEEN SYMMETRIC %2$s AND %3$s'], + 'is null' => [new IsNull('a'), '%1$s IS UNKNOWN'], + 'in' => [new In('a', [1, 2]), '%s = ANY (%s)'], + ]; + } + + #[Test] + #[DataProvider('predicateProvider')] + public function rendersTheCustomSpecification( + AbstractExpression $predicate, + string $specification, + ): void { + $predicate->setSpecification($specification); + + static::assertSame($specification, $predicate->getExpressionData()['spec']); + } +} diff --git a/test/unit/Sql/Predicate/ExpressionTest.php b/test/unit/Sql/Predicate/ExpressionTest.php index f6bc50ab2..590bffc21 100644 --- a/test/unit/Sql/Predicate/ExpressionTest.php +++ b/test/unit/Sql/Predicate/ExpressionTest.php @@ -9,12 +9,18 @@ use PhpDb\Sql\Argument\Value; use PhpDb\Sql\ArgumentInterface; use PhpDb\Sql\ArgumentType; +use PhpDb\Sql\Exception\RuntimeException; use PhpDb\Sql\Predicate\Expression; use PhpDb\Sql\Predicate\IsNull; +use PHPUnit\Framework\Attributes\CoversMethod; use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; +use function sprintf; + +#[CoversMethod(Expression::class, 'setSpecification')] +#[Group('unit')] final class ExpressionTest extends TestCase { #[Test] @@ -216,4 +222,17 @@ public function retrievingWherePartsReturnsSpecificationArrayOfLiteralAndParamet static::assertSame('foo.bar = %s AND id != %s', $expressionData['spec']); static::assertEquals([$parameter1, $parameter2], $expressionData['values']); } + + #[Test] + public function setSpecificationIsRejectedInFavourOfSetExpression(): void + { + $expression = new Expression('LOWER(?) = ?', ['x', 'y']); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage( + sprintf(RuntimeException::UNSUPPORTED_SPECIFICATION_OVERRIDE, Expression::class), + ); + + $expression->setSpecification('IGNORED %s %s'); + } } From dc1f4e6bcbb619bfe05bb99b4083ebce52222b78 Mon Sep 17 00:00:00 2001 From: Simon Mundy Date: Wed, 4 Nov 2026 15:45:46 +1100 Subject: [PATCH 2/2] Cover every predicate's specification override in rendered SQL SpecificationOverrideTest now includes the NotLike, NotBetween, IsNotNull and NotIn variants and asserts the SQL each renders, so the In and NotIn cases prove the list expansion end to end. The docs note names the negated variants too. --- docs/book/sql/where-having.md | 5 +- .../Predicate/SpecificationOverrideTest.php | 60 +++++++++++++++---- 2 files changed, 52 insertions(+), 13 deletions(-) diff --git a/docs/book/sql/where-having.md b/docs/book/sql/where-having.md index 214d6025f..d52488fa4 100644 --- a/docs/book/sql/where-having.md +++ b/docs/book/sql/where-having.md @@ -394,8 +394,9 @@ class Expression implements } ``` -> **Note:** Unlike `Operator`, `Like`, `Between`, `In` and `IsNull`, -> `Expression` does not accept a specification override. It always renders +> **Note:** Unlike `Operator`, `Like`/`NotLike`, `Between`/`NotBetween`, +> `In`/`NotIn` and `IsNull`/`IsNotNull`, `Expression` does not accept a +> specification override. It always renders > the string given to its constructor or `setExpression()`, so calling > `setSpecification()` on an `Expression` (or `Predicate\Expression`) throws > `PhpDb\Sql\Exception\RuntimeException`. Call `setExpression()` to change diff --git a/test/unit/Sql/Predicate/SpecificationOverrideTest.php b/test/unit/Sql/Predicate/SpecificationOverrideTest.php index 00319d27f..1f9977ac6 100644 --- a/test/unit/Sql/Predicate/SpecificationOverrideTest.php +++ b/test/unit/Sql/Predicate/SpecificationOverrideTest.php @@ -7,9 +7,15 @@ use PhpDb\Sql\AbstractExpression; use PhpDb\Sql\Predicate\Between; use PhpDb\Sql\Predicate\In; +use PhpDb\Sql\Predicate\IsNotNull; use PhpDb\Sql\Predicate\IsNull; use PhpDb\Sql\Predicate\Like; +use PhpDb\Sql\Predicate\NotBetween; +use PhpDb\Sql\Predicate\NotIn; +use PhpDb\Sql\Predicate\NotLike; use PhpDb\Sql\Predicate\Operator; +use PhpDb\Sql\Select; +use PhpDbTest\TestAsset\TrustingSql92Platform; use PHPUnit\Framework\Attributes\CoversMethod; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Group; @@ -18,8 +24,8 @@ /** * Predicates built on AbstractExpression render a specification set through - * setSpecification() in place of their default format. In expands its value set's - * placeholder to one per list item. + * setSpecification() in place of their default format, including their negated + * variants. In and NotIn expand their value set's placeholder to one per list item. */ #[CoversMethod(AbstractExpression::class, 'setSpecification')] #[CoversMethod(Between::class, 'getExpressionData')] @@ -34,19 +40,47 @@ final class SpecificationOverrideTest extends TestCase public static function predicateProvider(): array { return [ - 'operator' => [ + 'operator' => [ new Operator('a', Operator::OPERATOR_EQUAL_TO, 1), '%1$s <=> %2$s', - '%1$s <=> %2$s', + 'SELECT "t".* FROM "t" WHERE "a" <=> \'1\'', + ], + 'like' => [ + new Like('a', 'b%'), + '%1$s SOUNDS LIKE %2$s', + 'SELECT "t".* FROM "t" WHERE "a" SOUNDS LIKE \'b%\'', ], - 'like' => [new Like('a', 'b%'), '%1$s SOUNDS LIKE %2$s', '%1$s SOUNDS LIKE %2$s'], - 'between' => [ + 'not like' => [ + new NotLike('a', 'b%'), + 'NOT (%1$s SOUNDS LIKE %2$s)', + 'SELECT "t".* FROM "t" WHERE NOT ("a" SOUNDS LIKE \'b%\')', + ], + 'between' => [ new Between('a', 1, 2), '%1$s BETWEEN SYMMETRIC %2$s AND %3$s', - '%1$s BETWEEN SYMMETRIC %2$s AND %3$s', + 'SELECT "t".* FROM "t" WHERE "a" BETWEEN SYMMETRIC \'1\' AND \'2\'', + ], + 'not between' => [ + new NotBetween('a', 1, 2), + '%1$s NOT BETWEEN SYMMETRIC %2$s AND %3$s', + 'SELECT "t".* FROM "t" WHERE "a" NOT BETWEEN SYMMETRIC \'1\' AND \'2\'', + ], + 'is null' => [new IsNull('a'), '%1$s IS UNKNOWN', 'SELECT "t".* FROM "t" WHERE "a" IS UNKNOWN'], + 'is not null' => [ + new IsNotNull('a'), + '%1$s IS NOT UNKNOWN', + 'SELECT "t".* FROM "t" WHERE "a" IS NOT UNKNOWN', + ], + 'in' => [ + new In('a', [1, 2]), + '%s = ANY (%s)', + 'SELECT "t".* FROM "t" WHERE "a" = ANY (\'1\', \'2\')', + ], + 'not in' => [ + new NotIn('a', [1, 2]), + '%s <> ALL (%s)', + 'SELECT "t".* FROM "t" WHERE "a" <> ALL (\'1\', \'2\')', ], - 'is null' => [new IsNull('a'), '%1$s IS UNKNOWN', '%1$s IS UNKNOWN'], - 'in' => [new In('a', [1, 2]), '%s = ANY (%s)', '%1$s = ANY (%2$s, %3$s)'], ]; } @@ -55,10 +89,14 @@ public static function predicateProvider(): array public function rendersTheCustomSpecification( AbstractExpression $predicate, string $specification, - string $expected, + string $expectedSql, ): void { $predicate->setSpecification($specification); - static::assertSame($expected, $predicate->getExpressionData()['spec']); + static::assertSame( + $expectedSql, + (new Select('t'))->where($predicate) + ->getSqlString(new TrustingSql92Platform()), + ); } }