From af91ee97c981d32bcd531e71d31e16f1232c44ce Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Wed, 3 Dec 2014 21:31:29 +0100 Subject: [PATCH] introduce preCondition for setUserValue to provide atomic check-and-update --- lib/private/allconfig.php | 26 +++++-- lib/private/legacy/preferences.php | 10 ++- lib/private/preferences.php | 11 ++- lib/public/iconfig.php | 4 +- lib/public/preconditionnotmetexception.php | 30 ++++++++ tests/lib/allconfig.php | 85 ++++++++++++++++++++++ 6 files changed, 152 insertions(+), 14 deletions(-) create mode 100644 lib/public/preconditionnotmetexception.php diff --git a/lib/private/allconfig.php b/lib/private/allconfig.php index 17872fd4f6..ed4378cfc4 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 28af124db3..907aafbc91 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 9f33136aeb..cd4a9fd1c1 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 1d3502ffb4..fe0f1273a8 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 0000000000..ceba01a0b4 --- /dev/null +++ b/lib/public/preconditionnotmetexception.php @@ -0,0 +1,30 @@ + + * + * 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 . + * + */ + +// 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 a2da46a610..63ee60f207 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();