From 3d77ce5d34d64beb30815772ddcce3b9803cec1e Mon Sep 17 00:00:00 2001 From: chinpei215 Date: Sat, 2 Aug 2014 06:41:00 +0900 Subject: [PATCH 1/3] Fix serveral tests pass regardless of whether data is valid or not --- lib/Cake/Test/Case/Model/ModelWriteTest.php | 192 +++++++++++++++----- 1 file changed, 142 insertions(+), 50 deletions(-) diff --git a/lib/Cake/Test/Case/Model/ModelWriteTest.php b/lib/Cake/Test/Case/Model/ModelWriteTest.php index 875fdd19b..b9990eebf 100644 --- a/lib/Cake/Test/Case/Model/ModelWriteTest.php +++ b/lib/Cake/Test/Case/Model/ModelWriteTest.php @@ -4124,25 +4124,38 @@ class ModelWriteTest extends BaseModelTest { public function testSaveAllManyRowsTransactionNoRollback() { $this->loadFixtures('Post'); - $db = $this->getMock('DboSource', array('begin', 'connect', 'rollback', 'describe')); - - $db->expects($this->once()) - ->method('describe') - ->will($this->returnValue(array())); - $db->expects($this->once())->method('rollback'); - $Post = new TestPost(); - $Post->setDataSourceObject($db); - $Post->validate = array( 'title' => array('rule' => array('notEmpty')) ); + // If validation error occurs, rollback() should be called. + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->once())->method('begin')->will($this->returnValue(true)); + $db->expects($this->never())->method('commit'); + $db->expects($this->once())->method('rollback'); + + $Post->setDataSourceObject($db); + $data = array( array('author_id' => 1, 'title' => 'New Fourth Post'), array('author_id' => 1, 'title' => '') ); $Post->saveAll($data, array('atomic' => true, 'validate' => true)); + + // Otherwise, commit() should be called. + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->once())->method('begin')->will($this->returnValue(true)); + $db->expects($this->once())->method('commit'); + $db->expects($this->never())->method('rollback'); + + $Post->setDataSourceObject($db); + + $data = array( + array('author_id' => 1, 'title' => 'New Fourth Post'), + array('author_id' => 1, 'title' => 'New Fifth Post') + ); + $Post->saveAll($data, array('atomic' => true, 'validate' => true)); } /** @@ -4151,28 +4164,22 @@ class ModelWriteTest extends BaseModelTest { * @return void */ public function testSaveAllAssociatedTransactionNoRollback() { - $testDb = ConnectionManager::getDataSource('test'); - - $db = $this->getMock('DboSource', array('connect', 'rollback', 'describe', 'create', 'update', 'begin')); - $db->columns = $testDb->columns; - - $db->expects($this->once())->method('rollback'); - $db->expects($this->any())->method('describe') - ->will($this->returnValue(array( - 'id' => array('type' => 'integer', 'length' => 11), - 'title' => array('type' => 'string'), - 'body' => array('type' => 'text'), - 'published' => array('type' => 'string') - ))); + $this->loadFixtures('Post', 'Author'); $Post = new TestPost(); - $Post->setDataSourceObject($db); - $Post->Author->setDataSourceObject($db); - $Post->Author->validate = array( 'user' => array('rule' => array('notEmpty')) ); + // If validation error occurs, rollback() should be called. + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->once())->method('begin')->will($this->returnValue(true)); + $db->expects($this->never())->method('commit'); + $db->expects($this->once())->method('rollback'); + + $Post->setDataSourceObject($db); + $Post->Author->setDataSourceObject($db); + $data = array( 'Post' => array( 'title' => 'New post', @@ -4185,6 +4192,28 @@ class ModelWriteTest extends BaseModelTest { ) ); $Post->saveAll($data, array('validate' => true)); + + // Otherwise, commit() should be called. + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->once())->method('begin')->will($this->returnValue(true)); + $db->expects($this->once())->method('commit'); + $db->expects($this->never())->method('rollback'); + + $Post->setDataSourceObject($db); + $Post->Author->setDataSourceObject($db); + + $data = array( + 'Post' => array( + 'title' => 'New post', + 'body' => 'Content', + 'published' => 'Y' + ), + 'Author' => array( + 'user' => 'New user', + 'password' => "sekret" + ) + ); + $Post->saveAll($data, array('validate' => true)); } /** @@ -5557,25 +5586,38 @@ class ModelWriteTest extends BaseModelTest { public function testSaveManyTransactionNoRollback() { $this->loadFixtures('Post'); - $db = $this->getMock('DboSource', array('begin', 'connect', 'rollback', 'describe')); - - $db->expects($this->once()) - ->method('describe') - ->will($this->returnValue(array())); - $db->expects($this->once())->method('rollback'); - $Post = new TestPost(); - $Post->setDataSourceObject($db); - $Post->validate = array( 'title' => array('rule' => array('notEmpty')) ); + // If validation error occurs, rollback() should be called. + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->once())->method('begin')->will($this->returnValue(true)); + $db->expects($this->never())->method('commit'); + $db->expects($this->once())->method('rollback'); + + $Post->setDataSourceObject($db); + $data = array( array('author_id' => 1, 'title' => 'New Fourth Post'), array('author_id' => 1, 'title' => '') ); $Post->saveMany($data, array('validate' => true)); + + // Otherwise, commit() should be called. + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->once())->method('begin')->will($this->returnValue(true)); + $db->expects($this->once())->method('commit'); + $db->expects($this->never())->method('rollback'); + + $Post->setDataSourceObject($db); + + $data = array( + array('author_id' => 1, 'title' => 'New Fourth Post'), + array('author_id' => 1, 'title' => 'New Fifth Post') + ); + $Post->saveMany($data, array('validate' => true)); } /** @@ -5584,28 +5626,22 @@ class ModelWriteTest extends BaseModelTest { * @return void */ public function testSaveAssociatedTransactionNoRollback() { - $testDb = ConnectionManager::getDataSource('test'); - - $db = $this->getMock('DboSource', array('connect', 'rollback', 'describe', 'create', 'begin')); - $db->columns = $testDb->columns; - - $db->expects($this->once())->method('rollback'); - $db->expects($this->any())->method('describe') - ->will($this->returnValue(array( - 'id' => array('type' => 'integer', 'length' => 11), - 'title' => array('type' => 'string'), - 'body' => array('type' => 'text'), - 'published' => array('type' => 'string') - ))); + $this->loadFixtures('Post', 'Author'); $Post = new TestPost(); - $Post->setDataSourceObject($db); - $Post->Author->setDataSourceObject($db); - $Post->Author->validate = array( 'user' => array('rule' => array('notEmpty')) ); + // If validation error occurs, rollback() should be called. + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->once())->method('begin')->will($this->returnValue(true)); + $db->expects($this->never())->method('commit'); + $db->expects($this->once())->method('rollback'); + + $Post->setDataSourceObject($db); + $Post->Author->setDataSourceObject($db); + $data = array( 'Post' => array( 'title' => 'New post', @@ -5618,6 +5654,28 @@ class ModelWriteTest extends BaseModelTest { ) ); $Post->saveAssociated($data, array('validate' => true, 'atomic' => true)); + + // Otherwise, commit() should be called. + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->once())->method('begin')->will($this->returnValue(true)); + $db->expects($this->once())->method('commit'); + $db->expects($this->never())->method('rollback'); + + $Post->setDataSourceObject($db); + $Post->Author->setDataSourceObject($db); + + $data = array( + 'Post' => array( + 'title' => 'New post', + 'body' => 'Content', + 'published' => 'Y' + ), + 'Author' => array( + 'user' => 'New user', + 'password' => "sekret" + ) + ); + $Post->saveAssociated($data, array('validate' => true, 'atomic' => true)); } /** @@ -7315,4 +7373,38 @@ class ModelWriteTest extends BaseModelTest { $Model = $event->subject; $Model->getDataSource()->delete($Model, array($Model->alias . '.' . $Model->primaryKey => $Model->id)); } + +/** + * Creates a convenient mock DboSource + * + * We cannot call several methods via mock DboSource, such as DboSource::value() + * because mock DboSource has no $_connection. + * This method helps us to avoid this problem. + * + * @param array $methods Configurable method names. + * @return DboSource + */ + protected function _getMockDboSource($methods = array()) { + $testDb = ConnectionManager::getDataSource('test'); + + $passthrough = array_diff(array('value', 'begin', 'rollback', 'commit', 'describe', 'lastInsertId', 'execute'), $methods); + + $methods = array_merge($methods, $passthrough); + if (!in_array('connect', $methods)) { + $methods[] = 'connect'; // This will be called by DboSource::__construct(). + } + + $db = $this->getMock('DboSource', $methods); + $db->columns = $testDb->columns; + $db->startQuote = $testDb->startQuote; + $db->endQuote = $testDb->endQuote; + + foreach ($passthrough as $method) { + $db->expects($this->any()) + ->method($method) + ->will($this->returnCallback(array($testDb, $method))); + } + + return $db; + } } From 799500ce6d10d46cb7b3804454319ce1cbb47520 Mon Sep 17 00:00:00 2001 From: chinpei215 Date: Sat, 2 Aug 2014 10:12:33 +0900 Subject: [PATCH 2/3] Fix transactions do not get rollbacked in saveAssociated/saveMany Refs #2849 --- lib/Cake/Model/Model.php | 298 +++++++++++--------- lib/Cake/Test/Case/Model/ModelWriteTest.php | 92 ++++++ 2 files changed, 253 insertions(+), 137 deletions(-) diff --git a/lib/Cake/Model/Model.php b/lib/Cake/Model/Model.php index d654fcf2b..f04f1c27d 100644 --- a/lib/Cake/Model/Model.php +++ b/lib/Cake/Model/Model.php @@ -2215,6 +2215,7 @@ class Model extends Object implements CakeEventListener { * @return mixed If atomic: True on success, or false on failure. * Otherwise: array similar to the $data array passed, but values are set to true/false * depending on whether each record saved successfully. + * @throws PDOException * @link http://book.cakephp.org/2.0/en/models/saving-your-data.html#model-savemany-array-data-null-array-options-array */ public function saveMany($data = null, $options = array()) { @@ -2245,47 +2246,58 @@ class Model extends Object implements CakeEventListener { if ($options['atomic']) { $db = $this->getDataSource(); $transactionBegun = $db->begin(); + } else { + $transactionBegun = false; } - $return = array(); - foreach ($data as $key => $record) { - $validates = $this->create(null) !== null; - $saved = false; - if ($validates) { - if ($options['deep']) { - $saved = $this->saveAssociated($record, array_merge($options, array('atomic' => false))); - } else { - $saved = $this->save($record, $options); + try { + $return = array(); + foreach ($data as $key => $record) { + $validates = $this->create(null) !== null; + $saved = false; + if ($validates) { + if ($options['deep']) { + $saved = $this->saveAssociated($record, array_merge($options, array('atomic' => false))); + } else { + $saved = $this->save($record, $options); + } + } + + $validates = ($validates && ($saved === true || (is_array($saved) && !in_array(false, $saved, true)))); + if (!$validates) { + $validationErrors[$key] = $this->validationErrors; + } + + if (!$options['atomic']) { + $return[$key] = $validates; + } elseif (!$validates) { + break; } } - $validates = ($validates && ($saved === true || (is_array($saved) && !in_array(false, $saved, true)))); - if (!$validates) { - $validationErrors[$key] = $this->validationErrors; - } + $this->validationErrors = $validationErrors; if (!$options['atomic']) { - $return[$key] = $validates; - } elseif (!$validates) { - break; + return $return; } - } - $this->validationErrors = $validationErrors; + if ($validates) { + if ($transactionBegun) { + return $db->commit() !== false; + } + return true; + } - if (!$options['atomic']) { - return $return; - } - - if ($validates) { if ($transactionBegun) { - return $db->commit() !== false; + $db->rollback(); } - return true; + return false; + } catch (Exception $e) { + if ($transactionBegun) { + $db->rollback(); + } + throw $e; } - - $db->rollback(); - return false; } /** @@ -2337,6 +2349,7 @@ class Model extends Object implements CakeEventListener { * @return mixed If atomic: True on success, or false on failure. * Otherwise: array similar to the $data array passed, but values are set to true/false * depending on whether each record saved successfully. + * @throws PDOException * @link http://book.cakephp.org/2.0/en/models/saving-your-data.html#model-saveassociated-array-data-null-array-options-array */ public function saveAssociated($data = null, $options = array()) { @@ -2368,133 +2381,144 @@ class Model extends Object implements CakeEventListener { if ($options['atomic']) { $db = $this->getDataSource(); $transactionBegun = $db->begin(); + } else { + $transactionBegun = false; } - $associations = $this->getAssociated(); - $return = array(); - $validates = true; - foreach ($data as $association => $values) { - $isEmpty = empty($values) || (isset($values[$association]) && empty($values[$association])); - if ($isEmpty || !isset($associations[$association]) || $associations[$association] !== 'belongsTo') { - continue; - } - - $Model = $this->{$association}; - - $validates = $Model->create(null) !== null; - $saved = false; - if ($validates) { - if ($options['deep']) { - $saved = $Model->saveAssociated($values, array('atomic' => false) + $options); - } else { - $saved = $Model->save($values, array('atomic' => false) + $options); + try { + $associations = $this->getAssociated(); + $return = array(); + $validates = true; + foreach ($data as $association => $values) { + $isEmpty = empty($values) || (isset($values[$association]) && empty($values[$association])); + if ($isEmpty || !isset($associations[$association]) || $associations[$association] !== 'belongsTo') { + continue; } - $validates = ($saved === true || (is_array($saved) && !in_array(false, $saved, true))); - } - if ($validates) { - $key = $this->belongsTo[$association]['foreignKey']; - if (isset($data[$this->alias])) { - $data[$this->alias][$key] = $Model->id; - } else { - $data = array_merge(array($key => $Model->id), $data, array($key => $Model->id)); - } - $options = $this->_addToWhiteList($key, $options); - } else { - $validationErrors[$association] = $Model->validationErrors; - } + $Model = $this->{$association}; - $return[$association] = $validates; - } - - if ($validates && !($this->create(null) !== null && $this->save($data, $options))) { - $validationErrors[$this->alias] = $this->validationErrors; - $validates = false; - } - $return[$this->alias] = $validates; - - foreach ($data as $association => $values) { - if (!$validates) { - break; - } - - $isEmpty = empty($values) || (isset($values[$association]) && empty($values[$association])); - if ($isEmpty || !isset($associations[$association])) { - continue; - } - - $Model = $this->{$association}; - - $type = $associations[$association]; - $key = $this->{$type}[$association]['foreignKey']; - switch ($type) { - case 'hasOne': - if (isset($values[$association])) { - $values[$association][$key] = $this->id; + $validates = $Model->create(null) !== null; + $saved = false; + if ($validates) { + if ($options['deep']) { + $saved = $Model->saveAssociated($values, array('atomic' => false) + $options); } else { - $values = array_merge(array($key => $this->id), $values, array($key => $this->id)); + $saved = $Model->save($values, array('atomic' => false) + $options); } + $validates = ($saved === true || (is_array($saved) && !in_array(false, $saved, true))); + } - $validates = $Model->create(null) !== null; - $saved = false; + if ($validates) { + $key = $this->belongsTo[$association]['foreignKey']; + if (isset($data[$this->alias])) { + $data[$this->alias][$key] = $Model->id; + } else { + $data = array_merge(array($key => $Model->id), $data, array($key => $Model->id)); + } + $options = $this->_addToWhiteList($key, $options); + } else { + $validationErrors[$association] = $Model->validationErrors; + } + + $return[$association] = $validates; + } + + if ($validates && !($this->create(null) !== null && $this->save($data, $options))) { + $validationErrors[$this->alias] = $this->validationErrors; + $validates = false; + } + $return[$this->alias] = $validates; + + foreach ($data as $association => $values) { + if (!$validates) { + break; + } + + $isEmpty = empty($values) || (isset($values[$association]) && empty($values[$association])); + if ($isEmpty || !isset($associations[$association])) { + continue; + } + + $Model = $this->{$association}; + + $type = $associations[$association]; + $key = $this->{$type}[$association]['foreignKey']; + switch ($type) { + case 'hasOne': + if (isset($values[$association])) { + $values[$association][$key] = $this->id; + } else { + $values = array_merge(array($key => $this->id), $values, array($key => $this->id)); + } + + $validates = $Model->create(null) !== null; + $saved = false; + + if ($validates) { + $options = $Model->_addToWhiteList($key, $options); + if ($options['deep']) { + $saved = $Model->saveAssociated($values, array('atomic' => false) + $options); + } else { + $saved = $Model->save($values, $options); + } + } + + $validates = ($validates && ($saved === true || (is_array($saved) && !in_array(false, $saved, true)))); + if (!$validates) { + $validationErrors[$association] = $Model->validationErrors; + } + + $return[$association] = $validates; + break; + case 'hasMany': + foreach ($values as $i => $value) { + if (isset($values[$i][$association])) { + $values[$i][$association][$key] = $this->id; + } else { + $values[$i] = array_merge(array($key => $this->id), $value, array($key => $this->id)); + } + } - if ($validates) { $options = $Model->_addToWhiteList($key, $options); - if ($options['deep']) { - $saved = $Model->saveAssociated($values, array('atomic' => false) + $options); - } else { - $saved = $Model->save($values, $options); + $_return = $Model->saveMany($values, array('atomic' => false) + $options); + if (in_array(false, $_return, true)) { + $validationErrors[$association] = $Model->validationErrors; + $validates = false; } - } - $validates = ($validates && ($saved === true || (is_array($saved) && !in_array(false, $saved, true)))); - if (!$validates) { - $validationErrors[$association] = $Model->validationErrors; - } - - $return[$association] = $validates; - break; - case 'hasMany': - foreach ($values as $i => $value) { - if (isset($values[$i][$association])) { - $values[$i][$association][$key] = $this->id; - } else { - $values[$i] = array_merge(array($key => $this->id), $value, array($key => $this->id)); - } - } - - $options = $Model->_addToWhiteList($key, $options); - $_return = $Model->saveMany($values, array('atomic' => false) + $options); - if (in_array(false, $_return, true)) { - $validationErrors[$association] = $Model->validationErrors; - $validates = false; - } - - $return[$association] = $_return; - break; + $return[$association] = $_return; + break; + } } - } - $this->validationErrors = $validationErrors; + $this->validationErrors = $validationErrors; - if (isset($validationErrors[$this->alias])) { - $this->validationErrors = $validationErrors[$this->alias]; - unset($validationErrors[$this->alias]); - $this->validationErrors = array_merge($this->validationErrors, $validationErrors); - } + if (isset($validationErrors[$this->alias])) { + $this->validationErrors = $validationErrors[$this->alias]; + unset($validationErrors[$this->alias]); + $this->validationErrors = array_merge($this->validationErrors, $validationErrors); + } + + if (!$options['atomic']) { + return $return; + } + if ($validates) { + if ($transactionBegun) { + return $db->commit() !== false; + } + + return true; + } - if (!$options['atomic']) { - return $return; - } - if ($validates) { if ($transactionBegun) { - return $db->commit() !== false; + $db->rollback(); } - - return true; + return false; + } catch (Exception $e) { + if ($transactionBegun) { + $db->rollback(); + } + throw $e; } - - $db->rollback(); - return false; } /** diff --git a/lib/Cake/Test/Case/Model/ModelWriteTest.php b/lib/Cake/Test/Case/Model/ModelWriteTest.php index b9990eebf..2c7be723d 100644 --- a/lib/Cake/Test/Case/Model/ModelWriteTest.php +++ b/lib/Cake/Test/Case/Model/ModelWriteTest.php @@ -4143,6 +4143,25 @@ class ModelWriteTest extends BaseModelTest { ); $Post->saveAll($data, array('atomic' => true, 'validate' => true)); + // If exception thrown, rollback() should be called too. + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->once())->method('begin')->will($this->returnValue(true)); + $db->expects($this->never())->method('commit'); + $db->expects($this->once())->method('rollback'); + + $Post->setDataSourceObject($db); + + $data = array( + array('author_id' => 1, 'title' => 'New Fourth Post'), + array('author_id' => 1, 'title' => 'New Fifth Post', 'body' => $db->expression('PDO_EXCEPTION()')) + ); + + try { + $Post->saveAll($data, array('atomic' => true, 'validate' => true)); + $this->fail('No exception thrown'); + } catch (PDOException $e) { + } + // Otherwise, commit() should be called. $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); $db->expects($this->once())->method('begin')->will($this->returnValue(true)); @@ -4193,6 +4212,33 @@ class ModelWriteTest extends BaseModelTest { ); $Post->saveAll($data, array('validate' => true)); + // If exception thrown, rollback() should be called too. + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->once())->method('begin')->will($this->returnValue(true)); + $db->expects($this->never())->method('commit'); + $db->expects($this->once())->method('rollback'); + + $Post->setDataSourceObject($db); + $Post->Author->setDataSourceObject($db); + + $data = array( + 'Post' => array( + 'title' => 'New post', + 'body' => $db->expression('PDO_EXCEPTION()'), + 'published' => 'Y' + ), + 'Author' => array( + 'user' => 'New user', + 'password' => "sekret" + ) + ); + + try { + $Post->saveAll($data, array('validate' => true)); + $this->fail('No exception thrown'); + } catch (PDOException $e) { + } + // Otherwise, commit() should be called. $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); $db->expects($this->once())->method('begin')->will($this->returnValue(true)); @@ -5605,6 +5651,25 @@ class ModelWriteTest extends BaseModelTest { ); $Post->saveMany($data, array('validate' => true)); + // If exception thrown, rollback() should be called too. + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->once())->method('begin')->will($this->returnValue(true)); + $db->expects($this->never())->method('commit'); + $db->expects($this->once())->method('rollback'); + + $Post->setDataSourceObject($db); + + $data = array( + array('author_id' => 1, 'title' => 'New Fourth Post'), + array('author_id' => 1, 'title' => 'New Fifth Post', 'body' => $db->expression('PDO_EXCEPTION()')) + ); + + try { + $Post->saveMany($data, array('validate' => true)); + $this->fail('No exception thrown'); + } catch (PDOException $e) { + } + // Otherwise, commit() should be called. $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); $db->expects($this->once())->method('begin')->will($this->returnValue(true)); @@ -5655,6 +5720,33 @@ class ModelWriteTest extends BaseModelTest { ); $Post->saveAssociated($data, array('validate' => true, 'atomic' => true)); + // If exception thrown, commit() should be called. + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->once())->method('begin')->will($this->returnValue(true)); + $db->expects($this->never())->method('commit'); + $db->expects($this->once())->method('rollback'); + + $Post->setDataSourceObject($db); + $Post->Author->setDataSourceObject($db); + + $data = array( + 'Post' => array( + 'title' => 'New post', + 'body' => $db->expression('PDO_EXCEPTION()'), + 'published' => 'Y' + ), + 'Author' => array( + 'user' => 'New user', + 'password' => "sekret" + ) + ); + + try { + $Post->saveAssociated($data, array('validate' => true, 'atomic' => true)); + $this->fail('No exception thrown'); + } catch (PDOException $e) { + } + // Otherwise, commit() should be called. $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); $db->expects($this->once())->method('begin')->will($this->returnValue(true)); From cb376bf420a7414ef573d4d7c4ff7d23ef0a4514 Mon Sep 17 00:00:00 2001 From: chinpei215 Date: Sun, 3 Aug 2014 22:34:11 +0900 Subject: [PATCH 3/3] Add some more transaction tests And remove 2 else clauses. --- lib/Cake/Model/Model.php | 6 +- lib/Cake/Test/Case/Model/ModelWriteTest.php | 169 ++++++++++++++++++++ 2 files changed, 171 insertions(+), 4 deletions(-) diff --git a/lib/Cake/Model/Model.php b/lib/Cake/Model/Model.php index f04f1c27d..f5e8ed1d4 100644 --- a/lib/Cake/Model/Model.php +++ b/lib/Cake/Model/Model.php @@ -2243,11 +2243,10 @@ class Model extends Object implements CakeEventListener { $options['validate'] = false; } + $transactionBegun = false; if ($options['atomic']) { $db = $this->getDataSource(); $transactionBegun = $db->begin(); - } else { - $transactionBegun = false; } try { @@ -2378,11 +2377,10 @@ class Model extends Object implements CakeEventListener { $options['validate'] = false; } + $transactionBegun = false; if ($options['atomic']) { $db = $this->getDataSource(); $transactionBegun = $db->begin(); - } else { - $transactionBegun = false; } try { diff --git a/lib/Cake/Test/Case/Model/ModelWriteTest.php b/lib/Cake/Test/Case/Model/ModelWriteTest.php index 2c7be723d..6a523e5a1 100644 --- a/lib/Cake/Test/Case/Model/ModelWriteTest.php +++ b/lib/Cake/Test/Case/Model/ModelWriteTest.php @@ -36,6 +36,8 @@ class TestAuthor extends Author { protected $_dataSourceObject; + public $dataForAfterSave; + /** * Helper method to set a datasource object * @@ -74,6 +76,8 @@ class TestPost extends Post { protected $_dataSourceObject; + public $dataForAfterSave; + /** * Helper method to set a datasource object * @@ -7499,4 +7503,169 @@ class ModelWriteTest extends BaseModelTest { return $db; } + +/** + * Test that transactions behave correctly on nested saveMany calls. + * + * @return void + */ + public function testTransactionOnNestedSaveMany() { + $this->loadFixtures('Post'); + $Post = new TestPost(); + $Post->getEventManager()->attach(array($this, 'nestedSaveMany'), 'Model.afterSave'); + + // begin -> [ begin -> commit ] -> commit + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->exactly(2))->method('begin')->will($this->returnValue(true)); + $db->expects($this->exactly(2))->method('commit'); + $db->expects($this->never())->method('rollback'); + $Post->setDataSourceObject($db); + + $data = array( + array('author_id' => 1, 'title' => 'Outer Post'), + ); + $Post->dataForAfterSave = array( + array('author_id' => 1, 'title' => 'Inner Post'), + ); + $this->assertTrue($Post->saveMany($data)); + + // begin -> [ begin(false) ] -> commit + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->at(0))->method('begin')->will($this->returnValue(true)); + $db->expects($this->at(1))->method('begin')->will($this->returnValue(false)); + $db->expects($this->once())->method('commit'); + $db->expects($this->never())->method('rollback'); + $Post->setDataSourceObject($db); + + $data = array( + array('author_id' => 1, 'title' => 'Outer Post'), + ); + $Post->dataForAfterSave = array( + array('author_id' => 1, 'title' => 'Inner Post'), + ); + $this->assertTrue($Post->saveMany($data)); + + // begin -> [ begin -> rollback ] -> rollback + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->exactly(2))->method('begin')->will($this->returnValue(true)); + $db->expects($this->never())->method('commit'); + $db->expects($this->exactly(2))->method('rollback'); + $Post->setDataSourceObject($db); + $data = array( + array('author_id' => 1, 'title' => 'Outer Post'), + ); + $Post->dataForAfterSave = array( + array('author_id' => 1, 'title' => 'Inner Post', 'body' => $db->expression('PDO_EXCEPTION()')), + ); + + try { + $Post->saveMany($data); + $this->fail('No exception thrown'); + } catch(Exception $e) { + } + } + +/** + * Test that transaction behaves correctly on nested saveAssociated calls. + * + * @return void + */ + public function testTransactionOnNestedSaveAssociated() { + $this->loadFixtures('Author', 'Post'); + + $Author = new TestAuthor(); + $Author->getEventManager()->attach(array($this, 'nestedSaveAssociated'), 'Model.afterSave'); + + // begin -> [ begin -> commit ] -> commit + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->exactly(2))->method('begin')->will($this->returnValue(true)); + $db->expects($this->exactly(2))->method('commit'); + $db->expects($this->never())->method('rollback'); + $Author->setDataSourceObject($db); + $Author->Post->setDataSourceObject($db); + + $data = array( + 'Author' => array('user' => 'outer'), + 'Post' => array( + array('title' => 'Outer Post'), + ) + ); + $Author->dataForAfterSave = array( + 'Author' => array('user' => 'inner'), + 'Post' => array( + array('title' => 'Inner Post'), + ) + ); + $this->assertTrue($Author->saveAssociated($data)); + + // begin -> [ begin(false) ] -> commit + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->at(0))->method('begin')->will($this->returnValue(true)); + $db->expects($this->at(1))->method('begin')->will($this->returnValue(false)); + $db->expects($this->once())->method('commit'); + $db->expects($this->never())->method('rollback'); + $Author->setDataSourceObject($db); + $Author->Post->setDataSourceObject($db); + $data = array( + 'Author' => array('user' => 'outer'), + 'Post' => array( + array('title' => 'Outer Post'), + ) + ); + $Author->dataForAfterSave = array( + 'Author' => array('user' => 'inner'), + 'Post' => array( + array('title' => 'Inner Post'), + ) + ); + $this->assertTrue($Author->saveAssociated($data)); + + // begin -> [ begin -> rollback ] -> rollback + $db = $this->_getMockDboSource(array('begin', 'commit', 'rollback')); + $db->expects($this->exactly(2))->method('begin')->will($this->returnValue(true)); + $db->expects($this->never())->method('commit'); + $db->expects($this->exactly(2))->method('rollback'); + $Author->setDataSourceObject($db); + $Author->Post->setDataSourceObject($db); + $data = array( + 'Author' => array('user' => 'outer'), + 'Post' => array( + array('title' => 'Outer Post'), + ) + ); + $Author->dataForAfterSave = array( + 'Author' => array('user' => 'inner', 'password' => $db->expression('PDO_EXCEPTION()')), + 'Post' => array( + array('title' => 'Inner Post'), + ) + ); + + try { + $Author->saveAssociated($data); + $this->fail('No exception thrown'); + } catch(Exception $e) { + } + } + +/** + * A callback for testing nested saveMany. + * + * @param CakeEvent $event containing the Model + * @return void + */ + public function nestedSaveMany($event) { + $Model = $event->subject; + $Model->saveMany($Model->dataForAfterSave, array('callbacks' => false)); + } + +/** + * A callback for testing nested saveAssociated. + * + * @param CakeEvent $event containing the Model + * @return void + */ + public function nestedSaveAssociated($event) { + $Model = $event->subject; + $Model->saveAssociated($Model->dataForAfterSave, array('callbacks' => false)); + } }