Skip to content

Commit

Permalink
A symptomatic fix for #2722
Browse files Browse the repository at this point in the history
When a query is executed twice, for example qhen cloned or when marked
as dirty, it will run the contain process again. This is potentially
dangerous and confusing for some users. The best solution we have for
now is to discourage users from resuing a query that was executed before
until we find a proper way of handling this case.
  • Loading branch information
lorenzo committed Jan 28, 2014
1 parent 137ada4 commit 851e591
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 5 deletions.
6 changes: 4 additions & 2 deletions src/Database/Query.php
Expand Up @@ -659,20 +659,22 @@ public function join($tables = null, $types = [], $overwrite = false) {

$types += $this->defaultTypes();
$joins = [];
$i = count($this->_parts['join']);
foreach ($tables as $alias => $t) {
if (!is_array($t)) {
$t = ['table' => $t, 'conditions' => $this->newExpr()];
}
if (!($t['conditions']) instanceof ExpressionInterface) {
$t['conditions'] = $this->newExpr()->add($t['conditions'], $types);
}
$joins[] = $t + ['type' => 'INNER', 'alias' => is_string($alias) ? $alias : null];
$alias = is_string($alias) ? $alias : null;
$joins[$alias ?: $i++] = $t + ['type' => 'INNER', 'alias' => $alias];
}

if ($overwrite) {
$this->_parts['join'] = $joins;
} else {
$this->_parts['join'] = array_merge($this->_parts['join'], array_values($joins));
$this->_parts['join'] = array_merge($this->_parts['join'], $joins);
}

$this->_dirty();
Expand Down
2 changes: 1 addition & 1 deletion tests/TestCase/ORM/Association/BelongsToManyTest.php
Expand Up @@ -703,7 +703,7 @@ public function testEagerLoaderSubquery() {

$expected = clone $parent;
$joins = $expected->join();
unset($joins[1]);
unset($joins['bar']);
$expected
->contain([], true)
->select(['Articles__id' => 'Articles.id'], true)
Expand Down
2 changes: 1 addition & 1 deletion tests/TestCase/ORM/Association/HasManyTest.php
Expand Up @@ -335,7 +335,7 @@ public function testEagerLoaderSubquery() {

$expected = clone $parent;
$joins = $expected->join();
unset($joins[1]);
unset($joins['bar']);
$expected
->contain([], true)
->select(['Authors__id' => 'Authors.id'], true)
Expand Down
21 changes: 20 additions & 1 deletion tests/TestCase/ORM/QueryTest.php
Expand Up @@ -999,7 +999,7 @@ public function testApplyOptions() {
$expected = new QueryExpression(['a > b']);
$result = $query->clause('join');
$this->assertEquals([
['alias' => 'table_a', 'type' => 'INNER', 'conditions' => $expected]
'table_a' => ['alias' => 'table_a', 'type' => 'INNER', 'conditions' => $expected]
], $result);

$expected = new OrderByExpression(['a' => 'ASC']);
Expand Down Expand Up @@ -1818,4 +1818,23 @@ public function testQueryWithStackedFormatters() {
$this->assertEquals($expected, $query->toArray());
}

/**
* Tests that getting results from a query having a contained association
* will no attach joins twice if count() is called on it afterwards
*
* @return void
*/
public function testCountWithContainCallingAll() {
$table = TableRegistry::get('articles');
$table->belongsTo('authors');
$query = $table->find()
->select(['id', 'title'])
->contain('authors')
->limit(2);

$results = $query->all();
$this->assertCount(2, $results);
$this->assertEquals(3, $query->count());
}

}

2 comments on commit 851e591

@markstory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would keeping track of the containments being processed be a reasonable way of tracking and preventing this issue? That value could be invalidated when contain() or join() is called.

@lorenzo
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would help to some extent, that would definitely be part of a complete solution. My major concern is what happens if the second time a query is executed, the contain conditions, fields or anything related to them changed substantially, for example removing one of them should also remove the join(s) it attached.

Please sign in to comment.