From 230e93f5756dca187f77c8df3cf69e3d7e3d05ff Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Fri, 9 Nov 2018 10:35:12 +0100 Subject: [PATCH 1/3] Catch UniqueConstraintViolationException inside insertIfNotExist This is the most common case for the usage of this method. See also https://github.com/nextcloud/server/issues/12369 and the linked tickets. Signed-off-by: Morris Jobke --- lib/private/DB/Adapter.php | 16 ++++++++++++++-- lib/private/DB/AdapterSqlite.php | 16 ++++++++++++++-- lib/private/DB/Connection.php | 4 +++- lib/public/IDBConnection.php | 4 +++- 4 files changed, 34 insertions(+), 6 deletions(-) diff --git a/lib/private/DB/Adapter.php b/lib/private/DB/Adapter.php index e88e3cf09c6..8f062030604 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) @@ -111,6 +115,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..24650829aa3 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) @@ -82,6 +86,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..6a64925711a 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) diff --git a/lib/public/IDBConnection.php b/lib/public/IDBConnection.php index 4e450eae736..204d7bc99ed 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) From 5273639d0e16e946ec25a51cfdd6696fde3d07cd Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Fri, 9 Nov 2018 12:13:30 +0100 Subject: [PATCH 2/3] Add deprecation message ofr insertIfNotExist Signed-off-by: Morris Jobke --- lib/private/DB/Adapter.php | 1 + lib/private/DB/AdapterSqlite.php | 1 + lib/private/DB/Connection.php | 1 + lib/public/IDBConnection.php | 1 + 4 files changed, 4 insertions(+) diff --git a/lib/private/DB/Adapter.php b/lib/private/DB/Adapter.php index 8f062030604..b9a5f272c57 100644 --- a/lib/private/DB/Adapter.php +++ b/lib/private/DB/Adapter.php @@ -92,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)) { diff --git a/lib/private/DB/AdapterSqlite.php b/lib/private/DB/AdapterSqlite.php index 24650829aa3..0a482259b98 100644 --- a/lib/private/DB/AdapterSqlite.php +++ b/lib/private/DB/AdapterSqlite.php @@ -63,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)) { diff --git a/lib/private/DB/Connection.php b/lib/private/DB/Connection.php index 6a64925711a..c63ef0067c1 100644 --- a/lib/private/DB/Connection.php +++ b/lib/private/DB/Connection.php @@ -251,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 204d7bc99ed..b3abe464845 100644 --- a/lib/public/IDBConnection.php +++ b/lib/public/IDBConnection.php @@ -116,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); From 8e600067444f0478a7091109cf78757176251f69 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Mon, 12 Nov 2018 12:22:04 +0100 Subject: [PATCH 3/3] Exception is not thrown anymore Signed-off-by: Morris Jobke --- tests/lib/DB/ConnectionTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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', [