Skip to content

Commit 1584918

Browse files
sorbaughsorbaugh
authored
Merge pull request #59565 from nextcloud/backport/57360/stable31
[stable31] fix(trashbin): keep cache and db consistent
2 parents 8079ba1 + 185f427 commit 1584918

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

292292
$configuredTrashbinSize = static::getConfiguredTrashbinSize($owner);
293293
if ($configuredTrashbinSize >= 0 && $sourceInfo->getSize() >= $configuredTrashbinSize) {
294+
$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
294295
return false;
295296
}
296297

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

337+
$moveSuccessful = true;
338+
try {
300339
$inCache = $sourceStorage->getCache()->inCache($sourceInternalPath);
301340
$trashStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
302341
if ($inCache) {
303342
$trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
343+
} else {
344+
$sizeDifference = $sourceInfo->getSize();
345+
if ($sizeDifference < 0) {
346+
$sizeDifference = null;
347+
} else {
348+
$sizeDifference = (int)$sizeDifference;
349+
}
350+
$trashStorage->getUpdater()->update($trashInternalPath, null, $sizeDifference);
304351
}
305-
} catch (CopyRecursiveException $e) {
352+
} catch (\Exception $e) {
306353
$moveSuccessful = false;
307354
if ($trashStorage->file_exists($trashInternalPath)) {
308355
$trashStorage->unlink($trashInternalPath);
@@ -323,24 +370,31 @@ public static function move2trash($file_path, $ownerOnly = false) {
323370
} else {
324371
$trashStorage->getUpdater()->remove($trashInternalPath);
325372
}
326-
return false;
373+
$moveSuccessful = false;
327374
}
328375

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

@@ -528,12 +582,7 @@ public static function restore($file, $filename, $timestamp) {
528582
self::restoreVersions($view, $file, $filename, $uniqueFilename, $location, $timestamp);
529583

530584
if ($timestamp) {
531-
$query = \OC::$server->getDatabaseConnection()->getQueryBuilder();
532-
$query->delete('files_trash')
533-
->where($query->expr()->eq('user', $query->createNamedParameter($user)))
534-
->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
535-
->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
536-
$query->executeStatement();
585+
self::deleteTrashRow($user, $filename, $timestamp);
537586
}
538587

539588
return true;
@@ -672,13 +721,6 @@ public static function delete($filename, $user, $timestamp = null) {
672721
$size = 0;
673722

674723
if ($timestamp) {
675-
$query = \OC::$server->getDatabaseConnection()->getQueryBuilder();
676-
$query->delete('files_trash')
677-
->where($query->expr()->eq('user', $query->createNamedParameter($user)))
678-
->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
679-
->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
680-
$query->executeStatement();
681-
682724
$file = static::getTrashFilename($filename, $timestamp);
683725
} else {
684726
$file = $filename;
@@ -689,6 +731,9 @@ public static function delete($filename, $user, $timestamp = null) {
689731
try {
690732
$node = $userRoot->get('/files_trashbin/files/' . $file);
691733
} catch (NotFoundException $e) {
734+
if ($timestamp) {
735+
self::deleteTrashRow($user, $filename, $timestamp);
736+
}
692737
return $size;
693738
}
694739

@@ -702,9 +747,22 @@ public static function delete($filename, $user, $timestamp = null) {
702747
$node->delete();
703748
self::emitTrashbinPostDelete('/files_trashbin/files/' . $file);
704749

750+
if ($timestamp) {
751+
self::deleteTrashRow($user, $filename, $timestamp);
752+
}
753+
705754
return $size;
706755
}
707756

757+
private static function deleteTrashRow(string $user, string $filename, int $timestamp): void {
758+
$query = Server::get(IDBConnection::class)->getQueryBuilder();
759+
$query->delete('files_trash')
760+
->where($query->expr()->eq('user', $query->createNamedParameter($user)))
761+
->andWhere($query->expr()->eq('id', $query->createNamedParameter($filename)))
762+
->andWhere($query->expr()->eq('timestamp', $query->createNamedParameter($timestamp)));
763+
$query->executeStatement();
764+
}
765+
708766
/**
709767
* @param string $file
710768
* @param string $filename

‎apps/files_trashbin/tests/StorageTest.php‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,21 @@
44
* SPDX-FileCopyrightText: 2016 ownCloud, Inc.
55
* SPDX-License-Identifier: AGPL-3.0-only
66
*/
7+
78
namespace OCA\Files_Trashbin\Tests;
89

10+
use OC\Files\Cache\Updater;
911
use OC\Files\Filesystem;
12+
use OC\Files\ObjectStore\ObjectStoreStorage;
1013
use OC\Files\Storage\Common;
14+
use OC\Files\Storage\Local;
1115
use OC\Files\Storage\Temporary;
1216
use OC\Files\View;
1317
use OCA\Files_Trashbin\AppInfo\Application;
1418
use OCA\Files_Trashbin\Events\MoveToTrashEvent;
1519
use OCA\Files_Trashbin\Storage;
1620
use OCA\Files_Trashbin\Trash\ITrashManager;
21+
use OCA\Files_Trashbin\Trashbin;
1722
use OCP\AppFramework\Bootstrap\IBootContext;
1823
use OCP\AppFramework\Utility\ITimeFactory;
1924
use OCP\Constants;
@@ -125,6 +130,84 @@ public function testSingleStorageDeleteFile(): void {
125130
$this->assertEquals('test.txt', substr($name, 0, strrpos($name, '.')));
126131
}
127132

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

0 commit comments

Comments
 (0)