Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions .github/workflows/phpunit-mysql.yml
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ jobs:
submodules: true
repository: nextcloud/server
ref: ${{ matrix.server-versions }}
branch: "carl/trashitem-delete-optimization"

- name: Checkout Circles
uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7
Expand Down
79 changes: 79 additions & 0 deletions lib/Trash/TrashBackend.php
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ public function listTrashRoot(IUser $user): array {
return $this->getTrashForFolders($user, $folders);
}

public function getTrashRootItem(IUser $user, string $name): ?ITrashItem {
$folders = $this->folderManager->getFoldersForUser($user);

return $this->getTrashItemForFolders($user, $folders, $name);
}

/**
* @return list<ITrashItem>
*/
Expand Down Expand Up @@ -488,6 +494,79 @@ private function getTrashForFolders(IUser $user, array $folders): array {
return array_values(array_merge(...$items));
}

/**
* @param list<FolderDefinitionWithPermissions> $folders
*/
private function getTrashItemForFolders(IUser $user, array $folders, string $name): ?ITrashItem {
$folderIds = array_map(fn (FolderDefinitionWithPermissions $folder): int => $folder->id, $folders);
$strippedName = pathinfo($name, PATHINFO_FILENAME);
$rows = $this->trashManager->listTrashItemForFolders($folderIds, $strippedName);
$indexedRows = [];
foreach ($rows as $row) {
$key = $row['folder_id'] . '/' . $row['name'] . '/' . $row['deleted_time'];
$indexedRows[$key] = $row;
}

foreach ($folders as $folder) {
// note that we explicitly don't pass the user here, was we need to get all trash items,
// not only the trash items we have access to (so we can get their original paths)
// we apply acl filtering later to get the correct permissions again
$trashFolder = $this->setupTrashFolder($folder);
try {
$item = $trashFolder->get($name);
} catch (NotFoundException) {
continue;
Comment on lines +513 to +521

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to figure out the correct folder for an item name without iterating all folders? If that isn't possible, then iterating the folder is a problem if a name is not unique to all folders (which could result in loading an item from the wrong folder).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to go back to the thinking phase :(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So tested a bit more and the current code works fine in this situation. This is because $name include the deletion timestamp so the risk of collision is very small.

The code in master has the same collision issue as if two files with the same name are deleted in the same second, we returns two items and delete the first one

}

$userCanManageAcl = $this->folderManager->canManageACL($folder->id, $user, true);

$pathParts = pathinfo($item->getName());
$timestamp = (int)substr($pathParts['extension'] ?? '', 1);
$itemName = $pathParts['filename'];
$key = $folder->id . '/' . $itemName . '/' . $timestamp;

$originalLocation = $indexedRows[$key]['original_location'] ?? '';
$deletedBy = $indexedRows[$key]['deleted_by'] ?? '';

$trashItem = new GroupTrashItem(
$this,
$originalLocation,
$timestamp,
'/' . $folder->id . '/' . $item->getName(),
$item,
$user,
$folder->mountPoint,
$this->userManager->get($deletedBy),
$folder,
);
$originalLocation = $trashItem->getOriginalLocation();

// perform per-item ACL checks if the user doesn't have manage permissions
if ($folder->acl && !$userCanManageAcl) {
$this->aclManagerFactory->getACLManager($user)->preloadRulesForFolder($folder->storageId, $trashFolder->getId());
// if we for any reason lost track of the original location, hide the item for non-managers as a fail-safe
if ($trashItem->getInternalOriginalLocation() === '') {
continue;
}

if (!$this->userHasAccessToItem($trashItem)) {
continue;
}

// if a parent of the original location has also been deleted, we also need to check it based on the now-deleted parent path
foreach ($this->getDeletedParentOriginalPaths($trashItem->getOriginalLocation(), [$originalLocation => $trashItem]) as $parentItem) {
$pathInsideParentItem = dirname(substr($trashItem->getInternalOriginalLocation(), strlen($parentItem->getInternalOriginalLocation())));
if (!$this->userHasAccessToItem($parentItem, Constants::PERMISSION_READ, $pathInsideParentItem)) {
continue 2;
}
}
}
return $trashItem;
}

return null;
}

/**
* @param array<string, GroupTrashItem> $trashItemsByOriginalPath
* @return list<GroupTrashItem>
Expand Down
26 changes: 26 additions & 0 deletions lib/Trash/TrashManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,32 @@ public function __construct(
) {
}

/**
* @param int[] $folderIds
* @return list<array{trash_id: int, name: string, deleted_time: int, original_location: string, folder_id: int, file_id: ?int, deleted_by: ?string}>
*/
public function listTrashItemForFolders(array $folderIds, string $name): array {
$query = $this->connection->getQueryBuilder();

$query->select(['trash_id', 'name', 'deleted_time', 'original_location', 'folder_id', 'file_id', 'deleted_by'])
->from('group_folders_trash')
->where($query->expr()->in('folder_id', $query->createNamedParameter($folderIds, IQueryBuilder::PARAM_INT_ARRAY)))
->andWhere($query->expr()->eq('name', $query->createNamedParameter($name)));

/** @var list<array{trash_id: int|string, name: string, deleted_time: int|string, original_location: string, folder_id: int|string, file_id: null|int|string, deleted_by: ?string}> $rows */
$rows = $query->executeQuery()->fetchAll();

return array_map(fn (array $row): array => [
'trash_id' => (int)$row['trash_id'],
'name' => $row['name'],
'deleted_time' => (int)$row['deleted_time'],
'original_location' => $row['original_location'],
'folder_id' => (int)$row['folder_id'],
'file_id' => $row['file_id'] !== null ? (int)$row['file_id'] : null,
'deleted_by' => $row['deleted_by'] ?? null,
], $rows);
}

/**
* @param int[] $folderIds
* @return list<array{trash_id: int, name: string, deleted_time: int, original_location: string, folder_id: int, file_id: ?int, deleted_by: ?string}>
Expand Down
Loading