Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Bugfix for IN operator in JOIN conditions #637

Merged
merged 4 commits into from

6 participants

@arvenil

$c->addMultipleJoin doesn't support Criteria::IN in conditions e.g. without this patch the code below will throw error Criteria.php(925): strrpos() expects parameter 1 to be string, array given.

$c = new Criteria();
$c->addMultipleJoin(array(
        array(AuthorPeer::ID, BookPeer::AUTHOR_ID),
        array(BookPeer::ISBN, array(12, 34), Criteria::IN)
    ), Criteria::LEFT_JOIN);

This patch fixes this behavior in Criteria.

In ModelCriteria you can achieve this behavior with some plane sql - I've added test to ensure this is true.
https://github.com/arvenil/Propel/pull/new/in-operator-in-join-conditions#L2R916

test/testsuite/runtime/query/CriteriaTest.php
@@ -874,6 +874,47 @@ public function testAddJoinMultipleWithJoinTypeAndOperator()
$this->assertEquals($expect, $result);
}
+ public function testAddJoinMultipleWithInAndNotInOperators()
@staabm Collaborator
staabm added a note

Separate tests plz

@arvenil
arvenil added a note

Good point. Done. I've also simplified those tests - one assertion is enough for each test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fzaninotto
Collaborator

AddMultipleJoin() is deprecated, you should use addJoinCondition().

https://groups.google.com/forum/m/?fromgroups#!topic/propel-users/BbCa4C0qvR0

@arvenil

@fzaninotto I'm aware it is deprecated however does it mean bugs in such method can't be fixed?

I'm also aware of addJoinCondition() - that's why I've also added test case for it.
However firstly, project where I'm using this kind of join is still using Criteria (yeah, I know),
secondly, I have a question if you really can easily and nicely add such condition with addJoinCondition()?

With Criteria and addMultipleJoin():

$data = array(1, 7, 42);
$c = new Criteria();
$c->addMultipleJoin(array(
    array(AuthorPeer::ID, BookPeer::AUTHOR_ID),
    array(BookPeer::ISBN, $data, Criteria::IN)
    ), Criteria::LEFT_JOIN);

With ModelCriteria and addJoinCondition():

$data = array(1, 7, 42);
$con = Propel::getConnection(BookPeer::DATABASE_NAME);
$c = new ModelCriteria('bookstore', 'Book');
$c->join('Book.Author', Criteria::LEFT_JOIN);
// Below you need to know the number of array elements to create placeholders '?'
$c->addJoinCondition('Author', 'Book.isbn IN (?, ?, ?)', $data);
$books = BookPeer::doSelect($c, $con);

Is there a way to do this nicer with ModelCriteria?

@fzaninotto
Collaborator
@arvenil

So you are against including this bugfix?

@fzaninotto
Collaborator
@arvenil

Seriously?:D The method is @deprecated, so it means it should work but will be removed in future.
If for you this is dead code then please remove it now instead of telling people they shouldn't use it.

I could understand if I would only ask for fix, but I'm also providing code with bugfix:/

@marcj
Owner

In my opinion it should be included, because a bugfix for deprecated (but still used) stuff is equally important than fixing normal stuff. Also because methods that are marked as deprecated won't probably be removed in new 1.x releases, since it will break BC. BC breaking changes goes directly into Propel 2.x version.

@arvenil

@fzaninotto

As for knowing how many placeholders adding to the string, I think you can use a Criterion in addJoinCondition - that would solve the problem.

Could you provide some example how to do this? I can't find out how to use Criterion with addJoinCondition?

@fzaninotto
Collaborator
@staabm
Collaborator

I am with @fzaninotto here that we shouldn't add new features to already deprecated API. This will only encourage people to use this API more often which is not a good thing for deprecated stuff.

@arvenil

@fzaninotto

I think it's ModelCriteria::addJoinObject().

public function addJoinObject(Join $join, $name = null) accepts Join object, not Criterion? Still I don't see how can I achieve this.
Again, could you provide example code to show what you have suggested.

