Skip to content

fix(trashbin): Fix n+1 issue in propfind in trash root #54317

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

Jump to bottom
Open
CarlSchwan wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(trashbin): Fix n+1 issue in propfind in trash root #54317

CarlSchwan wants to merge 3 commits into from
+207 −11

Conversation

@CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Aug 7, 2025 •
edited
Loading

Summary

CustomPropertiesBackend is already able to cache the custom properties of a folder content. But the trash root is not a Directory but instead just a ICollection, so extend CacheEntry to also handle a TrashEntry.

DB queries before (with around 30 entries in trash, around half from a groupfolder):

image

DB queries after:

image

TODO

Checklist

@CarlSchwan CarlSchwan requested a review from a team as a code owner August 7, 2025 10:53
@CarlSchwan CarlSchwan requested review from come-nc, icewind1991 and nfebe and removed request for a team August 7, 2025 10:53
CarlSchwan
CarlSchwan commented Aug 7, 2025
View reviewed changes
apps/dav/lib/DAV/CustomPropertiesBackend.php Outdated
use OCA\DAV\CalDAV\DefaultCalendarValidator;
use OCA\DAV\Connector\Sabre\Directory;
use OCA\DAV\Connector\Sabre\FilesPlugin;
use OCA\Files_Trashbin\Sabre\TrashRoot;
Copy link
Member Author

@CarlSchwan CarlSchwan Aug 7, 2025

Choose a reason for hiding this comment

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

I don't really like depending on Files_Trashbin in the dav app, maybe worth creating a new interface that both TrashRoot and Directory inherit? Any name suggestion?

Copy link
Contributor

@come-nc come-nc Aug 7, 2025

Choose a reason for hiding this comment

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

ICachableCollection?

Copy link
Member

@icewind1991 icewind1991 Aug 11, 2025

Choose a reason for hiding this comment

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

IMergedCollection? since it is about a dav collection that is created from merging multiple collections

@CarlSchwan CarlSchwan added the 2. developing Work in progress label Aug 7, 2025
@CarlSchwan
Copy link
Member Author

CarlSchwan commented Aug 7, 2025

This doesn't work with the group folder app as the content of the trash is stored in __groupfolders/trash/<group-folder-id>/

@solracsf solracsf modified the milestone: Nextcloud 32 Aug 7, 2025
come-nc
come-nc reviewed Aug 7, 2025
View reviewed changes
apps/dav/lib/DAV/CustomPropertiesBackend.php Outdated
use OCA\DAV\CalDAV\DefaultCalendarValidator;
use OCA\DAV\Connector\Sabre\Directory;
use OCA\DAV\Connector\Sabre\FilesPlugin;
use OCA\Files_Trashbin\Sabre\TrashRoot;
Copy link
Contributor

@come-nc come-nc Aug 7, 2025

Choose a reason for hiding this comment

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

ICachableCollection?

@come-nc
Copy link
Contributor

come-nc commented Aug 7, 2025

This doesn't work with the group folder app as the content of the trash is stored in __groupfolders/trash/<group-folder-id>/

So it breaks stuff with groupfolders?

@CarlSchwan
Copy link
Member Author

CarlSchwan commented Aug 7, 2025

This doesn't work with the group folder app as the content of the trash is stored in __groupfolders/trash/<group-folder-id>/

So it breaks stuff with groupfolders?

doesn't break it but also doesn't fix the similar issue there...

I'm trying to rethink a bit how to do it so that it works for every trash backend

@CarlSchwan CarlSchwan marked this pull request as draft August 7, 2025 13:35
@CarlSchwan CarlSchwan force-pushed the scale-propfind-trash branch from e574092 to 9df9b4a Compare August 8, 2025 08:43
@CarlSchwan CarlSchwan mentioned this pull request Aug 8, 2025
Open
@CarlSchwan CarlSchwan added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 8, 2025
@CarlSchwan CarlSchwan marked this pull request as ready for review August 8, 2025 08:48
@CarlSchwan
Copy link
Member Author

CarlSchwan commented Aug 8, 2025

This doesn't work with the group folder app as the content of the trash is stored in __groupfolders/trash/<group-folder-id>/

So it breaks stuff with groupfolders?

doesn't break it but also doesn't fix the similar issue there...

I'm trying to rethink a bit how to do it so that it works for every trash backend

Found a solution, this adds a method to the ITrashBackend branch. I'm not sure it is allowed as it is an API break but then it's also in OCA and not OCP and we release the groupfolder app at the same time as server. Worst case, I can just add an interface :)

@CarlSchwan CarlSchwan force-pushed the scale-propfind-trash branch 2 times, most recently from 9c07e5a to 0d37a55 Compare August 11, 2025 08:41
icewind1991
icewind1991 reviewed Aug 11, 2025
View reviewed changes
apps/files_trashbin/lib/Trash/ITrashBackend.php Outdated
* @return Folder[]
* @since 32.0.0
*/
public function getTrashRootsForUser(IUser $user): array;
Copy link
Member

@icewind1991 icewind1991 Aug 11, 2025

Choose a reason for hiding this comment

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

This would encode further assumptions about how trash items are stored. If we ever want to expose "deleted but versioned" s3 objects or deleted items from filesystem snapshots this could be problematic.

Copy link
Member Author

@CarlSchwan CarlSchwan Aug 21, 2025

Choose a reason for hiding this comment

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

Right, I wonder if there is a better way to do it. the list of Folder is really only used for pre-caching them and does not necessarily need to be exhaustive for this usecase

@github-actions

This comment was marked as off-topic.

Sign in to view
@CarlSchwan CarlSchwan force-pushed the scale-propfind-trash branch 2 times, most recently from 3938b32 to 684df76 Compare August 22, 2025 10:10
come-nc
come-nc reviewed Aug 25, 2025
View reviewed changes
Carl Schwan added 3 commits September 12, 2025 16:16
@CarlSchwan
fix(trashbin): Fix n+1 issue in propfind in trash root
888f419 
CustomPropertiesBackend is already able to cache the custom properties
of a folder content. But the trash root is not a Directory but instead
just a ICollection, so extend CacheEntry to also handle a TrashEntry.

Signed-off-by: Carl Schwan <carl.schwan@nextclound.com>
@CarlSchwan
refactor(trash): Introduce ICacheableDirectory interface
569c073 
That interface can be implemented by any ICollection that is based on
one or multiple \OCP\Files\Folder to pre-fetch the custom properties.

Signed-off-by: Carl Schwan <carl.schwan@nextclound.com>
@CarlSchwan
feat(trashbin): Use a lazy folder for roots of trashbin
a015a44 
Introduze a LazyMountPoint which is similar to LazyFile/LazyFolder and
allow to avoid doing DB request when we just want the storage id and we
already have it.

Signed-off-by: Carl Schwan <carl.schwan@nextclound.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@icewind1991 icewind1991 icewind1991 left review comments

@come-nc come-nc come-nc approved these changes

@nfebe nfebe Awaiting requested review from nfebe nfebe is a code owner automatically assigned from nextcloud/server-backend

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

3. to review Waiting for reviews feedback-requested performance 🚀

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@CarlSchwan @come-nc @icewind1991 @solracsf