From 2916b0ba76b4b5715a88c0c7f9d2a358a988e9c8 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 11 May 2015 12:22:23 +0200 Subject: [PATCH 1/3] Always test the object and the legacy class --- tests/lib/appconfig.php | 186 +++++++++++++++++++++++++++------------- 1 file changed, 127 insertions(+), 59 deletions(-) diff --git a/tests/lib/appconfig.php b/tests/lib/appconfig.php index ead5b859277..9238d16bb86 100644 --- a/tests/lib/appconfig.php +++ b/tests/lib/appconfig.php @@ -8,16 +8,25 @@ */ class Test_Appconfig extends \Test\TestCase { - public static function setUpBeforeClass() { - parent::setUpBeforeClass(); + /** @var \OCP\IAppConfig */ + protected $appConfig; - $query = \OC_DB::prepare('DELETE FROM `*PREFIX*appconfig` WHERE `appid` = ?'); + /** @var \OCP\IDBConnection */ + protected $connection; + + public function setUp() { + parent::setUp(); + + $this->connection = \OC::$server->getDatabaseConnection(); + $this->registerAppConfig(new \OC\AppConfig(\OC::$server->getDatabaseConnection())); + + $query = $this->connection->prepare('DELETE FROM `*PREFIX*appconfig` WHERE `appid` = ?'); $query->execute(array('testapp')); $query->execute(array('someapp')); $query->execute(array('123456')); $query->execute(array('anotherapp')); - $query = \OC_DB::prepare('INSERT INTO `*PREFIX*appconfig` VALUES (?, ?, ?)'); + $query = $this->connection->prepare('INSERT INTO `*PREFIX*appconfig` VALUES (?, ?, ?)'); $query->execute(array('testapp', 'enabled', 'true')); $query->execute(array('testapp', 'installed_version', '1.2.3')); @@ -35,17 +44,41 @@ class Test_Appconfig extends \Test\TestCase { $query->execute(array('anotherapp', 'enabled', 'false')); } - public static function tearDownAfterClass() { - $query = \OC_DB::prepare('DELETE FROM `*PREFIX*appconfig` WHERE `appid` = ?'); + public function tearDown() { + $query = $this->connection->prepare('DELETE FROM `*PREFIX*appconfig` WHERE `appid` = ?'); $query->execute(array('testapp')); $query->execute(array('someapp')); $query->execute(array('123456')); $query->execute(array('anotherapp')); - parent::tearDownAfterClass(); + $this->registerAppConfig(new \OC\AppConfig(\OC::$server->getDatabaseConnection())); + parent::tearDown(); } - public function testGetApps() { + /** + * Register an app config object for testing purposes. + * + * @param \OCP\IAppConfig $appConfig + */ + protected function registerAppConfig($appConfig) { + \OC::$server->registerService('AppConfig', function ($c) use ($appConfig) { + return $appConfig; + }); + } + + public function getAppConfigs() { + return [ + ['\OC_Appconfig'], + [new \OC\AppConfig(\OC::$server->getDatabaseConnection())], + ]; + } + + /** + * @dataProvider getAppConfigs + * + * @param mixed $callable + */ + public function testGetApps($callable) { $query = \OC_DB::prepare('SELECT DISTINCT `appid` FROM `*PREFIX*appconfig` ORDER BY `appid`'); $result = $query->execute(); $expected = array(); @@ -53,11 +86,16 @@ class Test_Appconfig extends \Test\TestCase { $expected[] = $row['appid']; } sort($expected); - $apps = \OC_Appconfig::getApps(); + $apps = call_user_func([$callable, 'getApps']); $this->assertEquals($expected, $apps); } - public function testGetKeys() { + /** + * @dataProvider getAppConfigs + * + * @param mixed $callable + */ + public function testGetKeys($callable) { $query = \OC_DB::prepare('SELECT `configkey` FROM `*PREFIX*appconfig` WHERE `appid` = ?'); $result = $query->execute(array('testapp')); $expected = array(); @@ -65,46 +103,114 @@ class Test_Appconfig extends \Test\TestCase { $expected[] = $row["configkey"]; } sort($expected); - $keys = \OC_Appconfig::getKeys('testapp'); + $keys = call_user_func([$callable, 'getKeys'], 'testapp'); $this->assertEquals($expected, $keys); } - public function testGetValue() { + /** + * @dataProvider getAppConfigs + * + * @param mixed $callable + */ + public function testGetValue($callable) { $query = \OC_DB::prepare('SELECT `configvalue` FROM `*PREFIX*appconfig` WHERE `appid` = ? AND `configkey` = ?'); $result = $query->execute(array('testapp', 'installed_version')); $expected = $result->fetchRow(); - $value = \OC_Appconfig::getValue('testapp', 'installed_version'); + $value = call_user_func([$callable, 'getValue'], 'testapp', 'installed_version'); $this->assertEquals($expected['configvalue'], $value); - $value = \OC_Appconfig::getValue('testapp', 'nonexistant'); + $value = call_user_func([$callable, 'getValue'], 'testapp', 'nonexistant'); $this->assertNull($value); - $value = \OC_Appconfig::getValue('testapp', 'nonexistant', 'default'); + $value = call_user_func([$callable, 'getValue'], 'testapp', 'nonexistant', 'default'); $this->assertEquals('default', $value); } - public function testHasKey() { - $value = \OC_Appconfig::hasKey('testapp', 'installed_version'); + /** + * @dataProvider getAppConfigs + * + * @param mixed $callable + */ + public function testHasKey($callable) { + $value = call_user_func([$callable, 'hasKey'], 'testapp', 'installed_version'); $this->assertTrue($value); - $value = \OC_Appconfig::hasKey('nonexistant', 'nonexistant'); + $value = call_user_func([$callable, 'hasKey'], 'nonexistant', 'nonexistant'); $this->assertFalse($value); } - public function testSetValue() { - \OC_Appconfig::setValue('testapp', 'installed_version', '1.33.7'); + /** + * @dataProvider getAppConfigs + * + * @param mixed $callable + */ + public function testSetValue($callable) { + call_user_func([$callable, 'setValue'], 'testapp', 'installed_version', '1.33.7'); $query = \OC_DB::prepare('SELECT `configvalue` FROM `*PREFIX*appconfig` WHERE `appid` = ? AND `configkey` = ?'); $result = $query->execute(array('testapp', 'installed_version')); $value = $result->fetchRow(); $this->assertEquals('1.33.7', $value['configvalue']); - \OC_Appconfig::setValue('someapp', 'somekey', 'somevalue'); + call_user_func([$callable, 'setValue'], 'someapp', 'somekey', 'somevalue'); $query = \OC_DB::prepare('SELECT `configvalue` FROM `*PREFIX*appconfig` WHERE `appid` = ? AND `configkey` = ?'); $result = $query->execute(array('someapp', 'somekey')); $value = $result->fetchRow(); $this->assertEquals('somevalue', $value['configvalue']); } + /** + * @dataProvider getAppConfigs + * + * @param mixed $callable + */ + public function testDeleteKey($callable) { + call_user_func([$callable, 'deleteKey'], 'testapp', 'deletethis'); + $query = \OC_DB::prepare('SELECT `configvalue` FROM `*PREFIX*appconfig` WHERE `appid` = ? AND `configkey` = ?'); + $query->execute(array('testapp', 'deletethis')); + $result = (bool)$query->fetchRow(); + $this->assertFalse($result); + } + + /** + * @dataProvider getAppConfigs + * + * @param mixed $callable + */ + public function testDeleteApp($callable) { + call_user_func([$callable, 'deleteApp'], 'someapp'); + $query = \OC_DB::prepare('SELECT `configkey` FROM `*PREFIX*appconfig` WHERE `appid` = ?'); + $query->execute(array('someapp')); + $result = (bool)$query->fetchRow(); + $this->assertFalse($result); + } + + /** + * @dataProvider getAppConfigs + * + * @param mixed $callable + */ + public function testGetValues($callable) { + $this->assertFalse(call_user_func([$callable, 'getValues'], 'testapp', 'enabled')); + + $query = \OC_DB::prepare('SELECT `configkey`, `configvalue` FROM `*PREFIX*appconfig` WHERE `appid` = ?'); + $query->execute(array('testapp')); + $expected = array(); + while ($row = $query->fetchRow()) { + $expected[$row['configkey']] = $row['configvalue']; + } + $values = call_user_func([$callable, 'getValues'], 'testapp', false); + $this->assertEquals($expected, $values); + + $query = \OC_DB::prepare('SELECT `appid`, `configvalue` FROM `*PREFIX*appconfig` WHERE `configkey` = ?'); + $query->execute(array('enabled')); + $expected = array(); + while ($row = $query->fetchRow()) { + $expected[$row['appid']] = $row['configvalue']; + } + $values = call_user_func([$callable, 'getValues'], false, 'enabled'); + $this->assertEquals($expected, $values); + } + public function testSetValueUnchanged() { $statementMock = $this->getMock('\Doctrine\DBAL\Statement', array(), array(), '', false); $statementMock->expects($this->once()) @@ -170,42 +276,4 @@ class Test_Appconfig extends \Test\TestCase { $appconfig->setValue('bar', 'foo', 'v2'); $appconfig->setValue('bar', 'foo', 'v2'); } - - public function testDeleteKey() { - \OC_Appconfig::deleteKey('testapp', 'deletethis'); - $query = \OC_DB::prepare('SELECT `configvalue` FROM `*PREFIX*appconfig` WHERE `appid` = ? AND `configkey` = ?'); - $query->execute(array('testapp', 'deletethis')); - $result = (bool)$query->fetchRow(); - $this->assertFalse($result); - } - - public function testDeleteApp() { - \OC_Appconfig::deleteApp('someapp'); - $query = \OC_DB::prepare('SELECT `configkey` FROM `*PREFIX*appconfig` WHERE `appid` = ?'); - $query->execute(array('someapp')); - $result = (bool)$query->fetchRow(); - $this->assertFalse($result); - } - - public function testGetValues() { - $this->assertFalse(\OC_Appconfig::getValues('testapp', 'enabled')); - - $query = \OC_DB::prepare('SELECT `configkey`, `configvalue` FROM `*PREFIX*appconfig` WHERE `appid` = ?'); - $query->execute(array('testapp')); - $expected = array(); - while ($row = $query->fetchRow()) { - $expected[$row['configkey']] = $row['configvalue']; - } - $values = \OC_Appconfig::getValues('testapp', false); - $this->assertEquals($expected, $values); - - $query = \OC_DB::prepare('SELECT `appid`, `configvalue` FROM `*PREFIX*appconfig` WHERE `configkey` = ?'); - $query->execute(array('enabled')); - $expected = array(); - while ($row = $query->fetchRow()) { - $expected[$row['appid']] = $row['configvalue']; - } - $values = \OC_Appconfig::getValues(false, 'enabled'); - $this->assertEquals($expected, $values); - } } From 39497b9c3a02c383b2bd660c9114e65b732848e3 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 11 May 2015 12:29:28 +0200 Subject: [PATCH 2/3] Add a test for parallel insert --- tests/lib/appconfig.php | 41 +++++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/tests/lib/appconfig.php b/tests/lib/appconfig.php index 9238d16bb86..99ec8c2382a 100644 --- a/tests/lib/appconfig.php +++ b/tests/lib/appconfig.php @@ -113,11 +113,8 @@ class Test_Appconfig extends \Test\TestCase { * @param mixed $callable */ public function testGetValue($callable) { - $query = \OC_DB::prepare('SELECT `configvalue` FROM `*PREFIX*appconfig` WHERE `appid` = ? AND `configkey` = ?'); - $result = $query->execute(array('testapp', 'installed_version')); - $expected = $result->fetchRow(); $value = call_user_func([$callable, 'getValue'], 'testapp', 'installed_version'); - $this->assertEquals($expected['configvalue'], $value); + $this->assertConfigKey('testapp', 'installed_version', $value); $value = call_user_func([$callable, 'getValue'], 'testapp', 'nonexistant'); $this->assertNull($value); @@ -146,16 +143,10 @@ class Test_Appconfig extends \Test\TestCase { */ public function testSetValue($callable) { call_user_func([$callable, 'setValue'], 'testapp', 'installed_version', '1.33.7'); - $query = \OC_DB::prepare('SELECT `configvalue` FROM `*PREFIX*appconfig` WHERE `appid` = ? AND `configkey` = ?'); - $result = $query->execute(array('testapp', 'installed_version')); - $value = $result->fetchRow(); - $this->assertEquals('1.33.7', $value['configvalue']); + $this->assertConfigKey('testapp', 'installed_version', '1.33.7'); call_user_func([$callable, 'setValue'], 'someapp', 'somekey', 'somevalue'); - $query = \OC_DB::prepare('SELECT `configvalue` FROM `*PREFIX*appconfig` WHERE `appid` = ? AND `configkey` = ?'); - $result = $query->execute(array('someapp', 'somekey')); - $value = $result->fetchRow(); - $this->assertEquals('somevalue', $value['configvalue']); + $this->assertConfigKey('someapp', 'somekey', 'somevalue'); } /** @@ -276,4 +267,30 @@ class Test_Appconfig extends \Test\TestCase { $appconfig->setValue('bar', 'foo', 'v2'); $appconfig->setValue('bar', 'foo', 'v2'); } + + public function testSettingConfigParallel() { + $appConfig1 = new OC\AppConfig(\OC::$server->getDatabaseConnection()); + $appConfig2 = new OC\AppConfig(\OC::$server->getDatabaseConnection()); + $appConfig1->getValue('testapp', 'foo', 'v1'); + $appConfig2->getValue('testapp', 'foo', 'v1'); + + $appConfig1->setValue('testapp', 'foo', 'v1'); + $this->assertConfigKey('testapp', 'foo', 'v1'); + + $appConfig2->setValue('testapp', 'foo', 'v2'); + $this->assertConfigKey('testapp', 'foo', 'v2'); + } + + /** + * @param string $app + * @param string $key + * @param string $expected + * @throws \OC\DatabaseException + */ + protected function assertConfigKey($app, $key, $expected) { + $query = \OC_DB::prepare('SELECT `configvalue` FROM `*PREFIX*appconfig` WHERE `appid` = ? AND `configkey` = ?'); + $result = $query->execute([$app, $key]); + $actual = $result->fetchRow(); + $this->assertEquals($expected, $actual['configvalue']); + } } From dfed287dc02ba7b0fb8fe16542f9e7235eae9223 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 11 May 2015 12:37:14 +0200 Subject: [PATCH 3/3] Use insertIfNotExists to avoid problems with parallel calls --- lib/private/appconfig.php | 27 ++++++++++++++++----------- tests/lib/appconfig.php | 10 ++++++---- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/lib/private/appconfig.php b/lib/private/appconfig.php index a2095c571f3..37532616e1e 100644 --- a/lib/private/appconfig.php +++ b/lib/private/appconfig.php @@ -42,13 +42,14 @@ namespace OC; -use \OC\DB\Connection; +use OC\DB\Connection; +use OCP\IAppConfig; /** * This class provides an easy way for apps to store config values in the * database. */ -class AppConfig implements \OCP\IAppConfig { +class AppConfig implements IAppConfig { /** * @var \OC\DB\Connection $conn */ @@ -64,7 +65,7 @@ class AppConfig implements \OCP\IAppConfig { private $apps = null; /** - * @param \OC\DB\Connection $conn + * @param Connection $conn */ public function __construct(Connection $conn) { $this->conn = $conn; @@ -172,27 +173,31 @@ class AppConfig implements \OCP\IAppConfig { } /** - * sets a value in the appconfig + * Sets a value. If the key did not exist before it will be created. * * @param string $app app * @param string $key key * @param string $value value - * - * Sets a value. If the key did not exist before it will be created. + * @return void */ public function setValue($app, $key, $value) { + $inserted = false; // Does the key exist? no: insert, yes: update. if (!$this->hasKey($app, $key)) { - $data = array( + $inserted = (bool) $this->conn->insertIfNotExist('*PREFIX*appconfig', [ 'appid' => $app, 'configkey' => $key, 'configvalue' => $value, - ); - $this->conn->insert('*PREFIX*appconfig', $data); - } else { + ], [ + 'appid', + 'configkey', + ]); + } + + if (!$inserted) { $oldValue = $this->getValue($app, $key); if($oldValue === strval($value)) { - return true; + return; } $data = array( 'configvalue' => $value, diff --git a/tests/lib/appconfig.php b/tests/lib/appconfig.php index 99ec8c2382a..adff45706cc 100644 --- a/tests/lib/appconfig.php +++ b/tests/lib/appconfig.php @@ -215,7 +215,7 @@ class Test_Appconfig extends \Test\TestCase { .' WHERE `appid` = ?'), $this->equalTo(array('bar'))) ->will($this->returnValue($statementMock)); $connectionMock->expects($this->once()) - ->method('insert') + ->method('insertIfNotExist') ->with($this->equalTo('*PREFIX*appconfig'), $this->equalTo( array( @@ -223,7 +223,8 @@ class Test_Appconfig extends \Test\TestCase { 'configkey' => 'foo', 'configvalue' => 'v1', ) - )); + ), $this->equalTo(['appid', 'configkey'])) + ->willReturn(1); $connectionMock->expects($this->never()) ->method('update'); @@ -246,7 +247,7 @@ class Test_Appconfig extends \Test\TestCase { .' WHERE `appid` = ?'), $this->equalTo(array('bar'))) ->will($this->returnValue($statementMock)); $connectionMock->expects($this->once()) - ->method('insert') + ->method('insertIfNotExist') ->with($this->equalTo('*PREFIX*appconfig'), $this->equalTo( array( @@ -254,7 +255,8 @@ class Test_Appconfig extends \Test\TestCase { 'configkey' => 'foo', 'configvalue' => 'v1', ) - )); + ), $this->equalTo(['appid', 'configkey'])) + ->willReturn(1); $connectionMock->expects($this->once()) ->method('update') ->with($this->equalTo('*PREFIX*appconfig'),