diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index 189728cd587f..768d57a2049a 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/apps/files_trashbin/lib/Trashbin.php @@ -680,7 +680,7 @@ private static function restoreVersionsFromTrashbin(View $view, $filename, $targ $metaStorage->renameOrCopy('rename', $src . MetaStorage::VERSION_FILE_EXT, $user, $dst . MetaStorage::VERSION_FILE_EXT, $owner); } - $versions = self::getVersionsFromTrash($filenameOnlyWithoutTimestamp, $timestamp, $user); + $versions = self::getVersionsFromTrash($filenameOnlyWithoutTimestamp, $timestamp, $user, $dir); foreach ($versions as $v) { if ($timestamp) { $src = '/files_trashbin/versions/' . $dirAndFilename . '.v' . $v . '.d' . $timestamp; @@ -846,7 +846,7 @@ private static function deleteVersions(View $view, $file, $user) { $filenameOnlyWithoutTimestamp = $filenameOnly; $dirAndFilename = "{$dir}/{$filenameOnly}"; } - $versions = self::getVersionsFromTrash($filenameOnlyWithoutTimestamp, $timestamp, $user); + $versions = self::getVersionsFromTrash($filenameOnlyWithoutTimestamp, $timestamp, $user, $dir); foreach ($versions as $v) { if ($timestamp) { $size += $view->filesize('/files_trashbin/versions/' . $dirAndFilename . '.v' . $v . '.d' . $timestamp); @@ -957,9 +957,14 @@ private static function copy_recursive($source, $destination, View $view) { * * @param string $filename name of the file which should be restored * @param int $timestamp timestamp when the file was deleted + * @param string $user + * @param string $dir directory the file lives in, relative to the + * "files_trashbin/versions" root ('', '/' or '.' for the + * root itself). For deleted files that were inside a + * folder, their version files live in that same folder. * @return array */ - private static function getVersionsFromTrash($filename, $timestamp, $user) { + private static function getVersionsFromTrash($filename, $timestamp, $user, $dir = '') { $view = new View('/' . $user . '/files_trashbin/versions'); $versions = []; @@ -971,24 +976,41 @@ private static function getVersionsFromTrash($filename, $timestamp, $user) { self::$scannedVersions = true; } + // The version files for a deleted file all live in a single, known + // directory under "files_trashbin/versions" (the folder the file was in, + // or the versions root). Instead of running a non-sargable + // trailing-wildcard name search across the whole filecache (full table + // scan on MySQL, see issue #31682), list that one directory using the + // index-backed (parent, name) lookup and filter the matching + // ".v[.d]" entries in PHP. + $lookupDir = ($dir === '/' || $dir === '.') ? '' : \ltrim($dir, '/'); + $prefix = $filename . '.v'; + if ($timestamp) { - // fetch for old versions - $matches = $view->searchRaw($filename . '.v%.d' . $timestamp); + // match ".v.d" + $suffix = '.d' . $timestamp; $offset = -\strlen($timestamp) - 2; } else { - $matches = $view->searchRaw($filename . '.v%'); + // match ".v" + $suffix = ''; + $offset = 0; } - if (\is_array($matches)) { - foreach ($matches as $ma) { - if ($timestamp) { - $parts = \explode('.v', \substr($ma['path'], 0, $offset)); - $versions[] = (\end($parts)); - } else { - $parts = \explode('.v', $ma['path']); - $versions[] = (\end($parts)); + foreach ($view->getDirectoryContent($lookupDir) as $entry) { + $name = $entry->getName(); + if (\strpos($name, $prefix) !== 0) { + continue; + } + if ($suffix !== '') { + // must end with the deletion timestamp suffix + if (\substr($name, $offset) !== $suffix) { + continue; } + $parts = \explode('.v', \substr($name, 0, $offset)); + } else { + $parts = \explode('.v', $name); } + $versions[] = (\end($parts)); } return $versions; } diff --git a/apps/files_trashbin/tests/StorageTest.php b/apps/files_trashbin/tests/StorageTest.php index e9321713c42d..6d492a18a71f 100644 --- a/apps/files_trashbin/tests/StorageTest.php +++ b/apps/files_trashbin/tests/StorageTest.php @@ -910,6 +910,72 @@ public function testSingleStorageDeleteFileLoggedOut() { } } + /** + * Regression test for issue #31682. + * + * Trashbin::getVersionsFromTrash() must return the versions of one specific + * deleted file living in "files_trashbin/versions" and ignore version files + * that belong to other (similarly named) files or other deletion timestamps. + * This is exercised here against the real filecache, independent of the + * lookup strategy (index-backed directory listing instead of a wildcard + * name search). + */ + public function testGetVersionsFromTrash() { + $timestamp = 1234567890; + $versionsDir = $this->user . '/files_trashbin/versions'; + $this->rootView->mkdir($versionsDir); + + // versions of the file we want to look up + $this->rootView->file_put_contents($versionsDir . '/test.txt.v1.d' . $timestamp, 'v1'); + $this->rootView->file_put_contents($versionsDir . '/test.txt.v2.d' . $timestamp, 'v2'); + $this->rootView->file_put_contents($versionsDir . '/test.txt.v10.d' . $timestamp, 'v10'); + + // entries that must be ignored: + // - same file name but a different deletion timestamp + $this->rootView->file_put_contents($versionsDir . '/test.txt.v3.d999', 'other-ts'); + // - a different file whose name shares the prefix + $this->rootView->file_put_contents($versionsDir . '/test.txt.backup.v9.d' . $timestamp, 'other-file'); + // - an unrelated file + $this->rootView->file_put_contents($versionsDir . '/unrelated.txt.v1.d' . $timestamp, 'unrelated'); + + // a file deleted from inside a folder keeps its versions in that same + // sub-folder of "files_trashbin/versions" (non-timestamped lookup path). + $this->rootView->mkdir($versionsDir . '/folder'); + $this->rootView->file_put_contents($versionsDir . '/folder/nested.txt.v5', 'nested-v5'); + $this->rootView->file_put_contents($versionsDir . '/folder/nested.txt.v6', 'nested-v6'); + + // make sure the cache knows about the files we just created + list($storage, ) = $this->rootView->resolvePath($versionsDir); + $storage->getScanner()->scan('files_trashbin/versions'); + + $method = new \ReflectionMethod(\OCA\Files_Trashbin\Trashbin::class, 'getVersionsFromTrash'); + $method->setAccessible(true); + + // reset the per-request "already scanned" guard so the lookup re-scans + $scannedVersions = new \ReflectionProperty(\OCA\Files_Trashbin\Trashbin::class, 'scannedVersions'); + $scannedVersions->setAccessible(true); + + // timestamped lookup in the versions root + // (sort() uses SORT_REGULAR, so the numeric version strings sort numerically) + $scannedVersions->setValue(null, false); + $versions = $method->invoke(null, 'test.txt', $timestamp, $this->user); + \sort($versions); + $this->assertEquals(['1', '2', '10'], $versions); + + // non-timestamped lookup inside a sub-folder (issue #31682 regression: + // the previous whole-cache name search found these, the directory + // listing must look in the right sub-folder, not just the root) + $scannedVersions->setValue(null, false); + $versions = $method->invoke(null, 'nested.txt', null, $this->user, 'folder'); + \sort($versions); + $this->assertEquals(['5', '6'], $versions); + + // a file with no stored versions yields an empty result + $scannedVersions->setValue(null, false); + $versions = $method->invoke(null, 'no-such-file.txt', $timestamp, $this->user); + $this->assertEquals([], $versions); + } + private function markTestSkippedIfStorageHasOwnVersioning() { /** @var Storage $storage */ list($storage, $internalPath) = $this->userView->resolvePath('folder/inside.txt');