Skip to content

Commit f30144b

Browse files
DeepDiver1975claude
DeepDiver1975
and
claude
committed
fix(trashbin): avoid full filecache scan when restoring versions
`Trashbin::getVersionsFromTrash()` looked up a deleted file's versions with `View::searchRaw('<name>.v%[.d<timestamp>]')`, which reaches `Cache::search()` as `WHERE storage = ? AND name ILIKE ?`. The leading literal + trailing `%` is non-sargable, and on MySQL the `ILIKE`->`COLLATE` rewrite defeats any `name` index entirely, so every version restore — and every `ExpireTrash`/`ExpireVersions` cron run that restores versions — full scans `oc_filecache`. On large installations this is a heavy, repeated load spike (issue #31682). The version files for a given deleted file all live in a single known directory under `files_trashbin/versions` (the folder the file was in, or the versions root). Replace the whole-cache name search with an index-backed `getDirectoryContent()` of that one directory (uses the existing `(parent, name)` index) and filter the `<name>.v<version>[.d<timestamp>]` entries in PHP. The directory to list is now passed in from both call sites so version files of files deleted from inside a folder are still found. Behavior is preserved: the same versions are returned, in the same shape. Literal PHP matching is also slightly stricter than the previous SQL `LIKE` (filenames containing `_`/`%` are no longer treated as wildcards). Adds a regression test covering both the timestamped root lookup and the non-timestamped sub-folder lookup, and asserting that version files of other files / other timestamps are ignored. Fixes #31682 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
1 parent 4921c51 commit f30144b

2 files changed

Lines changed: 96 additions & 14 deletions

File tree

‎apps/files_trashbin/lib/Trashbin.php‎

Lines changed: 36 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -680,7 +680,7 @@ private static function restoreVersionsFromTrashbin(View $view, $filename, $targ
680680
$metaStorage->renameOrCopy('rename', $src . MetaStorage::VERSION_FILE_EXT, $user, $dst . MetaStorage::VERSION_FILE_EXT, $owner);
681681
}
682682

