Skip to content

Catch UniqueConstraintViolationException inside insertIfNotExist #12371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MorrisJobke merged 3 commits into from
Nov 12, 2018
Merged

Catch UniqueConstraintViolationException inside insertIfNotExist #12371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
230e93f
Catch UniqueConstraintViolationException inside insertIfNotExist
MorrisJobke Nov 9, 2018
5273639
Add deprecation message ofr insertIfNotExist
MorrisJobke Nov 9, 2018
8e60006
Exception is not thrown anymore
MorrisJobke Nov 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions lib/private/DB/Adapter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -79,7 +81,9 @@ public function unlockTable() {
}

/**
* 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)
Expand All @@ -88,6 +92,7 @@ public function unlockTable() {
* 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)) {
Expand All @@ -111,6 +116,14 @@ public function insertIfNotExist($table, $input, array $compare = null) {
$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;
}
}
}
17 changes: 15 additions & 2 deletions lib/private/DB/AdapterSqlite.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@

namespace OC\DB;

use Doctrine\DBAL\Exception\UniqueConstraintViolationException;

class AdapterSqlite extends Adapter {

/**
Expand All @@ -50,7 +52,9 @@ public function fixupStatement($statement) {
}

/**
* 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)
Expand All @@ -59,6 +63,7 @@ public function fixupStatement($statement) {
* 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)) {
Expand All @@ -82,6 +87,14 @@ public function insertIfNotExist($table, $input, array $compare = null) {
$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;
}
}
}
5 changes: 4 additions & 1 deletion lib/private/DB/Connection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,9 @@ public function realLastInsertId($seqName = 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)
Expand All @@ -249,6 +251,7 @@ public function realLastInsertId($seqName = null) {
* 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);
Expand Down
5 changes: 4 additions & 1 deletion lib/public/IDBConnection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ public function executeUpdate($query, array $params = array(), array $types = ar
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)
Expand All @@ -114,6 +116,7 @@ public function lastInsertId($table = null);
* @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);

Expand Down
3 changes: 1 addition & 2 deletions tests/lib/DB/ConnectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,11 +314,10 @@ public function insertIfNotExistsViolatingThrows() {

/**
* @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',
[
Expand Down