Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Bugfix for IN operator in JOIN conditions #637

Merged
merged 4 commits into from Apr 15, 2013
@@ -954,6 +954,7 @@ public function addMultipleJoin($conditions, $joinType = null)
foreach ($conditions as $condition) {
$left = $condition[0];
$right = $condition[1];
+ $operator = isset($condition[2]) ? $condition[2] : JOIN::EQUAL;
if ($pos = strrpos($left, '.')) {
$leftTableAlias = substr($left, 0, $pos);
$leftColumnName = substr($left, $pos + 1);
@@ -962,26 +963,28 @@ public function addMultipleJoin($conditions, $joinType = null)
list($leftTableName, $leftTableAlias) = array(null, null);
$leftColumnName = $left;
}
- if ($pos = strrpos($right, '.')) {
+ if (is_string($right) && $pos = strrpos($right, '.')) {
$rightTableAlias = substr($right, 0, $pos);
$rightColumnName = substr($right, $pos + 1);
list($rightTableName, $rightTableAlias) = $this->getTableNameAndAlias($rightTableAlias);
+ $conditionClause = $leftTableAlias ? $leftTableAlias . '.' : ($leftTableName ? $leftTableName . '.' : '');
+ $conditionClause .= $leftColumnName;
+ $conditionClause .= $operator;
+ $conditionClause .= $rightTableAlias ? $rightTableAlias . '.' : ($rightTableName ? $rightTableName . '.' : '');
+ $conditionClause .= $rightColumnName;
+ $comparison = Criteria::CUSTOM;
} else {
list($rightTableName, $rightTableAlias) = array(null, null);
- $rightColumnName = $right;
+ $conditionClause = $right;
+ $comparison = $operator;
}
if (!$join->getRightTableName()) {
$join->setRightTableName($rightTableName);
}
if (!$join->getRightTableAlias()) {
$join->setRightTableAlias($rightTableAlias);
}
- $conditionClause = $leftTableAlias ? $leftTableAlias . '.' : ($leftTableName ? $leftTableName . '.' : '');
- $conditionClause .= $leftColumnName;
- $conditionClause .= isset($condition[2]) ? $condition[2] : JOIN::EQUAL;
- $conditionClause .= $rightTableAlias ? $rightTableAlias . '.' : ($rightTableName ? $rightTableName . '.' : '');
- $conditionClause .= $rightColumnName;
- $criterion = $this->getNewCriterion($leftTableName . '.' . $leftColumnName, $conditionClause, Criteria::CUSTOM);
+ $criterion = $this->getNewCriterion($leftTableName . '.' . $leftColumnName, $conditionClause, $comparison);
if (null === $joinCondition) {
$joinCondition = $criterion;
} else {
@@ -800,10 +800,20 @@ public function testAddJoinMultipleValue()
addSelectColumn("TABLE_A.id");
$expect = 'SELECT TABLE_A.id FROM TABLE_A INNER JOIN TABLE_B '
- . 'ON (TABLE_A.FOO_ID=TABLE_B.id AND TABLE_A.BAR=3)';
+ . 'ON (TABLE_A.FOO_ID=TABLE_B.id AND TABLE_A.BAR=:p1)';
@staabm

staabm Mar 29, 2013

Member

Why did this change? Won't that mean that the api changed? (Or the result of it)

@arvenil

arvenil Mar 29, 2013

Contributor

I don't think this change api - however maybe I'm missing something?

The only change is that instead of directly putting value into sql statement it uses param binding (but only for arrays and integers). This is bug in addJoinMultiple() that it adds values directly to statement instead of binding them. The result set will be the same. I've added second assertion to prove that this doesn't change sql.

You may also ask why it doesn't bind values for strings and why I didn't fix that - well, this is probably one of the reason why this method i @deprecated. There is no way with this method to ensure that value provided as string is value which should be bind or if it is table name. So you shouldn't put values directly from user input to this method if they are strings as it is prone to sql injection.

http://trac.propelorm.org/ticket/878

$params = array();
$result = BasePeer::createSelectSql($c, $params);
$this->assertEquals($expect, $result);
+
+ $con = Propel::getConnection(BookPeer::DATABASE_NAME);
+ $c = new Criteria();
+ $c->addMultipleJoin(array(
+ array(AuthorPeer::ID, BookPeer::AUTHOR_ID),
+ array(BookPeer::ISBN, 3)
+ ));
+ AuthorPeer::doSelectOne($c, $con);
+ $expectedSQL = 'SELECT author.id, author.first_name, author.last_name, author.email, author.age FROM author INNER JOIN book ON (author.id=book.author_id AND book.isbn=3) LIMIT 1';
+ $this->assertEquals($expectedSQL, $con->getLastExecutedQuery());
}
/**
@@ -874,6 +884,32 @@ public function testAddJoinMultipleWithJoinTypeAndOperator()
$this->assertEquals($expect, $result);
}
+ public function testAddJoinMultipleWithInOperator()
+ {
+ $con = Propel::getConnection(BookPeer::DATABASE_NAME);
+ $c = new Criteria();
+ $c->addMultipleJoin(array(
+ array(AuthorPeer::ID, BookPeer::AUTHOR_ID),
+ array(BookPeer::ISBN, array(1, 7, 42), Criteria::IN)
+ ), Criteria::LEFT_JOIN);
+ AuthorPeer::doSelectOne($c, $con);
+ $expectedSQL = 'SELECT author.id, author.first_name, author.last_name, author.email, author.age FROM author LEFT JOIN book ON (author.id=book.author_id AND book.isbn IN (1,7,42)) LIMIT 1';
+ $this->assertEquals($expectedSQL, $con->getLastExecutedQuery());
+ }
+
+ public function testAddJoinMultipleWithNotInOperator()
+ {
+ $con = Propel::getConnection(BookPeer::DATABASE_NAME);
+ $c = new Criteria();
+ $c->addMultipleJoin(array(
+ array(AuthorPeer::ID, BookPeer::AUTHOR_ID),
+ array(BookPeer::ISBN, array(1, 7, 42), Criteria::NOT_IN)
+ ), Criteria::LEFT_JOIN);
+ AuthorPeer::doSelectOne($c, $con);
+ $expectedSQL = 'SELECT author.id, author.first_name, author.last_name, author.email, author.age FROM author LEFT JOIN book ON (author.id=book.author_id AND book.isbn NOT IN (1,7,42)) LIMIT 1';
+ $this->assertEquals($expectedSQL, $con->getLastExecutedQuery());
+ }
+
/**
* Tests adding duplicate joins.
* @link http://trac.propelorm.org/ticket/613
@@ -939,6 +939,30 @@ public function testAddJoinConditionSimple()
$this->assertEquals($expectedSQL, $con->getLastExecutedQuery(), 'addJoinCondition() allows the use of custom conditions');
}
+ public function testAddJoinConditionWithInOperator()
+ {
+ $con = Propel::getConnection(BookPeer::DATABASE_NAME);
+ $c = new ModelCriteria('bookstore', 'Author');
+ $c->join('Author.Book', Criteria::LEFT_JOIN);
+ $c->addJoinCondition('Book', 'Book.isbn IN ?', array(1, 7, 42));
+ $c->limit(1);
+ $books = AuthorPeer::doSelect($c, $con);
+ $expectedSQL = "SELECT author.id, author.first_name, author.last_name, author.email, author.age FROM `author` LEFT JOIN `book` ON (author.id=book.author_id AND book.isbn IN (1,7,42)) LIMIT 1";
+ $this->assertEquals($expectedSQL, $con->getLastExecutedQuery(), 'addJoinCondition() allows the use of custom conditions');
+ }
+
+ public function testAddJoinConditionWithNotInOperator()
+ {
+ $con = Propel::getConnection(BookPeer::DATABASE_NAME);
+ $c = new ModelCriteria('bookstore', 'Author');
+ $c->join('Author.Book', Criteria::LEFT_JOIN);
+ $c->addJoinCondition('Book', 'Book.isbn NOT IN ?', array(1, 7, 42));
+ $c->limit(1);
+ $books = AuthorPeer::doSelect($c, $con);
+ $expectedSQL = "SELECT author.id, author.first_name, author.last_name, author.email, author.age FROM `author` LEFT JOIN `book` ON (author.id=book.author_id AND book.isbn NOT IN (1,7,42)) LIMIT 1";
+ $this->assertEquals($expectedSQL, $con->getLastExecutedQuery(), 'addJoinCondition() allows the use of custom conditions');
+ }
+
public function testAddJoinConditionBinding()
{
$con = Propel::getConnection(BookPeer::DATABASE_NAME);