From 1f392d666af950da2209f1cf346885c991183d81 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 c5f32bbb4e7..f2b9b2a186b 100644 --- a/apps/workflowengine/lib/Check/FileSystemTags.php +++ b/apps/workflowengine/lib/Check/FileSystemTags.php @@ -36,6 +36,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; @@ -72,6 +73,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); } @@ -132,13 +138,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]; } @@ -154,9 +176,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 0479fff37cb08157d982c593ce21fb79957e8574 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 f2b9b2a186b..b6ad95a4ab6 100644 --- a/apps/workflowengine/lib/Check/FileSystemTags.php +++ b/apps/workflowengine/lib/Check/FileSystemTags.php @@ -140,22 +140,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(); @@ -173,7 +169,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 695165260f26b2f5b5584e619e54741db1a2d9cc 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 b6ad95a4ab6..d3f02729f2f 100644 --- a/apps/workflowengine/lib/Check/FileSystemTags.php +++ b/apps/workflowengine/lib/Check/FileSystemTags.php @@ -73,11 +73,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); } @@ -143,13 +138,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 5faffece67e..b44db7c13ce 100644 --- a/lib/private/Files/Storage/Wrapper/Wrapper.php +++ b/lib/private/Files/Storage/Wrapper/Wrapper.php @@ -486,7 +486,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) { @@ -497,6 +497,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 cbf9064b8ecde6f497146f6711fff83307a0730f 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 d3f02729f2f..008f47eca78 100644 --- a/apps/workflowengine/lib/Check/FileSystemTags.php +++ b/apps/workflowengine/lib/Check/FileSystemTags.php @@ -144,7 +144,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 b44db7c13ce..6bc66bf9c89 100644 --- a/lib/private/Files/Storage/Wrapper/Wrapper.php +++ b/lib/private/Files/Storage/Wrapper/Wrapper.php @@ -498,19 +498,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;