From 31fb8d0c71e8c473b01210a0a4956dfeee237141 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 7 Jan 2022 14:00:48 +0100 Subject: [PATCH 1/4] Optimize FileSystemTags workflow for groupfolder In https://github.com/nextcloud/server/pull/28774 we disabled the caching for the groupfolder application since it worked due to the fact that in groupfolders, getFileIds could be called with the same $cacheId and path for actually different groupfolders. This revert this change and instead add the folderId from the groupFolder to the cacheId. This solve the issue of the uniqueness of the cacheId inside GroupFolder. Downside is that we introduce groupfolder specific implementation inside the server repo. The seconf optimization is to not consider paths starting with __groupfolders in executeCheck. This is due to the fact that files in the groupfolder application call two times executeCheck one time with the url __groupfolder// and the other time with . The first time will always return an empty systemTags array while the second call will return the correct system tags. Signed-off-by: Carl Schwan --- .../lib/Check/FileSystemTags.php | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/apps/workflowengine/lib/Check/FileSystemTags.php b/apps/workflowengine/lib/Check/FileSystemTags.php index c9cce240eb48f..fe87d1a89d2e6 100644 --- a/apps/workflowengine/lib/Check/FileSystemTags.php +++ b/apps/workflowengine/lib/Check/FileSystemTags.php @@ -31,6 +31,7 @@ use OCP\SystemTag\TagNotFoundException; use OCP\WorkflowEngine\ICheck; use OCP\WorkflowEngine\IFileCheck; +use OC\Files\Storage\Wrapper\Wrapper; class FileSystemTags implements ICheck, IFileCheck { use TFileCheck; @@ -67,6 +68,11 @@ public function __construct(IL10N $l, ISystemTagManager $systemTagManager, ISyst * @return bool */ public function executeCheck($operator, $value) { + if (str_starts_with($this->path, '__groupfolders')) { + // System tags are always empty in this case and executeCheck is called + // a second time with the jailedPath + return false; + } $systemTags = $this->getSystemTags(); return ($operator === 'is') === in_array($value, $systemTags); } @@ -127,13 +133,29 @@ protected function getSystemTags() { * @return int[] */ protected function getFileIds(ICache $cache, $path, $isExternalStorage) { - // TODO: Fix caching inside group folders - // Do not cache file ids inside group folders because multiple file ids might be mapped to - // the same combination of cache id + path. /** @psalm-suppress InvalidArgument */ - $shouldCacheFileIds = !$this->storage->instanceOfStorage(\OCA\GroupFolders\Mount\GroupFolderStorage::class); - $cacheId = $cache->getNumericStorageId(); - if ($shouldCacheFileIds && isset($this->fileIds[$cacheId][$path])) { + if ($this->storage->instanceOfStorage(\OCA\GroupFolders\Mount\GroupFolderStorage::class)) { + static $groupFolderStorage = null; + if ($groupFolderStorage === null) { + // Special implementation for groupfolder since all groupfolder chare the same storage + // so add the group folder id in the cache key too. + $groupFolderStorage = $this->storage; + $groupFolderStoragClass = \OCA\GroupFolders\Mount\GroupFolderStorage::class; + while ($groupFolderStorage->instanceOfStorage(Wrapper::class)) { + if ($groupFolderStorage instanceof $groupFolderStoragClass) { + break; + } + /** + * @var Wrapper $sourceStorage + */ + $groupFolderStorage = $groupFolderStorage->getWrapperStorage(); + } + } + $cacheId = $cache->getNumericStorageId() . '/' . $groupFolderStorage->getFolderId(); + } else { + $cacheId = $cache->getNumericStorageId(); + } + if (isset($this->fileIds[$cacheId][$path])) { return $this->fileIds[$cacheId][$path]; } @@ -149,9 +171,7 @@ protected function getFileIds(ICache $cache, $path, $isExternalStorage) { $parentIds[] = $cache->getId($path); } - if ($shouldCacheFileIds) { - $this->fileIds[$cacheId][$path] = $parentIds; - } + $this->fileIds[$cacheId][$path] = $parentIds; return $parentIds; } From a735773d71b12805df9fe099c5b2f531df627c5d Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 7 Jan 2022 17:55:02 +0100 Subject: [PATCH 2/4] The storage is not static anymore Don't call twice $cache->getId Signed-off-by: Carl Schwan --- .../lib/Check/FileSystemTags.php | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/apps/workflowengine/lib/Check/FileSystemTags.php b/apps/workflowengine/lib/Check/FileSystemTags.php index fe87d1a89d2e6..d8377be91db7f 100644 --- a/apps/workflowengine/lib/Check/FileSystemTags.php +++ b/apps/workflowengine/lib/Check/FileSystemTags.php @@ -135,22 +135,18 @@ protected function getSystemTags() { protected function getFileIds(ICache $cache, $path, $isExternalStorage) { /** @psalm-suppress InvalidArgument */ if ($this->storage->instanceOfStorage(\OCA\GroupFolders\Mount\GroupFolderStorage::class)) { - static $groupFolderStorage = null; - if ($groupFolderStorage === null) { - // Special implementation for groupfolder since all groupfolder chare the same storage - // so add the group folder id in the cache key too. - $groupFolderStorage = $this->storage; - $groupFolderStoragClass = \OCA\GroupFolders\Mount\GroupFolderStorage::class; - while ($groupFolderStorage->instanceOfStorage(Wrapper::class)) { - if ($groupFolderStorage instanceof $groupFolderStoragClass) { - break; - } - /** - * @var Wrapper $sourceStorage - */ - $groupFolderStorage = $groupFolderStorage->getWrapperStorage(); + // Special implementation for groupfolder since all groupfolders share the same storage + // id so add the group folder id in the cache key too. + $groupFolderStorage = $this->storage; + $groupFolderStorageClass = \OCA\GroupFolders\Mount\GroupFolderStorage::class; + while ($groupFolderStorage->instanceOfStorage(Wrapper::class)) { + if ($groupFolderStorage instanceof $groupFolderStorageClass) { + break; } + /** @var Wrapper $groupFolderStorage */ + $groupFolderStorage = $groupFolderStorage->getWrapperStorage(); } + /** @psalm-suppress UndefinedMethod */ $cacheId = $cache->getNumericStorageId() . '/' . $groupFolderStorage->getFolderId(); } else { $cacheId = $cache->getNumericStorageId(); @@ -168,7 +164,7 @@ protected function getFileIds(ICache $cache, $path, $isExternalStorage) { $fileId = $cache->getId($path); if ($fileId !== -1) { - $parentIds[] = $cache->getId($path); + $parentIds[] = $fileId; } $this->fileIds[$cacheId][$path] = $parentIds; From 73e402f71517dbccf686fc9053126bc377080f72 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 10 Jan 2022 11:36:51 +0100 Subject: [PATCH 3/4] Add helper method in Wrapper Signed-off-by: Carl Schwan --- .../lib/Check/FileSystemTags.php | 17 +++++---------- lib/private/Files/Storage/Wrapper/Wrapper.php | 21 ++++++++++++++++++- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/apps/workflowengine/lib/Check/FileSystemTags.php b/apps/workflowengine/lib/Check/FileSystemTags.php index d8377be91db7f..5312c763ec70e 100644 --- a/apps/workflowengine/lib/Check/FileSystemTags.php +++ b/apps/workflowengine/lib/Check/FileSystemTags.php @@ -68,11 +68,6 @@ public function __construct(IL10N $l, ISystemTagManager $systemTagManager, ISyst * @return bool */ public function executeCheck($operator, $value) { - if (str_starts_with($this->path, '__groupfolders')) { - // System tags are always empty in this case and executeCheck is called - // a second time with the jailedPath - return false; - } $systemTags = $this->getSystemTags(); return ($operator === 'is') === in_array($value, $systemTags); } @@ -138,13 +133,11 @@ protected function getFileIds(ICache $cache, $path, $isExternalStorage) { // Special implementation for groupfolder since all groupfolders share the same storage // id so add the group folder id in the cache key too. $groupFolderStorage = $this->storage; - $groupFolderStorageClass = \OCA\GroupFolders\Mount\GroupFolderStorage::class; - while ($groupFolderStorage->instanceOfStorage(Wrapper::class)) { - if ($groupFolderStorage instanceof $groupFolderStorageClass) { - break; - } - /** @var Wrapper $groupFolderStorage */ - $groupFolderStorage = $groupFolderStorage->getWrapperStorage(); + if ($this->storage instanceof Wrapper) { + $groupFolderStorage = $this->storage->getInstanceOfStorage(\OCA\GroupFolders\Mount\GroupFolderStorage::class); + } + if ($groupFolderStorage === null) { + throw new \LogicException('Should not happen: Storage is instance of GroupFolderStorage but no group folder storage found while unwrapping.'); } /** @psalm-suppress UndefinedMethod */ $cacheId = $cache->getNumericStorageId() . '/' . $groupFolderStorage->getFolderId(); diff --git a/lib/private/Files/Storage/Wrapper/Wrapper.php b/lib/private/Files/Storage/Wrapper/Wrapper.php index 606fb3e24ee7c..94c497793ac73 100644 --- a/lib/private/Files/Storage/Wrapper/Wrapper.php +++ b/lib/private/Files/Storage/Wrapper/Wrapper.php @@ -487,7 +487,7 @@ public function isLocal() { /** * Check if the storage is an instance of $class or is a wrapper for a storage that is an instance of $class * - * @param string $class + * @param class-string $class * @return bool */ public function instanceOfStorage($class) { @@ -498,6 +498,25 @@ public function instanceOfStorage($class) { return is_a($this, $class) or $this->getWrapperStorage()->instanceOfStorage($class); } + /** + * @template T of IStorage + * @param class-string $class + * @return ?T + */ + public function getInstanceOfStorage(string $class): ?IStorage { + $storage = $this; + while ($storage->instanceOfStorage(Wrapper::class)) { + if ($storage instanceof $class) { + break; + } + $storage = $storage->getWrapperStorage(); + } + if (!is_a($storage, $class)) { + return null; + } + return $storage; + } + /** * Pass any methods custom to specific storage implementations to the wrapped storage * From 396157af18370a4dd8114a958c5cfe2da6f50393 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Thu, 13 Jan 2022 12:30:27 +0100 Subject: [PATCH 4/4] Fix psalm issues Signed-off-by: Carl Schwan --- apps/workflowengine/lib/Check/FileSystemTags.php | 5 ++++- lib/private/Files/Storage/Wrapper/Wrapper.php | 12 ++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/apps/workflowengine/lib/Check/FileSystemTags.php b/apps/workflowengine/lib/Check/FileSystemTags.php index 5312c763ec70e..944552bef5958 100644 --- a/apps/workflowengine/lib/Check/FileSystemTags.php +++ b/apps/workflowengine/lib/Check/FileSystemTags.php @@ -139,7 +139,10 @@ protected function getFileIds(ICache $cache, $path, $isExternalStorage) { if ($groupFolderStorage === null) { throw new \LogicException('Should not happen: Storage is instance of GroupFolderStorage but no group folder storage found while unwrapping.'); } - /** @psalm-suppress UndefinedMethod */ + /** + * @psalm-suppress UndefinedDocblockClass + * @psalm-suppress UndefinedInterfaceMethod + */ $cacheId = $cache->getNumericStorageId() . '/' . $groupFolderStorage->getFolderId(); } else { $cacheId = $cache->getNumericStorageId(); diff --git a/lib/private/Files/Storage/Wrapper/Wrapper.php b/lib/private/Files/Storage/Wrapper/Wrapper.php index 94c497793ac73..586262ddab46d 100644 --- a/lib/private/Files/Storage/Wrapper/Wrapper.php +++ b/lib/private/Files/Storage/Wrapper/Wrapper.php @@ -499,19 +499,19 @@ public function instanceOfStorage($class) { } /** - * @template T of IStorage - * @param class-string $class - * @return ?T + * @psalm-template T of IStorage + * @psalm-param class-string $class + * @psalm-return T|null */ - public function getInstanceOfStorage(string $class): ?IStorage { + public function getInstanceOfStorage(string $class) { $storage = $this; - while ($storage->instanceOfStorage(Wrapper::class)) { + while ($storage instanceof Wrapper) { if ($storage instanceof $class) { break; } $storage = $storage->getWrapperStorage(); } - if (!is_a($storage, $class)) { + if (!($storage instanceof $class)) { return null; } return $storage;