fix(trashbin): keep metadata consistent on move

Signed-off-by: Hoang Pham <hoangmaths96@gmail.com>

Author: hweihwang

apps/files_trashbin/lib/Trashbin.php

@@ -299,12 +299,60 @@ public static function move2trash($file_path, $ownerOnly = false) {
 
 		$configuredTrashbinSize = static::getConfiguredTrashbinSize($owner);
 		if ($configuredTrashbinSize >= 0 && $sourceInfo->getSize() >= $configuredTrashbinSize) {
+			$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
 			return false;
 		}
 
+		// there is still a possibility that the file has been deleted by a remote user
+		$deletedBy = self::overwriteDeletedBy($user);
+
+		$deleteTrashRow = static function () use ($owner, $filename, $timestamp): void {
+			$query = Server::get(IDBConnection::class)->getQueryBuilder();
+			$query->delete('files_trash')
+				->where($query->expr()->eq('user', $query->createNamedParameter($owner)))
+				->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
+				->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
+			$query->executeStatement();
+		};
+
+		$query = Server::get(IDBConnection::class)->getQueryBuilder();
+		$query->insert('files_trash')
+			->setValue('id', $query->createNamedParameter($filename))
+			->setValue('timestamp', $query->createNamedParameter($timestamp))
+			->setValue('location', $query->createNamedParameter($location))
+			->setValue('user', $query->createNamedParameter($owner))
+			->setValue('deleted_by', $query->createNamedParameter($deletedBy));
+		$inserted = false;
 		try {
-			$moveSuccessful = true;
+			$inserted = ($query->executeStatement() === 1);
+		} catch (\Throwable $e) {
+			Server::get(LoggerInterface::class)->error(
+				'trash bin database insert failed',
+				[
+					'app' => 'files_trashbin',
+					'exception' => $e,
+					'user' => $owner,
+					'filename' => $filename,
+					'timestamp' => $timestamp,
+				]
+			);
+		}
+		if (!$inserted) {
+			Server::get(LoggerInterface::class)->error(
+				'trash bin database couldn\'t be updated, skipping trash move',
+				[
+					'app' => 'files_trashbin',
+					'user' => $owner,
+					'filename' => $filename,
+					'timestamp' => $timestamp,
+				]
+			);
+			$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
+			return false;
+		}
 
+		$moveSuccessful = true;
+		try {
 			$inCache = $sourceStorage->getCache()->inCache($sourceInternalPath);
 			$trashStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
 			if ($inCache) {
@@ -333,65 +381,39 @@ public static function move2trash($file_path, $ownerOnly = false) {
 			} else {
 				$trashStorage->getUpdater()->remove($trashInternalPath);
 			}
-			return false;
+			$moveSuccessful = false;
 		}
 
-		if ($moveSuccessful) {
-			// there is still a possibility that the file has been deleted by a remote user
-			$deletedBy = self::overwriteDeletedBy($user);
-
-			$query = Server::get(IDBConnection::class)->getQueryBuilder();
-			$query->insert('files_trash')
-				->setValue('id', $query->createNamedParameter($filename))
-				->setValue('timestamp', $query->createNamedParameter($timestamp))
-				->setValue('location', $query->createNamedParameter($location))
-				->setValue('user', $query->createNamedParameter($owner))
-				->setValue('deleted_by', $query->createNamedParameter($deletedBy));
-			$inserted = false;
-			try {
-				$inserted = ($query->executeStatement() === 1);
-			} catch (\Throwable $e) {
-				Server::get(LoggerInterface::class)->error(
-					'trash bin database insert failed',
-					[
-						'app' => 'files_trashbin',
-						'exception' => $e,
-						'user' => $owner,
-						'filename' => $filename,
-						'timestamp' => $timestamp,
-					]
-				);
-			}
-			if (!$inserted) {
-				Server::get(LoggerInterface::class)->error(
-					'trash bin database couldn\'t be updated, removing trash payload',
-					[
-						'app' => 'files_trashbin',
-						'user' => $owner,
-						'filename' => $filename,
-						'timestamp' => $timestamp,
-					]
-				);
-				if ($trashStorage->file_exists($trashInternalPath)) {
-					if ($trashStorage->is_dir($trashInternalPath)) {
-						$trashStorage->rmdir($trashInternalPath);
-					} else {
-						$trashStorage->unlink($trashInternalPath);
-					}
+		if (!$moveSuccessful) {
+			Server::get(LoggerInterface::class)->error(
+				'trash move failed, removing trash metadata and payload',
+				[
+					'app' => 'files_trashbin',
+					'user' => $owner,
+					'filename' => $filename,
+					'timestamp' => $timestamp,
+				]
+			);
+			$deleteTrashRow();
+			if ($trashStorage->file_exists($trashInternalPath)) {
+				if ($trashStorage->is_dir($trashInternalPath)) {
+					$trashStorage->rmdir($trashInternalPath);
+				} else {
+					$trashStorage->unlink($trashInternalPath);
 				}
-				$trashStorage->getUpdater()->remove($trashInternalPath);
-				$moveSuccessful = false;
 			}
-			if ($inserted) {
-				Util::emitHook('\OCA\Files_Trashbin\Trashbin', 'post_moveToTrash', ['filePath' => Filesystem::normalizePath($file_path),
-					'trashPath' => Filesystem::normalizePath(static::getTrashFilename($filename, $timestamp))]);
+			$trashStorage->getUpdater()->remove($trashInternalPath);
+		}
+
+		if ($moveSuccessful) {
+			Util::emitHook('\OCA\Files_Trashbin\Trashbin', 'post_moveToTrash', ['filePath' => Filesystem::normalizePath($file_path),
+				'trashPath' => Filesystem::normalizePath(static::getTrashFilename($filename, $timestamp))]);
 
-				self::retainVersions($filename, $owner, $ownerPath, $timestamp);
+			self::retainVersions($filename, $owner, $ownerPath, $timestamp);
 
-				// if owner !== user we need to also add a copy to the users trash
-				if ($user !== $owner && $ownerOnly === false) {
-					self::copyFilesToUser($ownerPath, $owner, $file_path, $user, $timestamp);
-				}
+			// if owner !== user we need to also add a copy to the users trash
+			if ($user !== $owner && $ownerOnly === false) {
+				self::copyFilesToUser($ownerPath, $owner, $file_path, $user, $timestamp);
 			}
 		}