Add MongoDB GridFS Adapter #1794

GromNaN · 2024-05-13T10:03:22Z

GridFS is a file system that uses MongoDB as storage. I would like to propose this adapter which uses the MongoDB PHP Library.

Docs PR: #1795

The legacy GridFS Flysystem adapter had some significant number of installations, even if it used the legacy Mongo extension. It shows there's some interest.

Important notes:

Directories does not exist in GridFS. I use the same technic as for S3 when explicitly creating a directory: an empty file with a trailing / is created. The creation of a file with a name ending in a / is therefore prohibited.
Permissions are not supported by GridFS. When explicitly set, I store the visibility in the metadata.flysystem_visibility attribute of the file.
GridFS' contentType attribute is deprecated. The attribute metadata.contentType is used instead.
GridFS supports versioning by default. Writing to an existing file creates a new revision. Deleting removes all the revisions. Moving renames all the revisions.
For GitHub Action, the mongodb extension is installed and a mongodb server is started using Docker Compose.
I added some tests in the shared FilesystemAdapterTestCase in order to improve coverage on mimeType and visibility.

src/GridFS/.github/FUNDING.yml

src/GridFS/GridFSAdapter.php

jmikola · 2024-05-13T16:10:31Z

src/GridFS/GridFSAdapter.php

+        }
+
+        try {
+            $this->bucket->getFilesCollection()->updateMany(


Noted that this is the same approach used in mongodb/mongo-php-library#1207 (PHPLIB-1324), despite there being no standard approach documented in the GridFS spec.

src/GridFS/GridFSAdapter.php

jmikola · 2024-05-13T16:21:18Z

src/GridFS/GridFSAdapter.php

+        }
+    }
+
+    public function createDirectory(string $path, Config $config): void


Is this the same approach used in other storage drivers that don't have a native concept of directories (e.g. S3)? I wonder if it'd be sensible to add a boolean metadata field to identify this as a directory, as there is otherwise nothing to distinguish this from an empty file.

I'm thinking more about other tooling that may work with the same GridFS bucket, even if such a field isn't directly usable by this adapter.

Using this adapter, trailing / are forbidden. So a file with a trailing / is a directory.

I just added a metadata.flysystem_directory as marker.

src/GridFS/LICENSE

src/GridFS/composer.json

frankdejonge · 2024-05-14T07:17:39Z

Hi all, I'm out this week but will get on this ASAP to get this out. Thanks for the effort and the interest to contribute. Would there be anybody interested from your end to do continued maintenance in this adapter? And if so, who? Then I know who to rope in with issues.

GromNaN · 2024-05-14T07:41:55Z

Hello Frank, we are a team of currently 3 devs, led by @alcaeus. We use an open Jira to organise our work and Flysystem is one of the packages we want to maintain.

src/GridFS/GridFSAdapter.php

frankdejonge · 2024-05-18T15:22:43Z

src/GridFS/GridFSAdapter.php

