Skip to content

Commit 3663536

Browse files
susnuxsusnux
authored
Merge pull request #59513 from nextcloud/backport/57360/stable32
[stable32] fix(trashbin): keep cache and db consistent
2 parents 82a77a1 + e617364 commit 3663536

2 files changed

Lines changed: 171 additions & 30 deletions

File tree

‎apps/files_trashbin/lib/Trashbin.php‎

Lines changed: 88 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -295,18 +295,65 @@ public static function move2trash($file_path, $ownerOnly = false) {
295295

296296
$configuredTrashbinSize = static::getConfiguredTrashbinSize($owner);
297297
if ($configuredTrashbinSize >= 0 && $sourceInfo->getSize() >= $configuredTrashbinSize) {
298+
$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
298299
return false;
299300
}
300301

302+
// there is still a possibility that the file has been deleted by a remote user
303+
$deletedBy = self::overwriteDeletedBy($user);
304+
305+
$query = Server::get(IDBConnection::class)->getQueryBuilder();
306+
$query->insert('files_trash')
307+
->setValue('id', $query->createNamedParameter($filename))
308+
->setValue('timestamp', $query->createNamedParameter($timestamp))
309+
->setValue('location', $query->createNamedParameter($location))
310+
->setValue('user', $query->createNamedParameter($owner))
311+
->setValue('deleted_by', $query->createNamedParameter($deletedBy));
312+
$inserted = false;
301313
try {
302-
$moveSuccessful = true;
314+
$inserted = ($query->executeStatement() === 1);
315+
} catch (\Throwable $e) {
316+
Server::get(LoggerInterface::class)->error(
317+
'trash bin database insert failed',
318+
[
319+
'app' => 'files_trashbin',
320+
'exception' => $e,
321+
'user' => $owner,
322+
'filename' => $filename,
323+
'timestamp' => $timestamp,
324+
]
325+
);
326+
}
327+
if (!$inserted) {
328+
Server::get(LoggerInterface::class)->error(
329+
'trash bin database couldn\'t be updated, skipping trash move',
330+
[
331+
'app' => 'files_trashbin',
332+
'user' => $owner,
333+
'filename' => $filename,
334+
'timestamp' => $timestamp,
335+
]
336+
);
337+
$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
338+
return false;
339+
}
303340

341+
$moveSuccessful = true;
342+
try {
304343
$inCache = $sourceStorage->getCache()->inCache($sourceInternalPath);
305344
$trashStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
306345
if ($inCache) {
307346
$trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
347+
} else {
348+
$sizeDifference = $sourceInfo->getSize();
349+
if ($sizeDifference < 0) {
350+
$sizeDifference = null;
351+
} else {
352+
$sizeDifference = (int)$sizeDifference;
353+
}
354+
$trashStorage->getUpdater()->update($trashInternalPath, null, $sizeDifference);
308355
}
309-
} catch (CopyRecursiveException $e) {
356+
} catch (\Exception $e) {
310357
$moveSuccessful = false;
311358
if ($trashStorage->file_exists($trashInternalPath)) {
312359
$trashStorage->unlink($trashInternalPath);
@@ -327,24 +374,31 @@ public static function move2trash($file_path, $ownerOnly = false) {
327374
} else {
328375
$trashStorage->getUpdater()->remove($trashInternalPath);
329376
}
330-
return false;
377+
$moveSuccessful = false;
331378
}
332379

333-
if ($moveSuccessful) {
334-
// there is still a possibility that the file has been deleted by a remote user
335-
$deletedBy = self::overwriteDeletedBy($user);
336-
337-
$query = Server::get(IDBConnection::class)->getQueryBuilder();
338-
$query->insert('files_trash')
339-
->setValue('id', $query->createNamedParameter($filename))
340-
->setValue('timestamp', $query->createNamedParameter($timestamp))
341-
->setValue('location', $query->createNamedParameter($location))
342-
->setValue('user', $query->createNamedParameter($owner))
343-
->setValue('deleted_by', $query->createNamedParameter($deletedBy));
344-
$result = $query->executeStatement();
345-
if (!$result) {
346-
Server::get(LoggerInterface::class)->error('trash bin database couldn\'t be updated', ['app' => 'files_trashbin']);
380+
if (!$moveSuccessful) {
381+
Server::get(LoggerInterface::class)->error(
382+
'trash move failed, removing trash metadata and payload',
383+
[
384+
'app' => 'files_trashbin',
385+
'user' => $owner,
386+
'filename' => $filename,
387+
'timestamp' => $timestamp,
388+
]
389+
);
390+
self::deleteTrashRow($user, $filename, $timestamp);
391+
if ($trashStorage->file_exists($trashInternalPath)) {
392+
if ($trashStorage->is_dir($trashInternalPath)) {
393+
$trashStorage->rmdir($trashInternalPath);
394+
} else {
395+
$trashStorage->unlink($trashInternalPath);
396+
}
347397
}
398+
$trashStorage->getUpdater()->remove($trashInternalPath);
399+
}
400+
401+
if ($moveSuccessful) {
348402
Util::emitHook('\OCA\Files_Trashbin\Trashbin', 'post_moveToTrash', ['filePath' => Filesystem::normalizePath($file_path),
349403
'trashPath' => Filesystem::normalizePath(static::getTrashFilename($filename, $timestamp))]);
350404

@@ -535,12 +589,7 @@ public static function restore($file, $filename, $timestamp) {
535589
self::restoreVersions($view, $file, $filename, $uniqueFilename, $location, $timestamp);
536590

537591
if ($timestamp) {
538-
$query = Server::get(IDBConnection::class)->getQueryBuilder();
539-
$query->delete('files_trash')
540-
->where($query->expr()->eq('user', $query->createNamedParameter($user)))
541-
->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
542-
->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
543-
$query->executeStatement();
592+
self::deleteTrashRow($user, $filename, $timestamp);
544593
}
545594

546595
return true;
@@ -679,13 +728,6 @@ public static function delete($filename, $user, $timestamp = null) {
679728
$size = 0;
680729

681730
if ($timestamp) {
682-
$query = Server::get(IDBConnection::class)->getQueryBuilder();
683-
$query->delete('files_trash')
684-
->where($query->expr()->eq('user', $query->createNamedParameter($user)))
685-
->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
686-
->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
687-
$query->executeStatement();
688-
689731
$file = static::getTrashFilename($filename, $timestamp);
690732
} else {
691733
$file = $filename;
@@ -696,6 +738,9 @@ public static function delete($filename, $user, $timestamp = null) {
696738
try {
697739
$node = $userRoot->get('/files_trashbin/files/' . $file);
698740
} catch (NotFoundException $e) {
741+
if ($timestamp) {
742+
self::deleteTrashRow($user, $filename, $timestamp);
743+
}
699744
return $size;
700745
}
701746

@@ -709,9 +754,22 @@ public static function delete($filename, $user, $timestamp = null) {
709754
$node->delete();
710755
self::emitTrashbinPostDelete('/files_trashbin/files/' . $file);
711756

757+
if ($timestamp) {
758+
self::deleteTrashRow($user, $filename, $timestamp);
759+
}
760+
712761
return $size;
713762
}
714763

764+
private static function deleteTrashRow(string $user, string $filename, int $timestamp): void {
765+
$query = Server::get(IDBConnection::class)->getQueryBuilder();
766+
$query->delete('files_trash')
767+
->where($query->expr()->eq('user', $query->createNamedParameter($user)))
768+
->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
769+
->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
770+
$query->executeStatement();
771+
}
772+
715773
/**
716774
* @param string $file
717775
* @param string $filename

‎apps/files_trashbin/tests/StorageTest.php‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,16 +6,21 @@
66
* SPDX-FileCopyrightText: 2016 ownCloud, Inc.
77
* SPDX-License-Identifier: AGPL-3.0-only
88
*/
9+
910
namespace OCA\Files_Trashbin\Tests;
1011

12+
use OC\Files\Cache\Updater;
1113
use OC\Files\Filesystem;
14+
use OC\Files\ObjectStore\ObjectStoreStorage;
1215
use OC\Files\Storage\Common;
16+
use OC\Files\Storage\Local;
1317
use OC\Files\Storage\Temporary;
1418
use OC\Files\View;
1519
use OCA\Files_Trashbin\AppInfo\Application;
1620
use OCA\Files_Trashbin\Events\MoveToTrashEvent;
1721
use OCA\Files_Trashbin\Storage;
1822
use OCA\Files_Trashbin\Trash\ITrashManager;
23+
use OCA\Files_Trashbin\Trashbin;
1924
use OCP\AppFramework\Bootstrap\IBootContext;
2025
use OCP\AppFramework\Utility\ITimeFactory;
2126
use OCP\Constants;
@@ -118,6 +123,84 @@ public function testSingleStorageDeleteFile(): void {
118123
$this->assertEquals('test.txt', substr($name, 0, strrpos($name, '.')));
119124
}
120125

126+
public function testTrashEntryCreatedWhenSourceNotInCache(): void {
127+
$this->userView->file_put_contents('uncached.txt', 'foo');
128+
129+
[$storage, $internalPath] = $this->userView->resolvePath('uncached.txt');
130+
if ($storage->instanceOfStorage(ObjectStoreStorage::class)) {
131+
$this->markTestSkipped('object store always has the file in cache');
132+
}
133+
$cache = $storage->getCache();
134+
$cache->remove($internalPath);
135+
$this->assertFalse($cache->inCache($internalPath));
136+
137+
$this->userView->unlink('uncached.txt');
138+
139+
$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/');
140+
$this->assertCount(1, $results);
141+
$name = $results[0]->getName();
142+
$this->assertEquals('uncached.txt', substr($name, 0, strrpos($name, '.')));
143+
144+
[$trashStorage, $trashInternalPath] = $this->rootView->resolvePath('/' . $this->user . '/files_trashbin/files/' . $name);
145+
$this->assertTrue($trashStorage->getCache()->inCache($trashInternalPath));
146+
}
147+
148+
public function testTrashEntryNotCreatedWhenDeleteFailed(): void {
149+
$storage2 = $this->getMockBuilder(Temporary::class)
150+
->setConstructorArgs([])
151+
->onlyMethods(['unlink', 'instanceOfStorage'])
152+
->getMock();
153+
$storage2->method('unlink')
154+
->willReturn(false);
155+
156+
// disable same-storage move optimization
157+
$storage2->method('instanceOfStorage')
158+
->willReturnCallback(fn (string $class) => ($class !== Local::class) && (new Temporary([]))->instanceOfStorage($class));
159+
160+
161+
Filesystem::mount($storage2, [], $this->user . '/files/substorage');
162+
$this->userView->file_put_contents('substorage/test.txt', 'foo');
163+
164+
$this->assertFalse($this->userView->unlink('substorage/test.txt'));
165+
166+
$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/');
167+
$this->assertEmpty($results);
168+
169+
$trashData = Trashbin::getExtraData($this->user);
170+
$this->assertEmpty($trashData);
171+
}
172+
173+
public function testTrashEntryNotCreatedWhenCacheRowFailed(): void {
174+
$trashStorage = $this->getMockBuilder(Temporary::class)
175+
->setConstructorArgs([])
176+
->onlyMethods(['getUpdater'])
177+
->getMock();
178+
$updater = $this->getMockBuilder(Updater::class)
179+
->setConstructorArgs([$trashStorage])
180+
->onlyMethods(['renameFromStorage'])
181+
->getMock();
182+
$trashStorage->method('getUpdater')
183+
->willReturn($updater);
184+
$updater->method('renameFromStorage')
185+
->willThrowException(new \Exception());
186+
187+
Filesystem::mount($trashStorage, [], $this->user . '/files_trashbin');
188+
$this->userView->file_put_contents('test.txt', 'foo');
189+
190+
try {
191+
$this->assertFalse($this->userView->unlink('test.txt'));
192+
$this->fail();
193+
} catch (\Exception) {
194+
// expected
195+
}
196+
197+
$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/');
198+
$this->assertEmpty($results);
199+
200+
$trashData = Trashbin::getExtraData($this->user);
201+
$this->assertEmpty($trashData);
202+
}
203+
121204
/**
122205
* Test that deleting a folder puts it into the trashbin.
123206
*/

0 commit comments

Comments
 (0)