Skip to content

Commit

Permalink
Fix a race condition problem
Browse files Browse the repository at this point in the history
Prevents Model::save() from generating a query with WHERE 1 = 1 on race condition.

Refs #3857
  • Loading branch information
chinpei215 committed Jul 12, 2014
1 parent 03c2a8b commit ace30fd
Show file tree
Hide file tree
Showing 4 changed files with 108 additions and 2 deletions.
2 changes: 1 addition & 1 deletion lib/Cake/Model/Datasource/DboSource.php
Expand Up @@ -2399,7 +2399,7 @@ public function defaultConditions(Model $Model, $conditions, $useAlias = true) {
return $conditions;
}
$exists = $Model->exists();
if (!$exists && $conditions !== null) {
if (!$exists && ($conditions !== null || !empty($Model->__safeUpdateMode))) {
return false;
} elseif (!$exists) {
return null;
Expand Down
18 changes: 17 additions & 1 deletion lib/Cake/Model/Model.php
Expand Up @@ -593,6 +593,14 @@ class Model extends Object implements CakeEventListener {
*/
public $__backContainableAssociation = array();

/**
* Safe update mode
* If true, this prevents Model::save() from generating a query with WHERE 1 = 1 on race condition.
*
* @var bool
*/
public $__safeUpdateMode = false;

// @codingStandardsIgnoreEnd

/**
Expand Down Expand Up @@ -1686,6 +1694,7 @@ public function saveField($name, $value, $validate = false) {
*
* @param array $fieldList List of fields to allow to be saved
* @return mixed On success Model::$data if its not empty or true, false on failure
* @throws PDOException
* @link http://book.cakephp.org/2.0/en/models/saving-your-data.html
*/
public function save($data = null, $validate = true, $fieldList = array()) {
Expand Down Expand Up @@ -1820,7 +1829,14 @@ public function save($data = null, $validate = true, $fieldList = array()) {
$cache = $this->_prepareUpdateFields(array_combine($fields, $values));

if (!empty($this->id)) {
$success = (bool)$db->update($this, $fields, $values);
$this->__safeUpdateMode = true;
try {
$success = (bool)$db->update($this, $fields, $values);
} catch (Exception $e) {
$this->__safeUpdateMode = false;
throw $e;
}
$this->__safeUpdateMode = false;
} else {
if (empty($this->data[$this->alias][$this->primaryKey]) && $this->_isUUIDField($this->primaryKey)) {
if (array_key_exists($this->primaryKey, $this->data[$this->alias])) {
Expand Down
41 changes: 41 additions & 0 deletions lib/Cake/Test/Case/Model/Datasource/DboSourceTest.php
Expand Up @@ -1412,4 +1412,45 @@ public function testInsertMultiId() {
$result = $db->insertMulti('articles', array_keys($data[0]), $data);
$this->assertTrue($result, 'Data was saved');
}

/**
* Test defaultConditions()
*
* @return
*/
public function testDefaultConditions() {
$this->loadFixtures('Article');
$Article = ClassRegistry::init('Article');
$db = $Article->getDataSource();

// Creates a default set of conditions from the model if $conditions is null/empty.
$Article->id = 1;
$result = $db->defaultConditions($Article, null);
$this->assertEquals(array('Article.id' => 1), $result);

// $useAlias == false
$Article->id = 1;
$result = $db->defaultConditions($Article, null, false);
$this->assertEquals(array($db->fullTableName($Article, false) . '.id' => 1), $result);

// If conditions are supplied then they will be returned.
$Article->id = 1;
$result = $db->defaultConditions($Article, array('Article.title' => 'First article'));
$this->assertEquals(array('Article.title' => 'First article'), $result);

// If a model doesn't exist and no conditions were provided either null or false will be returned based on what was input.
$Article->id = 1000000;
$result = $db->defaultConditions($Article, null);
$this->assertNull($result);

$Article->id = 1000000;
$result = $db->defaultConditions($Article, false);
$this->assertFalse($result);

// Safe update mode
$Article->id = 1000000;
$Article->__safeUpdateMode = true;
$result = $db->defaultConditions($Article, null);
$this->assertFalse($result);
}
}
49 changes: 49 additions & 0 deletions lib/Cake/Test/Case/Model/ModelWriteTest.php
Expand Up @@ -7261,4 +7261,53 @@ public function testClear() {
$this->assertFalse(isset($model->data['Bid']['name']));
$this->assertFalse(isset($model->data['Bid']['message_id']));
}

/**
* Test that Model::save() doesn't generate a query with WHERE 1 = 1 on race condition.
*
* @link https://github.com/cakephp/cakephp/issues/3857
* @return
*/
public function testSafeUpdateMode() {
$this->loadFixtures('User');

$User = ClassRegistry::init('User');
$this->assertFalse($User->__safeUpdateMode);

$User->getEventManager()->attach(array($this, 'deleteMe'), 'Model.beforeSave');

$User->id = 1;
$User->set(array('user' => 'nobody'));
$User->save();

$users = $User->find('list', array('fields' => 'User.user'));

$expected = array(
2 => 'nate',
3 => 'larry',
4 => 'garrett',
);
$this->assertEquals($expected, $users);
$this->assertFalse($User->__safeUpdateMode);

$User->id = 2;
$User->set(array('user' => $User->getDataSource()->expression('PDO_EXCEPTION()')));
try {
$User->save(null, false);
$this->fail('No exception thrown');
} catch (PDOException $e) {
$this->assertFalse($User->__safeUpdateMode);
}
}

/**
* Emulates race condition
*
* @param $event
* @return
*/
public function deleteMe($event) {
$Model = $event->subject;
$Model->getDataSource()->delete($Model, array($Model->alias . '.' . $Model->primaryKey => $Model->id));
}
}

0 comments on commit ace30fd

Please sign in to comment.