From 8fa692388b29979c56ff6d0229d20e09e8ecba65 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 9 Mar 2015 17:25:02 +0100 Subject: [PATCH 01/11] Allow specifying the compare-array for insertIfNotExists() --- lib/private/allconfig.php | 15 +++++++++++---- lib/private/appframework/db/db.php | 4 ++-- lib/private/db.php | 4 ++-- lib/private/db/adapter.php | 11 +++++++---- lib/private/db/adaptersqlite.php | 11 +++++++---- lib/private/db/connection.php | 4 ++-- lib/public/db.php | 4 ++-- lib/public/idbconnection.php | 2 +- 8 files changed, 34 insertions(+), 21 deletions(-) diff --git a/lib/private/allconfig.php b/lib/private/allconfig.php index 00defd920d..b8bba7986e 100644 --- a/lib/private/allconfig.php +++ b/lib/private/allconfig.php @@ -189,11 +189,18 @@ class AllConfig implements \OCP\IConfig { return; } - $data = array($value, $userId, $appName, $key); + $affectedRows = 0; if (!$exists && $preCondition === null) { - $sql = 'INSERT INTO `*PREFIX*preferences` (`configvalue`, `userid`, `appid`, `configkey`)'. - 'VALUES (?, ?, ?, ?)'; + $this->connection->insertIfNotExist('*PREFIX*preferences', [ + 'configvalue' => $value, + 'userid' => $userId, + 'appid' => $appName, + 'configkey' => $key, + ], ['configvalue', 'userid', 'appid']); + $affectedRows = 1; } elseif ($exists) { + $data = array($value, $userId, $appName, $key); + $sql = 'UPDATE `*PREFIX*preferences` SET `configvalue` = ? '. 'WHERE `userid` = ? AND `appid` = ? AND `configkey` = ? '; @@ -206,8 +213,8 @@ class AllConfig implements \OCP\IConfig { } $data[] = $preCondition; } + $affectedRows = $this->connection->executeUpdate($sql, $data); } - $affectedRows = $this->connection->executeUpdate($sql, $data); // only add to the cache if we already loaded data for the user if ($affectedRows > 0 && isset($this->userCache[$userId])) { diff --git a/lib/private/appframework/db/db.php b/lib/private/appframework/db/db.php index 5387e36d62..18c32c948c 100644 --- a/lib/private/appframework/db/db.php +++ b/lib/private/appframework/db/db.php @@ -138,8 +138,8 @@ class Db implements IDb { * @return bool * */ - public function insertIfNotExist($table, $input) { - return $this->connection->insertIfNotExist($table, $input); + public function insertIfNotExist($table, $input, $compare = null) { + return $this->connection->insertIfNotExist($table, $input, $compare); } /** diff --git a/lib/private/db.php b/lib/private/db.php index dc25092e27..3993ae2774 100644 --- a/lib/private/db.php +++ b/lib/private/db.php @@ -172,8 +172,8 @@ class OC_DB { * @param array $input An array of fieldname/value pairs * @return boolean number of updated rows */ - public static function insertIfNotExist($table, $input) { - return \OC::$server->getDatabaseConnection()->insertIfNotExist($table, $input); + public static function insertIfNotExist($table, $input, $compare = null) { + return \OC::$server->getDatabaseConnection()->insertIfNotExist($table, $input, $compare); } /** diff --git a/lib/private/db/adapter.php b/lib/private/db/adapter.php index 58b3514b92..ee6898dde8 100644 --- a/lib/private/db/adapter.php +++ b/lib/private/db/adapter.php @@ -46,19 +46,22 @@ class Adapter { * @throws \OC\HintException * @return int count of inserted rows */ - public function insertIfNotExist($table, $input) { + public function insertIfNotExist($table, $input, $compare = null) { + if ($compare === null) { + $compare = array_keys($input); + } $query = 'INSERT INTO `' .$table . '` (`' . implode('`,`', array_keys($input)) . '`) SELECT ' . str_repeat('?,', count($input)-1).'? ' // Is there a prettier alternative? . 'FROM `' . $table . '` WHERE '; $inserts = array_values($input); - foreach($input as $key => $value) { + foreach($compare as $key) { $query .= '`' . $key . '`'; - if (is_null($value)) { + if (is_null($input[$key])) { $query .= ' IS NULL AND '; } else { - $inserts[] = $value; + $inserts[] = $input[$key]; $query .= ' = ? AND '; } } diff --git a/lib/private/db/adaptersqlite.php b/lib/private/db/adaptersqlite.php index df4a804feb..8b3c4ebc83 100644 --- a/lib/private/db/adaptersqlite.php +++ b/lib/private/db/adaptersqlite.php @@ -18,19 +18,22 @@ class AdapterSqlite extends Adapter { return $statement; } - public function insertIfNotExist($table, $input) { + public function insertIfNotExist($table, $input, $compare = null) { + if ($compare === null) { + $compare = array_keys($input); + } $fieldList = '`' . implode('`,`', array_keys($input)) . '`'; $query = "INSERT INTO `$table` ($fieldList) SELECT " . str_repeat('?,', count($input)-1).'? ' . " WHERE NOT EXISTS (SELECT 1 FROM `$table` WHERE "; $inserts = array_values($input); - foreach($input as $key => $value) { + foreach($compare as $key) { $query .= '`' . $key . '`'; - if (is_null($value)) { + if (is_null($input[$key])) { $query .= ' IS NULL AND '; } else { - $inserts[] = $value; + $inserts[] = $input[$key]; $query .= ' = ? AND '; } } diff --git a/lib/private/db/connection.php b/lib/private/db/connection.php index 6ba29fc2cc..cc94c862b8 100644 --- a/lib/private/db/connection.php +++ b/lib/private/db/connection.php @@ -164,8 +164,8 @@ class Connection extends \Doctrine\DBAL\Connection implements IDBConnection { * @throws \OC\HintException * @return bool The return value from execute() */ - public function insertIfNotExist($table, $input) { - return $this->adapter->insertIfNotExist($table, $input); + public function insertIfNotExist($table, $input, $compare = null) { + return $this->adapter->insertIfNotExist($table, $input, $compare); } /** diff --git a/lib/public/db.php b/lib/public/db.php index e8fc817106..50e519bbe9 100644 --- a/lib/public/db.php +++ b/lib/public/db.php @@ -64,8 +64,8 @@ class DB { * @return bool * */ - public static function insertIfNotExist($table, $input) { - return(\OC_DB::insertIfNotExist($table, $input)); + public static function insertIfNotExist($table, $input, $compare = null) { + return(\OC_DB::insertIfNotExist($table, $input, $compare)); } /** diff --git a/lib/public/idbconnection.php b/lib/public/idbconnection.php index 0d3274d90e..3cc7ff3248 100644 --- a/lib/public/idbconnection.php +++ b/lib/public/idbconnection.php @@ -94,7 +94,7 @@ interface IDBConnection { * @return bool * */ - public function insertIfNotExist($table, $input); + public function insertIfNotExist($table, $input, $compare = null); /** * Start a transaction From c917ea183ce0acf7c49f8c7104c9ffa54f2b804f Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 9 Mar 2015 18:20:51 +0100 Subject: [PATCH 02/11] Only check unique keys for the comparison on filecache insert & update otherwise --- lib/private/files/cache/cache.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/private/files/cache/cache.php b/lib/private/files/cache/cache.php index 62c32ce659..d39b59b23e 100644 --- a/lib/private/files/cache/cache.php +++ b/lib/private/files/cache/cache.php @@ -251,10 +251,15 @@ class Cache { return trim($item, "`"); }, $queryParts); $values = array_combine($queryParts, $params); - if (\OC::$server->getDatabaseConnection()->insertIfNotExist('*PREFIX*filecache', $values)) { + if (\OC::$server->getDatabaseConnection()->insertIfNotExist('*PREFIX*filecache', $values, [ + 'storage', + 'path_hash', + ])) { return (int)\OC_DB::insertid('*PREFIX*filecache'); } + // The file was created in the mean time + $this->update($id, $data); return $this->getId($file); } } From 940163e16b6ee49132abfcb4177021c571e488cd Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 9 Mar 2015 19:02:34 +0100 Subject: [PATCH 03/11] insertIfNotExists() for storage insertion --- lib/private/files/cache/storage.php | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/private/files/cache/storage.php b/lib/private/files/cache/storage.php index d7d57811a7..eaa4601e4f 100644 --- a/lib/private/files/cache/storage.php +++ b/lib/private/files/cache/storage.php @@ -35,9 +35,15 @@ class Storage { if ($row = $result->fetchRow()) { $this->numericId = $row['numeric_id']; } else { - $sql = 'INSERT INTO `*PREFIX*storages` (`id`) VALUES(?)'; - \OC_DB::executeAudited($sql, array($this->storageId)); - $this->numericId = \OC_DB::insertid('*PREFIX*storages'); + $connection = \OC_DB::getConnection(); + if ($connection->insertIfNotExist('*PREFIX*storages', ['id' => $this->storageId])) { + $this->numericId = \OC_DB::insertid('*PREFIX*storages'); + } else { + $result = \OC_DB::executeAudited($sql, array($this->storageId)); + if ($row = $result->fetchRow()) { + $this->numericId = $row['numeric_id']; + } + } } } From 3115d66d60927dfff7c2cf5523da7dfaa9d11773 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 9 Mar 2015 19:07:22 +0100 Subject: [PATCH 04/11] Better save then sorry --- lib/private/files/cache/storage.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/private/files/cache/storage.php b/lib/private/files/cache/storage.php index eaa4601e4f..e5185f5c13 100644 --- a/lib/private/files/cache/storage.php +++ b/lib/private/files/cache/storage.php @@ -42,6 +42,8 @@ class Storage { $result = \OC_DB::executeAudited($sql, array($this->storageId)); if ($row = $result->fetchRow()) { $this->numericId = $row['numeric_id']; + } else { + throw new \Exception('Storage exists when inserting and does not exist on select... go away'); } } } From 2747a83a49ac803cc127614a6e0b40a244831bdf Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 9 Mar 2015 19:27:02 +0100 Subject: [PATCH 05/11] Get the id before using it --- lib/private/files/cache/cache.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/private/files/cache/cache.php b/lib/private/files/cache/cache.php index d39b59b23e..f377e9777d 100644 --- a/lib/private/files/cache/cache.php +++ b/lib/private/files/cache/cache.php @@ -259,8 +259,9 @@ class Cache { } // The file was created in the mean time + $id = $this->getId($file); $this->update($id, $data); - return $this->getId($file); + return $id; } } From b966a4eb17729230c461b9075143203bd50ed9e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Mon, 9 Mar 2015 20:53:08 +0100 Subject: [PATCH 06/11] Adding unit test which shows insertIfNotExists to fall apart in certain situations --- tests/data/db_structure.xml | 55 +++++++++++++++++++++++++++++++++++++ tests/lib/db.php | 47 +++++++++++++++++++++++-------- 2 files changed, 91 insertions(+), 11 deletions(-) diff --git a/tests/data/db_structure.xml b/tests/data/db_structure.xml index 858c9ab100..371da94483 100644 --- a/tests/data/db_structure.xml +++ b/tests/data/db_structure.xml @@ -238,4 +238,59 @@ + + *dbprefix*uniconst + + + + id + integer + 0 + true + 1 + 4 + + + + + storage + integer + + true + 4 + + + + path_hash + text + + true + 32 + + + + etag + text + + false + 40 + + + + + true + + storage + ascending + + + path_hash + ascending + + + + + +
+ diff --git a/tests/lib/db.php b/tests/lib/db.php index aefbb3624e..73eef3a4d0 100644 --- a/tests/lib/db.php +++ b/tests/lib/db.php @@ -32,13 +32,18 @@ class Test_DB extends \Test\TestCase { */ private $table4; + /** + * @var string + */ + private $table5; + protected function setUp() { parent::setUp(); - $dbfile = OC::$SERVERROOT.'/tests/data/db_structure.xml'; + $dbFile = OC::$SERVERROOT.'/tests/data/db_structure.xml'; - $r = '_'.OC_Util::generateRandomBytes(4).'_'; - $content = file_get_contents( $dbfile ); + $r = $this->getUniqueID('_', 4).'_'; + $content = file_get_contents( $dbFile ); $content = str_replace( '*dbprefix*', '*dbprefix*'.$r, $content ); file_put_contents( self::$schema_file, $content ); OC_DB::createDbFromStructure(self::$schema_file); @@ -48,6 +53,7 @@ class Test_DB extends \Test\TestCase { $this->table2 = $this->test_prefix.'cntcts_cards'; $this->table3 = $this->test_prefix.'vcategory'; $this->table4 = $this->test_prefix.'decimal'; + $this->table5 = $this->test_prefix.'uniconst'; } protected function tearDown() { @@ -110,7 +116,7 @@ class Test_DB extends \Test\TestCase { } public function testinsertIfNotExist() { - $categoryentries = array( + $categoryEntries = array( array('user' => 'test', 'type' => 'contact', 'category' => 'Family', 'expectedResult' => 1), array('user' => 'test', 'type' => 'contact', 'category' => 'Friends', 'expectedResult' => 1), array('user' => 'test', 'type' => 'contact', 'category' => 'Coworkers', 'expectedResult' => 1), @@ -118,7 +124,7 @@ class Test_DB extends \Test\TestCase { array('user' => 'test', 'type' => 'contact', 'category' => 'School', 'expectedResult' => 1), ); - foreach($categoryentries as $entry) { + foreach($categoryEntries as $entry) { $result = OC_DB::insertIfNotExist('*PREFIX*'.$this->table3, array( 'uid' => $entry['user'], @@ -135,13 +141,13 @@ class Test_DB extends \Test\TestCase { } public function testInsertIfNotExistNull() { - $categoryentries = array( + $categoryEntries = array( array('addressbookid' => 123, 'fullname' => null, 'expectedResult' => 1), array('addressbookid' => 123, 'fullname' => null, 'expectedResult' => 0), array('addressbookid' => 123, 'fullname' => 'test', 'expectedResult' => 1), ); - foreach($categoryentries as $entry) { + foreach($categoryEntries as $entry) { $result = OC_DB::insertIfNotExist('*PREFIX*'.$this->table2, array( 'addressbookid' => $entry['addressbookid'], @@ -156,14 +162,14 @@ class Test_DB extends \Test\TestCase { $this->assertEquals(2, count($result->fetchAll())); } - public function testinsertIfNotExistDontOverwrite() { - $fullname = 'fullname test'; + public function testInsertIfNotExistDonTOverwrite() { + $fullName = 'fullname test'; $uri = 'uri_1'; $carddata = 'This is a vCard'; // Normal test to have same known data inserted. $query = OC_DB::prepare('INSERT INTO `*PREFIX*'.$this->table2.'` (`fullname`, `uri`, `carddata`) VALUES (?, ?, ?)'); - $result = $query->execute(array($fullname, $uri, $carddata)); + $result = $query->execute(array($fullName, $uri, $carddata)); $this->assertEquals(1, $result); $query = OC_DB::prepare('SELECT `fullname`, `uri`, `carddata` FROM `*PREFIX*'.$this->table2.'` WHERE `uri` = ?'); $result = $query->execute(array($uri)); @@ -176,7 +182,7 @@ class Test_DB extends \Test\TestCase { // Try to insert a new row $result = OC_DB::insertIfNotExist('*PREFIX*'.$this->table2, array( - 'fullname' => $fullname, + 'fullname' => $fullName, 'uri' => $uri, )); $this->assertEquals(0, $result); @@ -192,6 +198,25 @@ class Test_DB extends \Test\TestCase { $this->assertEquals($carddata, $rowset[0]['carddata']); } + public function testInsertIfNotExistsViolating() { + $result = OC_DB::insertIfNotExist('*PREFIX*'.$this->table5, + array( + 'storage' => 1, + 'path_hash' => md5('welcome.txt'), + 'etag' => $this->getUniqueID() + )); + $this->assertEquals(1, $result); + + $result = OC_DB::insertIfNotExist('*PREFIX*'.$this->table5, + array( + 'storage' => 1, + 'path_hash' => md5('welcome.txt'), + 'etag' => $this->getUniqueID() + ),['storage', 'path_hash']); + + $this->assertEquals(0, $result); + } + public function testUtf8Data() { $table = "*PREFIX*{$this->table2}"; $expected = "Ћö雙喜\xE2\x80\xA2"; From 89be55a672afe0e09a33d4997ec10c0e833d4885 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Mon, 9 Mar 2015 22:12:31 +0100 Subject: [PATCH 07/11] let insertIfNotExist() throw the native DBALException - no need to hide the real exception --- lib/private/db.php | 17 ++--------------- lib/private/db/adapter.php | 17 ++--------------- lib/private/db/adaptersqlite.php | 22 ++++++++-------------- lib/private/db/connection.php | 10 +++++----- lib/private/db/mdb2schemamanager.php | 2 +- lib/public/db.php | 2 +- lib/public/idbconnection.php | 2 +- tests/lib/db.php | 10 +++++----- 8 files changed, 25 insertions(+), 57 deletions(-) diff --git a/lib/private/db.php b/lib/private/db.php index 3993ae2774..c265492669 100644 --- a/lib/private/db.php +++ b/lib/private/db.php @@ -20,8 +20,6 @@ * */ -define('MDB2_SCHEMA_DUMP_STRUCTURE', '1'); - /** * This class manages the access to the database. It basically is a wrapper for * Doctrine with some adaptions. @@ -40,8 +38,7 @@ class OC_DB { * * @return \OC\DB\MDB2SchemaManager */ - private static function getMDB2SchemaManager() - { + private static function getMDB2SchemaManager() { return new \OC\DB\MDB2SchemaManager(\OC::$server->getDatabaseConnection()); } @@ -166,16 +163,6 @@ class OC_DB { return \OC::$server->getDatabaseConnection()->lastInsertId($table); } - /** - * Insert a row if a matching row doesn't exists. - * @param string $table The table to insert into in the form '*PREFIX*tableName' - * @param array $input An array of fieldname/value pairs - * @return boolean number of updated rows - */ - public static function insertIfNotExist($table, $input, $compare = null) { - return \OC::$server->getDatabaseConnection()->insertIfNotExist($table, $input, $compare); - } - /** * Start a transaction */ @@ -205,7 +192,7 @@ class OC_DB { * * TODO: write more documentation */ - public static function getDbStructure( $file, $mode = 0) { + public static function getDbStructure($file) { $schemaManager = self::getMDB2SchemaManager(); return $schemaManager->getDbStructure($file); } diff --git a/lib/private/db/adapter.php b/lib/private/db/adapter.php index ee6898dde8..bd1604caf2 100644 --- a/lib/private/db/adapter.php +++ b/lib/private/db/adapter.php @@ -43,7 +43,7 @@ class Adapter { * insert the @input values when they do not exist yet * @param string $table name * @param array $input key->value pair, key has to be sanitized properly - * @throws \OC\HintException + * @throws \Doctrine\DBAL\DBALException * @return int count of inserted rows */ public function insertIfNotExist($table, $input, $compare = null) { @@ -68,19 +68,6 @@ class Adapter { $query = substr($query, 0, strlen($query) - 5); $query .= ' HAVING COUNT(*) = 0'; - try { - return $this->conn->executeUpdate($query, $inserts); - } catch(\Doctrine\DBAL\DBALException $e) { - $entry = 'DB Error: "'.$e->getMessage() . '"
'; - $entry .= 'Offending command was: ' . $query.'
'; - \OC_Log::write('core', $entry, \OC_Log::FATAL); - $l = \OC::$server->getL10N('lib'); - throw new \OC\HintException( - $l->t('Database Error'), - $l->t('Please contact your system administrator.'), - 0, - $e - ); - } + return $this->conn->executeUpdate($query, $inserts); } } diff --git a/lib/private/db/adaptersqlite.php b/lib/private/db/adaptersqlite.php index 8b3c4ebc83..f93183bee9 100644 --- a/lib/private/db/adaptersqlite.php +++ b/lib/private/db/adaptersqlite.php @@ -18,6 +18,13 @@ class AdapterSqlite extends Adapter { return $statement; } + /** + * @param string $table + * @param array $input + * @param null $compare + * @return int + * @throws \Doctrine\DBAL\DBALException + */ public function insertIfNotExist($table, $input, $compare = null) { if ($compare === null) { $compare = array_keys($input); @@ -40,19 +47,6 @@ class AdapterSqlite extends Adapter { $query = substr($query, 0, strlen($query) - 5); $query .= ')'; - try { - return $this->conn->executeUpdate($query, $inserts); - } catch(\Doctrine\DBAL\DBALException $e) { - $entry = 'DB Error: "'.$e->getMessage() . '"
'; - $entry .= 'Offending command was: ' . $query.'
'; - \OC_Log::write('core', $entry, \OC_Log::FATAL); - $l = \OC::$server->getL10N('lib'); - throw new \OC\HintException( - $l->t('Database Error'), - $l->t('Please contact your system administrator.'), - 0, - $e - ); - } + return $this->conn->executeUpdate($query, $inserts); } } diff --git a/lib/private/db/connection.php b/lib/private/db/connection.php index cc94c862b8..cdbfc94a03 100644 --- a/lib/private/db/connection.php +++ b/lib/private/db/connection.php @@ -152,16 +152,16 @@ class Connection extends \Doctrine\DBAL\Connection implements IDBConnection { } // internal use - public function realLastInsertId($seqName = null) - { + public function realLastInsertId($seqName = null) { return parent::lastInsertId($seqName); } /** - * Insert a row if a matching row doesn't exists. + * Insert a row if a matching row does not exists. * @param string $table. The table to insert into in the form '*PREFIX*tableName' - * @param array $input. An array of fieldname/value pairs - * @throws \OC\HintException + * @param array $input. An array of field name/value pairs + * @param array $compare + * @throws \Doctrine\DBAL\DBALException * @return bool The return value from execute() */ public function insertIfNotExist($table, $input, $compare = null) { diff --git a/lib/private/db/mdb2schemamanager.php b/lib/private/db/mdb2schemamanager.php index 358360d0b4..66c97f9e3b 100644 --- a/lib/private/db/mdb2schemamanager.php +++ b/lib/private/db/mdb2schemamanager.php @@ -35,7 +35,7 @@ class MDB2SchemaManager { * * TODO: write more documentation */ - public function getDbStructure($file, $mode = MDB2_SCHEMA_DUMP_STRUCTURE) { + public function getDbStructure($file) { return \OC_DB_MDB2SchemaWriter::saveSchemaToFile($file, $this->conn); } diff --git a/lib/public/db.php b/lib/public/db.php index 50e519bbe9..d8d81f239b 100644 --- a/lib/public/db.php +++ b/lib/public/db.php @@ -65,7 +65,7 @@ class DB { * */ public static function insertIfNotExist($table, $input, $compare = null) { - return(\OC_DB::insertIfNotExist($table, $input, $compare)); + return \OC::$server->getDatabaseConnection()->insertIfNotExist($table, $input, $compare); } /** diff --git a/lib/public/idbconnection.php b/lib/public/idbconnection.php index 3cc7ff3248..c8a2c5796b 100644 --- a/lib/public/idbconnection.php +++ b/lib/public/idbconnection.php @@ -80,7 +80,7 @@ interface IDBConnection { * Insert a row if a matching row doesn't exists. * @param string $table The table name (will replace *PREFIX*) to perform the replace on. * @param array $input - * @throws \OC\HintException + * @throws \Doctrine\DBAL\DBALException * * The input array if in the form: * diff --git a/tests/lib/db.php b/tests/lib/db.php index 73eef3a4d0..056ce53543 100644 --- a/tests/lib/db.php +++ b/tests/lib/db.php @@ -125,7 +125,7 @@ class Test_DB extends \Test\TestCase { ); foreach($categoryEntries as $entry) { - $result = OC_DB::insertIfNotExist('*PREFIX*'.$this->table3, + $result = \OCP\DB::insertIfNotExist('*PREFIX*'.$this->table3, array( 'uid' => $entry['user'], 'type' => $entry['type'], @@ -148,7 +148,7 @@ class Test_DB extends \Test\TestCase { ); foreach($categoryEntries as $entry) { - $result = OC_DB::insertIfNotExist('*PREFIX*'.$this->table2, + $result = \OCP\DB::insertIfNotExist('*PREFIX*'.$this->table2, array( 'addressbookid' => $entry['addressbookid'], 'fullname' => $entry['fullname'], @@ -180,7 +180,7 @@ class Test_DB extends \Test\TestCase { $this->assertEquals($carddata, $rowset[0]['carddata']); // Try to insert a new row - $result = OC_DB::insertIfNotExist('*PREFIX*'.$this->table2, + $result = \OCP\DB::insertIfNotExist('*PREFIX*'.$this->table2, array( 'fullname' => $fullName, 'uri' => $uri, @@ -199,7 +199,7 @@ class Test_DB extends \Test\TestCase { } public function testInsertIfNotExistsViolating() { - $result = OC_DB::insertIfNotExist('*PREFIX*'.$this->table5, + $result = \OCP\DB::insertIfNotExist('*PREFIX*'.$this->table5, array( 'storage' => 1, 'path_hash' => md5('welcome.txt'), @@ -207,7 +207,7 @@ class Test_DB extends \Test\TestCase { )); $this->assertEquals(1, $result); - $result = OC_DB::insertIfNotExist('*PREFIX*'.$this->table5, + $result = \OCP\DB::insertIfNotExist('*PREFIX*'.$this->table5, array( 'storage' => 1, 'path_hash' => md5('welcome.txt'), From 87431605b86674ec23c464c5a27b84c7771a2306 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 10 Mar 2015 09:26:45 +0100 Subject: [PATCH 08/11] Add test for UniqueConstraintViolationException on wrong key --- tests/lib/db.php | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/lib/db.php b/tests/lib/db.php index 056ce53543..1fb384ca9c 100644 --- a/tests/lib/db.php +++ b/tests/lib/db.php @@ -217,6 +217,38 @@ class Test_DB extends \Test\TestCase { $this->assertEquals(0, $result); } + public function insertIfNotExistsViolatingThrows() { + return [ + [null], + [['etag']], + ]; + } + + /** + * @dataProvider insertIfNotExistsViolatingThrows + * @expectedException \Doctrine\DBAL\Exception\UniqueConstraintViolationException + * + * @param array $compareKeys + */ + public function testInsertIfNotExistsViolatingThrows($compareKeys) { + $result = \OCP\DB::insertIfNotExist('*PREFIX*'.$this->table5, + array( + 'storage' => 1, + 'path_hash' => md5('welcome.txt'), + 'etag' => $this->getUniqueID() + )); + $this->assertEquals(1, $result); + + $result = \OCP\DB::insertIfNotExist('*PREFIX*'.$this->table5, + array( + 'storage' => 1, + 'path_hash' => md5('welcome.txt'), + 'etag' => $this->getUniqueID() + ), $compareKeys); + + $this->assertEquals(0, $result); + } + public function testUtf8Data() { $table = "*PREFIX*{$this->table2}"; $expected = "Ћö雙喜\xE2\x80\xA2"; From d1511cdbee24b27eb966eee334f3d686cbaec3d7 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 11 Mar 2015 09:00:47 +0100 Subject: [PATCH 09/11] Fix doc blocks of insertIfNotExists() method --- lib/private/appframework/db/db.php | 24 ++++++++---------------- lib/private/db/adapter.php | 13 ++++++++----- lib/private/db/adaptersqlite.php | 13 ++++++++----- lib/private/db/connection.php | 14 ++++++++------ lib/public/db.php | 22 ++++++++-------------- lib/public/idbconnection.php | 24 ++++++++---------------- 6 files changed, 48 insertions(+), 62 deletions(-) diff --git a/lib/private/appframework/db/db.php b/lib/private/appframework/db/db.php index 18c32c948c..0824e108f4 100644 --- a/lib/private/appframework/db/db.php +++ b/lib/private/appframework/db/db.php @@ -121,24 +121,16 @@ class Db implements IDb { } /** - * Insert a row if a matching row doesn't exists. - * @param string $table The table name (will replace *PREFIX*) to perform the replace on. - * @param array $input - * @throws \OC\HintException - * - * The input array if in the form: - * - * array ( 'id' => array ( 'value' => 6, - * 'key' => true - * ), - * 'name' => array ('value' => 'Stoyan'), - * 'family' => array ('value' => 'Stefanov'), - * 'birth_date' => array ('value' => '1975-06-20') - * ); - * @return bool + * Insert a row if the matching row does not exists. * + * @param string $table The table name (will replace *PREFIX* with the actual prefix) + * @param array $input data that should be inserted into the table (column name => value) + * @param array|null $compare List of values that should be checked for "if not exists" + * If this is null or an empty array, all keys of $input will be compared + * @return int number of inserted rows + * @throws \Doctrine\DBAL\DBALException */ - public function insertIfNotExist($table, $input, $compare = null) { + public function insertIfNotExist($table, $input, array $compare = null) { return $this->connection->insertIfNotExist($table, $input, $compare); } diff --git a/lib/private/db/adapter.php b/lib/private/db/adapter.php index bd1604caf2..d0641f113f 100644 --- a/lib/private/db/adapter.php +++ b/lib/private/db/adapter.php @@ -40,13 +40,16 @@ class Adapter { } /** - * insert the @input values when they do not exist yet - * @param string $table name - * @param array $input key->value pair, key has to be sanitized properly + * Insert a row if the matching row does not exists. + * + * @param string $table The table name (will replace *PREFIX* with the actual prefix) + * @param array $input data that should be inserted into the table (column name => value) + * @param array|null $compare List of values that should be checked for "if not exists" + * If this is null or an empty array, all keys of $input will be compared + * @return int number of inserted rows * @throws \Doctrine\DBAL\DBALException - * @return int count of inserted rows */ - public function insertIfNotExist($table, $input, $compare = null) { + public function insertIfNotExist($table, $input, array $compare = null) { if ($compare === null) { $compare = array_keys($input); } diff --git a/lib/private/db/adaptersqlite.php b/lib/private/db/adaptersqlite.php index f93183bee9..1528285e11 100644 --- a/lib/private/db/adaptersqlite.php +++ b/lib/private/db/adaptersqlite.php @@ -19,13 +19,16 @@ class AdapterSqlite extends Adapter { } /** - * @param string $table - * @param array $input - * @param null $compare - * @return int + * Insert a row if the matching row does not exists. + * + * @param string $table The table name (will replace *PREFIX* with the actual prefix) + * @param array $input data that should be inserted into the table (column name => value) + * @param array|null $compare List of values that should be checked for "if not exists" + * If this is null or an empty array, all keys of $input will be compared + * @return int number of inserted rows * @throws \Doctrine\DBAL\DBALException */ - public function insertIfNotExist($table, $input, $compare = null) { + public function insertIfNotExist($table, $input, array $compare = null) { if ($compare === null) { $compare = array_keys($input); } diff --git a/lib/private/db/connection.php b/lib/private/db/connection.php index cdbfc94a03..023e265f24 100644 --- a/lib/private/db/connection.php +++ b/lib/private/db/connection.php @@ -157,14 +157,16 @@ class Connection extends \Doctrine\DBAL\Connection implements IDBConnection { } /** - * Insert a row if a matching row does not exists. - * @param string $table. The table to insert into in the form '*PREFIX*tableName' - * @param array $input. An array of field name/value pairs - * @param array $compare + * Insert a row if the matching row does not exists. + * + * @param string $table The table name (will replace *PREFIX* with the actual prefix) + * @param array $input data that should be inserted into the table (column name => value) + * @param array|null $compare List of values that should be checked for "if not exists" + * If this is null or an empty array, all keys of $input will be compared + * @return int number of inserted rows * @throws \Doctrine\DBAL\DBALException - * @return bool The return value from execute() */ - public function insertIfNotExist($table, $input, $compare = null) { + public function insertIfNotExist($table, $input, array $compare = null) { return $this->adapter->insertIfNotExist($table, $input, $compare); } diff --git a/lib/public/db.php b/lib/public/db.php index d8d81f239b..595ecd66bd 100644 --- a/lib/public/db.php +++ b/lib/public/db.php @@ -48,23 +48,17 @@ class DB { } /** - * Insert a row if a matching row doesn't exists. - * @param string $table The optional table name (will replace *PREFIX*) and add sequence suffix - * @param array $input + * Insert a row if the matching row does not exists. * - * The input array if in the form: - * - * array ( 'id' => array ( 'value' => 6, - * 'key' => true - * ), - * 'name' => array ('value' => 'Stoyan'), - * 'family' => array ('value' => 'Stefanov'), - * 'birth_date' => array ('value' => '1975-06-20') - * ); - * @return bool + * @param string $table The table name (will replace *PREFIX* with the actual prefix) + * @param array $input data that should be inserted into the table (column name => value) + * @param array|null $compare List of values that should be checked for "if not exists" + * If this is null or an empty array, all keys of $input will be compared + * @return int number of inserted rows + * @throws \Doctrine\DBAL\DBALException * */ - public static function insertIfNotExist($table, $input, $compare = null) { + public static function insertIfNotExist($table, $input, array $compare = null) { return \OC::$server->getDatabaseConnection()->insertIfNotExist($table, $input, $compare); } diff --git a/lib/public/idbconnection.php b/lib/public/idbconnection.php index c8a2c5796b..1117c2da9b 100644 --- a/lib/public/idbconnection.php +++ b/lib/public/idbconnection.php @@ -77,24 +77,16 @@ interface IDBConnection { public function lastInsertId($table = null); /** - * Insert a row if a matching row doesn't exists. - * @param string $table The table name (will replace *PREFIX*) to perform the replace on. - * @param array $input + * Insert a row if the matching row does not exists. + * + * @param string $table The table name (will replace *PREFIX* with the actual prefix) + * @param array $input data that should be inserted into the table (column name => value) + * @param array|null $compare List of values that should be checked for "if not exists" + * If this is null or an empty array, all keys of $input will be compared + * @return int number of inserted rows * @throws \Doctrine\DBAL\DBALException - * - * The input array if in the form: - * - * array ( 'id' => array ( 'value' => 6, - * 'key' => true - * ), - * 'name' => array ('value' => 'Stoyan'), - * 'family' => array ('value' => 'Stefanov'), - * 'birth_date' => array ('value' => '1975-06-20') - * ); - * @return bool - * */ - public function insertIfNotExist($table, $input, $compare = null); + public function insertIfNotExist($table, $input, array $compare = null); /** * Start a transaction From 2af8fea2beb44b4f7f0e4b10adf284da37936a78 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 11 Mar 2015 09:33:50 +0100 Subject: [PATCH 10/11] Throw a RuntimeException in the cache aswell --- lib/private/files/cache/cache.php | 10 +++++++--- lib/private/files/cache/storage.php | 3 ++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/private/files/cache/cache.php b/lib/private/files/cache/cache.php index f377e9777d..64661ca115 100644 --- a/lib/private/files/cache/cache.php +++ b/lib/private/files/cache/cache.php @@ -214,6 +214,7 @@ class Cache { * @param array $data * * @return int file id + * @throws \RuntimeException */ public function put($file, array $data) { if (($id = $this->getId($file)) > -1) { @@ -259,9 +260,12 @@ class Cache { } // The file was created in the mean time - $id = $this->getId($file); - $this->update($id, $data); - return $id; + if (($id = $this->getId($file)) > -1) { + $this->update($id, $data); + return $id; + } else { + throw new \RuntimeException('File entry exists when inserting and does not exist on select... go away'); + } } } diff --git a/lib/private/files/cache/storage.php b/lib/private/files/cache/storage.php index e5185f5c13..9f2739bbed 100644 --- a/lib/private/files/cache/storage.php +++ b/lib/private/files/cache/storage.php @@ -21,6 +21,7 @@ class Storage { /** * @param \OC\Files\Storage\Storage|string $storage + * @throws \RuntimeException */ public function __construct($storage) { if ($storage instanceof \OC\Files\Storage\Storage) { @@ -43,7 +44,7 @@ class Storage { if ($row = $result->fetchRow()) { $this->numericId = $row['numeric_id']; } else { - throw new \Exception('Storage exists when inserting and does not exist on select... go away'); + throw new \RuntimeException('Storage exists when inserting and does not exist on select... go away'); } } } From fefcbb966b6c41b9440f67b8121b9c97e1ce7df0 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 12 Mar 2015 09:18:43 +0100 Subject: [PATCH 11/11] Also use all keys for an empty array, just in case --- lib/private/db/adapter.php | 2 +- lib/private/db/adaptersqlite.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/private/db/adapter.php b/lib/private/db/adapter.php index d0641f113f..de7b04c74d 100644 --- a/lib/private/db/adapter.php +++ b/lib/private/db/adapter.php @@ -50,7 +50,7 @@ class Adapter { * @throws \Doctrine\DBAL\DBALException */ public function insertIfNotExist($table, $input, array $compare = null) { - if ($compare === null) { + if (empty($compare)) { $compare = array_keys($input); } $query = 'INSERT INTO `' .$table . '` (`' diff --git a/lib/private/db/adaptersqlite.php b/lib/private/db/adaptersqlite.php index 1528285e11..31f88940f0 100644 --- a/lib/private/db/adaptersqlite.php +++ b/lib/private/db/adaptersqlite.php @@ -29,7 +29,7 @@ class AdapterSqlite extends Adapter { * @throws \Doctrine\DBAL\DBALException */ public function insertIfNotExist($table, $input, array $compare = null) { - if ($compare === null) { + if (empty($compare)) { $compare = array_keys($input); } $fieldList = '`' . implode('`,`', array_keys($input)) . '`';