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 c9cce240eb4..fe87d1a89d2 100644 --- a/apps/workflowengine/lib/Check/FileSystemTags.php +++ b/apps/workflowengine/lib/Check/FileSystemTags.php @@ -31,6 +31,7 @@ use OCP\SystemTag\ISystemTagObjectMapper; 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 @@ class FileSystemTags implements ICheck, IFileCheck { * @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 @@ class FileSystemTags implements ICheck, IFileCheck { * @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 @@ class FileSystemTags implements ICheck, IFileCheck { $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 fe87d1a89d2..d8377be91db 100644 --- a/apps/workflowengine/lib/Check/FileSystemTags.php +++ b/apps/workflowengine/lib/Check/FileSystemTags.php @@ -135,22 +135,18 @@ class FileSystemTags implements ICheck, IFileCheck { 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 @@ class FileSystemTags implements ICheck, IFileCheck { $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 d8377be91db..5312c763ec7 100644 --- a/apps/workflowengine/lib/Check/FileSystemTags.php +++ b/apps/workflowengine/lib/Check/FileSystemTags.php @@ -68,11 +68,6 @@ class FileSystemTags implements ICheck, IFileCheck { * @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 @@ class FileSystemTags implements ICheck, IFileCheck { // 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 606fb3e24ee..94c497793ac 100644 --- a/lib/private/Files/Storage/Wrapper/Wrapper.php +++ b/lib/private/Files/Storage/Wrapper/Wrapper.php @@ -487,7 +487,7 @@ class Wrapper implements \OC\Files\Storage\Storage, ILockingStorage, IWriteStrea /** * 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 @@ class Wrapper implements \OC\Files\Storage\Storage, ILockingStorage, IWriteStrea 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 5312c763ec7..944552bef59 100644 --- a/apps/workflowengine/lib/Check/FileSystemTags.php +++ b/apps/workflowengine/lib/Check/FileSystemTags.php @@ -139,7 +139,10 @@ class FileSystemTags implements ICheck, IFileCheck { 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 94c497793ac..586262ddab4 100644 --- a/lib/private/Files/Storage/Wrapper/Wrapper.php +++ b/lib/private/Files/Storage/Wrapper/Wrapper.php @@ -499,19 +499,19 @@ class Wrapper implements \OC\Files\Storage\Storage, ILockingStorage, IWriteStrea } /** - * @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;