From aaaf9c7e8c144f430c98dacc9388f8a9cfdad39c 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/6] 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 2938accddcc44..ac440650d6263 100644 --- a/apps/files_versions/lib/Storage.php +++ b/apps/files_versions/lib/Storage.php @@ -404,12 +404,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 7642537e0b6e7..f60d5e63fd0bb 100644 --- a/apps/files_versions/lib/Versions/VersionManager.php +++ b/apps/files_versions/lib/Versions/VersionManager.php @@ -99,7 +99,13 @@ public function createVersion(IUser $user, FileInfo $file) { public function rollback(IVersion $version) { $backend = $version->getBackend(); - return self::handleAppLocks(fn(): ?bool => $backend->rollback($version)); + $result = self::handleAppLocks(fn(): ?bool => $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 2741810c7289f2200cd000c0dee8eef20e637fc7 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/6] 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 | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/apps/files_versions/lib/Versions/VersionManager.php b/apps/files_versions/lib/Versions/VersionManager.php index f60d5e63fd0bb..2cd5cc5b1f804 100644 --- a/apps/files_versions/lib/Versions/VersionManager.php +++ b/apps/files_versions/lib/Versions/VersionManager.php @@ -101,7 +101,7 @@ public function rollback(IVersion $version) { $backend = $version->getBackend(); $result = self::handleAppLocks(fn(): ?bool => $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 b8f58ecff2b14..87ae694904a5f 100644 --- a/apps/files_versions/tests/VersioningTest.php +++ b/apps/files_versions/tests/VersioningTest.php @@ -647,6 +647,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(); } @@ -789,7 +790,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 69998422d506e10f405c8065bd87e67773793497 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 10 Mar 2023 17:10:06 +0100 Subject: [PATCH 3/6] don't re-get fileinfo for versioned file if it's not shared Signed-off-by: Robin Appelman --- .../files_versions/lib/Versions/LegacyVersionsBackend.php | 8 ++++++++ apps/files_versions/tests/VersioningTest.php | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/apps/files_versions/lib/Versions/LegacyVersionsBackend.php b/apps/files_versions/lib/Versions/LegacyVersionsBackend.php index 4ea0985e11346..f19dae723e324 100644 --- a/apps/files_versions/lib/Versions/LegacyVersionsBackend.php +++ b/apps/files_versions/lib/Versions/LegacyVersionsBackend.php @@ -58,6 +58,14 @@ 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()); + $file = array_pop($nodes); + + if (!$file) { + throw new NotFoundException("version file not found for share owner"); + } } $userFolder = $this->rootFolder->getUserFolder($user->getUID()); diff --git a/apps/files_versions/tests/VersioningTest.php b/apps/files_versions/tests/VersioningTest.php index 87ae694904a5f..f2319fdeba960 100644 --- a/apps/files_versions/tests/VersioningTest.php +++ b/apps/files_versions/tests/VersioningTest.php @@ -38,6 +38,7 @@ use OCP\IConfig; use OCP\IUser; use OCP\Share\IShare; +use OCA\Files_Versions\Versions\IVersionManager; /** * Class Test_Files_versions @@ -647,7 +648,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 5b77597c5299206d9c8d02b79e1917b5d9a8874a 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/6] 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 2cd5cc5b1f804..205f5c8aebf59 100644 --- a/apps/files_versions/lib/Versions/VersionManager.php +++ b/apps/files_versions/lib/Versions/VersionManager.php @@ -100,11 +100,14 @@ public function createVersion(IUser $user, FileInfo $file) { public function rollback(IVersion $version) { $backend = $version->getBackend(); $result = self::handleAppLocks(fn(): ?bool => $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; } From 92c736701366d6c55d8c349c76591918d7d21c95 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 6 Jul 2023 16:09:45 +0200 Subject: [PATCH 5/6] properly register test mount for user Signed-off-by: Robin Appelman --- apps/files_versions/tests/VersioningTest.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/files_versions/tests/VersioningTest.php b/apps/files_versions/tests/VersioningTest.php index f2319fdeba960..5fc4c9132b428 100644 --- a/apps/files_versions/tests/VersioningTest.php +++ b/apps/files_versions/tests/VersioningTest.php @@ -39,6 +39,7 @@ use OCP\IUser; use OCP\Share\IShare; use OCA\Files_Versions\Versions\IVersionManager; +use Test\Traits\MountProviderTrait; /** * Class Test_Files_versions @@ -50,6 +51,7 @@ class VersioningTest extends \Test\TestCase { public const TEST_VERSIONS_USER = 'test-versions-user'; public const TEST_VERSIONS_USER2 = 'test-versions-user2'; public const USERS_VERSIONS_ROOT = '/test-versions-user/files_versions'; + use MountProviderTrait; /** * @var \OC\Files\View @@ -647,7 +649,8 @@ public function testRestoreSameStorage() { public function testRestoreCrossStorage() { $storage2 = new Temporary([]); - \OC\Files\Filesystem::mount($storage2, [], self::TEST_VERSIONS_USER . '/files/sub'); + $this->registerMount(self::TEST_VERSIONS_USER, $storage2, self::TEST_VERSIONS_USER . '/files/sub'); + $this->loginAsUser(self::TEST_VERSIONS_USER); $this->doTestRestore(); } From 5121ccaa3ac5a681bcf7ac8439f29db79bdb91f6 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 7 Jul 2023 18:30:17 +0200 Subject: [PATCH 6/6] style: satisfy code style checker Signed-off-by: Arthur Schiwon --- lib/private/Files/Node/Folder.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Files/Node/Folder.php b/lib/private/Files/Node/Folder.php index 5669f2a4b4599..beb2ac60d559e 100644 --- a/lib/private/Files/Node/Folder.php +++ b/lib/private/Files/Node/Folder.php @@ -326,7 +326,7 @@ protected function getAppDataDirectoryName(): string { * @param int $id * @return array */ - protected function getByIdInRootMount(int $id): array { + protected function getByIdInRootMount(int $id): array { if (!method_exists($this->root, 'createNode')) { // Always expected to be false. Being a method of Folder, this is // always implemented. For it is an internal method and should not