Merge pull request #14766 from owncloud/fix-insertifnotexists-poc

Allow specifying the compare-array for insertIfNotExists()

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])) {

lib/private/appframework/db/db.php

@@ -121,25 +121,17 @@ 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) {
-		return $this->connection->insertIfNotExist($table, $input);
+	public function insertIfNotExist($table, $input, array $compare = null) {
+		return $this->connection->insertIfNotExist($table, $input, $compare);
 	}
 
 	/**

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) {
-		return \OC::$server->getDatabaseConnection()->insertIfNotExist($table, $input);
-	}
-
 	/**
 	 * 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);
 	}

lib/private/db/adapter.php

@@ -40,44 +40,37 @@ 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
-	 * @return int count of inserted rows
+	 * 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) {
+	public function insertIfNotExist($table, $input, array $compare = null) {
+		if (empty($compare)) {
+			$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 ';
 			}
 		}
 		$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() . '"<br />';
-			$entry .= 'Offending command was: ' . $query.'<br />';
-			\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);
 	}
 }

lib/private/db/adaptersqlite.php

@@ -18,38 +18,38 @@ class AdapterSqlite extends Adapter {
 		return $statement;
 	}
 
-	public function insertIfNotExist($table, $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
+	 */
+	public function insertIfNotExist($table, $input, array $compare = null) {
+		if (empty($compare)) {
+			$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 ';
 			}
 		}
 		$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() . '"<br />';
-			$entry .= 'Offending command was: ' . $query.'<br />';
-			\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);
 	}
 }

lib/private/db/connection.php

@@ -152,20 +152,22 @@ 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.
-	 * @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
-	 * @return bool The return value from execute()
+	 * 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) {
-		return $this->adapter->insertIfNotExist($table, $input);
+	public function insertIfNotExist($table, $input, array $compare = null) {
+		return $this->adapter->insertIfNotExist($table, $input, $compare);
 	}
 
 	/**

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);
 	}
 

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) {
@@ -251,11 +252,20 @@ 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');
 			}
 
-			return $this->getId($file);
+			// The file was created in the mean time
+			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');
+			}
 		}
 	}
 

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) {
@@ -35,9 +36,17 @@ 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'];
+				} else {
+					throw new \RuntimeException('Storage exists when inserting and does not exist on select... go away');
+				}
+			}
 		}
 	}
 

lib/public/db.php

@@ -48,24 +48,18 @@ 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
-	 *
-	 * The input array if in the form:
+	 * Insert a row if the matching row does not exists.
 	 *
-	 * 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) {
-		return(\OC_DB::insertIfNotExist($table, $input));
+	public static function insertIfNotExist($table, $input, array $compare = null) {
+		return \OC::$server->getDatabaseConnection()->insertIfNotExist($table, $input, $compare);
 	}
 
 	/**

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
-	 * @throws \OC\HintException
+	 * 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
-	 *
-	 */
-	public function insertIfNotExist($table, $input);
+	 * @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, array $compare = null);
 
 	/**
 	 * Start a transaction

tests/data/db_structure.xml

@@ -238,4 +238,59 @@
 	</declaration>
 </table>
 
+	<table>
+		<name>*dbprefix*uniconst</name>
+		<declaration>
+
+			<field>
+				<name>id</name>
+				<type>integer</type>
+				<default>0</default>
+				<notnull>true</notnull>
+				<autoincrement>1</autoincrement>
+				<length>4</length>
+			</field>
+
+			<!-- Foreign Key storages::numeric_id -->
+			<field>
+				<name>storage</name>
+				<type>integer</type>
+				<default></default>
+				<notnull>true</notnull>
+				<length>4</length>
+			</field>
+
+			<field>
+				<name>path_hash</name>
+				<type>text</type>
+				<default></default>
+				<notnull>true</notnull>
+				<length>32</length>
+			</field>
+
+			<field>
+				<name>etag</name>
+				<type>text</type>
+				<default></default>
+				<notnull>false</notnull>
+				<length>40</length>
+			</field>
+
+			<index>
+				<!--<name>fs_storage_path_hash</name>-->
+				<unique>true</unique>
+				<field>
+					<name>storage</name>
+					<sorting>ascending</sorting>
+				</field>
+				<field>
+					<name>path_hash</name>
+					<sorting>ascending</sorting>
+				</field>
+			</index>
+
+		</declaration>
+
+	</table>
+
 </database>

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,8 +124,8 @@ class Test_DB extends \Test\TestCase {
 				array('user' => 'test', 'type' => 'contact', 'category' => 'School',    'expectedResult' => 1),
 			);
 
-		foreach($categoryentries as $entry) {
-			$result = OC_DB::insertIfNotExist('*PREFIX*'.$this->table3,
+		foreach($categoryEntries as $entry) {
+			$result = \OCP\DB::insertIfNotExist('*PREFIX*'.$this->table3,
 				array(
 					'uid' => $entry['user'],
 					'type' => $entry['type'],
@@ -135,14 +141,14 @@ 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) {
-			$result = OC_DB::insertIfNotExist('*PREFIX*'.$this->table2,
+		foreach($categoryEntries as $entry) {
+			$result = \OCP\DB::insertIfNotExist('*PREFIX*'.$this->table2,
 				array(
 					'addressbookid' => $entry['addressbookid'],
 					'fullname' => $entry['fullname'],
@@ -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));
@@ -174,9 +180,9 @@ 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,
+				'fullname' => $fullName,
 				'uri' => $uri,
 			));
 		$this->assertEquals(0, $result);
@@ -192,6 +198,57 @@ class Test_DB extends \Test\TestCase {
 		$this->assertEquals($carddata, $rowset[0]['carddata']);
 	}
 
+	public function testInsertIfNotExistsViolating() {
+		$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()
+			),['storage', 'path_hash']);
+
+		$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";