Skip to content

Commit 46dcb71

Browse files
AndyScherzingerAndyScherzinger
authored
Merge pull request #61531 from nextcloud/backport/61011/stable34
[stable34] fix(preview): First cleanup from filecache and then from preview table
2 parents 90ac33a + 357a6ee commit 46dcb71

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
@@ -11,15 +11,15 @@
1111
use OCP\AppFramework\Attribute\Consumable;
1212

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

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

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

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

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

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

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

8280
public function testCleanupWhenNotDeletable(): void {
83-
$this->previewService->expects($this->once())->method('deleteAll');
84-
8581
$previewFolder = $this->createMock(Folder::class);
8682
$previewFolder->expects($this->once())
8783
->method('isDeletable')
@@ -92,7 +88,6 @@ public function testCleanupWhenNotDeletable(): void {
9288

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

9792
$this->rootFolder->expects($this->once())
9893
->method('getAppDataDirectoryName')
@@ -111,8 +106,6 @@ public function testCleanupWhenNotDeletable(): void {
111106

112107
#[\PHPUnit\Framework\Attributes\DataProvider('dataForTestCleanupWithDeleteException')]
113108
public function testCleanupWithDeleteException(string $exceptionClass, string $errorMessage): void {
114-
$this->previewService->expects($this->once())->method('deleteAll');
115-
116109
$previewFolder = $this->createMock(Folder::class);
117110
$previewFolder->expects($this->once())
118111
->method('isDeletable')
@@ -124,7 +117,6 @@ public function testCleanupWithDeleteException(string $exceptionClass, string $e
124117

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

129121
$this->rootFolder->expects($this->once())
130122
->method('getAppDataDirectoryName')
@@ -148,53 +140,16 @@ public static function dataForTestCleanupWithDeleteException(): array {
148140
];
149141
}
150142

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

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

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

‎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)