From b284fddd9405cb7a2bacc8748d9a28c6938c3e49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Wed, 10 Jul 2019 17:46:25 +0200 Subject: [PATCH] Improving IN expression handling A IN expression with a parameter (like `status IN (:statuses)`) can now accept parameters... or not. - `status IN (:statuses)` - `status IN :statuses` are now both valid. Previously, the first was only valid in $magicQuery->buildPreparedStatement and the later was only valid in $magicQuery->build, which caused confusion. --- .../Node/AbstractTwoOperandsOperator.php | 12 ++---- src/SQLParser/Node/BypassableInterface.php | 12 ++++++ src/SQLParser/Node/Expression.php | 15 ++++++- src/SQLParser/Node/In.php | 41 +++++++++++++++++-- src/SQLParser/Node/Parameter.php | 14 +++++-- tests/Mouf/Database/MagicQueryTest.php | 14 +++++++ 6 files changed, 92 insertions(+), 16 deletions(-) create mode 100644 src/SQLParser/Node/BypassableInterface.php diff --git a/src/SQLParser/Node/AbstractTwoOperandsOperator.php b/src/SQLParser/Node/AbstractTwoOperandsOperator.php index e641c10..079685f 100644 --- a/src/SQLParser/Node/AbstractTwoOperandsOperator.php +++ b/src/SQLParser/Node/AbstractTwoOperandsOperator.php @@ -100,15 +100,11 @@ public function toSql(array $parameters = array(), Connection $dbConnection = nu { if ($conditionsMode == self::CONDITION_GUESS) { $bypass = false; - if ($this->leftOperand instanceof Parameter) { - if ($this->leftOperand->isDiscardedOnNull() && !isset($parameters[$this->leftOperand->getName()])) { - $bypass = true; - } + if ($this->leftOperand instanceof BypassableInterface && $this->leftOperand->canBeBypassed($parameters)) { + $bypass = true; } - if ($this->rightOperand instanceof Parameter) { - if ($this->rightOperand->isDiscardedOnNull() && !isset($parameters[$this->rightOperand->getName()])) { - $bypass = true; - } + if ($this->rightOperand instanceof BypassableInterface && $this->rightOperand->canBeBypassed($parameters)) { + $bypass = true; } if ($bypass === true) { return; diff --git a/src/SQLParser/Node/BypassableInterface.php b/src/SQLParser/Node/BypassableInterface.php new file mode 100644 index 0000000..1c21daa --- /dev/null +++ b/src/SQLParser/Node/BypassableInterface.php @@ -0,0 +1,12 @@ + */ -class Expression implements NodeInterface +class Expression implements NodeInterface, BypassableInterface { private $baseExpression; @@ -257,4 +257,17 @@ public function walk(VisitorInterface $visitor) return $visitor->leaveNode($node); } + + /** + * Returns if this node should be removed from the tree. + */ + public function canBeBypassed(array $parameters): bool + { + foreach ($this->subTree as $node) { + if (!$node instanceof BypassableInterface || !$node->canBeBypassed($parameters)) { + return false; + } + } + return true; + } } diff --git a/src/SQLParser/Node/In.php b/src/SQLParser/Node/In.php index 07bbd0f..02a871a 100644 --- a/src/SQLParser/Node/In.php +++ b/src/SQLParser/Node/In.php @@ -24,15 +24,48 @@ protected function getOperatorSymbol() protected function getSql(array $parameters = array(), Connection $dbConnection = null, $indent = 0, $conditionsMode = self::CONDITION_APPLY, bool $extrapolateParameters = true) { $rightOperand = $this->getRightOperand(); - if ($rightOperand instanceof Parameter) { - if (!isset($parameters[$rightOperand->getName()])) { - throw new MagicQueryException("Missing parameter '" . $rightOperand->getName() . "' for 'IN' operand."); + + $rightOperand = $this->refactorParameterToExpression($rightOperand); + + $this->setRightOperand($rightOperand); + + $parameterNode = $this->getParameter($rightOperand); + + if ($parameterNode !== null) { + if (!isset($parameters[$parameterNode->getName()])) { + throw new MagicQueryException("Missing parameter '" . $parameterNode->getName() . "' for 'IN' operand."); } - if ($parameters[$rightOperand->getName()] === []) { + if ($parameters[$parameterNode->getName()] === []) { return "FALSE"; } } return parent::getSql($parameters, $dbConnection, $indent, $conditionsMode, $extrapolateParameters); } + + protected function refactorParameterToExpression(NodeInterface $rightOperand): NodeInterface + { + if ($rightOperand instanceof Parameter) { + $expression = new Expression(); + $expression->setSubTree([$rightOperand]); + $expression->setBrackets(true); + return $expression; + } + return $rightOperand; + } + + protected function getParameter(NodeInterface $operand): ?Parameter + { + if (!$operand instanceof Expression) { + return null; + } + $subtree = $operand->getSubTree(); + if (!isset($subtree[0])) { + return null; + } + if ($subtree[0] instanceof Parameter) { + return $subtree[0]; + } + return null; + } } diff --git a/src/SQLParser/Node/Parameter.php b/src/SQLParser/Node/Parameter.php index f9c08b2..91c9357 100644 --- a/src/SQLParser/Node/Parameter.php +++ b/src/SQLParser/Node/Parameter.php @@ -42,7 +42,7 @@ * * @author David Négrier */ -class Parameter implements NodeInterface +class Parameter implements NodeInterface, BypassableInterface { protected $name; protected $discardedOnNull = true; @@ -175,9 +175,9 @@ public function toSql(array $parameters = array(), Connection $dbConnection = nu return 'null'; } else { if (is_array($parameters[$this->name])) { - return '('.implode(',', array_map(function ($item) { + return implode(',', array_map(function ($item) { return "'".addslashes($this->autoPrepend.$item.$this->autoAppend)."'"; - }, $parameters[$this->name])).')'; + }, $parameters[$this->name])); } else { return "'".addslashes($this->autoPrepend.$parameters[$this->name].$this->autoAppend)."'"; } @@ -217,4 +217,12 @@ public function isDiscardedOnNull() { return $this->discardedOnNull; } + + /** + * Returns if this node should be removed from the tree. + */ + public function canBeBypassed(array $parameters): bool + { + return $this->isDiscardedOnNull() && !isset($parameters[$this->getName()]); + } } diff --git a/tests/Mouf/Database/MagicQueryTest.php b/tests/Mouf/Database/MagicQueryTest.php index 84b23ca..cf83918 100644 --- a/tests/Mouf/Database/MagicQueryTest.php +++ b/tests/Mouf/Database/MagicQueryTest.php @@ -72,6 +72,12 @@ public function testStandardSelect() $sql = 'SELECT * FROM users WHERE status in :status'; $this->assertEquals("SELECT * FROM users WHERE status IN ('2','4')", self::simplifySql($magicQuery->build($sql, ['status' => [2, 4]]))); + $sql = 'SELECT * FROM users WHERE status in (:status)'; + $this->assertEquals("SELECT * FROM users WHERE status IN ('2','4')", self::simplifySql($magicQuery->build($sql, ['status' => [2, 4]]))); + + $sql = 'SELECT * FROM users WHERE status IN :statuses'; + $this->assertEquals('SELECT * FROM users WHERE status IN (\'1\',\'2\')', self::simplifySql($magicQuery->build($sql, ['statuses' => [1, 2]]))); + $sql = 'SELECT * FROM myTable where someField BETWEEN :value1 AND :value2'; $this->assertEquals("SELECT * FROM myTable WHERE someField BETWEEN '2' AND '4'", self::simplifySql($magicQuery->build($sql, ['value1' => 2, 'value2' => 4]))); $this->assertEquals("SELECT * FROM myTable WHERE someField >= '2'", self::simplifySql($magicQuery->build($sql, ['value1' => 2]))); @@ -413,5 +419,13 @@ public function testBuildPreparedStatement() // Test cache $this->assertEquals("SELECT id FROM users WHERE name LIKE :name LIMIT :offset, 2", self::simplifySql($magicQuery->buildPreparedStatement($sql, ['name' => 'bar', 'offset' => 10]))); $this->assertEquals("SELECT id FROM users WHERE name LIKE :name LIMIT 2", self::simplifySql($magicQuery->buildPreparedStatement($sql, ['name' => 'bar']))); + + $sql = 'SELECT id FROM users WHERE status IN (:status)'; + $this->assertEquals("SELECT id FROM users WHERE status IN (:status)", self::simplifySql($magicQuery->buildPreparedStatement($sql, ['status' => [1,2]]))); + $this->assertEquals("SELECT id FROM users", self::simplifySql($magicQuery->buildPreparedStatement($sql, ['status' => null]))); + + // Let's check that MagicQuery is cleverly adding parenthesis if the user forgot those in the "IN" statement. + $sql = 'SELECT id FROM users WHERE status IN :status'; + $this->assertEquals("SELECT id FROM users WHERE status IN (:status)", self::simplifySql($magicQuery->buildPreparedStatement($sql, ['status' => [1,2]]))); } }