683-
$versions = self::getVersionsFromTrash($filenameOnlyWithoutTimestamp, $timestamp, $user);
683+
$versions = self::getVersionsFromTrash($filenameOnlyWithoutTimestamp, $timestamp, $user, $dir);
684684
foreach ($versions as $v) {
685685
if ($timestamp) {
686686
$src = '/files_trashbin/versions/' . $dirAndFilename . '.v' . $v . '.d' . $timestamp;
@@ -846,7 +846,7 @@ private static function deleteVersions(View $view, $file, $user) {
846846
$filenameOnlyWithoutTimestamp = $filenameOnly;
847847
$dirAndFilename = "{$dir}/{$filenameOnly}";
848848
}
849-
$versions = self::getVersionsFromTrash($filenameOnlyWithoutTimestamp, $timestamp, $user);
849+
$versions = self::getVersionsFromTrash($filenameOnlyWithoutTimestamp, $timestamp, $user, $dir);
850850
foreach ($versions as $v) {
851851
if ($timestamp) {
852852
$size += $view->filesize('/files_trashbin/versions/' . $dirAndFilename . '.v' . $v . '.d' . $timestamp);
@@ -957,9 +957,14 @@ private static function copy_recursive($source, $destination, View $view) {
957957
*
958958
* @param string $filename name of the file which should be restored
959959
* @param int $timestamp timestamp when the file was deleted
960+
* @param string $user
961+
* @param string $dir directory the file lives in, relative to the
962+
* "files_trashbin/versions" root ('', '/' or '.' for the
963+
* root itself). For deleted files that were inside a
964+
* folder, their version files live in that same folder.
960965
* @return array
961966
*/
962-
private static function getVersionsFromTrash($filename, $timestamp, $user) {
967+
private static function getVersionsFromTrash($filename, $timestamp, $user, $dir = '') {
963968
$view = new View('/' . $user . '/files_trashbin/versions');
964969
$versions = [];
965970

@@ -971,24 +976,41 @@ private static function getVersionsFromTrash($filename, $timestamp, $user) {
971976
self::$scannedVersions = true;
972977
}
973978

979+
// The version files for a deleted file all live in a single, known
980+
// directory under "files_trashbin/versions" (the folder the file was in,
981+
// or the versions root). Instead of running a non-sargable
982+
// trailing-wildcard name search across the whole filecache (full table
983+
// scan on MySQL, see issue #31682), list that one directory using the
984+
// index-backed (parent, name) lookup and filter the matching
985+
// "<filename>.v<version>[.d<timestamp>]" entries in PHP.
986+
$lookupDir = ($dir === '/' || $dir === '.') ? '' : \ltrim($dir, '/');
987+
$prefix = $filename . '.v';
988+
974989
if ($timestamp) {
975-
// fetch for old versions
976-
$matches = $view->searchRaw($filename . '.v%.d' . $timestamp);
990+
// match "<filename>.v<version>.d<timestamp>"
991+
$suffix = '.d' . $timestamp;
977992
$offset = -\strlen($timestamp) - 2;
978993
} else {
979-
$matches = $view->searchRaw($filename . '.v%');
994+
// match "<filename>.v<version>"
995+
$suffix = '';
996+
$offset = 0;
980997
}
981998

982-
if (\is_array($matches)) {
983-
foreach ($matches as $ma) {
984-
if ($timestamp) {
985-
$parts = \explode('.v', \substr($ma['path'], 0, $offset));
986-
$versions[] = (\end($parts));
987-
} else {
988-
$parts = \explode('.v', $ma['path']);
989-
$versions[] = (\end($parts));
999+
foreach ($view->getDirectoryContent($lookupDir) as $entry) {
1000+
$name = $entry->getName();
1001+
if (\strpos($name, $prefix) !== 0) {
1002+
continue;
1003+
}
1004+
if ($suffix !== '') {
1005+
// must end with the deletion timestamp suffix
1006+
if (\substr($name, $offset) !== $suffix) {
1007+
continue;
9901008
}
1009+
$parts = \explode('.v', \substr($name, 0, $offset));
1010+
} else {
1011+
$parts = \explode('.v', $name);
9911012
}
1013+
$versions[] = (\end($parts));
9921014
}
9931015
return $versions;
9941016
}

‎apps/files_trashbin/tests/StorageTest.php‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -910,6 +910,66 @@ public function testSingleStorageDeleteFileLoggedOut() {
910910
}
911911
}
912912

913+
/**
914+
* Regression test for issue #31682.
915+
*
916+
* Trashbin::getVersionsFromTrash() must return the versions of one specific
917+
* deleted file living in "files_trashbin/versions" and ignore version files
918+
* that belong to other (similarly named) files or other deletion timestamps.
919+
* This is exercised here against the real filecache, independent of the
920+
* lookup strategy (index-backed directory listing instead of a wildcard
921+
* name search).
922+
*/
923+
public function testGetVersionsFromTrash() {
924+
$timestamp = 1234567890;
925+
$versionsDir = $this->user . '/files_trashbin/versions';
926+
$this->rootView->mkdir($versionsDir);
927+
928+
// versions of the file we want to look up
929+
$this->rootView->file_put_contents($versionsDir . '/test.txt.v1.d' . $timestamp, 'v1');
930+
$this->rootView->file_put_contents($versionsDir . '/test.txt.v2.d' . $timestamp, 'v2');
931+
$this->rootView->file_put_contents($versionsDir . '/test.txt.v10.d' . $timestamp, 'v10');
932+
933+
// entries that must be ignored:
934+
// - same file name but a different deletion timestamp
935+
$this->rootView->file_put_contents($versionsDir . '/test.txt.v3.d999', 'other-ts');
936+
// - a different file whose name shares the prefix
937+
$this->rootView->file_put_contents($versionsDir . '/test.txt.backup.v9.d' . $timestamp, 'other-file');
938+
// - an unrelated file
939+
$this->rootView->file_put_contents($versionsDir . '/unrelated.txt.v1.d' . $timestamp, 'unrelated');
940+
941+
// a file deleted from inside a folder keeps its versions in that same
942+
// sub-folder of "files_trashbin/versions" (non-timestamped lookup path).
943+
$this->rootView->mkdir($versionsDir . '/folder');
944+
$this->rootView->file_put_contents($versionsDir . '/folder/nested.txt.v5', 'nested-v5');
945+
$this->rootView->file_put_contents($versionsDir . '/folder/nested.txt.v6', 'nested-v6');
946+
947+
// make sure the cache knows about the files we just created
948+
list($storage, ) = $this->rootView->resolvePath($versionsDir);
949+
$storage->getScanner()->scan('files_trashbin/versions');
950+
951+
$method = new \ReflectionMethod(\OCA\Files_Trashbin\Trashbin::class, 'getVersionsFromTrash');
952+
$method->setAccessible(true);
953+
954+
// reset the per-request "already scanned" guard so the lookup re-scans
955+
$scannedVersions = new \ReflectionProperty(\OCA\Files_Trashbin\Trashbin::class, 'scannedVersions');
956+
$scannedVersions->setAccessible(true);
957+
958+
// timestamped lookup in the versions root
959+
$scannedVersions->setValue(null, false);
960+
$versions = $method->invoke(null, 'test.txt', $timestamp, $this->user);
961+
\sort($versions);
962+
$this->assertEquals(['1', '10', '2'], $versions);
963+
964+
// non-timestamped lookup inside a sub-folder (issue #31682 regression:
965+
// the previous whole-cache name search found these, the directory
966+
// listing must look in the right sub-folder, not just the root)
967+
$scannedVersions->setValue(null, false);
968+
$versions = $method->invoke(null, 'nested.txt', null, $this->user, 'folder');
969+
\sort($versions);
970+
$this->assertEquals(['5', '6'], $versions);
971+
}
972+
913973
private function markTestSkippedIfStorageHasOwnVersioning() {
914974
/** @var Storage $storage */
915975
list($storage, $internalPath) = $this->userView->resolvePath('folder/inside.txt');

0 commit comments

Comments
 (0)