Also, why would you put the WHERE IN condition in the join condition and not in the WHERE part?

You don't see a point of adding conditions in JOIN part? Then why we need addJoinConditions() or addMultipleJoin(). HINT: LEFT JOIN.

@staabm This is bugfix, not new feature. You can use any other operator from Criteria e.g Criteria::GREATHER_THAN, except Criteria::IN and Criteria::NOT_IN.

Btw. I understand reasons, why dealing with @depracated method is waste of time... however I don't understand why be so against if someone else (me:)) wastes this time:)

@havvg

I definitely see the point of this. A WHERE-condition from the SELECT clause's view is completely different from an ON-condition for the joint table.

Also, I agree with @arvenil fixing a bug even in deprecated methods is valid and should be done.

Just to be curious; did you try setJoinCondition does it work with Criteria::IN? You can pass in a Criterion there.

I'm not 100% sure, but this should work (from your example):

$data = array(1, 7, 42);
$con = Propel::getConnection(BookPeer::DATABASE_NAME);
$c = new ModelCriteria('bookstore', 'Book');
$c->join('Book.Author', Criteria::LEFT_JOIN);

// This should transform into IN (:p1,:p2,:p3)
$c->addJoinCondition('Author', 'Book.isbn IN ?', $data);

$books = BookPeer::doSelect($c, $con);
@arvenil

Thanks @havvg ! This works.

I've added proper testcases. I've also simplified changes in addMultipleJoin - now it looks better. This also implied that in case of integer values it binds parameters instead of putting them directly into prepared statement.

@staabm staabm commented on the diff
test/testsuite/runtime/query/CriteriaTest.php
@@ -800,7 +800,7 @@ 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 Collaborator
staabm added a note

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

@arvenil
arvenil added a note

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@staabm
Collaborator

whats the state of this PR? In case we agree this should land, it would be nice @arvenil could rebase it...

@willdurand @fzaninotto @marcj @havvg @jaugustin opinions?

I am ok to land it, because it is a bugfix.

arvenil added some commits
@arvenil arvenil Add to Criteria support for IN and NOT IN operators in JOIN conditions
e.g. JOIN x ON (x.id = y.x_id AND y.foo IN (42, 51))
0ad1698
@arvenil arvenil Use Criterion always when possible
Using Criterion allows to bind values in statements.
You can't use Criterion only for strings because in this method
you can't distinguish if it is a table name or value provided by user.
a6a20bd
@arvenil arvenil Add test for ModelCriteria to prove it support IN operator in JOIN co…
…nditions
cb913c3
@arvenil arvenil Additional assertion to ensure binding params doesn't change actual r…
…esult
80179d6
@arvenil

Rebased (also I took opportunity to cleanup commits)

@willdurand willdurand merged commit f227d5d into from
@willdurand
Owner

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 13, 2013
  1. @arvenil

    Add to Criteria support for IN and NOT IN operators in JOIN conditions

    arvenil authored
    e.g. JOIN x ON (x.id = y.x_id AND y.foo IN (42, 51))
  2. @arvenil

    Use Criterion always when possible

    arvenil authored
    Using Criterion allows to bind values in statements.
    You can't use Criterion only for strings because in this method
    you can't distinguish if it is a table name or value provided by user.
  3. @arvenil
  4. @arvenil
This page is out of date. Refresh to see the latest.
View
19 runtime/lib/query/Criteria.php
@@ -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,13 +963,20 @@ 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);
@@ -976,12 +984,7 @@ public function addMultipleJoin($conditions, $joinType = null)
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 {
View
38 test/testsuite/runtime/query/CriteriaTest.php
@@ -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 Collaborator
staabm added a note

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

@arvenil
arvenil added a note

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
$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
View
24 test/testsuite/runtime/query/ModelCriteriaTest.php
@@ -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);
Something went wrong with that request. Please try again.