Skip to content

fix(api): fix performance issue on PROPFIND api #4676

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

Open
tpelingan wants to merge 1 commit into
base: master
Choose a base branch
from
+24 −13
Open

fix(api): fix performance issue on PROPFIND api #4676

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 24 additions & 13 deletions lib/Mount/FolderStorageManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ class FolderStorageManager {
private readonly bool $enableEncryption;
/** @var array<string, Folder> */
private array $cachedFolders = [];
/** @var array<int, IStorage> */
private array $cachedSeparateStorages = [];

public function __construct(
private readonly IRootFolder $rootFolder,
Expand Down Expand Up @@ -97,16 +99,22 @@ private function getBaseStorageForFolderSeparate(
bool $init = false,
array $options = [],
): IStorage {
if ($this->primaryObjectStoreConfig->hasObjectStore()) {
// Cache the raw Local/ObjectStoreStorage instance per folder ID within a request.
// The underlying storage is the same regardless of user or type — only the Jail root and ACL wrapper differ.
if (isset($this->cachedSeparateStorages[$folderId])) {
$storage = $this->cachedSeparateStorages[$folderId];
} elseif ($this->primaryObjectStoreConfig->hasObjectStore()) {
$bucket = $options['bucket'] ?? null;
if ($bucket !== null && !is_string($bucket)) {
throw new Exception('bucket is not a string.');
}
/** @var Storage $storage */
$storage = $this->getBaseStorageForFolderSeparateStorageObject($folderId, $init, $bucket);
$this->cachedSeparateStorages[$folderId] = $storage;
} else {
/** @var Storage $storage */
$storage = $this->getBaseStorageForFolderSeparateStorageLocal($folderId, $init);
$this->cachedSeparateStorages[$folderId] = $storage;
}

if ($folder?->acl && $user) {
Expand Down Expand Up @@ -232,17 +240,21 @@ private function getBaseStorageForFolderRootJail(
}
}

try {
/** @var Folder $storageFolder */
$storageFolder = $parentFolder->get((string)$folderId);
} catch (NotFoundException) {
$storageFolder = $parentFolder->newFolder((string)$folderId);
}
/** @var Storage $rootStorage */
$rootStorage = $storageFolder->getStorage();
$rootPath = $storageFolder->getInternalPath();

// apply acl before jail, trash doesn't get the ACL wrapper as it does its own ACL filtering
// The root storage is the same for ALL non-separate groupfolders.
$rootStorage = $parentFolder->getStorage();

// The internal path is deterministic:
// files: __groupfolders/{folderId}
// trash: __groupfolders/trash/{folderId}
// versions: __groupfolders/versions/{folderId}
// This avoids an N+1 query problem where each folder previously triggered
// a separate SELECT on oc_filecache via parentFolder->get(folderId).
$basePath = $parentFolder->getInternalPath();
Comment on lines +251 to +252
Copy link
Copy Markdown
Member

@provokateurin provokateurin May 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, but this is needed in order to create the folder if it is missing, so you can't remove it.

Copy link
Copy Markdown
Author

@tpelingan tpelingan May 20, 2026

Choose a reason for hiding this comment

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

interesting. what if we do a query only if the folder is missing?

Copy link
Copy Markdown
Author

@tpelingan tpelingan May 20, 2026

Choose a reason for hiding this comment

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

and how do you suggest that we fix the N+1 query issue introduced here on version 20?

Copy link
Copy Markdown
Member

@provokateurin provokateurin May 21, 2026

Choose a reason for hiding this comment

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

But you can't know if the folder exists or not without doing a query.

A solution might be to create these subfolders when the groupfolder is created, but I'm not even sure if that would make it work.

Copy link
Copy Markdown
Author

@tpelingan tpelingan May 23, 2026

Choose a reason for hiding this comment

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

I see. Can we not make the GET PROPFIND API be purely read only? I'm a bit surprised that these operations need to happen. Is there a specific reason or can we maybe remove the folder creation logic?

$rootPath = $type !== 'files' && $type !== ''
? $basePath . '/' . $type . '/' . $folderId
: $basePath . '/' . $folderId;

// Apply ACL before jail. Trash uses its own ACL filtering, so skip it there.
if ($folder && $folder->acl && $user && $type !== 'trash') {
$aclManager = $this->aclManagerFactory->getACLManager($user);
$rootStorage = new ACLStorageWrapper([
Expand All @@ -260,7 +272,6 @@ private function getBaseStorageForFolderRootJail(
'root' => $rootPath,
]);
}

return new Jail([
'storage' => $rootStorage,
'root' => $rootPath,
Expand Down