diff --git a/lib/private/Preview/Generator.php b/lib/private/Preview/Generator.php index fd8bf9cc00e..94fea7314b5 100644 --- a/lib/private/Preview/Generator.php +++ b/lib/private/Preview/Generator.php @@ -594,7 +594,7 @@ class Generator { throw $e; } - $this->storageFactory->deletePreview($previewEntry); + $this->storageFactory->deleteUnreferencedPreview($previewEntry); $this->logger->debug('Generating a preview but one already exists.', ['exception' => $e]); diff --git a/lib/private/Preview/Storage/IPreviewStorage.php b/lib/private/Preview/Storage/IPreviewStorage.php index 28d2dc53ef5..85e91655a8f 100644 --- a/lib/private/Preview/Storage/IPreviewStorage.php +++ b/lib/private/Preview/Storage/IPreviewStorage.php @@ -35,6 +35,19 @@ interface IPreviewStorage { */ public function deletePreview(Preview $preview): void; + /** + * Delete the stored data of a preview that never got a database row, for + * example after losing the insert race on the unique constraint. + * + * Only backends that key their storage by the preview id hold data that + * belongs to this entity alone. The local storage derives its path from + * the preview specification, so the file is the very same one the winner + * of the race is now referencing and has to be kept. + * + * @throws NotPermittedException + */ + public function deleteUnreferencedPreview(Preview $preview): void; + /** * Migration helper * diff --git a/lib/private/Preview/Storage/LocalPreviewStorage.php b/lib/private/Preview/Storage/LocalPreviewStorage.php index e66ebae192d..b2218a4d02d 100644 --- a/lib/private/Preview/Storage/LocalPreviewStorage.php +++ b/lib/private/Preview/Storage/LocalPreviewStorage.php @@ -69,6 +69,13 @@ class LocalPreviewStorage implements IPreviewStorage { } } + #[Override] + public function deleteUnreferencedPreview(Preview $preview): void { + // constructPath() keys on the file id and the preview specification, not + // on the preview id, so this file is shared with the preview that won the + // race. Deleting it would leave that one with a row but no file. + } + public function getRootFolder(): string { return $this->config->getSystemValueString('datadirectory', OC::$SERVERROOT . '/data'); } diff --git a/lib/private/Preview/Storage/ObjectStorePreviewStorage.php b/lib/private/Preview/Storage/ObjectStorePreviewStorage.php index f77da26eaea..0a2f5e8f012 100644 --- a/lib/private/Preview/Storage/ObjectStorePreviewStorage.php +++ b/lib/private/Preview/Storage/ObjectStorePreviewStorage.php @@ -167,6 +167,13 @@ class ObjectStorePreviewStorage implements IPreviewStorage { return $this->objectStoreCache[$objectStoreName][$bucketName]; } + #[Override] + public function deleteUnreferencedPreview(Preview $preview): void { + // The urn embeds the preview id, so this object belongs to this entity + // alone and can be removed safely. + $this->deletePreview($preview); + } + public function getUrn(Preview $preview, array $config): string { if ($preview->getOldFileId()) { return ($config['arguments']['objectPrefix'] ?? 'urn:oid:') . $preview->getOldFileId(); diff --git a/lib/private/Preview/Storage/StorageFactory.php b/lib/private/Preview/Storage/StorageFactory.php index 39cd11e09cf..e61e3402ac7 100644 --- a/lib/private/Preview/Storage/StorageFactory.php +++ b/lib/private/Preview/Storage/StorageFactory.php @@ -37,6 +37,11 @@ class StorageFactory implements IPreviewStorage { $this->getBackend()->deletePreview($preview); } + #[Override] + public function deleteUnreferencedPreview(Preview $preview): void { + $this->getBackend()->deleteUnreferencedPreview($preview); + } + private function getBackend(): IPreviewStorage { if ($this->backend) { return $this->backend; diff --git a/tests/lib/Preview/Storage/LocalPreviewStorageTest.php b/tests/lib/Preview/Storage/LocalPreviewStorageTest.php index c532a1575e8..c46b3c36203 100644 --- a/tests/lib/Preview/Storage/LocalPreviewStorageTest.php +++ b/tests/lib/Preview/Storage/LocalPreviewStorageTest.php @@ -9,6 +9,7 @@ declare(strict_types=1); namespace Test\Preview\Storage; +use OC\Preview\Db\Preview; use OC\Preview\Db\PreviewMapper; use OC\Preview\Storage\LocalPreviewStorage; use OCP\DB\Exception as DBException; @@ -19,6 +20,7 @@ use OCP\DB\QueryBuilder\ITypedQueryBuilder; use OCP\Files\IMimeTypeDetector; use OCP\Files\IMimeTypeLoader; use OCP\Files\IRootFolder; +use OCP\Files\NotFoundException; use OCP\IAppConfig; use OCP\IConfig; use OCP\IDBConnection; @@ -287,4 +289,43 @@ class LocalPreviewStorageTest extends TestCase { $this->assertSame(3, $count); } + + private function makePreview(int $fileId = self::FILE_ID): Preview { + $preview = new Preview(); + $preview->setFileId($fileId); + $preview->setWidth(1024); + $preview->setHeight(768); + $preview->setCropped(false); + $preview->setMax(true); + $preview->setMimetype('image/jpeg'); + + return $preview; + } + + /** + * Losing the insert race must not remove the local file: the path is derived + * from the preview specification, so it is the same file the preview that + * won the race now points at. + */ + public function testDeleteUnreferencedPreviewKeepsTheSharedFile(): void { + $preview = $this->makePreview(); + $this->storage->writePreview($preview, 'preview data'); + + $this->storage->deleteUnreferencedPreview($preview); + + $this->assertSame('preview data', stream_get_contents($this->storage->readPreview($preview))); + } + + /** + * Deleting a preview for real still has to remove the file. + */ + public function testDeletePreviewRemovesTheFile(): void { + $preview = $this->makePreview(); + $this->storage->writePreview($preview, 'preview data'); + + $this->storage->deletePreview($preview); + + $this->expectException(NotFoundException::class); + $this->storage->readPreview($preview); + } }