Skip to content

refactor(files_trashbin): Clean up move2trash and expand test coverage #63069

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
joshtrichards wants to merge 4 commits into
base: master
Choose a base branch
from
+134 −41
Draft

refactor(files_trashbin): Clean up move2trash and expand test coverage #63069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5a2669
refactor(files_trashbin): Flatten Trashbin::copyFilesToUser()
joshtrichards Aug 8, 2026
841f0b0
refactor(files_trashbin): avoid constructing View prematurely in move…
joshtrichards Aug 8, 2026
24938ce
test(file_trashbin): improve Trashbin coverage
joshtrichards Aug 8, 2026
cb21322
chore(files_trashbin): fixup typo in Trashbin class
joshtrichards Aug 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 72 additions & 41 deletions apps/files_trashbin/lib/Trashbin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,15 +179,22 @@ private static function setUpTrash($user): void {
}

/**
* copy file to owners trash
* Copy a deleted item to the trash bin of a user who is not its owner.
*
* @param string $sourcePath
* @param string $owner
* @param string $targetPath
* @param string $user
* @param int $timestamp
* The owner's trash bin contains the original item. The additional copy is
* needed so that the user who deleted an item from a shared location can see
* it in their own trash bin.
*
* If the user's trash bin has insufficient available space, no copy or
* metadata entry is created.
*
* @param string $sourcePath Path to the item in the owner's files root.
* @param string $owner Owner of the deleted item.
* @param string $targetPath Original path of the item in the deleting user's files root.
* @param string $user User who deleted the item.
* @param int $timestamp Timestamp used to identify the trash item.
*/
private static function copyFilesToUser($sourcePath, $owner, $targetPath, $user, $timestamp): void {
private static function copyFilesToUser(string $sourcePath, string $owner, string $targetPath, string $user, int $timestamp): void {
self::setUpTrash($owner);

$targetFilename = basename($targetPath);
Expand All @@ -202,32 +209,43 @@ private static function copyFilesToUser($sourcePath, $owner, $targetPath, $user,
$free = $view->free_space($target);
$isUnknownOrUnlimitedFreeSpace = $free < 0;
$isEnoughFreeSpaceLeft = $view->filesize($source) < $free;
if ($isUnknownOrUnlimitedFreeSpace || $isEnoughFreeSpaceLeft) {
self::copy_recursive($source, $target, $view);
}

if ($view->file_exists($target)) {
$query = Server::get(IDBConnection::class)->getQueryBuilder();
$query->insert('files_trash')
->setValue('id', $query->createNamedParameter($targetFilename))
->setValue('timestamp', $query->createNamedParameter($timestamp))
->setValue('location', $query->createNamedParameter($targetLocation))
->setValue('user', $query->createNamedParameter($user))
->setValue('deleted_by', $query->createNamedParameter($user));
$result = $query->executeStatement();
if (!$result) {
Server::get(LoggerInterface::class)->error('trash bin database couldn\'t be updated for the files owner', ['app' => 'files_trashbin']);
}

if (!$isUnknownOrUnlimitedFreeSpace && !$isEnoughFreeSpaceLeft) {
return;
}

self::copy_recursive($source, $target, $view);

if (!$view->file_exists($target)) {
return;
}

$query = Server::get(IDBConnection::class)->getQueryBuilder();
$query->insert('files_trash')
->setValue('id', $query->createNamedParameter($targetFilename))
->setValue('timestamp', $query->createNamedParameter($timestamp))
->setValue('location', $query->createNamedParameter($targetLocation))
->setValue('user', $query->createNamedParameter($user))
->setValue('deleted_by', $query->createNamedParameter($user));
$result = $query->executeStatement();
if (!$result) {
Server::get(LoggerInterface::class)->error('trash bin database couldn\'t be updated for the files owner', ['app' => 'files_trashbin']);
}
}

/**
* move file to the trash bin
* Move a file or directory to the owner's trash bin.
*
* @param string $file_path path to the deleted file/directory relative to the files root directory
* @param bool $ownerOnly delete for owner only (if file gets moved out of a shared folder)
* For items owned by another user, a copy is normally added to the
* deleting user's trash bin as well. Set $ownerOnly to skip that copy.
*
* @return bool
* @param string $file_path Path to the deleted file or directory relative
* to the files root directory.
* @param bool $ownerOnly Delete for the owner only, for example when an
* item is moved out of a shared folder.
*
* @return bool True if the item was moved successfully or was already
* absent; false if the trash move failed or was rejected.
*/
public static function move2trash($file_path, $ownerOnly = false) {
// get the user for which the filesystem is setup
Expand All @@ -236,18 +254,17 @@ public static function move2trash($file_path, $ownerOnly = false) {
[$owner, $ownerPath] = self::getUidAndFilename($file_path);

// if no owner found (ex: ext storage + share link), will use the current user's trashbin then
if (is_null($owner)) {
if ($owner === null) {
$owner = $user;
$ownerPath = $file_path;
}

$ownerView = new View('/' . $owner);

// file has been deleted in between
if (is_null($ownerPath) || $ownerPath === '') {
if ($ownerPath === null || $ownerPath === '') {
return true;
}

$ownerView = new View('/' . $owner);
$sourceInfo = $ownerView->getFileInfo('/files/' . $ownerPath);

if ($sourceInfo === false) {
Expand All @@ -256,7 +273,7 @@ public static function move2trash($file_path, $ownerOnly = false) {

self::setUpTrash($user);
if ($owner !== $user) {
// also setup for owner
// The original payload is stored in the owner's trash bin.
self::setUpTrash($owner);
}

Expand All @@ -274,16 +291,16 @@ public static function move2trash($file_path, $ownerOnly = false) {
$trashPath = '/files_trashbin/files/' . static::getTrashFilename($filename, $timestamp);
$gotLock = false;

// Keep trying until we obtain the lock for a unique trash filename.
do {
/** @var ILockingStorage & IStorage $trashStorage */
[$trashStorage, $trashInternalPath] = $ownerView->resolvePath($trashPath);
try {
$trashStorage->acquireLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
$gotLock = true;
} catch (LockedException $e) {
// a file with the same name is being deleted concurrently
// nudge the timestamp a bit to resolve the conflict

// Another deletion is using this filename. Incrementing the
// timestamp gives this item a distinct trash filename.
$timestamp = $timestamp + 1;

$trashPath = '/files_trashbin/files/' . static::getTrashFilename($filename, $timestamp);
Expand All @@ -306,6 +323,8 @@ public static function move2trash($file_path, $ownerOnly = false) {
// there is still a possibility that the file has been deleted by a remote user
$deletedBy = self::overwriteDeletedBy($user);

// Insert metadata before moving the payload; failed moves remove this row
// again so metadata and the trash payload remain consistent.
$query = Server::get(IDBConnection::class)->getQueryBuilder();
$query->insert('files_trash')
->setValue('id', $query->createNamedParameter($filename))
Expand Down Expand Up @@ -347,8 +366,10 @@ public static function move2trash($file_path, $ownerOnly = false) {
$inCache = $sourceStorage->getCache()->inCache($sourceInternalPath);
$trashStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
if ($inCache) {
// Preserve the existing cache entry when the source was cached.
$trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
} else {
// Populate the destination cache when the source had no cache entry.
$sizeDifference = $sourceInfo->getSize();
if ($sizeDifference < 0) {
$sizeDifference = null;
Expand All @@ -365,15 +386,18 @@ public static function move2trash($file_path, $ownerOnly = false) {
Server::get(LoggerInterface::class)->error('Couldn\'t move ' . $file_path . ' to the trash bin', ['app' => 'files_trashbin']);
}

if ($sourceStorage->file_exists($sourceInternalPath)) { // failed to delete the original file, abort
// A successful trash move must remove the original source.
if ($sourceStorage->file_exists($sourceInternalPath)) {
// The move may have succeeded at the storage level, but the source
// is still present. Restore cache state and mark the operation failed.
if ($sourceStorage->is_dir($sourceInternalPath)) {
$sourceStorage->rmdir($sourceInternalPath);
} else {
$sourceStorage->unlink($sourceInternalPath);
}

if ($sourceStorage->file_exists($sourceInternalPath)) {
// undo the cache move
// Restore the cache relationship to match the fact that the source remains.
$sourceStorage->getUpdater()->renameFromStorage($trashStorage, $trashInternalPath, $sourceInternalPath);
} else {
$trashStorage->getUpdater()->remove($trashInternalPath);
Expand Down Expand Up @@ -403,12 +427,19 @@ public static function move2trash($file_path, $ownerOnly = false) {
}

if ($moveSuccessful) {
Util::emitHook('\OCA\Files_Trashbin\Trashbin', 'post_moveToTrash', ['filePath' => Filesystem::normalizePath($file_path),
'trashPath' => Filesystem::normalizePath(static::getTrashFilename($filename, $timestamp))]);
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);

// if owner !== user we need to also add a copy to the users trash
// Shared items are stored in the owner's trash and copied to the
// deleting user's trash for visibility there.
if ($user !== $owner && $ownerOnly === false) {
self::copyFilesToUser($ownerPath, $owner, $file_path, $user, $timestamp);
}
Expand All @@ -418,7 +449,7 @@ public static function move2trash($file_path, $ownerOnly = false) {

self::scheduleExpire($user);

// if owner !== user we also need to update the owners trash size
// Expiration must be scheduled for both trash bins when they differ.
if ($owner !== $user) {
self::scheduleExpire($owner);
}
Expand Down
62 changes: 62 additions & 0 deletions apps/files_trashbin/tests/TrashbinTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,34 @@ public function testExpireOldFilesShared(): void {
$this->verifyArray($filesInTrashUser1AfterDelete, ['user1-2.txt', 'user1-4.txt']);
}

public function testDeletingSharedFileAddsItemToBothTrashBins(): void {
$user1Folder = Server::get(IRootFolder::class)->getUserFolder(self::TEST_TRASHBIN_USER1);
$file = $user1Folder->newFile('shared.txt');
$file->putContent('shared content');

$share = Server::get(\OCP\Share\IManager::class)->newShare();
$share->setShareType(IShare::TYPE_USER)
->setNode($file)
->setSharedBy(self::TEST_TRASHBIN_USER1)
->setSharedWith(self::TEST_TRASHBIN_USER2)
->setPermissions(Constants::PERMISSION_ALL);
$share = Server::get(\OCP\Share\IManager::class)->createShare($share);
Server::get(\OCP\Share\IManager::class)->acceptShare($share, self::TEST_TRASHBIN_USER2);

self::loginHelper(self::TEST_TRASHBIN_USER2);
$this->assertTrue(Filesystem::file_exists('shared.txt'));
Filesystem::unlink('shared.txt');

$this->verifyArray(
Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER1),
['shared.txt']
);
$this->verifyArray(
Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER2),
['shared.txt']
);
}

/**
* verify that the array contains the expected results
*
Expand Down Expand Up @@ -687,6 +715,40 @@ public function testTrashSizePropagation(): void {
$this->assertEquals(3, $view->getFileInfo('')->getSize());
}

public function testMoveToTrashFallsBackToPermanentDeletionWhenConfiguredSizeIsReached(): void {
$config = Server::get(IConfig::class);
$config->setUserValue(
self::TEST_TRASHBIN_USER1,
'files_trashbin',
'trashbin_size',
'1'
);

try {
$userFolder = Server::get(IRootFolder::class)->getUserFolder(self::TEST_TRASHBIN_USER1);
$file = $userFolder->newFile('too-large.txt');
$file->putContent('too large');

$this->assertTrue($userFolder->nodeExists('too-large.txt'));

// Node::delete() returns void. Since the trash move is rejected,
// Storage falls back to permanently deleting the source file.
$file->delete();

$this->assertFalse($userFolder->nodeExists('too-large.txt'));
$this->assertCount(
0,
Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER1)
);
} finally {
$config->deleteUserValue(
self::TEST_TRASHBIN_USER1,
'files_trashbin',
'trashbin_size'
);
}
}

/**
* @param string $user
* @param bool $create
Expand Down
Loading