Skip to content

Commit eb0d86a

Browse files
susnuxsusnux
authored
Merge pull request #59512 from nextcloud/backport/57360/stable33
[stable33] fix(trashbin): keep cache and db consistent
2 parents e322c63 + 12fcd13 commit eb0d86a

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
@@ -296,18 +296,65 @@ public static function move2trash($file_path, $ownerOnly = false) {
296296

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

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

342+
$moveSuccessful = true;
343+
try {
305344
$inCache = $sourceStorage->getCache()->inCache($sourceInternalPath);
306345
$trashStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
307346
if ($inCache) {
308347
$trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
348+
} else {
349+
$sizeDifference = $sourceInfo->getSize();
350+
if ($sizeDifference < 0) {
351+
$sizeDifference = null;
352+
} else {
353+
$sizeDifference = (int)$sizeDifference;
354+
}
355+
$trashStorage->getUpdater()->update($trashInternalPath, null, $sizeDifference);
309356
}
310-
} catch (CopyRecursiveException $e) {
357+
} catch (\Exception $e) {
311358
$moveSuccessful = false;
312359
if ($trashStorage->file_exists($trashInternalPath)) {
313360
$trashStorage->unlink($trashInternalPath);
@@ -328,24 +375,31 @@ public static function move2trash($file_path, $ownerOnly = false) {
328375
} else {
329376
$trashStorage->getUpdater()->remove($trashInternalPath);
330377
}
331-
return false;
378+
$moveSuccessful = false;
332379
}
333380

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

@@ -538,12 +592,7 @@ public static function restore($file, $filename, $timestamp) {
538592
self::restoreVersions($view, $file, $filename, $uniqueFilename, $location, $timestamp);
539593

540594
if ($timestamp) {
541-
$query = Server::get(IDBConnection::class)->getQueryBuilder();
542-
$query->delete('files_trash')
543-
->where($query->expr()->eq('user', $query->createNamedParameter($user)))
544-
->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
545-
->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
546-
$query->executeStatement();
595+
self::deleteTrashRow($user, $filename, $timestamp);
547596
}
548597

549598
return true;
@@ -682,13 +731,6 @@ public static function delete($filename, $user, $timestamp = null) {
682731
$size = 0;
683732

684733
if ($timestamp) {
685-
$query = Server::get(IDBConnection::class)->getQueryBuilder();
686-
$query->delete('files_trash')
687-
->where($query->expr()->eq('user', $query->createNamedParameter($user)))
688-
->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
689-
->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
690-
$query->executeStatement();
691-
692734
$file = static::getTrashFilename($filename, $timestamp);
693735
} else {
694736
$file = $filename;
@@ -699,6 +741,9 @@ public static function delete($filename, $user, $timestamp = null) {
699741
try {
700742
$node = $userRoot->get('/files_trashbin/files/' . $file);
701743
} catch (NotFoundException $e) {
744+
if ($timestamp) {
745+
self::deleteTrashRow($user, $filename, $timestamp);
746+
}
702747
return $size;
703748
}
704749

@@ -712,9 +757,22 @@ public static function delete($filename, $user, $timestamp = null) {
712757
$node->delete();
713758
self::emitTrashbinPostDelete('/files_trashbin/files/' . $file);
714759

760+
if ($timestamp) {
761+
self::deleteTrashRow($user, $filename, $timestamp);
762+
}
763+
715764
return $size;
716765
}
717766

767+
private static function deleteTrashRow(string $user, string $filename, int $timestamp): void {
768+
$query = Server::get(IDBConnection::class)->getQueryBuilder();
769+
$query->delete('files_trash')
770+
->where($query->expr()->eq('user', $query->createNamedParameter($user)))
771+
->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
772+
->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
773+
$query->executeStatement();
774+
}
775+
718776
/**
719777
* @param string $file
720778
* @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)