introduce preCondition for setUserValue to provide atomic check-and-update

6 files changed, 153 insertions, 15 deletions

diff --git a/lib/private/allconfig.php b/lib/private/allconfig.php
index 17872fd4f69..ed4378cfc44 100644
--- a/lib/private/allconfig.php
+++ b/lib/private/allconfig.php
@@ -9,6 +9,7 @@
 
 namespace OC;
 use OCP\IDBConnection;
+use OCP\PreConditionNotMetException;
 
 /**
  * Class to combine all the configuration options ownCloud offers
@@ -140,8 +141,10 @@ class AllConfig implements \OCP\IConfig {
 	 * @param string $appName the appName that we want to store the value under
 	 * @param string $key the key under which the value is being stored
 	 * @param string $value the value that you want to store
+	 * @param string $preCondition only update if the config value was previously the value passed as $preCondition
+	 * @throws \OCP\PreConditionNotMetException if a precondition is specified and is not met
 	 */
-	public function setUserValue($userId, $appName, $key, $value) {
+	public function setUserValue($userId, $appName, $key, $value, $preCondition = null) {
 		// Check if the key does exist
 		$sql  = 'SELECT `configvalue` FROM `*PREFIX*preferences` '.
 				'WHERE `userid` = ? AND `appid` = ? AND `configkey` = ?';
@@ -154,15 +157,24 @@ class AllConfig implements \OCP\IConfig {
 			return;
 		}
 
-		if (!$exists) {
+		$data = array($value, $userId, $appName, $key);
+		if (!$exists && $preCondition === null) {
 			$sql  = 'INSERT INTO `*PREFIX*preferences` (`configvalue`, `userid`, `appid`, `configkey`)'.
 					'VALUES (?, ?, ?, ?)';
-		} else {
+		} elseif ($exists) {
 			$sql  = 'UPDATE `*PREFIX*preferences` SET `configvalue` = ? '.
-					'WHERE `userid` = ? AND `appid` = ? AND `configkey` = ?';
-
+					'WHERE `userid` = ? AND `appid` = ? AND `configkey` = ? ';
+
+			if($preCondition !== null) {
+				if($this->getSystemValue('dbtype', 'sqlite') === 'oci') {
+					//oracle hack: need to explicitly cast CLOB to CHAR for comparison
+					$sql .= 'AND to_char(`configvalue`) = ?';
+				} else {
+					$sql .= 'AND `configvalue` = ?';
+				}
+				$data[] = $preCondition;
+			}
 		}
-		$data = array($value, $userId, $appName, $key);
 		$affectedRows = $this->connection->executeUpdate($sql, $data);
 
 		// only add to the cache if we already loaded data for the user
@@ -172,6 +184,10 @@ class AllConfig implements \OCP\IConfig {
 			}
 			$this->userCache[$userId][$appName][$key] = $value;
 		}
+
+		if ($preCondition !== null && $affectedRows === 0) {
+			throw new PreConditionNotMetException;
+		}
 	}
 
 	/**
diff --git a/lib/private/legacy/preferences.php b/lib/private/legacy/preferences.php
index 28af124db32..907aafbc915 100644
--- a/lib/private/legacy/preferences.php
+++ b/lib/private/legacy/preferences.php
@@ -70,10 +70,12 @@ class OC_Preferences{
 	 * will be added automagically.
 	 */
 	public static function setValue( $user, $app, $key, $value, $preCondition = null ) {
-		return \OC::$server->getConfig()->setUserValue($user, $app, $key, $value);
-
-		// TODO maybe catch exceptions and then return false
-		return true;
+		try {
+			\OC::$server->getConfig()->setUserValue($user, $app, $key, $value, $preCondition);
+			return true;
+		} catch(\OCP\PreConditionNotMetException $e) {
+			return false;
+		}
 	}
 
 	/**
diff --git a/lib/private/preferences.php b/lib/private/preferences.php
index 9f33136aeb2..cd4a9fd1c19 100644
--- a/lib/private/preferences.php
+++ b/lib/private/preferences.php
@@ -37,6 +37,7 @@
 namespace OC;
 
 use OCP\IDBConnection;
+use OCP\PreConditionNotMetException;
 
 
 /**
@@ -111,10 +112,12 @@ class Preferences {
 	 * will be added automagically.
 	 */
 	public function setValue($user, $app, $key, $value, $preCondition = null) {
-		return $this->config->setUserValue($user, $app, $key, $value);
-
-		// TODO maybe catch exceptions and then return false
-		return true;
+		try {
+			$this->config->setUserValue($user, $app, $key, $value, $preCondition);
+			return true;
+		} catch(PreConditionNotMetException $e) {
+			return false;
+		}
 	}
 
 	/**
diff --git a/lib/public/iconfig.php b/lib/public/iconfig.php
index 1d3502ffb4c..fe0f1273a80 100644
--- a/lib/public/iconfig.php
+++ b/lib/public/iconfig.php
@@ -109,8 +109,10 @@ interface IConfig {
 	 * @param string $appName the appName that we want to store the value under
 	 * @param string $key the key under which the value is being stored
 	 * @param string $value the value that you want to store
+	 * @param string $preCondition only update if the config value was previously the value passed as $preCondition
+	 * @throws \OCP\PreConditionNotMetException if a precondition is specified and is not met
 	 */
-	public function setUserValue($userId, $appName, $key, $value);
+	public function setUserValue($userId, $appName, $key, $value, $preCondition = null);
 
 	/**
 	 * Shortcut for getting a user defined value
diff --git a/lib/public/preconditionnotmetexception.php b/lib/public/preconditionnotmetexception.php
new file mode 100644
index 00000000000..ceba01a0b4b
--- /dev/null
+++ b/lib/public/preconditionnotmetexception.php
@@ -0,0 +1,30 @@
+<?php
+/**
+ * ownCloud
+ *
+ * @author Morris Jobke
+ * @copyright 2014 Morris Jobke <hey@morrisjobke.de>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU AFFERO GENERAL PUBLIC LICENSE
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU AFFERO GENERAL PUBLIC LICENSE for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public
+ * License along with this library.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+// use OCP namespace for all classes that are considered public.
+// This means that they should be used by apps instead of the internal ownCloud classes
+namespace OCP;
+
+/**
+ * Exception if the precondition of the config update method isn't met
+ */
+class PreConditionNotMetException extends \Exception {}
diff --git a/tests/lib/allconfig.php b/tests/lib/allconfig.php
index a2da46a6105..63ee60f2078 100644
--- a/tests/lib/allconfig.php
+++ b/tests/lib/allconfig.php
@@ -80,6 +80,91 @@ class TestAllConfig extends \Test\TestCase {
 		$config->deleteUserValue('userSet', 'appSet', 'keySet');
 	}
 
+	public function testSetUserValueWithPreCondition() {
+		// mock the check for the database to run the correct SQL statements for each database type
+		$systemConfig = $this->getMock('\OC\SystemConfig');
+		$systemConfig->expects($this->once())
+			->method('getValue')
+			->with($this->equalTo('dbtype'),
+				$this->equalTo('sqlite'))
+			->will($this->returnValue(\OC::$server->getConfig()->getSystemValue('dbtype', 'sqlite')));
+		$config = $this->getConfig($systemConfig);
+
+		$selectAllSQL = 'SELECT `userid`, `appid`, `configkey`, `configvalue` FROM `*PREFIX*preferences` WHERE `userid` = ?';
+
+		$config->setUserValue('userPreCond', 'appPreCond', 'keyPreCond', 'valuePreCond');
+
+		$result = $this->connection->executeQuery($selectAllSQL, array('userPreCond'))->fetchAll();
+
+		$this->assertEquals(1, count($result));
+		$this->assertEquals(array(
+			'userid'      => 'userPreCond',
+			'appid'       => 'appPreCond',
+			'configkey'   => 'keyPreCond',
+			'configvalue' => 'valuePreCond'
+		), $result[0]);
+
+		// test if the method overwrites existing database entries with valid precond
+		$config->setUserValue('userPreCond', 'appPreCond', 'keyPreCond', 'valuePreCond2', 'valuePreCond');
+
+		$result = $this->connection->executeQuery($selectAllSQL, array('userPreCond'))->fetchAll();
+
+		$this->assertEquals(1, count($result));
+		$this->assertEquals(array(
+			'userid'      => 'userPreCond',
+			'appid'       => 'appPreCond',
+			'configkey'   => 'keyPreCond',
+			'configvalue' => 'valuePreCond2'
+		), $result[0]);
+
+		// cleanup
+		$config->deleteUserValue('userPreCond', 'appPreCond', 'keyPreCond');
+	}
+
+	/**
+	 * @expectedException \OCP\PreConditionNotMetException
+	 */
+	public function testSetUserValueWithPreConditionFailure() {
+		// mock the check for the database to run the correct SQL statements for each database type
+		$systemConfig = $this->getMock('\OC\SystemConfig');
+		$systemConfig->expects($this->once())
+			->method('getValue')
+			->with($this->equalTo('dbtype'),
+				$this->equalTo('sqlite'))
+			->will($this->returnValue(\OC::$server->getConfig()->getSystemValue('dbtype', 'sqlite')));
+		$config = $this->getConfig($systemConfig);
+
+		$selectAllSQL = 'SELECT `userid`, `appid`, `configkey`, `configvalue` FROM `*PREFIX*preferences` WHERE `userid` = ?';
+
+		$config->setUserValue('userPreCond1', 'appPreCond', 'keyPreCond', 'valuePreCond');
+
+		$result = $this->connection->executeQuery($selectAllSQL, array('userPreCond1'))->fetchAll();
+
+		$this->assertEquals(1, count($result));
+		$this->assertEquals(array(
+			'userid'      => 'userPreCond1',
+			'appid'       => 'appPreCond',
+			'configkey'   => 'keyPreCond',
+			'configvalue' => 'valuePreCond'
+		), $result[0]);
+
+		// test if the method overwrites existing database entries with valid precond
+		$config->setUserValue('userPreCond1', 'appPreCond', 'keyPreCond', 'valuePreCond2', 'valuePreCond3');
+
+		$result = $this->connection->executeQuery($selectAllSQL, array('userPreCond1'))->fetchAll();
+
+		$this->assertEquals(1, count($result));
+		$this->assertEquals(array(
+			'userid'      => 'userPreCond1',
+			'appid'       => 'appPreCond',
+			'configkey'   => 'keyPreCond',
+			'configvalue' => 'valuePreCond'
+		), $result[0]);
+
+		// cleanup
+		$config->deleteUserValue('userPreCond1', 'appPreCond', 'keyPreCond');
+	}
+
 	public function testSetUserValueUnchanged() {
 		$resultMock = $this->getMockBuilder('\Doctrine\DBAL\Driver\Statement')
 			->disableOriginalConstructor()->getMock();