+        }
+
+        // Deleting a file that does not exist is no-op
+        while ($file = $this->findFile($path)) {


why is this a while loop?

this block may be affected by a race condition when two deletions are happening concurrently. In other implementations we try to delete and ignore the exception if it was caused by a "file not found" situation. Can we inspect the exception to see if occurs and ignore the error when that's the case?

The while was necessary to remove all the revisions of the file name. I replaced the code with a "find all revisions" followed by "delete" on each revision. Good point about race-condition, I added a try-catch.

src/GridFS/GridFSAdapter.php

Fix ext-mongodb explicit dependency for exception Use generic type for file object

…-conditions

src/GridFS/GridFSAdapter.php

jmikola · 2024-05-20T14:34:50Z

src/GridFS/GridFSAdapter.php

+        ];
+
+        if ($visibility = $config->get(Config::OPTION_VISIBILITY)) {
+            $options['metadata'][self::METADATA_VISIBILITY] = $visibility;


What's the purpose of this option? I don't follow why it's only injected into metadata when truthy. I noted this same logic appears earlier in the write and writeStream methods.

It seems contrary to the logic in setVisibility() below, which could end up storing a true or false value to the field.

I don't want to store visibility unless it's explicitly set. setVisibility requires a value. For write, writeStream, createDirectory and copy, this is an option.

@frankdejonge Since there's no concept of file-level permission in GridFS, would it be better to disable the functionality altogether, as is done for WebDAV?

src/GridFS/GridFSAdapter.php

jmikola · 2024-05-20T15:04:44Z

src/GridFS/GridFSAdapter.php

+class GridFSAdapter implements FilesystemAdapter
+{
+    private const METADATA_DIRECTORY = 'flysystem_directory';
+    private const METADATA_VISIBILITY = 'flysystem_visibility';


What's the purpose of this metadata field? I only see it being set during write operations and incorporated into returned FileAttributes or DirectoryAttributes.

Related to #1794 (comment)

jmikola

LGTM. I'll defer to you both to decide whether to remove the the visibility option (https://github.com/thephpleague/flysystem/pull/1794/files#r1606938015).

frankdejonge · 2024-05-21T18:49:38Z

Looks like the test case GridFSAdapterTest::move_all_revisions is failing.

GromNaN · 2024-05-21T18:53:42Z

Looks like the test case GridFSAdapterTest::move_all_revisions is failing.

Where do you see that? It seems to work in GitHub Actions: https://github.com/thephpleague/flysystem/actions/runs/9161336038/job/25242807530?pr=1794

frankdejonge · 2024-05-21T19:19:50Z

@GromNaN this is an example; https://github.com/thephpleague/flysystem/actions/runs/9180056390/job/25244027378?pr=1794 might be because of eventual consistency in MongoDB? Is that a thing? Previous run had more errors on the same test case.

…evisions order

GromNaN · 2024-05-21T20:27:20Z

Ok, I suspect that it's because 2 files are created in the same millisecond and revisions are sorted using this date.

frankdejonge · 2024-05-22T06:25:44Z

@GromNaN still one failing, is there another way to deal with this or to configure it? https://github.com/GromNaN/flysystem/actions/runs/9181466558/job/25248713035

GromNaN · 2024-05-22T06:41:59Z

It seems that my previous guess was correct but the fix incorrectly implemented. I added usleep(1000) (1ms) between each insertion of a revision to have a different uploadDate.

jmikola reviewed May 13, 2024

View reviewed changes

GromNaN force-pushed the gridfs branch 3 times, most recently from 511da2b to 8c7daa1 Compare May 14, 2024 06:59

GridFS Adapter

f716044

GromNaN force-pushed the gridfs branch from 8c7daa1 to f716044 Compare May 14, 2024 07:36

GromNaN mentioned this pull request May 14, 2024

Add docs for GridFS adapter #1795

Merged

frankdejonge reviewed May 18, 2024

View reviewed changes

src/GridFS/GridFSAdapter.php Outdated Show resolved Hide resolved

frankdejonge reviewed May 18, 2024

View reviewed changes

src/GridFS/GridFSAdapter.php Outdated Show resolved Hide resolved

frankdejonge reviewed May 18, 2024

View reviewed changes

src/GridFS/GridFSAdapter.php Outdated Show resolved Hide resolved

GromNaN added 2 commits May 20, 2024 10:15

Always map all attrributes

cf3a3e9

Fix ext-mongodb explicit dependency for exception Use generic type for file object

Factorize delete operation and ignore missing file ignore due to race…

c9a0436

…-conditions

GromNaN requested review from frankdejonge and jmikola May 20, 2024 11:29

jmikola reviewed May 20, 2024

View reviewed changes

GromNaN added 3 commits May 20, 2024 17:19

Use openDownloadStreamByName

9ec096c

Use mapFileAttributes in mimeType

86d94c5

Use file index in the specified order

f4a0e6c

GromNaN force-pushed the gridfs branch from 3840dc5 to f4a0e6c Compare May 20, 2024 15:46

jmikola approved these changes May 20, 2024

View reviewed changes

Update package name

39dcc17

Ensure files are created in a different millisecond in test to keep r…

8fef05c

…evisions order

GromNaN force-pushed the gridfs branch from 2355f43 to 8fef05c Compare May 21, 2024 20:25

GromNaN force-pushed the gridfs branch from e66f1a3 to 22b1361 Compare May 21, 2024 21:05

Set readPreference in tests

635eb5f

GromNaN force-pushed the gridfs branch from 22b1361 to 635eb5f Compare May 22, 2024 06:35

Remove read preference, as primary is the default

612b104

frankdejonge merged commit 17f5fab into thephpleague:3.x May 22, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MongoDB GridFS Adapter #1794

Add MongoDB GridFS Adapter #1794

GromNaN commented May 13, 2024 •

edited

jmikola May 13, 2024

jmikola May 13, 2024

GromNaN May 13, 2024

frankdejonge commented May 14, 2024

GromNaN commented May 14, 2024 •

edited

frankdejonge May 18, 2024

GromNaN May 20, 2024

jmikola May 20, 2024

GromNaN May 20, 2024

jmikola May 20, 2024

GromNaN May 20, 2024

jmikola left a comment

frankdejonge commented May 21, 2024

GromNaN commented May 21, 2024

frankdejonge commented May 21, 2024

GromNaN commented May 21, 2024

frankdejonge commented May 22, 2024

GromNaN commented May 22, 2024

Add MongoDB GridFS Adapter #1794

Add MongoDB GridFS Adapter #1794

Conversation

GromNaN commented May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankdejonge commented May 14, 2024

GromNaN commented May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

frankdejonge commented May 21, 2024

GromNaN commented May 21, 2024

frankdejonge commented May 21, 2024

GromNaN commented May 21, 2024

frankdejonge commented May 22, 2024

GromNaN commented May 22, 2024

GromNaN commented May 13, 2024 •

edited

GromNaN commented May 14, 2024 •

edited