Skip to content

perf: Don't fetch all the trash items when deleting a single item#4875

Open
CarlSchwan wants to merge 3 commits into
masterfrom
carl/trashitem-delete-optimization
Open

perf: Don't fetch all the trash items when deleting a single item#4875
CarlSchwan wants to merge 3 commits into
masterfrom
carl/trashitem-delete-optimization

Conversation

@CarlSchwan

Copy link
Copy Markdown
Member

🤖 AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@CarlSchwan CarlSchwan marked this pull request as draft July 8, 2026 09:38
Signed-off-by: Carl Schwan <carlschwan@kde.org>
@CarlSchwan CarlSchwan force-pushed the carl/trashitem-delete-optimization branch from 3935d67 to ed49e0f Compare July 9, 2026 08:39
Comment on lines +510 to +518
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;

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

Signed-off-by: Carl Schwan <carlschwan@kde.org>
Signed-off-by: Carl Schwan <carlschwan@kde.org>
@CarlSchwan CarlSchwan marked this pull request as ready for review July 9, 2026 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants