diff --git a/apps/dav/tests/unit/Command/RemoveInvalidSharesTest.php b/apps/dav/tests/unit/Command/RemoveInvalidSharesTest.php index 50ca649b431ff..4122934763fc0 100644 --- a/apps/dav/tests/unit/Command/RemoveInvalidSharesTest.php +++ b/apps/dav/tests/unit/Command/RemoveInvalidSharesTest.php @@ -11,6 +11,7 @@ use OCA\DAV\Command\RemoveInvalidShares; use OCA\DAV\Connector\Sabre\Principal; use OCA\DAV\DAV\RemoteUserPrincipalBackend; +use OCP\DB\Exception; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; use OCP\Server; @@ -39,18 +40,25 @@ protected function setUp(): void { $this->principalBackend = $this->createMock(Principal::class); $this->remoteUserPrincipalBackend = $this->createMock(RemoteUserPrincipalBackend::class); - $this->db->insertIfNotExist('*PREFIX*dav_shares', [ - 'principaluri' => 'principal:unknown', - 'type' => 'calendar', - 'access' => 2, - 'resourceid' => 666, - ]); - $this->db->insertIfNotExist('*PREFIX*dav_shares', [ - 'principaluri' => 'principals/remote-users/foobar', - 'type' => 'calendar', - 'access' => 2, - 'resourceid' => 666, - ]); + $qb = $this->db->getQueryBuilder(); + $qb->insert('dav_shares'); + + foreach (['principal:unknown', 'principals/remote-users/foobar'] as $principaluri) { + $qb->values([ + 'principaluri' => $qb->createNamedParameter($principaluri), + 'type' => $qb->createNamedParameter('calendar'), + 'access' => $qb->createNamedParameter(2, IQueryBuilder::PARAM_INT), + 'resourceid' => $qb->createNamedParameter(666, IQueryBuilder::PARAM_INT), + ]); + + try { + $qb->executeStatement(); + } catch (Exception $e) { + if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + throw $e; + } + } + } $this->command = new RemoveInvalidShares( $this->db, diff --git a/apps/files_sharing/lib/External/Manager.php b/apps/files_sharing/lib/External/Manager.php index 43cd7f8d12541..60954fb4a170e 100644 --- a/apps/files_sharing/lib/External/Manager.php +++ b/apps/files_sharing/lib/External/Manager.php @@ -88,27 +88,43 @@ public function addShare($remote, $token, $password, $name, $owner, $shareType, // using the original share item name. $tmpMountPointName = '{{TemporaryMountPointName#' . $name . '}}'; $mountPoint = $tmpMountPointName; - $hash = md5($tmpMountPointName); - $data = [ - 'remote' => $remote, - 'share_token' => $token, - 'password' => $password, - 'name' => $name, - 'owner' => $owner, - 'user' => $user, - 'mountpoint' => $mountPoint, - 'mountpoint_hash' => $hash, - 'accepted' => $accepted, - 'remote_id' => $remoteId, - 'share_type' => $shareType, + + $qb = $this->connection->getQueryBuilder(); + $qb->insert('share_external'); + + $values = [ + 'remote' => $qb->createNamedParameter($remote), + 'share_token' => $qb->createNamedParameter($token), + 'password' => $qb->createNamedParameter($password), + 'name' => $qb->createNamedParameter($name), + 'owner' => $qb->createNamedParameter($owner), + 'user' => $qb->createNamedParameter($user), + 'accepted' => $qb->createNamedParameter($accepted, IQueryBuilder::PARAM_INT), + 'remote_id' => $qb->createNamedParameter($remoteId), + 'share_type' => $qb->createNamedParameter($shareType, IQueryBuilder::PARAM_INT), ]; $i = 1; - while (!$this->connection->insertIfNotExist('*PREFIX*share_external', $data, ['user', 'mountpoint_hash'])) { + while (true) { // The external share already exists for the user - $data['mountpoint'] = $tmpMountPointName . '-' . $i; - $data['mountpoint_hash'] = md5($data['mountpoint']); - $i++; + $qb->values(array_merge($values, [ + 'mountpoint' => $qb->createNamedParameter($mountPoint), + 'mountpoint_hash' => $qb->createNamedParameter(md5($mountPoint)), + ])); + + try { + $qb->executeStatement(); + + break; + } catch (Exception $e) { + if ($e->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + $mountPoint = $tmpMountPointName . '-' . $i; + $i++; + continue; + } + + throw $e; + } } return null; } diff --git a/apps/user_ldap/lib/Mapping/AbstractMapping.php b/apps/user_ldap/lib/Mapping/AbstractMapping.php index 338d49ad03b4a..819738282b3dc 100644 --- a/apps/user_ldap/lib/Mapping/AbstractMapping.php +++ b/apps/user_ldap/lib/Mapping/AbstractMapping.php @@ -416,23 +416,29 @@ public function map($fdn, $name, $uuid) { return false; } - $row = [ - 'ldap_dn_hash' => $this->getDNHash($fdn), - 'ldap_dn' => $fdn, - 'owncloud_name' => $name, - 'directory_uuid' => $uuid - ]; + $qb = $this->dbc->getQueryBuilder(); + $qb + ->insert($this->getTableName()) + ->values([ + 'ldap_dn_hash' => $qb->createNamedParameter($this->getDNHash($fdn)), + 'ldap_dn' => $qb->createNamedParameter($fdn), + 'owncloud_name' => $qb->createNamedParameter($name), + 'directory_uuid' => $qb->createNamedParameter($uuid), + ]); try { - $result = $this->dbc->insertIfNotExist($this->getTableName(), $row); - if ((bool)$result === true) { - $this->cache[$fdn] = $name; - $this->localNameToDnCache?->set($name, $fdn, self::LOCAL_CACHE_TTL); + $qb->executeStatement(); + + $this->cache[$fdn] = $name; + $this->localNameToDnCache?->set($name, $fdn, self::LOCAL_CACHE_TTL); + + return true; + } catch (\OCP\DB\Exception $e) { + if ($e->getReason() === \OCP\DB\Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + return false; } - // insertIfNotExist returns values as int - return (bool)$result; - } catch (\Exception $e) { - return false; + + throw $e; } } diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 0e03b79bb1807..710731f567387 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -1691,7 +1691,6 @@ - @@ -2921,7 +2920,6 @@ - diff --git a/lib/private/DB/Adapter.php b/lib/private/DB/Adapter.php index 8f1b8e6d75f48..bfbabca64a045 100644 --- a/lib/private/DB/Adapter.php +++ b/lib/private/DB/Adapter.php @@ -8,7 +8,6 @@ namespace OC\DB; use Doctrine\DBAL\Exception; -use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use OC\DB\Exceptions\DbalException; /** @@ -64,54 +63,6 @@ public function unlockTable() { $this->conn->commit(); } - /** - * 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) - * @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 - * Please note: text fields (clob) must not be used in the compare array - * @return int number of inserted rows - * @throws Exception - * @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) { - $compare = $compare ?: array_keys($input); - - // Prepare column names and generate placeholders - $columns = '`' . implode('`,`', array_keys($input)) . '`'; - $placeholders = implode(', ', array_fill(0, count($input), '?')); - - $query = 'INSERT INTO `' . $table . '` (' . $columns . ') ' - . 'SELECT ' . $placeholders . ' ' - . 'FROM `' . $table . '` WHERE '; - - $inserts = array_values($input); - foreach ($compare as $key) { - $query .= '`' . $key . '`'; - if (is_null($input[$key])) { - $query .= ' IS NULL AND '; - } else { - $inserts[] = $input[$key]; - $query .= ' = ? AND '; - } - } - $query = substr($query, 0, -5); - $query .= ' HAVING COUNT(*) = 0'; - - try { - return $this->conn->executeUpdate($query, $inserts); - } catch (UniqueConstraintViolationException $e) { - // This exception indicates a concurrent insert happened between - // the insert and the sub-select in the insert, which is safe to ignore. - // More details: https://github.com/nextcloud/server/pull/12315 - return 0; - } - } - /** * @throws \OCP\DB\Exception */ diff --git a/lib/private/DB/AdapterSqlite.php b/lib/private/DB/AdapterSqlite.php index aeadf55ecf7b5..5031ab4f9482f 100644 --- a/lib/private/DB/AdapterSqlite.php +++ b/lib/private/DB/AdapterSqlite.php @@ -7,8 +7,6 @@ */ namespace OC\DB; -use Doctrine\DBAL\Exception\UniqueConstraintViolationException; - class AdapterSqlite extends Adapter { /** * @param string $tableName @@ -30,53 +28,6 @@ public function fixupStatement($statement) { return $statement; } - /** - * 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) - * @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 - * Please note: text fields (clob) must not be used in the compare array - * @return int number of inserted rows - * @throws \Doctrine\DBAL\Exception - * @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)) { - $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 ($compare as $key) { - $query .= '`' . $key . '`'; - if (is_null($input[$key])) { - $query .= ' IS NULL AND '; - } else { - $inserts[] = $input[$key]; - $query .= ' = ? AND '; - } - } - $query = substr($query, 0, -5); - $query .= ')'; - - 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; - } - } - public function insertIgnoreConflict(string $table, array $values): int { $builder = $this->conn->getQueryBuilder(); $builder->insert($table); diff --git a/lib/private/DB/Connection.php b/lib/private/DB/Connection.php index f86cbc341a4d9..2268ea4d74786 100644 --- a/lib/private/DB/Connection.php +++ b/lib/private/DB/Connection.php @@ -531,29 +531,6 @@ public function realLastInsertId($seqName = null) { return parent::lastInsertId($seqName); } - /** - * 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) - * @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 - * Please note: text fields (clob) must not be used in the compare array - * @return int number of inserted rows - * @throws \Doctrine\DBAL\Exception - * @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) { - try { - return $this->adapter->insertIfNotExist($table, $input, $compare); - } catch (\Exception $e) { - $this->logDatabaseException($e); - throw $e; - } - } - public function insertIgnoreConflict(string $table, array $values) : int { try { return $this->adapter->insertIgnoreConflict($table, $values); diff --git a/lib/private/DB/ConnectionAdapter.php b/lib/private/DB/ConnectionAdapter.php index d9ccb3c54f219..05d18b90c42dd 100644 --- a/lib/private/DB/ConnectionAdapter.php +++ b/lib/private/DB/ConnectionAdapter.php @@ -78,14 +78,6 @@ public function lastInsertId(string $table): int { } } - public function insertIfNotExist(string $table, array $input, ?array $compare = null) { - try { - return $this->inner->insertIfNotExist($table, $input, $compare); - } catch (Exception $e) { - throw DbalException::wrap($e); - } - } - public function insertIgnoreConflict(string $table, array $values): int { try { return $this->inner->insertIgnoreConflict($table, $values); diff --git a/lib/private/DB/MigrationService.php b/lib/private/DB/MigrationService.php index 985c331570f38..607df6b1faf27 100644 --- a/lib/private/DB/MigrationService.php +++ b/lib/private/DB/MigrationService.php @@ -18,6 +18,7 @@ use OCP\App\IAppManager; use OCP\AppFramework\App; use OCP\AppFramework\QueryException; +use OCP\DB\Exception; use OCP\DB\ISchemaWrapper; use OCP\DB\Types; use OCP\IDBConnection; @@ -283,10 +284,20 @@ private function shallBeExecuted($m, $knownMigrations) { * @param string $version */ private function markAsExecuted($version) { - $this->connection->insertIfNotExist('*PREFIX*migrations', [ - 'app' => $this->appName, - 'version' => $version - ]); + $qb = $this->connection->getQueryBuilder(); + $qb + ->insert('migrations') + ->values([ + 'app' => $qb->createNamedParameter($this->appName), + 'version' => $qb->createNamedParameter($version), + ]); + try { + $qb->executeStatement(); + } catch (Exception $e) { + if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + throw $e; + } + } } /** diff --git a/lib/private/Files/Cache/Storage.php b/lib/private/Files/Cache/Storage.php index a2fdfc76cc70d..8a39d21aa0b10 100644 --- a/lib/private/Files/Cache/Storage.php +++ b/lib/private/Files/Cache/Storage.php @@ -7,6 +7,7 @@ */ namespace OC\Files\Cache; +use OCP\DB\Exception; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Files\Storage\IStorage; use OCP\IDBConnection; @@ -56,9 +57,22 @@ public function __construct($storage, $isAvailable, IDBConnection $connection) { $this->numericId = (int)$row['numeric_id']; } else { $available = $isAvailable ? 1 : 0; - if ($connection->insertIfNotExist('*PREFIX*storages', ['id' => $this->storageId, 'available' => $available])) { + $qb = $connection->getQueryBuilder(); + $qb + ->insert('storages') + ->values([ + 'id' => $qb->createNamedParameter($this->storageId), + 'available' => $qb->createNamedParameter($available, IQueryBuilder::PARAM_INT), + ]); + + try { + $qb->executeStatement(); $this->numericId = $connection->lastInsertId('*PREFIX*storages'); - } else { + } catch (Exception $e) { + if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + throw $e; + } + if ($row = self::getStorageById($this->storageId)) { $this->numericId = (int)$row['numeric_id']; } else { diff --git a/lib/public/IDBConnection.php b/lib/public/IDBConnection.php index 05ac0da2d7a2e..7019136916928 100644 --- a/lib/public/IDBConnection.php +++ b/lib/public/IDBConnection.php @@ -133,24 +133,6 @@ public function executeStatement($sql, array $params = [], array $types = []): i */ public function lastInsertId(string $table): int; - /** - * 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) - * @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 - * Please note: text fields (clob) must not be used in the compare array - * @return int number of inserted rows - * @throws Exception used to be the removed dbal exception, since 21.0.0 it's \OCP\DB\Exception - * @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 (\OCP\DB\Exception $e) { if ($e->getReason() === \OCP\DB\Exception::REASON_CONSTRAINT_VIOLATION) {} }" 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(string $table, array $input, ?array $compare = null); - - /** * * Insert a row if the row does not exist. Eventual conflicts during insert will be ignored. diff --git a/tests/lib/DB/MigrationsTest.php b/tests/lib/DB/MigrationsTest.php index da92261b85005..0dfde354f3891 100644 --- a/tests/lib/DB/MigrationsTest.php +++ b/tests/lib/DB/MigrationsTest.php @@ -22,6 +22,7 @@ use OC\Migration\MetadataManager; use OCP\App\AppPathNotFoundException; use OCP\App\IAppManager; +use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; use OCP\Migration\Attributes\AddColumn; use OCP\Migration\Attributes\AddIndex; @@ -113,6 +114,17 @@ public function testExecuteStepWithSchemaChange(): void { $this->db->expects($this->once()) ->method('migrateToSchema'); + $qb = $this->createMock(IQueryBuilder::class); + $qb + ->expects($this->once()) + ->method('insert') + ->willReturn($qb); + + $this->db + ->expects($this->once()) + ->method('getQueryBuilder') + ->willReturn($qb); + $wrappedSchema = $this->createMock(Schema::class); $wrappedSchema->expects($this->exactly(2)) ->method('getTables') @@ -156,6 +168,17 @@ public function testExecuteStepWithoutSchemaChange(): void { $this->db->expects($this->never()) ->method('migrateToSchema'); + $qb = $this->createMock(IQueryBuilder::class); + $qb + ->expects($this->once()) + ->method('insert') + ->willReturn($qb); + + $this->db + ->expects($this->once()) + ->method('getQueryBuilder') + ->willReturn($qb); + $step = $this->createMock(IMigrationStep::class); $step->expects($this->once()) ->method('preSchemaChange');