Skip to content

Commit a213e6b

Browse files
CarlSchwanCarlSchwan
authored
Merge pull request #61011 from nextcloud/carl/cleanup-preview
fix(preview): First cleanup from filecache and then from preview table
2 parents cc97151 + 09aea63 commit a213e6b

6 files changed

Lines changed: 58 additions & 83 deletions

File tree

‎core/BackgroundJobs/PreviewMigrationJob.php‎

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use OCP\IConfig;
2020
use OCP\IDBConnection;
2121
use Override;
22+
use Psr\Log\LoggerInterface;
2223

2324
class PreviewMigrationJob extends TimedJob {
2425
private string $previewRootPath;
@@ -30,6 +31,7 @@ public function __construct(
3031
private readonly IDBConnection $connection,
3132
private readonly IRootFolder $rootFolder,
3233
private readonly PreviewMigrationService $migrationService,
34+
private readonly LoggerInterface $logger,
3335
) {
3436
parent::__construct($time);
3537

@@ -49,10 +51,14 @@ protected function run(mixed $argument): void {
4951
$qb = $this->connection->getQueryBuilder();
5052
$qb->select('path')
5153
->from('filecache')
52-
// Hierarchical preview folder structure
53-
->where($qb->expr()->like('path', $qb->createNamedParameter($this->previewRootPath . '%/%/%/%/%/%/%/%/%')))
54-
// Legacy flat preview folder structure
55-
->orWhere($qb->expr()->like('path', $qb->createNamedParameter($this->previewRootPath . '%/%.%')))
54+
->where($qb->expr()->orX(
55+
// Hierarchical preview folder structure
56+
$qb->expr()->like('path', $qb->createNamedParameter($this->previewRootPath . '%/%/%/%/%/%/%/%/%')),
57+
// Legacy flat preview folder structure
58+
$qb->expr()->like('path', $qb->createNamedParameter($this->previewRootPath . '%/%.%'))
59+
))->andWhere(
60+
$qb->expr()->eq('storage', $qb->createNamedParameter($this->rootFolder->getMountPoint()->getNumericStorageId()))
61+
)
5662
->hintShardKey('storage', $this->rootFolder->getMountPoint()->getNumericStorageId())
5763
->setMaxResults(100);
5864

@@ -95,11 +101,23 @@ private function processQueryResult(IResult $result): bool {
95101
}
96102

97103
foreach ($fileIds as $fileId) {
98-
$this->migrationService->migrateFileId($fileId, flatPath: false);
104+
try {
105+
$this->migrationService->migrateFileId($fileId, flatPath: false);
106+
} catch (\Exception $e) {
107+
$this->logger->error('Failed to migrate preview with fileId: ' . $fileId . ' (hierarchical file structure)', [
108+
'exception' => $e,
109+
]);
110+
}
99111
}
100112

101113
foreach ($flatFileIds as $fileId) {
102-
$this->migrationService->migrateFileId($fileId, flatPath: true);
114+
try {
115+
$this->migrationService->migrateFileId($fileId, flatPath: true);
116+
} catch (\Exception $e) {
117+
$this->logger->error('Failed to migrate preview with fileId: ' . $fileId . ' (legacy file structure)', [
118+
'exception' => $e,
119+
]);
120+
}
103121
}
104122
return $foundPreview;
105123
}

‎core/Command/Preview/Cleanup.php‎

Lines changed: 4 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,11 @@ protected function configure(): void {
3939

4040
#[\Override]
4141
protected function execute(InputInterface $input, OutputInterface $output): int {
42-
if ($this->deletePreviewFromPreviewTable($output) !== 0) {
42+
if ($this->deletePreviewFromFileCacheTable($output) !== 0) {
4343
return 1;
4444
}
4545

46-
return $this->deletePreviewFromFileCacheTable($output);
46+
return $this->deletePreviewFromPreviewTable($output);
4747
}
4848

4949
/**
@@ -77,9 +77,8 @@ private function deletePreviewFromFileCacheTable(OutputInterface $output): int {
7777
$previewFolder = $appDataFolder->get('preview');
7878

7979
} catch (NotFoundException $e) {
80-
$this->logger->error("Previews can't be removed: appdata folder can't be found", ['exception' => $e]);
81-
$output->writeln("Previews can't be removed: preview folder isn't deletable");
82-
return 1;
80+
$this->logger->info("Legacy previews can't be removed: appdata folder can't be found", ['exception' => $e]);
81+
return 0;
8382
}
8483

8584
if (!$previewFolder->isDeletable()) {
@@ -102,16 +101,6 @@ private function deletePreviewFromFileCacheTable(OutputInterface $output): int {
102101
return 1;
103102
}
104103

105-
try {
106-
$appDataFolder->newFolder('preview');
107-
$this->logger->debug('Preview folder recreated');
108-
$output->writeln('Preview folder recreated', OutputInterface::VERBOSITY_VERBOSE);
109-
} catch (NotPermittedException $e) {
110-
$output->writeln("Preview folder was deleted, but you don't have the permission to create preview folder");
111-
$this->logger->error("Preview folder was deleted, but you don't have the permission to create preview folder", ['exception' => $e]);
112-
return 1;
113-
}
114-
115104
$output->writeln('Previews removed');
116105
return 0;
117106
}

‎lib/private/Preview/PreviewMigrationService.php‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,9 @@ public function migrateFileId(int $fileId, bool $flatPath): array {
9393
->where($qb->expr()->eq('fileid', $qb->createNamedParameter($fileId)))
9494
->setMaxResults(1);
9595

96-
$result = $qb->executeQuery();
97-
$result = $result->fetchAssociative();
96+
$cursor = $qb->executeQuery();
97+
$result = $cursor->fetchAssociative();
98+
$cursor->closeCursor();
9899

99100
if ($result !== false) {
100101
foreach ($previewFiles as $previewFile) {
@@ -108,10 +109,15 @@ public function migrateFileId(int $fileId, bool $flatPath): array {
108109
$preview->generateId();
109110
try {
110111
$preview = $this->previewMapper->insert($preview);
111-
} catch (Exception) {
112+
} catch (Exception $e) {
113+
if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
114+
throw $e;
115+
}
116+
117+
$delete = $this->connection->getQueryBuilder();
112118
// We already have this preview in the preview table, skip
113-
$qb->delete('filecache')
114-
->where($qb->expr()->eq('fileid', $qb->createNamedParameter($file->getId())))
119+
$delete->delete('filecache')
120+
->where($delete->expr()->eq('fileid', $delete->createNamedParameter($file->getId())))
115121
->hintShardKey('storage', $this->rootFolder->getMountPoint()->getNumericStorageId())
116122
->executeStatement();
117123
continue;
@@ -179,7 +185,11 @@ private function deleteFolder(string $path): void {
179185
break;
180186
}
181187

182-
$folder = $this->appData->getFolder($current);
188+
try {
189+
$folder = $this->appData->getFolder($current);
190+
} catch (NotFoundException) {
191+
break;
192+
}
183193
if (count($folder->getDirectoryListing()) !== 0) {
184194
break;
185195
}

‎lib/public/Migration/Attributes/IndexMigrationAttribute.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,15 @@
1212
use OCP\AppFramework\Attribute\Consumable;
1313

1414
/**
15-
* generic class related to migration attribute about index changes
15+
* Generic class related to migration attribute about index changes
1616
*/
1717
#[Consumable(since: '30.0.0')]
1818
class IndexMigrationAttribute extends MigrationAttribute {
1919
/**
2020
* @param string $table name of the database table
2121
* @param IndexType|null $type type of the index
2222
* @param string $description description of the migration
23-
* @param array $notes notes abour the migration/index
23+
* @param array $notes notes about the migration/index
2424
* @since 30.0.0
2525
*/
2626
public function __construct(

‎tests/Core/Command/Preview/CleanupTest.php‎

Lines changed: 6 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@ public function testCleanup(): void {
5656

5757
$appDataFolder = $this->createMock(Folder::class);
5858
$appDataFolder->expects($this->once())->method('get')->with('preview')->willReturn($previewFolder);
59-
$appDataFolder->expects($this->once())->method('newFolder')->with('preview');
6059

6160
$this->rootFolder->expects($this->once())
6261
->method('getAppDataDirectoryName')
@@ -67,22 +66,19 @@ public function testCleanup(): void {
6766
->with('appdata_some_id')
6867
->willReturn($appDataFolder);
6968

70-
$this->output->expects($this->exactly(3))->method('writeln')
69+
$this->output->expects($this->exactly(2))->method('writeln')
7170
->with(self::callback(function (string $message): bool {
7271
static $i = 0;
7372
return match (++$i) {
7473
1 => $message === 'Preview folder deleted',
75-
2 => $message === 'Preview folder recreated',
76-
3 => $message === 'Previews removed'
74+
2 => $message === 'Previews removed'
7775
};
7876
}));
7977

8078
$this->assertEquals(0, $this->repair->run($this->input, $this->output));
8179
}
8280

8381
public function testCleanupWhenNotDeletable(): void {
84-
$this->previewService->expects($this->once())->method('deleteAll');
85-
8682
$previewFolder = $this->createMock(Folder::class);
8783
$previewFolder->expects($this->once())
8884
->method('isDeletable')
@@ -93,7 +89,6 @@ public function testCleanupWhenNotDeletable(): void {
9389

9490
$appDataFolder = $this->createMock(Folder::class);
9591
$appDataFolder->expects($this->once())->method('get')->with('preview')->willReturn($previewFolder);
96-
$appDataFolder->expects($this->never())->method('newFolder')->with('preview');
9792

9893
$this->rootFolder->expects($this->once())
9994
->method('getAppDataDirectoryName')
@@ -112,8 +107,6 @@ public function testCleanupWhenNotDeletable(): void {
112107

113108
#[\PHPUnit\Framework\Attributes\DataProvider('dataForTestCleanupWithDeleteException')]
114109
public function testCleanupWithDeleteException(string $exceptionClass, string $errorMessage): void {
115-
$this->previewService->expects($this->once())->method('deleteAll');
116-
117110
$previewFolder = $this->createMock(Folder::class);
118111
$previewFolder->expects($this->once())
119112
->method('isDeletable')
@@ -125,7 +118,6 @@ public function testCleanupWithDeleteException(string $exceptionClass, string $e
125118

126119
$appDataFolder = $this->createMock(Folder::class);
127120
$appDataFolder->expects($this->once())->method('get')->with('preview')->willReturn($previewFolder);
128-
$appDataFolder->expects($this->never())->method('newFolder')->with('preview');
129121

130122
$this->rootFolder->expects($this->once())
131123
->method('getAppDataDirectoryName')
@@ -149,53 +141,16 @@ public static function dataForTestCleanupWithDeleteException(): array {
149141
];
150142
}
151143

152-
public function testCleanupWithCreateException(): void {
153-
$this->previewService->expects($this->once())->method('deleteAll');
154-
155-
$previewFolder = $this->createMock(Folder::class);
156-
$previewFolder->expects($this->once())
157-
->method('isDeletable')
158-
->willReturn(true);
159-
160-
$previewFolder->expects($this->once())
161-
->method('delete');
162-
163-
$appDataFolder = $this->createMock(Folder::class);
164-
$appDataFolder->expects($this->once())->method('get')->with('preview')->willReturn($previewFolder);
165-
$appDataFolder->expects($this->once())->method('newFolder')->with('preview')->willThrowException(new NotPermittedException());
166-
167-
$this->rootFolder->expects($this->once())
168-
->method('getAppDataDirectoryName')
169-
->willReturn('appdata_some_id');
170-
171-
$this->rootFolder->expects($this->once())
172-
->method('get')
173-
->with('appdata_some_id')
174-
->willReturn($appDataFolder);
175-
176-
$this->output->expects($this->exactly(2))->method('writeln')
177-
->with(self::callback(function (string $message): bool {
178-
static $i = 0;
179-
return match (++$i) {
180-
1 => $message === 'Preview folder deleted',
181-
2 => $message === "Preview folder was deleted, but you don't have the permission to create preview folder",
182-
};
183-
}));
184-
185-
$this->logger->expects($this->once())->method('error')->with("Preview folder was deleted, but you don't have the permission to create preview folder");
186-
187-
$this->assertEquals(1, $this->repair->run($this->input, $this->output));
188-
}
189-
190144
public function testCleanupWithPreviewServiceException(): void {
145+
$this->rootFolder->method('getAppDataDirectoryName')
146+
->willThrowException(new NotFoundException());
147+
191148
$this->previewService->expects($this->once())->method('deleteAll')
192149
->willThrowException(new NotPermittedException('abc'));
193150

151+
$this->logger->expects($this->once())->method('info')->with("Legacy previews can't be removed: appdata folder can't be found");
194152
$this->logger->expects($this->once())->method('error')->with("Previews can't be removed: exception occurred: abc");
195153

196-
$this->rootFolder->expects($this->never())
197-
->method('get');
198-
199154
$this->assertEquals(1, $this->repair->run($this->input, $this->output));
200155
}
201156
}

‎tests/lib/Preview/PreviewMigrationJobTest.php‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,8 @@ public function testMigrationLegacyPath(): void {
131131
$this->previewMapper,
132132
$this->storageFactory,
133133
Server::get(IAppDataFactory::class),
134-
)
134+
),
135+
$this->logger,
135136
);
136137
$this->invokePrivate($job, 'run', [[]]);
137138
$this->assertEquals(0, count($this->previewAppData->getDirectoryListing()));
@@ -168,7 +169,8 @@ public function testMigrationPath(): void {
168169
$this->previewMapper,
169170
$this->storageFactory,
170171
Server::get(IAppDataFactory::class),
171-
)
172+
),
173+
$this->logger,
172174
);
173175
$this->invokePrivate($job, 'run', [[]]);
174176
$this->assertEquals(0, count($this->previewAppData->getDirectoryListing()));
@@ -213,7 +215,8 @@ public function testMigrationPathWithVersion(): void {
213215
$this->previewMapper,
214216
$this->storageFactory,
215217
Server::get(IAppDataFactory::class),
216-
)
218+
),
219+
$this->logger,
217220
);
218221
$this->invokePrivate($job, 'run', [[]]);
219222
$previews = iterator_to_array($this->previewMapper->getAvailablePreviewsForFile(5));

0 commit comments

Comments
 (0)