diff options
| -rw-r--r-- | lib/private/DB/Adapter.php | 17 | ||||
| -rw-r--r-- | lib/private/DB/AdapterSqlite.php | 17 | ||||
| -rw-r--r-- | lib/private/DB/Connection.php | 5 | ||||
| -rw-r--r-- | lib/public/IDBConnection.php | 5 | ||||
| -rw-r--r-- | tests/lib/DB/ConnectionTest.php | 3 |
5 files changed, 39 insertions, 8 deletions
diff --git a/lib/private/DB/Adapter.php b/lib/private/DB/Adapter.php index e88e3cf09c6..b9a5f272c57 100644 --- a/lib/private/DB/Adapter.php +++ b/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; + } } } diff --git a/lib/private/DB/AdapterSqlite.php b/lib/private/DB/AdapterSqlite.php index dbeec9ded43..0a482259b98 100644 --- a/lib/private/DB/AdapterSqlite.php +++ b/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; + } } } diff --git a/lib/private/DB/Connection.php b/lib/private/DB/Connection.php index 8bb959ffbc1..c63ef0067c1 100644 --- a/lib/private/DB/Connection.php +++ b/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); diff --git a/lib/public/IDBConnection.php b/lib/public/IDBConnection.php index 4e450eae736..b3abe464845 100644 --- a/lib/public/IDBConnection.php +++ b/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); diff --git a/tests/lib/DB/ConnectionTest.php b/tests/lib/DB/ConnectionTest.php index 857bb3cd79f..02dd6a1495c 100644 --- a/tests/lib/DB/ConnectionTest.php +++ b/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', [ |