Merge pull request #3959 from chinpei215/master-issue3857-fix

Fix a race condition problem

Fixes #3857

lib/Cake/Model/Datasource/DboSource.php

@@ -2399,7 +2399,7 @@ class DboSource extends DataSource {
 			return $conditions;
 		}
 		$exists = $Model->exists();
-		if (!$exists && $conditions !== null) {
+		if (!$exists && ($conditions !== null || !empty($Model->__safeUpdateMode))) {
 			return false;
 		} elseif (!$exists) {
 			return null;

lib/Cake/Model/Model.php

@@ -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
 
 /**
@@ -1686,6 +1694,7 @@ class Model extends Object implements CakeEventListener {
  *
  * @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()) {
@@ -1820,7 +1829,14 @@ class Model extends Object implements CakeEventListener {
 			$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])) {

lib/Cake/Test/Case/Model/Datasource/DboSourceTest.php

@@ -33,6 +33,9 @@ require_once dirname(dirname(__FILE__)) . DS . 'models.php';
  */
 class MockPDO extends PDO {
 
+/**
+ * Constructor.
+ */
 	public function __construct() {
 	}
 
@@ -1412,4 +1415,45 @@ class DboSourceTest extends CakeTestCase {
 		$result = $db->insertMulti('articles', array_keys($data[0]), $data);
 		$this->assertTrue($result, 'Data was saved');
 	}
+
+/**
+ * Test defaultConditions()
+ *
+ * @return void
+ */
+	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);
+	}
 }

lib/Cake/Test/Case/Model/ModelWriteTest.php

@@ -6371,6 +6371,11 @@ class ModelWriteTest extends BaseModelTest {
 		$this->assertEquals(array(6, 4, 5, 2), $result);
 	}
 
+/**
+ * testToggleBoolFields method
+ *
+ * @return void
+ */
 	public function testToggleBoolFields() {
 		$this->loadFixtures('CounterCacheUser', 'CounterCachePost');
 		$Post = new CounterCachePost();
@@ -7261,4 +7266,53 @@ class ModelWriteTest extends BaseModelTest {
 		$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 void
+ */
+	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 CakeEvent $event containing the Model
+ * @return void
+ */
+	public function deleteMe($event) {
+		$Model = $event->subject;
+		$Model->getDataSource()->delete($Model, array($Model->alias . '.' . $Model->primaryKey => $Model->id));
+	}
 }