Merge pull request #12371 from nextcloud/bugfix/12369/catch-unique-constraint-violation-exception-in-insertIfNotExist

Catch UniqueConstraintViolationException inside insertIfNotExist

lib/private/DB/Adapter.php

@@ -27,6 +27,8 @@
 
 namespace OC\DB;
 
+use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
+
 /**
  * This handles the way we use to write queries, into something that can be
  * handled by the database abstraction layer.
@@ -79,7 +81,9 @@ class Adapter {
 	}
 
 	/**
-	 * Insert a row if the matching row does not exists.
+	 * Insert a row if the matching row does not exists. To accomplish proper race condition avoidance
+	 * it is needed that there is also a unique constraint on the values. Then this method will
+	 * catch the exception and return 0.
 	 *
 	 * @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)
@@ -88,6 +92,7 @@ class Adapter {
 	 *				Please note: text fields (clob) must not be used in the compare array
 	 * @return int number of inserted rows
 	 * @throws \Doctrine\DBAL\DBALException
+	 * @deprecated 15.0.0 - use unique index and "try { $db->insert() } catch (UniqueConstraintViolationException $e) {}" instead, because it is more reliable and does not have the risk for deadlocks - see https://github.com/nextcloud/server/pull/12371
 	 */
 	public function insertIfNotExist($table, $input, array $compare = null) {
 		if (empty($compare)) {
@@ -111,6 +116,14 @@ class Adapter {
 		$query = substr($query, 0, -5);
 		$query .= ' HAVING COUNT(*) = 0';
 
-		return $this->conn->executeUpdate($query, $inserts);
+		try {
+			return $this->conn->executeUpdate($query, $inserts);
+		} catch (UniqueConstraintViolationException $e) {
+			// if this is thrown then a concurrent insert happened between the insert and the sub-select in the insert, that should have avoided it
+			// it's fine to ignore this then
+			//
+			// more discussions about this can be found at https://github.com/nextcloud/server/pull/12315
+			return 0;
+		}
 	}
 }

lib/private/DB/AdapterSqlite.php

@@ -27,6 +27,8 @@
 
 namespace OC\DB;
 
+use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
+
 class AdapterSqlite extends Adapter {
 
 	/**
@@ -50,7 +52,9 @@ class AdapterSqlite extends Adapter {
 	}
 
 	/**
-	 * Insert a row if the matching row does not exists.
+	 * Insert a row if the matching row does not exists. To accomplish proper race condition avoidance
+	 * it is needed that there is also a unique constraint on the values. Then this method will
+	 * catch the exception and return 0.
 	 *
 	 * @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)
@@ -59,6 +63,7 @@ class AdapterSqlite extends Adapter {
 	 *				Please note: text fields (clob) must not be used in the compare array
 	 * @return int number of inserted rows
 	 * @throws \Doctrine\DBAL\DBALException
+	 * @deprecated 15.0.0 - use unique index and "try { $db->insert() } catch (UniqueConstraintViolationException $e) {}" instead, because it is more reliable and does not have the risk for deadlocks - see https://github.com/nextcloud/server/pull/12371
 	 */
 	public function insertIfNotExist($table, $input, array $compare = null) {
 		if (empty($compare)) {
@@ -82,6 +87,14 @@ class AdapterSqlite extends Adapter {
 		$query = substr($query, 0, -5);
 		$query .= ')';
 
-		return $this->conn->executeUpdate($query, $inserts);
+		try {
+			return $this->conn->executeUpdate($query, $inserts);
+		} catch (UniqueConstraintViolationException $e) {
+			// if this is thrown then a concurrent insert happened between the insert and the sub-select in the insert, that should have avoided it
+			// it's fine to ignore this then
+			//
+			// more discussions about this can be found at https://github.com/nextcloud/server/pull/12315
+			return 0;
+		}
 	}
 }

lib/private/DB/Connection.php

@@ -240,7 +240,9 @@ class Connection extends ReconnectWrapper implements IDBConnection {
 	}
 
 	/**
-	 * Insert a row if the matching row does not exists.
+	 * Insert a row if the matching row does not exists. To accomplish proper race condition avoidance
+	 * it is needed that there is also a unique constraint on the values. Then this method will
+	 * catch the exception and return 0.
 	 *
 	 * @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)
@@ -249,6 +251,7 @@ class Connection extends ReconnectWrapper implements IDBConnection {
 	 *				Please note: text fields (clob) must not be used in the compare array
 	 * @return int number of inserted rows
 	 * @throws \Doctrine\DBAL\DBALException
+	 * @deprecated 15.0.0 - use unique index and "try { $db->insert() } catch (UniqueConstraintViolationException $e) {}" instead, because it is more reliable and does not have the risk for deadlocks - see https://github.com/nextcloud/server/pull/12371
 	 */
 	public function insertIfNotExist($table, $input, array $compare = null) {
 		return $this->adapter->insertIfNotExist($table, $input, $compare);

lib/public/IDBConnection.php

@@ -104,7 +104,9 @@ interface IDBConnection {
 	public function lastInsertId($table = null);
 
 	/**
-	 * Insert a row if the matching row does not exists.
+	 * Insert a row if the matching row does not exists. To accomplish proper race condition avoidance
+	 * it is needed that there is also a unique constraint on the values. Then this method will
+	 * catch the exception and return 0.
 	 *
 	 * @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)
@@ -114,6 +116,7 @@ interface IDBConnection {
 	 * @return int number of inserted rows
 	 * @throws \Doctrine\DBAL\DBALException
 	 * @since 6.0.0 - parameter $compare was added in 8.1.0, return type changed from boolean in 8.1.0
+	 * @deprecated 15.0.0 - use unique index and "try { $db->insert() } catch (UniqueConstraintViolationException $e) {}" instead, because it is more reliable and does not have the risk for deadlocks - see https://github.com/nextcloud/server/pull/12371
 	 */
 	public function insertIfNotExist($table, $input, array $compare = null);
 

tests/lib/DB/ConnectionTest.php

@@ -314,11 +314,10 @@ class ConnectionTest extends \Test\TestCase {
 
 	/**
 	 * @dataProvider insertIfNotExistsViolatingThrows
-	 * @expectedException \Doctrine\DBAL\Exception\UniqueConstraintViolationException
 	 *
 	 * @param array $compareKeys
 	 */
-	public function testInsertIfNotExistsViolatingThrows($compareKeys) {
+	public function testInsertIfNotExistsViolatingUnique($compareKeys) {
 		$this->makeTestTable();
 		$result = $this->connection->insertIfNotExist('*PREFIX*table',
 			[