fix(trash): share the local storage instance of a separate-storage folder - #5100
Open
solracsf wants to merge 1 commit into
Open
fix(trash): share the local storage instance of a separate-storage folder#5100solracsf wants to merge 1 commit into
solracsf wants to merge 1 commit into
Conversation
…lder Each call to getBaseStorageForFolderSeparateStorageLocal() created a new Local instance, so the files, trash and versions storages of one team folder never counted as the same storage. With ACL enabled, moving an item to or from the trash therefore skipped the rename shortcut and fell back to Common::copyFromStorage(): a recursive copy in PHP with an ACL lookup per file, followed by a recursive delete of the source. Besides being very slow for large folders (restoring a big tree hammers the database with one ACL query per file), the copy goes through the acting user's ACL view while the delete does not, so files the user cannot read were silently deleted from disk when that user trashed or restored their parent folder. Reuse one Local instance per folder so these moves are plain renames, as they already are for folders stored in the root storage. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
solracsf
added
3. to review
solracsf
requested review from
CarlSchwan,
artonge,
icewind1991 and
provokateurin
September 11, 2026 08:29
Member
Author
|
/backport to stable35 |
Member
Author
|
/backport to stable34 |
Member
Author
|
/backport to stable33 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For a team folder with its own storage,
getBaseStorageForFolderSeparateStorageLocal()builds a newLocalon every call, so the files, trash and versions storages of that folder are different objects.Common::moveFromStorage()only turns a move into a rename when both sides unwrap to the same instance, andLocal::canDoCrossStorageMove()refuses ACL wrapped sources.So, with ACL enabled, every move to or from the trash ends up as a recursive copy in PHP followed by a recursive delete of the source.
That has two effects:
Reusing one
Localper folder makes these moves renames again, the same as for folders in the root storage. Object storage isn't affected, it already only moves the metadata.Restoring a trashed folder as a user who can't read one of its files (MariaDB 11.8, local storage, min / median of 5 runs):
Before the change 5,243 of those 5,297 queries went to
oc_group_folders_acl, after it there are none.The new test fails without the change on MariaDB and PostgreSQL with local storage, and passes with it with both local storage and S3, along with the rest of
TrashBackendTest.Fix #1574