From abc4bca439f8edbbdaf555b28db2597205d742c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Fri, 15 Dec 2023 22:48:26 +0100 Subject: [PATCH 1/8] PHPLIB-1323 Implement unlink for GridFS stream wrapper --- ...ucket-registerGlobalStreamWrapperAlias.txt | 3 ++ src/GridFS/StreamWrapper.php | 13 +++++++++ src/GridFS/WritableStream.php | 28 +++++++++++++++++++ tests/GridFS/StreamWrapperFunctionalTest.php | 15 ++++++++++ 4 files changed, 59 insertions(+) diff --git a/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt b/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt index 4dcc79ce9..c565163a4 100644 --- a/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt +++ b/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt @@ -49,6 +49,7 @@ Supported stream functions: - :php:`file() ` - :php:`fopen() ` with "r", "rb", "w", and "wb" modes - :php:`rename() ` rename all revisions of a file in the same bucket +- :php:`unlink() ` (remove all revisions of a file) In read mode, the stream context can contain the option ``gridfs['revision']`` to specify the revision number of the file to read. If omitted, the most recent @@ -86,6 +87,8 @@ Each call to these functions makes a request to the server. echo file_get_contents('gridfs://mybucket/hello.txt'); + unlink('gridfs://mybucket/hello.txt'); + The output would then resemble: .. code-block:: none diff --git a/src/GridFS/StreamWrapper.php b/src/GridFS/StreamWrapper.php index 7a77f11bd..58aa0a457 100644 --- a/src/GridFS/StreamWrapper.php +++ b/src/GridFS/StreamWrapper.php @@ -324,6 +324,19 @@ public function stream_write(string $data): int return $this->stream->writeBytes($data); } + public function unlink(string $path): bool + { + try { + $this->stream_open($path, 'w', 0, $openedPath); + } catch (FileNotFoundException $e) { + return false; + } + + assert($this->stream instanceof WritableStream); + + return $this->stream->delete() > 0; + } + /** @return false|array */ public function url_stat(string $path, int $flags) { diff --git a/src/GridFS/WritableStream.php b/src/GridFS/WritableStream.php index 096968bb6..1ee2387a9 100644 --- a/src/GridFS/WritableStream.php +++ b/src/GridFS/WritableStream.php @@ -19,6 +19,7 @@ use HashContext; use MongoDB\BSON\Binary; +use MongoDB\BSON\Document; use MongoDB\BSON\ObjectId; use MongoDB\BSON\UTCDateTime; use MongoDB\Driver\Exception\RuntimeException as DriverRuntimeException; @@ -173,6 +174,33 @@ public function close(): void $this->isClosed = true; } + /** + * Delete all files and chunks associated with this stream filename. + * + * @return int The number of deleted files + */ + public function delete(): int + { + /** @var Document[] $revisions */ + $revisions = $this->collectionWrapper->findFiles(['filename' => $this->file['filename']], [ + 'codec' => null, + 'typeMap' => ['root' => 'bson'], + 'projection' => ['_id' => 1], + ]); + + $count = 0; + + foreach ($revisions as $file) { + $this->collectionWrapper->findFileById($file->get('_id')); + $this->collectionWrapper->deleteFileAndChunksById($file->get('_id')); + $count++; + } + + $this->abort(); + + return $count; + } + /** * Return the stream's file document. */ diff --git a/tests/GridFS/StreamWrapperFunctionalTest.php b/tests/GridFS/StreamWrapperFunctionalTest.php index d9ab66552..55a030953 100644 --- a/tests/GridFS/StreamWrapperFunctionalTest.php +++ b/tests/GridFS/StreamWrapperFunctionalTest.php @@ -29,6 +29,7 @@ use function stream_context_create; use function stream_get_contents; use function time; +use function unlink; use function usleep; use const SEEK_CUR; @@ -387,4 +388,18 @@ public function testRenamePathMismatch(): void rename('gridfs://bucket/filename', 'gridfs://other/newname'); } + + public function testUnlinkAllRevisions(): void + { + $this->bucket->registerGlobalStreamWrapperAlias('bucket'); + $path = 'gridfs://bucket/path/to/filename'; + + file_put_contents($path, 'version 0'); + file_put_contents($path, 'version 1'); + + $result = unlink($path); + + $this->assertTrue($result); + $this->assertFalse(file_exists($path)); + } } From 6da793c227a06bf7f45dafc658af942ce04ecc38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Mon, 8 Jan 2024 14:50:04 +0100 Subject: [PATCH 2/8] Delete all file revisions by bulk --- psalm-baseline.xml | 10 ++++++++++ src/GridFS/CollectionWrapper.php | 21 +++++++++++++++++++++ src/GridFS/WritableStream.php | 23 +++++------------------ 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 972023cc9..89929cc65 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -85,6 +85,11 @@ $context + + + $ids[] + + n]]> @@ -100,6 +105,11 @@ $context + + + file['filename']]]> + + $this[$key] diff --git a/src/GridFS/CollectionWrapper.php b/src/GridFS/CollectionWrapper.php index 31c5df7bd..ec38cb03a 100644 --- a/src/GridFS/CollectionWrapper.php +++ b/src/GridFS/CollectionWrapper.php @@ -91,6 +91,27 @@ public function deleteFileAndChunksById($id): void $this->chunksCollection->deleteMany(['files_id' => $id]); } + public function deleteFileAndChunksByFilename(string $filename): int + { + /** @var iterable $files */ + $files = $this->findFiles(['filename' => $filename], [ + 'codec' => null, + 'typeMap' => ['root' => 'array'], + 'projection' => ['_id' => 1], + ]); + + /** @var list $ids */ + $ids = []; + foreach ($files as $file) { + $ids[] = $file['_id']; + } + + $count = $this->filesCollection->deleteMany(['_id' => ['$in' => $ids]])->getDeletedCount(); + $this->chunksCollection->deleteMany(['files_id' => ['$in' => $ids]]); + + return $count ?? 0; + } + /** * Drops the GridFS files and chunks collections. */ diff --git a/src/GridFS/WritableStream.php b/src/GridFS/WritableStream.php index 1ee2387a9..f5ef86c31 100644 --- a/src/GridFS/WritableStream.php +++ b/src/GridFS/WritableStream.php @@ -19,7 +19,6 @@ use HashContext; use MongoDB\BSON\Binary; -use MongoDB\BSON\Document; use MongoDB\BSON\ObjectId; use MongoDB\BSON\UTCDateTime; use MongoDB\Driver\Exception\RuntimeException as DriverRuntimeException; @@ -181,24 +180,12 @@ public function close(): void */ public function delete(): int { - /** @var Document[] $revisions */ - $revisions = $this->collectionWrapper->findFiles(['filename' => $this->file['filename']], [ - 'codec' => null, - 'typeMap' => ['root' => 'bson'], - 'projection' => ['_id' => 1], - ]); - - $count = 0; - - foreach ($revisions as $file) { - $this->collectionWrapper->findFileById($file->get('_id')); - $this->collectionWrapper->deleteFileAndChunksById($file->get('_id')); - $count++; + try { + return $this->collectionWrapper->deleteFileAndChunksByFilename($this->file['filename']); + } finally { + // Prevent further operations on this stream + $this->abort(); } - - $this->abort(); - - return $count; } /** From fc32d0ac42346cf345ffa504466dfc3e0afe9ed1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Tue, 9 Jan 2024 13:46:33 +0100 Subject: [PATCH 3/8] Fix method order --- src/GridFS/CollectionWrapper.php | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/GridFS/CollectionWrapper.php b/src/GridFS/CollectionWrapper.php index ec38cb03a..d770e8991 100644 --- a/src/GridFS/CollectionWrapper.php +++ b/src/GridFS/CollectionWrapper.php @@ -81,16 +81,8 @@ public function deleteChunksByFilesId($id): void } /** - * Deletes a GridFS file and related chunks by ID. - * - * @param mixed $id + * Delete all GridFS files and chunks for a given filename. */ - public function deleteFileAndChunksById($id): void - { - $this->filesCollection->deleteOne(['_id' => $id]); - $this->chunksCollection->deleteMany(['files_id' => $id]); - } - public function deleteFileAndChunksByFilename(string $filename): int { /** @var iterable $files */ @@ -112,6 +104,17 @@ public function deleteFileAndChunksByFilename(string $filename): int return $count ?? 0; } + /** + * Deletes a GridFS file and related chunks by ID. + * + * @param mixed $id + */ + public function deleteFileAndChunksById($id): void + { + $this->filesCollection->deleteOne(['_id' => $id]); + $this->chunksCollection->deleteMany(['files_id' => $id]); + } + /** * Drops the GridFS files and chunks collections. */ From f8231a784abdb4dd60eb53b5664724c83f2013bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Tue, 9 Jan 2024 17:42:56 +0100 Subject: [PATCH 4/8] Refactor rename and unlink to use the context directly --- ...ucket-registerGlobalStreamWrapperAlias.txt | 2 +- psalm-baseline.xml | 23 +++-- src/GridFS/CollectionWrapper.php | 1 - src/GridFS/ReadableStream.php | 16 ---- src/GridFS/StreamWrapper.php | 87 ++++++++++--------- src/GridFS/WritableStream.php | 15 ---- 6 files changed, 61 insertions(+), 83 deletions(-) diff --git a/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt b/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt index c565163a4..71bd06c63 100644 --- a/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt +++ b/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt @@ -49,7 +49,7 @@ Supported stream functions: - :php:`file() ` - :php:`fopen() ` with "r", "rb", "w", and "wb" modes - :php:`rename() ` rename all revisions of a file in the same bucket -- :php:`unlink() ` (remove all revisions of a file) +- :php:`unlink() ` remove all revisions of a file in the same bucket In read mode, the stream context can contain the option ``gridfs['revision']`` to specify the revision number of the file to read. If omitted, the most recent diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 89929cc65..a8ff05b22 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -98,17 +98,26 @@ - $context - $context + getContext($path, $mode)]]> + getContext($path, $mode)]]> - + $context - - - + + + array{collectionWrapper: CollectionWrapper, file: object}|array{collectionWrapper: CollectionWrapper, filename: string, options: array + - file['filename']]]> + filename]]> + filename]]> + + $context + + + + + diff --git a/src/GridFS/CollectionWrapper.php b/src/GridFS/CollectionWrapper.php index d770e8991..e44ecfb7c 100644 --- a/src/GridFS/CollectionWrapper.php +++ b/src/GridFS/CollectionWrapper.php @@ -87,7 +87,6 @@ public function deleteFileAndChunksByFilename(string $filename): int { /** @var iterable $files */ $files = $this->findFiles(['filename' => $filename], [ - 'codec' => null, 'typeMap' => ['root' => 'array'], 'projection' => ['_id' => 1], ]); diff --git a/src/GridFS/ReadableStream.php b/src/GridFS/ReadableStream.php index 402f21280..a0664e821 100644 --- a/src/GridFS/ReadableStream.php +++ b/src/GridFS/ReadableStream.php @@ -22,14 +22,12 @@ use MongoDB\Driver\CursorInterface; use MongoDB\Exception\InvalidArgumentException; use MongoDB\GridFS\Exception\CorruptFileException; -use MongoDB\GridFS\Exception\LogicException; use function assert; use function ceil; use function floor; use function is_integer; use function is_object; -use function is_string; use function property_exists; use function sprintf; use function strlen; @@ -178,20 +176,6 @@ public function readBytes(int $length): string return $data; } - /** - * Rename all revisions of the file. - */ - public function rename(string $newFilename): bool - { - if (! isset($this->file->filename) || ! is_string($this->file->filename)) { - throw new LogicException('Cannot rename file without a filename'); - } - - $this->collectionWrapper->updateFilenameForFilename($this->file->filename, $newFilename); - - return true; - } - /** * Seeks the chunk and buffer offsets for the next read operation. * diff --git a/src/GridFS/StreamWrapper.php b/src/GridFS/StreamWrapper.php index 58aa0a457..2ea3de7db 100644 --- a/src/GridFS/StreamWrapper.php +++ b/src/GridFS/StreamWrapper.php @@ -106,15 +106,15 @@ public function rename(string $fromPath, string $toPath): bool } try { - $this->stream_open($fromPath, 'r', 0, $openedPath); + $context = $this->getContext($fromPath, 'r'); } catch (FileNotFoundException $e) { return false; } - $newName = explode('/', $toPath, 4)[3] ?? ''; - assert($this->stream instanceof ReadableStream); + $newFilename = explode('/', $toPath, 4)[3] ?? ''; + $context['collectionWrapper']->updateFilenameForFilename($context['file']->filename, $newFilename); - return $this->stream->rename($newName); + return true; } /** @@ -170,41 +170,12 @@ public function stream_eof(): bool */ public function stream_open(string $path, string $mode, int $options, ?string &$openedPath): bool { - $context = []; - - /** - * The Bucket methods { @see Bucket::openUploadStream() } and { @see Bucket::openDownloadStreamByFile() } - * always set an internal context. But the context can also be set by the user. - */ - if (is_resource($this->context)) { - $context = stream_context_get_options($this->context)['gridfs'] ?? []; - - if (! is_array($context)) { - throw LogicException::invalidContext($context); - } - } - - // When the stream is opened using fopen(), the context is not required, it can contain only options. - if (! isset($context['collectionWrapper'])) { - $bucketAlias = explode('/', $path, 4)[2] ?? ''; - - if (! isset(self::$contextResolvers[$bucketAlias])) { - throw LogicException::bucketAliasNotRegistered($bucketAlias); - } - - $context = self::$contextResolvers[$bucketAlias]($path, $mode, $context); - } - - if (! $context['collectionWrapper'] instanceof CollectionWrapper) { - throw LogicException::invalidContextCollectionWrapper($context['collectionWrapper']); - } - if ($mode === 'r' || $mode === 'rb') { - return $this->initReadableStream($context); + return $this->initReadableStream($this->getContext($path, $mode)); } if ($mode === 'w' || $mode === 'wb') { - return $this->initWritableStream($context); + return $this->initWritableStream($this->getContext($path, $mode)); } throw LogicException::openModeNotSupported($mode); @@ -326,15 +297,10 @@ public function stream_write(string $data): int public function unlink(string $path): bool { - try { - $this->stream_open($path, 'w', 0, $openedPath); - } catch (FileNotFoundException $e) { - return false; - } + $context = $this->getContext($path, 'r'); + $count = $context['collectionWrapper']->deleteFileAndChunksByFilename($context['file']->filename); - assert($this->stream instanceof WritableStream); - - return $this->stream->delete() > 0; + return $count > 0; } /** @return false|array */ @@ -351,6 +317,41 @@ public function url_stat(string $path, int $flags) return $this->stream_stat(); } + /** @return array{collectionWrapper: CollectionWrapper, file: object}|array{collectionWrapper: CollectionWrapper, filename: string, options: array */ + private function getContext(string $path, string $mode): array + { + $context = []; + + /** + * The Bucket methods { @see Bucket::openUploadStream() } and { @see Bucket::openDownloadStreamByFile() } + * always set an internal context. But the context can also be set by the user. + */ + if (is_resource($this->context)) { + $context = stream_context_get_options($this->context)['gridfs'] ?? []; + + if (! is_array($context)) { + throw LogicException::invalidContext($context); + } + } + + // When the stream is opened using fopen(), the context is not required, it can contain only options. + if (! isset($context['collectionWrapper'])) { + $bucketAlias = explode('/', $path, 4)[2] ?? ''; + + if (! isset(self::$contextResolvers[$bucketAlias])) { + throw LogicException::bucketAliasNotRegistered($bucketAlias); + } + + $context = self::$contextResolvers[$bucketAlias]($path, $mode, $context); + } + + if (! $context['collectionWrapper'] instanceof CollectionWrapper) { + throw LogicException::invalidContextCollectionWrapper($context['collectionWrapper']); + } + + return $context; + } + /** * Returns a stat template with default values. */ diff --git a/src/GridFS/WritableStream.php b/src/GridFS/WritableStream.php index f5ef86c31..096968bb6 100644 --- a/src/GridFS/WritableStream.php +++ b/src/GridFS/WritableStream.php @@ -173,21 +173,6 @@ public function close(): void $this->isClosed = true; } - /** - * Delete all files and chunks associated with this stream filename. - * - * @return int The number of deleted files - */ - public function delete(): int - { - try { - return $this->collectionWrapper->deleteFileAndChunksByFilename($this->file['filename']); - } finally { - // Prevent further operations on this stream - $this->abort(); - } - } - /** * Return the stream's file document. */ From c2bcab1e5b2b6fd89dfb3129d37c617b03481b35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Wed, 10 Jan 2024 11:50:56 +0100 Subject: [PATCH 5/8] Fix phpdoc --- src/GridFS/StreamWrapper.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/GridFS/StreamWrapper.php b/src/GridFS/StreamWrapper.php index 2ea3de7db..68c679e63 100644 --- a/src/GridFS/StreamWrapper.php +++ b/src/GridFS/StreamWrapper.php @@ -317,7 +317,7 @@ public function url_stat(string $path, int $flags) return $this->stream_stat(); } - /** @return array{collectionWrapper: CollectionWrapper, file: object}|array{collectionWrapper: CollectionWrapper, filename: string, options: array */ + /** @return array{collectionWrapper: CollectionWrapper, file: object}|array{collectionWrapper: CollectionWrapper, filename: string, options: array} */ private function getContext(string $path, string $mode): array { $context = []; From 981ede2317f5fb9a524916d13fb9c21061f295f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Wed, 10 Jan 2024 15:14:35 +0100 Subject: [PATCH 6/8] Remove exception from rename and unlink. Return false --- ...ucket-registerGlobalStreamWrapperAlias.txt | 7 +++-- psalm-baseline.xml | 27 ++++++++----------- src/GridFS/CollectionWrapper.php | 8 +++--- src/GridFS/StreamWrapper.php | 21 +++++++-------- tests/GridFS/StreamWrapperFunctionalTest.php | 9 ++++++- 5 files changed, 38 insertions(+), 34 deletions(-) diff --git a/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt b/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt index 71bd06c63..99f296d60 100644 --- a/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt +++ b/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt @@ -48,8 +48,8 @@ Supported stream functions: - :php:`filesize() ` - :php:`file() ` - :php:`fopen() ` with "r", "rb", "w", and "wb" modes -- :php:`rename() ` rename all revisions of a file in the same bucket -- :php:`unlink() ` remove all revisions of a file in the same bucket +- :php:`rename() ` +- :php:`unlink() ` In read mode, the stream context can contain the option ``gridfs['revision']`` to specify the revision number of the file to read. If omitted, the most recent @@ -58,6 +58,9 @@ revision is read (revision ``-1``). In write mode, the stream context can contain the option ``gridfs['chunkSizeBytes']``. If omitted, the defaults are inherited from the ``Bucket`` instance option. +The functions `rename` and `unlink` will rename or remove all revisions of a +filename. If the filename does not exist, these functions will return ``false``. + Example ------- diff --git a/psalm-baseline.xml b/psalm-baseline.xml index a8ff05b22..0eac0407a 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -97,27 +97,22 @@ - + + private function getContext(string $path, string $mode): array + + getContext($path, $mode)]]> getContext($path, $mode)]]> - - - $context - - - array{collectionWrapper: CollectionWrapper, file: object}|array{collectionWrapper: CollectionWrapper, filename: string, options: array - - - filename]]> - filename]]> - + $context + $count + $count - - - - + + deleteFileAndChunksByFilename + updateFilenameForFilename + diff --git a/src/GridFS/CollectionWrapper.php b/src/GridFS/CollectionWrapper.php index e44ecfb7c..e9d8eef9f 100644 --- a/src/GridFS/CollectionWrapper.php +++ b/src/GridFS/CollectionWrapper.php @@ -83,7 +83,7 @@ public function deleteChunksByFilesId($id): void /** * Delete all GridFS files and chunks for a given filename. */ - public function deleteFileAndChunksByFilename(string $filename): int + public function deleteFileAndChunksByFilename(string $filename): ?int { /** @var iterable $files */ $files = $this->findFiles(['filename' => $filename], [ @@ -100,7 +100,7 @@ public function deleteFileAndChunksByFilename(string $filename): int $count = $this->filesCollection->deleteMany(['_id' => ['$in' => $ids]])->getDeletedCount(); $this->chunksCollection->deleteMany(['files_id' => ['$in' => $ids]]); - return $count ?? 0; + return $count; } /** @@ -279,12 +279,12 @@ public function insertFile($file): void /** * Updates the filename field in the file document for all the files with a given filename. */ - public function updateFilenameForFilename(string $filename, string $newFilename): UpdateResult + public function updateFilenameForFilename(string $filename, string $newFilename): ?int { return $this->filesCollection->updateMany( ['filename' => $filename], ['$set' => ['filename' => $newFilename]], - ); + )->getModifiedCount(); } /** diff --git a/src/GridFS/StreamWrapper.php b/src/GridFS/StreamWrapper.php index 68c679e63..66d71d11c 100644 --- a/src/GridFS/StreamWrapper.php +++ b/src/GridFS/StreamWrapper.php @@ -105,16 +105,14 @@ public function rename(string $fromPath, string $toPath): bool throw LogicException::renamePathMismatch($fromPath, $toPath); } - try { - $context = $this->getContext($fromPath, 'r'); - } catch (FileNotFoundException $e) { - return false; - } + $context = $this->getContext($fromPath, 'w'); $newFilename = explode('/', $toPath, 4)[3] ?? ''; - $context['collectionWrapper']->updateFilenameForFilename($context['file']->filename, $newFilename); + $count = $context['collectionWrapper']->updateFilenameForFilename($context['filename'], $newFilename); - return true; + // If $count === 0, the file does not exist. + // If $count is null, the update is unacknowledged, the operation is considered successful. + return $count !== 0; } /** @@ -297,10 +295,10 @@ public function stream_write(string $data): int public function unlink(string $path): bool { - $context = $this->getContext($path, 'r'); - $count = $context['collectionWrapper']->deleteFileAndChunksByFilename($context['file']->filename); + $context = $this->getContext($path, 'w'); + $count = $context['collectionWrapper']->deleteFileAndChunksByFilename($context['filename']); - return $count > 0; + return $count !== 0; } /** @return false|array */ @@ -317,7 +315,7 @@ public function url_stat(string $path, int $flags) return $this->stream_stat(); } - /** @return array{collectionWrapper: CollectionWrapper, file: object}|array{collectionWrapper: CollectionWrapper, filename: string, options: array} */ + /** @psalm-return ($mode == 'r' ? array{collectionWrapper: CollectionWrapper, file: object} : array{collectionWrapper: CollectionWrapper, filename: string, options: array}) */ private function getContext(string $path, string $mode): array { $context = []; @@ -342,6 +340,7 @@ private function getContext(string $path, string $mode): array throw LogicException::bucketAliasNotRegistered($bucketAlias); } + /** @see Bucket::resolveStreamContext() */ $context = self::$contextResolvers[$bucketAlias]($path, $mode, $context); } diff --git a/tests/GridFS/StreamWrapperFunctionalTest.php b/tests/GridFS/StreamWrapperFunctionalTest.php index 55a030953..2a7429720 100644 --- a/tests/GridFS/StreamWrapperFunctionalTest.php +++ b/tests/GridFS/StreamWrapperFunctionalTest.php @@ -375,10 +375,14 @@ public function testRenameAllRevisions(): void $this->assertSame(6, file_put_contents($path, 'foobar')); $this->assertSame(6, file_put_contents($path, 'foobar')); - $this->assertTrue(rename($path, $path . '.renamed')); + $result = rename($path, $path . '.renamed'); + $this->assertTrue($result); $this->assertTrue(file_exists($path . '.renamed')); $this->assertFalse(file_exists($path)); $this->assertSame('foobar', file_get_contents($path . '.renamed')); + + $result = rename($path, $path . '.renamed'); + $this->assertFalse($result, 'File does not exist anymore'); } public function testRenamePathMismatch(): void @@ -401,5 +405,8 @@ public function testUnlinkAllRevisions(): void $this->assertTrue($result); $this->assertFalse(file_exists($path)); + + $result = unlink($path); + $this->assertFalse($result, 'File does not exist anymore'); } } From 70c71e81f0716792d9a2b951f94265190ca7b69f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Wed, 10 Jan 2024 16:50:30 +0100 Subject: [PATCH 7/8] Thrown a FileNotFoundException in rename and unlink --- ...ucket-registerGlobalStreamWrapperAlias.txt | 2 +- .../Exception/FileNotFoundException.php | 11 +++++++++ src/GridFS/StreamWrapper.php | 23 +++++++++++++++---- tests/GridFS/StreamWrapperFunctionalTest.php | 10 ++++---- 4 files changed, 37 insertions(+), 9 deletions(-) diff --git a/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt b/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt index 99f296d60..59991e613 100644 --- a/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt +++ b/docs/reference/method/MongoDBGridFSBucket-registerGlobalStreamWrapperAlias.txt @@ -59,7 +59,7 @@ In write mode, the stream context can contain the option ``gridfs['chunkSizeByte If omitted, the defaults are inherited from the ``Bucket`` instance option. The functions `rename` and `unlink` will rename or remove all revisions of a -filename. If the filename does not exist, these functions will return ``false``. +filename. If the filename does not exist, these functions throw a ``FileNotFoundException``. Example ------- diff --git a/src/GridFS/Exception/FileNotFoundException.php b/src/GridFS/Exception/FileNotFoundException.php index 14d4dceb7..dbf8b08e1 100644 --- a/src/GridFS/Exception/FileNotFoundException.php +++ b/src/GridFS/Exception/FileNotFoundException.php @@ -25,6 +25,17 @@ class FileNotFoundException extends RuntimeException { + /** + * Thrown when a file cannot be found by its filename. + * + * @param string $filename Filename + * @return self + */ + public static function byFilename(string $filename) + { + return new self(sprintf('File with name "%s" not found', $filename)); + } + /** * Thrown when a file cannot be found by its filename and revision. * diff --git a/src/GridFS/StreamWrapper.php b/src/GridFS/StreamWrapper.php index 66d71d11c..95c39cdb3 100644 --- a/src/GridFS/StreamWrapper.php +++ b/src/GridFS/StreamWrapper.php @@ -96,7 +96,8 @@ public static function register(string $protocol = 'gridfs'): void /** * Rename all revisions of a filename. * - * @return bool True on success or false on failure. + * @return true + * @throws FileNotFoundException */ public function rename(string $fromPath, string $toPath): bool { @@ -110,9 +111,12 @@ public function rename(string $fromPath, string $toPath): bool $newFilename = explode('/', $toPath, 4)[3] ?? ''; $count = $context['collectionWrapper']->updateFilenameForFilename($context['filename'], $newFilename); - // If $count === 0, the file does not exist. + if ($count === 0) { + throw FileNotFoundException::byFilename($fromPath); + } + // If $count is null, the update is unacknowledged, the operation is considered successful. - return $count !== 0; + return true; } /** @@ -293,12 +297,23 @@ public function stream_write(string $data): int return $this->stream->writeBytes($data); } + /** + * Remove all revisions of a filename. + * + * @return true + * @throws FileNotFoundException + */ public function unlink(string $path): bool { $context = $this->getContext($path, 'w'); $count = $context['collectionWrapper']->deleteFileAndChunksByFilename($context['filename']); - return $count !== 0; + if ($count === 0) { + throw FileNotFoundException::byFilename($path); + } + + // If $count is null, the update is unacknowledged, the operation is considered successful. + return true; } /** @return false|array */ diff --git a/tests/GridFS/StreamWrapperFunctionalTest.php b/tests/GridFS/StreamWrapperFunctionalTest.php index 2a7429720..53b68ea43 100644 --- a/tests/GridFS/StreamWrapperFunctionalTest.php +++ b/tests/GridFS/StreamWrapperFunctionalTest.php @@ -381,8 +381,9 @@ public function testRenameAllRevisions(): void $this->assertFalse(file_exists($path)); $this->assertSame('foobar', file_get_contents($path . '.renamed')); - $result = rename($path, $path . '.renamed'); - $this->assertFalse($result, 'File does not exist anymore'); + $this->expectException(FileNotFoundException::class); + $this->expectExceptionMessage('File with name "gridfs://bucket/filename" not found'); + rename($path, $path . '.renamed'); } public function testRenamePathMismatch(): void @@ -406,7 +407,8 @@ public function testUnlinkAllRevisions(): void $this->assertTrue($result); $this->assertFalse(file_exists($path)); - $result = unlink($path); - $this->assertFalse($result, 'File does not exist anymore'); + $this->expectException(FileNotFoundException::class); + $this->expectExceptionMessage('File with name "gridfs://bucket/path/to/filename" not found'); + unlink($path); } } From 1fa8976f1d8d1d4f5588506c2b1703548e455cc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Thu, 11 Jan 2024 10:09:11 +0100 Subject: [PATCH 8/8] Fix rename with same file name --- src/GridFS/CollectionWrapper.php | 2 +- src/GridFS/StreamWrapper.php | 5 ++++- tests/GridFS/StreamWrapperFunctionalTest.php | 18 ++++++++++++++++++ 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/GridFS/CollectionWrapper.php b/src/GridFS/CollectionWrapper.php index e9d8eef9f..25e8b32a7 100644 --- a/src/GridFS/CollectionWrapper.php +++ b/src/GridFS/CollectionWrapper.php @@ -284,7 +284,7 @@ public function updateFilenameForFilename(string $filename, string $newFilename) return $this->filesCollection->updateMany( ['filename' => $filename], ['$set' => ['filename' => $newFilename]], - )->getModifiedCount(); + )->getMatchedCount(); } /** diff --git a/src/GridFS/StreamWrapper.php b/src/GridFS/StreamWrapper.php index 95c39cdb3..b433c8dd6 100644 --- a/src/GridFS/StreamWrapper.php +++ b/src/GridFS/StreamWrapper.php @@ -330,7 +330,10 @@ public function url_stat(string $path, int $flags) return $this->stream_stat(); } - /** @psalm-return ($mode == 'r' ? array{collectionWrapper: CollectionWrapper, file: object} : array{collectionWrapper: CollectionWrapper, filename: string, options: array}) */ + /** + * @return array{collectionWrapper: CollectionWrapper, file: object}|array{collectionWrapper: CollectionWrapper, filename: string, options: array} + * @psalm-return ($mode == 'r' or $mode == 'rb' ? array{collectionWrapper: CollectionWrapper, file: object} : array{collectionWrapper: CollectionWrapper, filename: string, options: array}) + */ private function getContext(string $path, string $mode): array { $context = []; diff --git a/tests/GridFS/StreamWrapperFunctionalTest.php b/tests/GridFS/StreamWrapperFunctionalTest.php index 53b68ea43..8aea21915 100644 --- a/tests/GridFS/StreamWrapperFunctionalTest.php +++ b/tests/GridFS/StreamWrapperFunctionalTest.php @@ -386,6 +386,24 @@ public function testRenameAllRevisions(): void rename($path, $path . '.renamed'); } + public function testRenameSameFilename(): void + { + $this->bucket->registerGlobalStreamWrapperAlias('bucket'); + $path = 'gridfs://bucket/filename'; + + $this->assertSame(6, file_put_contents($path, 'foobar')); + + $result = rename($path, $path); + $this->assertTrue($result); + $this->assertTrue(file_exists($path)); + $this->assertSame('foobar', file_get_contents($path)); + + $path = 'gridfs://bucket/missing'; + $this->expectException(FileNotFoundException::class); + $this->expectExceptionMessage('File with name "gridfs://bucket/missing" not found'); + rename($path, $path); + } + public function testRenamePathMismatch(): void { $this->expectException(LogicException::class);