diff --git a/lib/private/DB/Adapter.php b/lib/private/DB/Adapter.php index 0e379d5627f9a..e0edb7ba9faa7 100644 --- a/lib/private/DB/Adapter.php +++ b/lib/private/DB/Adapter.php @@ -10,7 +10,6 @@ use Doctrine\DBAL\Exception; use Doctrine\DBAL\Exception\UniqueConstraintViolationException; -use OC\DB\Exceptions\DbalException; /** * This handles the way we use to write queries, into something that can be @@ -113,18 +112,21 @@ public function insertIfNotExist($table, $input, ?array $compare = null) { * @throws \OCP\DB\Exception */ public function insertIgnoreConflict(string $table, array $values) : int { - try { - $builder = $this->conn->getQueryBuilder(); - $builder->insert($table); - foreach ($values as $key => $value) { - $builder->setValue($key, $builder->createNamedParameter($value)); - } - return $builder->executeStatement(); - } catch (DbalException $e) { - if ($e->getReason() === \OCP\DB\Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { - return 0; - } - throw $e; + $builder = $this->conn->getQueryBuilder(); + $builder->insert($table); + foreach ($values as $key => $value) { + $builder->setValue($key, $builder->createNamedParameter($value)); } + $builder->ignoreConflictsOnInsert(); + return $builder->executeStatement(); + } + + /** + * Transform an INSERT statement into a conflict tolerant one. Platforms + * without native support return the statement unchanged, the resulting + * constraint violation is handled by the query builder. + */ + public function getInsertIgnoreConflictSql(string $sql): string { + return $sql; } } diff --git a/lib/private/DB/AdapterMySQL.php b/lib/private/DB/AdapterMySQL.php index a4312a26d9047..6b0fab0560e03 100644 --- a/lib/private/DB/AdapterMySQL.php +++ b/lib/private/DB/AdapterMySQL.php @@ -40,32 +40,19 @@ protected function getCollation(): string { return $this->collation; } + /** + * We can't use ON DUPLICATE KEY UPDATE here because Nextcloud use the CLIENT_FOUND_ROWS flag + * With this flag the MySQL returns the number of selected rows + * instead of the number of affected/modified rows + * It's impossible to change this behaviour at runtime or for a single query + * Then, the result is 1 if a row is inserted and also 1 if a row is updated with same or different values + * + * With INSERT IGNORE, the result is 1 when a row is inserted, 0 otherwise + * + * Risk: it can also ignore other errors like type mismatch or truncated data… + */ #[\Override] - public function insertIgnoreConflict(string $table, array $values): int { - $builder = $this->conn->getQueryBuilder(); - $builder->insert($table); - $updates = []; - foreach ($values as $key => $value) { - $builder->setValue($key, $builder->createNamedParameter($value)); - } - - /* - * We can't use ON DUPLICATE KEY UPDATE here because Nextcloud use the CLIENT_FOUND_ROWS flag - * With this flag the MySQL returns the number of selected rows - * instead of the number of affected/modified rows - * It's impossible to change this behaviour at runtime or for a single query - * Then, the result is 1 if a row is inserted and also 1 if a row is updated with same or different values - * - * With INSERT IGNORE, the result is 1 when a row is inserted, 0 otherwise - * - * Risk: it can also ignore other errors like type mismatch or truncated data… - */ - $res = $this->conn->executeStatement( - preg_replace('/^INSERT/i', 'INSERT IGNORE', $builder->getSQL()), - $builder->getParameters(), - $builder->getParameterTypes() - ); - - return $res; + public function getInsertIgnoreConflictSql(string $sql): string { + return preg_replace('/^INSERT/i', 'INSERT IGNORE', $sql) ?? $sql; } } diff --git a/lib/private/DB/AdapterPgSql.php b/lib/private/DB/AdapterPgSql.php index 3c0617651c60f..79cf74f3a7a7e 100644 --- a/lib/private/DB/AdapterPgSql.php +++ b/lib/private/DB/AdapterPgSql.php @@ -26,16 +26,12 @@ public function fixupStatement($statement) { return $statement; } + /** + * "upsert" is only available since PgSQL 9.5, but the generic way + * would leave error logs in the DB. + */ #[\Override] - public function insertIgnoreConflict(string $table, array $values) : int { - // "upsert" is only available since PgSQL 9.5, but the generic way - // would leave error logs in the DB. - $builder = $this->conn->getQueryBuilder(); - $builder->insert($table); - foreach ($values as $key => $value) { - $builder->setValue($key, $builder->createNamedParameter($value)); - } - $queryString = $builder->getSQL() . ' ON CONFLICT DO NOTHING'; - return $this->conn->executeStatement($queryString, $builder->getParameters(), $builder->getParameterTypes()); + public function getInsertIgnoreConflictSql(string $sql): string { + return $sql . ' ON CONFLICT DO NOTHING'; } } diff --git a/lib/private/DB/AdapterSqlite.php b/lib/private/DB/AdapterSqlite.php index 223a839c6d8fc..35a74181a51cc 100644 --- a/lib/private/DB/AdapterSqlite.php +++ b/lib/private/DB/AdapterSqlite.php @@ -83,18 +83,7 @@ public function insertIfNotExist($table, $input, ?array $compare = null) { } #[\Override] - public function insertIgnoreConflict(string $table, array $values): int { - $builder = $this->conn->getQueryBuilder(); - $builder->insert($table); - $updates = []; - foreach ($values as $key => $value) { - $builder->setValue($key, $builder->createNamedParameter($value)); - } - - return $this->conn->executeStatement( - $builder->getSQL() . ' ON CONFLICT DO NOTHING', - $builder->getParameters(), - $builder->getParameterTypes() - ); + public function getInsertIgnoreConflictSql(string $sql): string { + return $sql . ' ON CONFLICT DO NOTHING'; } } diff --git a/lib/private/DB/Connection.php b/lib/private/DB/Connection.php index 0ad9886e7c05d..aee103bf0342b 100644 --- a/lib/private/DB/Connection.php +++ b/lib/private/DB/Connection.php @@ -991,4 +991,8 @@ public function getShardDefinition(string $name): ?ShardDefinition { public function getCrossShardMoveHelper(): CrossShardMoveHelper { return new CrossShardMoveHelper($this->shardConnectionManager); } + + public function getInsertIgnoreConflictSql(string $sql): string { + return $this->adapter->getInsertIgnoreConflictSql($sql); + } } diff --git a/lib/private/DB/ConnectionAdapter.php b/lib/private/DB/ConnectionAdapter.php index 11ac4b3809e81..5910d4de5c16e 100644 --- a/lib/private/DB/ConnectionAdapter.php +++ b/lib/private/DB/ConnectionAdapter.php @@ -299,4 +299,8 @@ public function getShardDefinition(string $name): ?ShardDefinition { public function getCrossShardMoveHelper(): CrossShardMoveHelper { return $this->inner->getCrossShardMoveHelper(); } + + public function getInsertIgnoreConflictSql(string $sql): string { + return $this->inner->getInsertIgnoreConflictSql($sql); + } } diff --git a/lib/private/DB/QueryBuilder/ExtendedQueryBuilder.php b/lib/private/DB/QueryBuilder/ExtendedQueryBuilder.php index 5b63e1792bacb..e7515d09f25b0 100644 --- a/lib/private/DB/QueryBuilder/ExtendedQueryBuilder.php +++ b/lib/private/DB/QueryBuilder/ExtendedQueryBuilder.php @@ -359,4 +359,10 @@ public function forUpdate(ConflictResolutionMode $conflictResolutionMode = Confl $this->builder->forUpdate($conflictResolutionMode); return $this; } + + #[\Override] + public function ignoreConflictsOnInsert(): self { + $this->builder->ignoreConflictsOnInsert(); + return $this; + } } diff --git a/lib/private/DB/QueryBuilder/QueryBuilder.php b/lib/private/DB/QueryBuilder/QueryBuilder.php index 11264d28361e3..5d13b090db36d 100644 --- a/lib/private/DB/QueryBuilder/QueryBuilder.php +++ b/lib/private/DB/QueryBuilder/QueryBuilder.php @@ -19,6 +19,7 @@ use OC\DB\QueryBuilder\FunctionBuilder\PgSqlFunctionBuilder; use OC\DB\QueryBuilder\FunctionBuilder\SqliteFunctionBuilder; use OC\SystemConfig; +use OCP\DB\Exception; use OCP\DB\IResult; use OCP\DB\QueryBuilder\ConflictResolutionMode; use OCP\DB\QueryBuilder\ICompositeExpression; @@ -40,6 +41,7 @@ class QueryBuilder extends TypedQueryBuilder { private bool $nonEmptyWhere = false; protected ?string $lastInsertedTable = null; private array $selectedColumns = []; + private bool $insertIgnoreConflicts = false; /** * Initializes a new QueryBuilder. @@ -277,11 +279,29 @@ public function executeStatement(?IDBConnection $connection = null): int { $connection = $this->connection; } - return $connection->executeStatement( - $this->getSQL(), - $this->getParameters(), - $this->getParameterTypes(), - ); + try { + return $connection->executeStatement( + $this->getSQL(), + $this->getParameters(), + $this->getParameterTypes(), + ); + } catch (Exception $e) { + // fallback for platforms without native conflict tolerant inserts + if ($this->insertIgnoreConflicts + && $e->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + return 0; + } + throw $e; + } + } + + #[\Override] + public function ignoreConflictsOnInsert(): self { + if ($this->getType() !== \Doctrine\DBAL\Query\QueryBuilder::INSERT) { + throw new \LogicException('ignoreConflictsOnInsert() can only be used on INSERT queries'); + } + $this->insertIgnoreConflicts = true; + return $this; } /** @@ -298,7 +318,12 @@ public function executeStatement(?IDBConnection $connection = null): int { */ #[\Override] public function getSQL() { - return $this->queryBuilder->getSQL(); + $sql = $this->queryBuilder->getSQL(); + if ($this->insertIgnoreConflicts + && $this->getType() === \Doctrine\DBAL\Query\QueryBuilder::INSERT) { + return $this->connection->getInsertIgnoreConflictSql($sql); + } + return $sql; } /** diff --git a/lib/public/AppFramework/Db/QBMapper.php b/lib/public/AppFramework/Db/QBMapper.php index 1fde5c71aaa04..6402bcbb6bf59 100644 --- a/lib/public/AppFramework/Db/QBMapper.php +++ b/lib/public/AppFramework/Db/QBMapper.php @@ -153,6 +153,45 @@ public function insertOrUpdate(Entity $entity): Entity { } } + /** + * Creates a new entry in the db from an entity, ignoring conflicts on + * unique constraints + * + * @param Entity $entity the entity that should be created + * @psalm-param T $entity the entity that should be created + * @return int number of inserted rows (0 if a conflicting row already exists) + * @throws Exception + * @since 36.0.0 + */ + public function insertIgnoreConflict(Entity $entity): int { + if ($entity instanceof SnowflakeAwareEntity) { + /** @psalm-suppress DocblockTypeContradiction */ + $entity->generateId(); + } + + // get updated fields to save, fields have to be set using a setter to + // be saved + $properties = $entity->getUpdatedFields(); + + $qb = $this->db->getQueryBuilder(); + $qb->insert($this->tableName); + $qb->ignoreConflictsOnInsert(); + + // build the fields + foreach ($properties as $property => $updated) { + if ($property === 'id' && $entity->id === null) { + continue; + } + + $column = $entity->propertyToColumn($property); + $getter = 'get' . ucfirst($property); + $type = $this->getParameterTypeForProperty($entity, $property); + $qb->setValue($column, $qb->createNamedParameter($entity->$getter(), $type)); + } + + return $qb->executeStatement(); + } + /** * Updates an entry in the db from an entity * diff --git a/lib/public/DB/QueryBuilder/IQueryBuilder.php b/lib/public/DB/QueryBuilder/IQueryBuilder.php index 8cb89d8f976d4..edb59c644ecab 100644 --- a/lib/public/DB/QueryBuilder/IQueryBuilder.php +++ b/lib/public/DB/QueryBuilder/IQueryBuilder.php @@ -225,6 +225,23 @@ public function executeQuery(?IDBConnection $connection = null): IResult; */ public function executeStatement(?IDBConnection $connection = null): int; + /** + * Ignore unique constraint conflicts for INSERT queries. + * + * Rows conflicting with an existing row on a unique constraint are skipped + * instead of raising an error, the row count returned by + * {@see self::executeStatement()} is reduced accordingly. On platforms + * without native support for conflict tolerant inserts the resulting + * constraint violation is caught and reported as 0 affected rows. + * + * Must only be called on INSERT queries. + * + * @return $this + * @since 36.0.0 + * @throws \LogicException when called on a non-INSERT query + */ + public function ignoreConflictsOnInsert(): self; + /** * Gets the complete SQL string formed by the current specifications of this QueryBuilder. * diff --git a/lib/public/DB/QueryBuilder/ITypedQueryBuilder.php b/lib/public/DB/QueryBuilder/ITypedQueryBuilder.php index 823cb862786e8..39453475987c2 100644 --- a/lib/public/DB/QueryBuilder/ITypedQueryBuilder.php +++ b/lib/public/DB/QueryBuilder/ITypedQueryBuilder.php @@ -347,4 +347,12 @@ public function runAcrossAllShards(): self; */ #[Override] public function forUpdate(ConflictResolutionMode $conflictResolutionMode = ConflictResolutionMode::Ordinary): self; + + /** + * @inheritDoc + * @return $this + * @since 36.0.0 + */ + #[Override] + public function ignoreConflictsOnInsert(): self; } diff --git a/tests/lib/AppFramework/Db/QBMapperDBTest.php b/tests/lib/AppFramework/Db/QBMapperDBTest.php index 24b8f06e6ce63..c691761a9914d 100644 --- a/tests/lib/AppFramework/Db/QBMapperDBTest.php +++ b/tests/lib/AppFramework/Db/QBMapperDBTest.php @@ -127,6 +127,24 @@ public function testUpdateDateTime(): void { $this->assertEquals($datetime->format('Y-m-d H:i:s'), $dbEntity->getDatetime()->format('Y-m-d H:i:s')); } + public function testInsertIgnoreConflict(): void { + $mapper = new QBDBTestMapper($this->connection); + + $entity = new QBDBTestEntity(); + $entity->setId(200001); + $entity->setDatetime(new \DateTimeImmutable('2000-01-01 23:45:00')); + $this->assertSame(1, $mapper->insertIgnoreConflict($entity)); + + $conflicting = new QBDBTestEntity(); + $conflicting->setId(200001); + $conflicting->setDatetime(new \DateTimeImmutable('2010-05-05 05:05:05')); + $this->assertSame(0, $mapper->insertIgnoreConflict($conflicting)); + + // the conflicting insert must not have modified the existing row + $dbEntity = $mapper->getById(200001); + $this->assertEquals('2000-01-01 23:45:00', $dbEntity->getDatetime()->format('Y-m-d H:i:s')); + } + protected function prepareTestingTable(): void { if ($this->schemaSetup) { $this->connection->getQueryBuilder()->delete('testing')->executeStatement(); diff --git a/tests/lib/AppFramework/Db/QBMapperTest.php b/tests/lib/AppFramework/Db/QBMapperTest.php index 09cfee14720d7..fd3485c80eb55 100644 --- a/tests/lib/AppFramework/Db/QBMapperTest.php +++ b/tests/lib/AppFramework/Db/QBMapperTest.php @@ -212,6 +212,58 @@ public function testUpdateEntityParameterTypeMapping(): void { $this->mapper->update($entity); } + public function testInsertIgnoreConflictEntityParameterTypeMapping(): void { + $datetime = new \DateTimeImmutable(); + $entity = new QBTestEntity(); + $entity->setIntProp(123); + $entity->setBoolProp(true); + $entity->setStringProp('string'); + $entity->setDatetimeProp($datetime); + + $intParam = $this->qb->createNamedParameter('int_prop', IQueryBuilder::PARAM_INT); + $boolParam = $this->qb->createNamedParameter('bool_prop', IQueryBuilder::PARAM_BOOL); + $stringParam = $this->qb->createNamedParameter('string_prop', IQueryBuilder::PARAM_STR); + $datetimeParam = $this->qb->createNamedParameter('datetime_prop', IQueryBuilder::PARAM_DATETIME_IMMUTABLE); + + $this->qb->expects($this->once()) + ->method('insert') + ->with($this->equalTo('table')); + $this->qb->expects($this->once()) + ->method('ignoreConflictsOnInsert'); + + $createNamedParameterCalls = [ + [123, IQueryBuilder::PARAM_INT, null], + [true, IQueryBuilder::PARAM_BOOL, null], + ['string', IQueryBuilder::PARAM_STR, null], + [$datetime, IQueryBuilder::PARAM_DATETIME_IMMUTABLE, null], + ]; + $this->qb->expects($this->exactly(4)) + ->method('createNamedParameter') + ->willReturnCallback(function () use (&$createNamedParameterCalls): void { + $expected = array_shift($createNamedParameterCalls); + $this->assertEquals($expected, func_get_args()); + }); + + $setValueCalls = [ + ['int_prop', $intParam], + ['bool_prop', $boolParam], + ['string_prop', $stringParam], + ['datetime_prop', $datetimeParam], + ]; + $this->qb->expects($this->exactly(4)) + ->method('setValue') + ->willReturnCallback(function () use (&$setValueCalls): void { + $expected = array_shift($setValueCalls); + $this->assertEquals($expected, func_get_args()); + }); + + $this->qb->expects($this->once()) + ->method('executeStatement') + ->willReturn(1); + + $this->assertSame(1, $this->mapper->insertIgnoreConflict($entity)); + } + public function testGetParameterTypeForProperty(): void { $entity = new QBTestEntity(); diff --git a/tests/lib/DB/QueryBuilder/QueryBuilderTest.php b/tests/lib/DB/QueryBuilder/QueryBuilderTest.php index e39ba0b78c62c..b19db7d1d94c0 100644 --- a/tests/lib/DB/QueryBuilder/QueryBuilderTest.php +++ b/tests/lib/DB/QueryBuilder/QueryBuilderTest.php @@ -1449,4 +1449,108 @@ public function testExecuteWithParametersTooMany(): void { $this->invokePrivate($this->queryBuilder, 'connection', [$this->getConnection()]); $this->queryBuilder->executeQuery(); } + + public function testIgnoreConflictsOnInsertSql(): void { + $this->queryBuilder->insert('appconfig') + ->setValue('appid', $this->queryBuilder->createNamedParameter('testIgnoreConflicts')) + ->setValue('configkey', $this->queryBuilder->createNamedParameter('testing')); + $this->queryBuilder->ignoreConflictsOnInsert(); + + $sql = $this->queryBuilder->getSQL(); + match ($this->connection->getDatabaseProvider()) { + IDBConnection::PLATFORM_MYSQL, + IDBConnection::PLATFORM_MARIADB => $this->assertStringStartsWith('INSERT IGNORE INTO', $sql), + IDBConnection::PLATFORM_POSTGRES, + IDBConnection::PLATFORM_SQLITE => $this->assertStringEndsWith('ON CONFLICT DO NOTHING', $sql), + default => $this->assertStringStartsWith('INSERT INTO', $sql), + }; + } + + public function testIgnoreConflictsOnInsertExecution(): void { + $buildInsert = function (IQueryBuilder $qb): void { + $qb->insert('*PREFIX*appconfig') + ->values([ + 'appid' => $qb->createNamedParameter('testIgnoreConflicts'), + 'configkey' => $qb->createNamedParameter('testing'), + 'configvalue' => $qb->createNamedParameter('42'), + ]) + ->ignoreConflictsOnInsert(); + }; + + $qb = $this->connection->getQueryBuilder(); + $buildInsert($qb); + $this->assertSame(1, $qb->executeStatement()); + + $qb = $this->connection->getQueryBuilder(); + $buildInsert($qb); + $this->assertSame(0, $qb->executeStatement()); + + $qb = $this->connection->getQueryBuilder(); + $qb->delete('*PREFIX*appconfig') + ->where($qb->expr()->eq('appid', $qb->createNamedParameter('testIgnoreConflicts'))) + ->executeStatement(); + } + + public function testIgnoreConflictsOnInsertOnSelect(): void { + $this->queryBuilder->select('*')->from('appconfig'); + $this->expectException(\LogicException::class); + $this->queryBuilder->ignoreConflictsOnInsert(); + } + + private function prepareInsertThrowing(\OCP\DB\Exception $exception): void { + $this->queryBuilder->insert('appconfig') + ->setValue('appid', $this->queryBuilder->createNamedParameter('testIgnoreConflicts')); + + $connection = $this->createMock(ConnectionAdapter::class); + $connection->method('executeStatement') + ->willThrowException($exception); + $this->invokePrivate($this->queryBuilder, 'connection', [$connection]); + } + + public function testIgnoreConflictsOnInsertCatchesUniqueViolation(): void { + $this->prepareInsertThrowing(new class extends \OCP\DB\Exception { + #[\Override] + public function getReason(): ?int { + return self::REASON_UNIQUE_CONSTRAINT_VIOLATION; + } + }); + $this->queryBuilder->ignoreConflictsOnInsert(); + + $this->assertSame(0, $this->queryBuilder->executeStatement()); + } + + public function testIgnoreConflictsOnInsertRethrowsOtherErrors(): void { + $exception = new class extends \OCP\DB\Exception { + #[\Override] + public function getReason(): ?int { + return self::REASON_DEADLOCK; + } + }; + $this->prepareInsertThrowing($exception); + $this->queryBuilder->ignoreConflictsOnInsert(); + + try { + $this->queryBuilder->executeStatement(); + $this->fail('Expected the exception to be rethrown'); + } catch (\OCP\DB\Exception $e) { + $this->assertSame($exception, $e); + } + } + + public function testUniqueViolationNotCaughtWithoutFlag(): void { + $exception = new class extends \OCP\DB\Exception { + #[\Override] + public function getReason(): ?int { + return self::REASON_UNIQUE_CONSTRAINT_VIOLATION; + } + }; + $this->prepareInsertThrowing($exception); + + try { + $this->queryBuilder->executeStatement(); + $this->fail('Expected the exception to be rethrown'); + } catch (\OCP\DB\Exception $e) { + $this->assertSame($exception, $e); + } + } }