From 6fed53dee89956ba8cfabd42b77663c6027f43ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 13 Feb 2023 14:30:40 +0100 Subject: [PATCH 1/4] fix: Make sure that rollback hook is triggered on all version backends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- apps/files_versions/lib/Storage.php | 6 ------ apps/files_versions/lib/Versions/VersionManager.php | 8 +++++++- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/apps/files_versions/lib/Storage.php b/apps/files_versions/lib/Storage.php index 75ca7e4bceef0..8fddf589a44a9 100644 --- a/apps/files_versions/lib/Storage.php +++ b/apps/files_versions/lib/Storage.php @@ -417,12 +417,6 @@ public static function rollback(string $file, int $revision, IUser $user) { $node = $userFolder->get($file); - // TODO: move away from those legacy hooks! - \OC_Hook::emit('\OCP\Versions', 'rollback', [ - 'path' => $filename, - 'revision' => $revision, - 'node' => $node, - ]); return true; } elseif ($versionCreated) { self::deleteVersion($users_view, $version); diff --git a/apps/files_versions/lib/Versions/VersionManager.php b/apps/files_versions/lib/Versions/VersionManager.php index bfae0937df81d..c11b429202bdf 100644 --- a/apps/files_versions/lib/Versions/VersionManager.php +++ b/apps/files_versions/lib/Versions/VersionManager.php @@ -94,7 +94,13 @@ public function createVersion(IUser $user, FileInfo $file) { public function rollback(IVersion $version) { $backend = $version->getBackend(); - return $backend->rollback($version); + $result = $backend->rollback($version); + \OC_Hook::emit('\OCP\Versions', 'rollback', [ + 'path' => \OC\Files\Filesystem::getView()->getRelativePath($version->getSourceFile()->getPath()), + 'revision' => $version->getRevisionId(), + 'node' => $version->getSourceFile(), + ]); + return $result; } public function read(IVersion $version) { From ce4269925ba83db1de887d4edf2521c15d649041 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 14 Feb 2023 12:38:51 +0100 Subject: [PATCH 2/4] tests(files_versions): Tear down fs to clear mount cache before testing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- apps/files_versions/lib/Versions/VersionManager.php | 2 +- apps/files_versions/tests/VersioningTest.php | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/apps/files_versions/lib/Versions/VersionManager.php b/apps/files_versions/lib/Versions/VersionManager.php index c11b429202bdf..5a2480d623331 100644 --- a/apps/files_versions/lib/Versions/VersionManager.php +++ b/apps/files_versions/lib/Versions/VersionManager.php @@ -96,7 +96,7 @@ public function rollback(IVersion $version) { $backend = $version->getBackend(); $result = $backend->rollback($version); \OC_Hook::emit('\OCP\Versions', 'rollback', [ - 'path' => \OC\Files\Filesystem::getView()->getRelativePath($version->getSourceFile()->getPath()), + 'path' => $version->getVersionPath(), 'revision' => $version->getRevisionId(), 'node' => $version->getSourceFile(), ]); diff --git a/apps/files_versions/tests/VersioningTest.php b/apps/files_versions/tests/VersioningTest.php index 4f171031ab313..547f1eca4a6e9 100644 --- a/apps/files_versions/tests/VersioningTest.php +++ b/apps/files_versions/tests/VersioningTest.php @@ -37,6 +37,7 @@ use OC\Files\Storage\Temporary; use OCA\Files_Versions\Db\VersionEntity; use OCA\Files_Versions\Db\VersionsMapper; +use OCA\Files_Versions\Versions\IVersionManager; use OCP\Files\IMimeTypeLoader; use OCP\IConfig; use OCP\IUser; @@ -661,6 +662,7 @@ public function testRestoreSameStorage() { public function testRestoreCrossStorage() { $storage2 = new Temporary([]); \OC\Files\Filesystem::mount($storage2, [], self::TEST_VERSIONS_USER . '/files/sub'); + \OC\Files\Filesystem::tearDown(); $this->doTestRestore(); } @@ -822,7 +824,12 @@ private function doTestRestore() { $params = []; $this->connectMockHooks('rollback', $params); - $this->assertTrue(\OCA\Files_Versions\Storage::rollback('sub/test.txt', $t2, $this->user1)); + $versionManager = \OCP\Server::get(IVersionManager::class); + $versions = $versionManager->getVersionsForFile($this->user1, $info1); + $version = array_filter($versions, function ($version) use ($t2) { + return $version->getRevisionId() === $t2; + }); + $this->assertTrue($versionManager->rollback(current($version))); $expectedParams = [ 'path' => '/sub/test.txt', ]; From 13340114699d07044aba67aa1f725ccd76f3162e Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 10 Mar 2023 17:10:06 +0100 Subject: [PATCH 3/4] don't re-get fileinfo for versioned file if it's not shared Signed-off-by: Robin Appelman --- .../lib/Versions/LegacyVersionsBackend.php | 30 +++++++++++-------- apps/files_versions/tests/VersioningTest.php | 1 - 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/apps/files_versions/lib/Versions/LegacyVersionsBackend.php b/apps/files_versions/lib/Versions/LegacyVersionsBackend.php index cbfbc001e0c64..0ad0eb8a439e9 100644 --- a/apps/files_versions/lib/Versions/LegacyVersionsBackend.php +++ b/apps/files_versions/lib/Versions/LegacyVersionsBackend.php @@ -69,13 +69,17 @@ public function getVersionsForFile(IUser $user, FileInfo $file): array { if ($storage->instanceOfStorage(SharedStorage::class)) { $owner = $storage->getOwner(''); $user = $this->userManager->get($owner); - } - $userFolder = $this->rootFolder->getUserFolder($user->getUID()); - $nodes = $userFolder->getById($file->getId()); - $file2 = array_pop($nodes); + $userFolder = $this->rootFolder->getUserFolder($user->getUID()); + $nodes = $userFolder->getById($file->getId()); + $file = array_pop($nodes); + + if (!$file) { + throw new NotFoundException("version file not found for share owner"); + } + } - $versions = $this->getVersionsForFileFromDB($file2, $user); + $versions = $this->getVersionsForFileFromDB($file, $user); if (count($versions) > 0) { return $versions; @@ -83,18 +87,18 @@ public function getVersionsForFile(IUser $user, FileInfo $file): array { // Insert the entry in the DB for the current version. $versionEntity = new VersionEntity(); - $versionEntity->setFileId($file2->getId()); - $versionEntity->setTimestamp($file2->getMTime()); - $versionEntity->setSize($file2->getSize()); - $versionEntity->setMimetype($this->mimeTypeLoader->getId($file2->getMimetype())); + $versionEntity->setFileId($file->getId()); + $versionEntity->setTimestamp($file->getMTime()); + $versionEntity->setSize($file->getSize()); + $versionEntity->setMimetype($this->mimeTypeLoader->getId($file->getMimetype())); $versionEntity->setMetadata([]); $this->versionsMapper->insert($versionEntity); // Insert entries in the DB for existing versions. - $versionsOnFS = Storage::getVersions($user->getUID(), $userFolder->getRelativePath($file2->getPath())); + $versionsOnFS = Storage::getVersions($user->getUID(), $userFolder->getRelativePath($file->getPath())); foreach ($versionsOnFS as $version) { $versionEntity = new VersionEntity(); - $versionEntity->setFileId($file2->getId()); + $versionEntity->setFileId($file->getId()); $versionEntity->setTimestamp((int)$version['version']); $versionEntity->setSize((int)$version['size']); $versionEntity->setMimetype($this->mimeTypeLoader->getId($version['mimetype'])); @@ -102,13 +106,13 @@ public function getVersionsForFile(IUser $user, FileInfo $file): array { $this->versionsMapper->insert($versionEntity); } - return $this->getVersionsForFileFromDB($file2, $user); + return $this->getVersionsForFileFromDB($file, $user); } /** * @return IVersion[] */ - private function getVersionsForFileFromDB(Node $file, IUser $user): array { + private function getVersionsForFileFromDB(FileInfo $file, IUser $user): array { $userFolder = $this->rootFolder->getUserFolder($user->getUID()); return array_map( diff --git a/apps/files_versions/tests/VersioningTest.php b/apps/files_versions/tests/VersioningTest.php index 547f1eca4a6e9..f454ba30b86c3 100644 --- a/apps/files_versions/tests/VersioningTest.php +++ b/apps/files_versions/tests/VersioningTest.php @@ -662,7 +662,6 @@ public function testRestoreSameStorage() { public function testRestoreCrossStorage() { $storage2 = new Temporary([]); \OC\Files\Filesystem::mount($storage2, [], self::TEST_VERSIONS_USER . '/files/sub'); - \OC\Files\Filesystem::tearDown(); $this->doTestRestore(); } From 75fc65e0e6b44e931404712983731afb5ca9429f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 13 Mar 2023 10:57:12 +0100 Subject: [PATCH 4/4] fix: Check return type on rollback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- apps/files_versions/lib/Versions/VersionManager.php | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/apps/files_versions/lib/Versions/VersionManager.php b/apps/files_versions/lib/Versions/VersionManager.php index 5a2480d623331..b3b6d6f85e8fd 100644 --- a/apps/files_versions/lib/Versions/VersionManager.php +++ b/apps/files_versions/lib/Versions/VersionManager.php @@ -95,11 +95,14 @@ public function createVersion(IUser $user, FileInfo $file) { public function rollback(IVersion $version) { $backend = $version->getBackend(); $result = $backend->rollback($version); - \OC_Hook::emit('\OCP\Versions', 'rollback', [ - 'path' => $version->getVersionPath(), - 'revision' => $version->getRevisionId(), - 'node' => $version->getSourceFile(), - ]); + // rollback doesn't have a return type yet and some implementations don't return anything + if ($result === null || $result === true) { + \OC_Hook::emit('\OCP\Versions', 'rollback', [ + 'path' => $version->getVersionPath(), + 'revision' => $version->getRevisionId(), + 'node' => $version->getSourceFile(), + ]); + } return $result; }