Fixing issue where DboMysql would perform joins on delete queries eve…

…n if the conditions did not require joins. Added a more intelligent join detector, that removes joins if all the conditions are simple. This will only be able to optimize array conditions. Test cases added. Fixes #1571
cakephp · Mar 25, 2011 · 6cc1688 · 6cc1688
1 parent 532555b
commit 6cc1688
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 59 deletions.
diff --git a/cake/libs/model/datasources/dbo/dbo_mysql.php b/cake/libs/model/datasources/dbo/dbo_mysql.php
@@ -218,12 +218,21 @@ function delete(&$model, $conditions = null) {
 		if (empty($conditions)) {
 			$alias = $joins = false;
 		}
-		$conditions = $this->conditions($this->defaultConditions($model, $conditions, $alias), true, true, $model);
+		$complexConditions = false;
+		foreach ((array)$conditions as $key => $value) {
+			if (strpos($key, $model->alias) === false) {
+				$complexConditions = true;
+				break;
+			}
+		}
+		if (!$complexConditions) {
+			$joins = false;
+		}
 
+		$conditions = $this->conditions($this->defaultConditions($model, $conditions, $alias), true, true, $model);
 		if ($conditions === false) {
 			return false;
 		}
-
 		if ($this->execute($this->renderStatement('delete', compact('alias', 'table', 'joins', 'conditions'))) === false) {
 			$model->onError();
 			return false;

diff --git a/cake/tests/cases/libs/model/datasources/dbo/dbo_mysql.test.php b/cake/tests/cases/libs/model/datasources/dbo/dbo_mysql.test.php
@@ -18,58 +18,10 @@
  * @license       MIT License (http://www.opensource.org/licenses/mit-license.php)
  */
 App::import('Core', array('Model', 'DataSource', 'DboSource', 'DboMysql'));
+App::import('Core', array('AppModel', 'Model'));
+require_once dirname(dirname(dirname(__FILE__))) . DS . 'models.php';
 
-Mock::generatePartial('DboMysql', 'QueryMockDboMysql', array('query'));
-
-/**
- * DboMysqlTestDb class
- *
- * @package       cake
- * @subpackage    cake.tests.cases.libs.model.datasources
- */
-class DboMysqlTestDb extends DboMysql {
-
-/**
- * simulated property
- *
- * @var array
- * @access public
- */
-	var $simulated = array();
-
-/**
- * testing property
- *
- * @var bool true
- * @access public
- */
-	var $testing = true;
-
-/**
- * execute method
- *
- * @param mixed $sql
- * @access protected
- * @return void
- */
-	function _execute($sql) {
-		if ($this->testing) {
-			$this->simulated[] = $sql;
-			return null;
-		}
-		return parent::_execute($sql);
-	}
-
-/**
- * getLastQuery method
- *
- * @access public
- * @return void
- */
-	function getLastQuery() {
-		return $this->simulated[count($this->simulated) - 1];
-	}
-}
+Mock::generatePartial('DboMysql', 'QueryMockDboMysql', array('query', 'execute'));
 
 /**
  * MysqlTestModel class
@@ -160,7 +112,7 @@ function schema() {
  * @subpackage    cake.tests.cases.libs.model.datasources.dbo
  */
 class DboMysqlTest extends CakeTestCase {
-	var $fixtures = array('core.binary_test');
+	var $fixtures = array('core.binary_test', 'core.post', 'core.author');
 /**
  * The Dbo instance to be tested
  *
@@ -583,7 +535,7 @@ function testColumn() {
  */
 	function testAlterSchemaIndexes() {
 		App::import('Model', 'CakeSchema');
-		$this->db->cacheSources = $this->db->testing = false;
+		$this->db->cacheSources = false;
 
 		$schema1 =& new CakeSchema(array(
 			'name' => 'AlterTest1',
@@ -673,7 +625,7 @@ function testBlobSaving() {
  */
 	function testAlteringTableParameters() {
 		App::import('Model', 'CakeSchema');
-		$this->db->cacheSources = $this->db->testing = false;
+		$this->db->cacheSources = false;
 
 		$schema1 =& new CakeSchema(array(
 			'name' => 'AlterTest1',
@@ -757,7 +709,7 @@ function testAlteringTwoTables() {
  * @return void
  */
 	function testReadTableParameters() {
-		$this->db->cacheSources = $this->db->testing = false;
+		$this->db->cacheSources = false;
 		$this->db->query('CREATE TABLE ' . $this->db->fullTableName('tinyint') . ' (id int(11) AUTO_INCREMENT, bool tinyint(1), small_int tinyint(2), primary key(id)) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;');
 		$result = $this->db->readTableParameters('tinyint');
 		$expected = array(
@@ -784,7 +736,7 @@ function testReadTableParameters() {
  * @return void
  */
 	function testBuildTableParameters() {
-		$this->db->cacheSources = $this->db->testing = false;
+		$this->db->cacheSources = false;
 		$data = array(
 			'charset' => 'utf8',
 			'collate' => 'utf8_unicode_ci',
@@ -804,7 +756,7 @@ function testBuildTableParameters() {
  * @return void
  */
 	function testGetCharsetName() {
-		$this->db->cacheSources = $this->db->testing = false;
+		$this->db->cacheSources = false;
 		$result = $this->db->getCharsetName('utf8_unicode_ci');
 		$this->assertEqual($result, 'utf8');
 		$result = $this->db->getCharsetName('cp1250_general_ci');
@@ -863,4 +815,47 @@ function testDescribeGettingFieldParameters() {
 		$this->db->execute($this->db->dropSchema($schema));
 	}
 
+/**
+ * test that simple delete conditions don't create joins using a mock.
+ *
+ * @return void
+ */
+	function testSimpleDeleteConditionsNoJoins() {
+		$model =& new Post();
+		$mockDbo =& new QueryMockDboMysql($this);
+		$mockDbo->expectAt(0, 'execute', array(new PatternExpectation('/AS\s+`Post`\s+WHERE\s+`Post/')));
+		$mockDbo->setReturnValue('execute', true);
+
+		$mockDbo->delete($model, array('Post.id' => 1));
+	}
+
+/**
+ * test deleting with joins, a MySQL specific feature.
+ *
+ * @return void
+ */
+	function testDeleteWithJoins() {
+		$model =& new Post();
+		$mockDbo =& new QueryMockDboMysql($this);
+		$mockDbo->expectAt(0, 'execute', array(new PatternExpectation('/LEFT JOIN `authors`/')));
+		$mockDbo->setReturnValue('execute', true);
+
+		$mockDbo->delete($model, array('Author.id' => 1));
+	}
+
+/**
+ * test joins on delete with multiple conditions.
+ *
+ * @return void
+ */
+	function testDeleteWithJoinsAndMultipleConditions() {
+		$model =& new Post();
+		$mockDbo =& new QueryMockDboMysql($this);
+		$mockDbo->expectAt(0, 'execute', array(new PatternExpectation('/LEFT JOIN `authors`/')));
+		$mockDbo->expectAt(1, 'execute', array(new PatternExpectation('/LEFT JOIN `authors`/')));
+		$mockDbo->setReturnValue('execute', true);
+
+		$mockDbo->delete($model, array('Author.id' => 1, 'Post.id' => 2));
+		$mockDbo->delete($model, array('Post.id' => 2, 'Author.id' => 1));
+	}
 }