[2.0] Add GridFS implementation on top of mongodb/mongodb #1790

alcaeus · 2018-05-13T10:13:12Z

Q	A
Type	feature
BC Break	yes
Fixed issues	#1697

Summary

This PR brings back support for GridFS. The idea is to provide mapped documents for GridFS files, while not re-implementing upload/download functionality currently in mongodb/mongodb. The general idea is:

Classes used for GridFS files are no longer mapped as documents, but rather as files
For classes mapped as files, only the fields specified in the GridFS specification (and not marked as deprecated) are allowed: _id, length, chunkSize, filename, uploadDate and metadata
Of the above fields, only the metadata field is checked for changes (if mapped)
Calling persist on a new file instance is not supported - the file has to be uploaded using the methods in the new GridFSRepository. This is because the specification suggests using streams, which isn't entirely feasible at this time

Mapping

Annotation mapping is done using special annotations (e.g. @File, @File\Filename, @File\UploadDate) that have predefined types and database field names and are marked as notSaved as a precaution.

XML mapping is done using special field mappings:

<gridfs-file name="Doctrine\ODM\MongoDB\Tests\Mapping\AbstractMappingDriverFile">
    <field fieldName="id" id="true" />
    <length fieldName="size" />
    <chunk-size />
    <upload-date />
    <filename fieldName="name" />

    <metadata target-document="Doctrine\ODM\MongoDB\Tests\Mapping\AbstractMappingDriverFileMetadata" />
</gridfs-file>

Note: the metadata field is treated like any other embed-one relationship; the repository does not check the given metadata object for its type when uploading a new file (see below).

Files and objects

Files can be read and hydrated into objects like all other documents. File contents are no longer available in the object itself but has to be read using the repository methods (downloadToStream at this time).

GridFS repository

This PR introduces a new interface for GridFS repositories along with a default implementation. The repository provides additional methods to upload and download files from and to streams. The repository is the only way to upload files, as mentioned above persisting a new file is not allowed and will cause an exception in UnitOfWork when calling persist($newFile). Once a file has been read from the database, it can be updated like all other documents.

BC breaks

Doctrine\ODM\MongoDB\Configuration: setDefaultRepositoryClassName and getDefaultRepositoryClassName have been replaced by setDefaultDocumentRepositoryClassName and getDefaultDocumentRepositoryClassName, respectively to accomodate the new GridFS repositories.
The @File annotation has been changed and is no longer a property-level annotation but a class-level annotation
The GridFS implementation no longer allows mapping arbitrary fields on the file level. Any fields not indicated in the GridFS specification need to be saved in the metadata document
The Doctrine\ODM\MongoDB\DocumentRepository class was moved to Doctrine\ODM\MongoDB\Repository\DocumentRepository.

Todo

Finalize tests
~~Document BC breaks in UPGRADE document~~
Write docs for GridFS feature, including migration

malarzm

Amazing work so far! Thanks @alcaeus

malarzm · 2018-05-14T13:56:26Z

lib/Doctrine/ODM/MongoDB/Configuration.php

     */
-    public function setDefaultRepositoryClassName($className)
+    public function setDefaultDocumentRepositoryClassName($className)


A note for future: this requires deprecation notice and maybe a forward compatibility in 1.3

lib/Doctrine/ODM/MongoDB/Configuration.php

+    }
+
+    /**
+     * Get default repository class for documents.


lib/Doctrine/ODM/MongoDB/Configuration.php

    }

    /**
-     * Get default repository class.
+     * Get default repository class for GridFS files.


lib/Doctrine/ODM/MongoDB/Repository/DocumentRepository.php

@@ -2,15 +2,22 @@

 declare(strict_types=1);

-namespace Doctrine\ODM\MongoDB;
+namespace Doctrine\ODM\MongoDB\Repository;


lib/Doctrine/ODM/MongoDB/Configuration.php

+     */
+    public function setDefaultGridFSRepositoryClassName($className)
+    {
+        $reflectionClass = new \ReflectionClass($className);


jwage · 2018-05-14T14:19:15Z

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php

+
+    private function isAllowedGridFSField(string $name): bool
+    {
+        return in_array($name, ['_id', 'chunkSize', 'filename', 'length', 'metadata', 'uploadDate'], true);


Move this array to a constant?

jwage · 2018-05-14T14:21:19Z

lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php

@@ -451,6 +461,15 @@ public function update($document, array $options = [])
     */
    public function delete($document, array $options = [])
    {
+        if ($this->bucket) {
+            $id = $this->uow->getDocumentIdentifier($document);
+            $id = $this->class->getDatabaseIdentifierValue($id);


Renaming these variables might make things a little more clear.

$documentIdentifier = $this->uow->getDocumentIdentifier($document); $databaseIdentifier = $this->class->getDatabaseIdentifierValue($documentIdentifier);

lib/Doctrine/ODM/MongoDB/Configuration.php

     */
-    public function setDefaultRepositoryClassName($className)
+    public function setDefaultDocumentRepositoryClassName($className)


lib/Doctrine/ODM/MongoDB/DocumentManager.php

        $metadata = $this->metadataFactory->getMetadataFor($className);
+        if ($metadata->isFile) {
+            return $this->getDocumentBucket($className)->getFilesCollection();


jmikola · 2018-05-14T18:34:22Z

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php

@@ -758,6 +765,10 @@ public function setDiscriminatorField($discriminatorField)
            return;
        }

+        if ($this->isFile) {
+            throw MappingException::discriminatorNotAllowedForGridFS($this->name);


Would it make sense for this to go above the previous conditional that allows null assignment?

The initial thought was to allow an extending class to remove the discriminator. On second thought, that kind of object hierarchy doesn't really make sense - I'll move the check.

jmikola · 2018-05-14T18:34:48Z

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php

@@ -820,6 +835,10 @@ public function setDefaultDiscriminatorValue($defaultDiscriminatorValue)
            return;
        }

+        if ($this->isFile) {
+            throw MappingException::discriminatorNotAllowedForGridFS($this->name);


Likewise a question if it makes sense to do this before the null assignment condition above.

jmikola · 2018-05-14T18:36:32Z

lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php

@@ -81,6 +85,9 @@ public function loadMetadataForClass($className, \Doctrine\Common\Persistence\Ma
                $class->setCollection((string) $xmlRoot['collection']);
            }
        }
+        if (isset($xmlRoot['bucket'])) {
+            $class->setCollection((string) $xmlRoot['bucket']);


In this case, are you setting the GridFS bucket prefix (e.g. "fs") as the collection name? If so, does that mean that interpretation of the collection property depends on the value of $class->isFile?

Unfortunately, yes. The alternative would be to bring in a bucket property and use that. That would mean consuming bucket if $class->isFile is true and collection if it isn't. It doesn't change the logic, but it removes the dual-use of the collection property.

I think that may be clearer, but I'll defer to @jwage and @malarzm.

I think we should enhance this to make it more clear and remove the dual-use of the collection property.

jmikola · 2018-05-14T18:43:43Z

lib/Doctrine/ODM/MongoDB/Repository/DefaultGridFSRepository.php

+
+class DefaultGridFSRepository extends DocumentRepository implements GridFSRepository
+{
+    public function downloadToStream($id, $destination): void


Would it make sense to have placeholder doc blocks here, even if they only report @see MongoDB\GridFS\Bucket::method()?

Isn't the style for return types with a space before : also used in ODM?
public function downloadToStream($id, $destination) : void

We currently exclude the rule - this will be adjusted in a separate PR.

As for the original comment, I'll add a docblock with a reference to the original method.

jmikola · 2018-05-14T18:45:12Z

lib/Doctrine/ODM/MongoDB/Repository/DefaultGridFSRepository.php

+
+        $id = $this->getDocumentBucket()->uploadFromStream($filename, $source, $options);
+
+        // TODO: apply primary read preference


Is this to ensure that the subsequent find() sees the same document that was just uploaded? Is there a particular reason you don't want to just return the document's ID as the library does?

Correct - it's to avoid issues like doctrine/mongodb#324 from the start. I haven't considered only returning the ID, it would've been nice to get a file you can use directly. The other option would be to return a proxy to avoid a read cycle that may be useless if the user doesn't want to use the newly persisted file.

jmikola · 2018-05-14T18:46:21Z

lib/Doctrine/ODM/MongoDB/Repository/DefaultGridFSRepository.php

+    public function uploadFromFile(string $source, ?string $filename = null, $metadata = null)
+    {
+        $resource = fopen($source, 'r');
+        if (! $resource) {


Should this be a strict check for false? I'm not sure if a valid file pointer in PHP can possibly evaluate to zero.

As far as I know a successful created resource is never true with a check like this $resource == false.

Using a type-safe check never hurts - will update this.

jmikola · 2018-05-14T18:47:27Z

lib/Doctrine/ODM/MongoDB/Repository/DefaultGridFSRepository.php

+            throw MongoDBException::cannotReadGridFSSourceFile($source);
+        }
+
+        if (! $filename) {


Would it make sense to explicitly check for null here, in case someone wanted to use an empty string or "0" as a filename?

Yes, the check should be stricter.

lib/Doctrine/ODM/MongoDB/SchemaManager.php

@@ -225,6 +234,10 @@ public function ensureDocumentIndexes($documentName, $timeout = null)
            throw new \InvalidArgumentException('Cannot create document indexes for mapped super classes, embedded documents or query result documents.');
        }

+        if ($class->isFile) {
+            $this->ensureGridFSIndexes($class);


SenseException · 2018-05-14T21:31:44Z

lib/Doctrine/ODM/MongoDB/SchemaManager.php

+    {
+        $chunksCollection = $this->dm->getDocumentBucket($class->getName())->getChunksCollection();
+        foreach ($chunksCollection->listIndexes() as $index) {
+            if ($index->isUnique() && $index->getKey() === ['files_id' => 1, 'n' => 1]) {


Could this duplicated array be in a constant or variable? It's also used in createIndex() a few lines later.

SenseException · 2018-05-14T21:32:28Z

lib/Doctrine/ODM/MongoDB/SchemaManager.php

+    {
+        $filesCollection = $this->dm->getDocumentCollection($class->getName());
+        foreach ($filesCollection->listIndexes() as $index) {
+            if ($index->getKey() === ['filename' => 1, 'uploadDate' => 1]) {


Same as with the array above.

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

+                try {
+                    $gridFSMetadata = $class->getFieldMappingByDbFieldName('metadata');
+                    $gridFSMetadataProperty = $gridFSMetadata['fieldName'];
+                } catch (MappingException $e) {


lib/Doctrine/ODM/MongoDB/Mapping/Annotations/File/Metadata.php

+/**
+ * @Annotation
+ */
+final class Metadata extends AbstractField


alcaeus · 2018-06-04T06:13:14Z

XML mappings have been finalized, see original comment for example. I've also created #1806 to track changing the ID mapping to the same kind of mapping as with GridFS. This is mainly done to avoid allowing generic field mappings in GridFS files, with the added benefit of clearer, more concise ID mappings for all documents.

jwage · 2018-06-09T19:38:51Z

docs/en/reference/annotations-reference.rst

+Optional attributes:
+
+-
+    targetDocument - A |FQCN| of the target document.


Wrap targetDocument, discriminatorField, etc. in ``? We do it in other parts of the docs and it is done in storing-files-with-mongogridfs.rst in this PR.

Copy-pasted it from the original documentation, I'll fix it throughout the file for consistency 👍

malarzm · 2018-07-31T19:14:32Z

docs/en/reference/storing-files-with-mongogridfs.rst

-    $image = $profile->getImage();
+    $stream = fopen('tmp/path/to/copy', 'w+');
+    try {
+        $repository->downloadToStream($file->getid(), $stream);


Nitpicking, but you have lowercase "i" in getId() ;)

Haha, will fix before merging.

malarzm

Feel free to disregard my comments and let's have this merged 👍

malarzm · 2018-07-31T19:18:08Z

lib/Doctrine/ODM/MongoDB/Configuration.php

@@ -342,20 +345,39 @@ public function getFilterParameters(string $name): ?array
    /**
     * @throws MongoDBException If not is a ObjectRepository.
     */
-    public function setDefaultRepositoryClassName(string $className): void
+    public function setDefaultDocumentRepositoryClassName(string $className): void


I think compat layer for this could actually should land in 1.3

Yes, definitely.

malarzm · 2018-07-31T19:19:52Z

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php

@@ -183,6 +185,12 @@ class ClassMetadata implements BaseClassMetadata
     */
    public $collection;

+    /**
+     * READ-ONLY: The name of the GridFS bucket the document is mapped to.
+     * @var string


This should be string|null I think

malarzm · 2018-07-31T19:20:44Z

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php

+     *
+     * @var int|null
+     */
+    public $chunkSizeBytes = null;


= null is redundant

malarzm · 2018-07-31T19:22:48Z

lib/Doctrine/ODM/MongoDB/MongoDBException.php

+
+    public static function cannotPersistGridFSFile(string $className): self
+    {
+        return new self(sprintf('Cannot persist GridFS file for class "%s" through UnitOfWork', $className));


Missing "." by the end

malarzm · 2018-07-31T19:22:57Z

lib/Doctrine/ODM/MongoDB/MongoDBException.php

+
+    public static function cannotReadGridFSSourceFile(string $filename): self
+    {
+        return new self(sprintf('Cannot open file "%s" for uploading to GridFS', $filename));


Missing "." by the end

docs/en/reference/annotations-reference.rst

+
+.. _file_chunksize:
+
+@File\ChunkSize


jmikola · 2018-07-31T19:17:12Z

docs/en/reference/storing-files-with-mongogridfs.rst

+    ObjectId, although you can override this in the mapping.
+-
+    ``chunkSize`` stores the size of a single chunk in bytes. By default, chunks
+    are 255 kb in size.


"kb" may be ambiguous. I'd suggest quoting the PHPLIB docs here: "261120 (i.e. 255 KiB)"

https://github.com/mongodb/mongo-php-library/blob/bd148eab0493e38354e45e2cd7db59b90fdcad79/src/GridFS/Bucket.php#L69-L70

Updated, thanks for the suggestion.

jmikola · 2018-07-31T19:18:29Z

docs/en/reference/storing-files-with-mongogridfs.rst

-    echo $image->getFile()->getBytes();
+With XML mappings, the fields are automatically mapped to camel-cased properties.
+To change property names, simply override the ``fieldName`` attribute for each
+field. You can not override any other options for GridFS fields.


Should this be "cannot"? I'm not sure what is most commonly used in the docs.

A quick search favours "cannot" in our docs - updated.

jmikola · 2018-07-31T19:20:10Z

docs/en/reference/storing-files-with-mongogridfs.rst

-You can of course make references to this Image document from
-another document. Imagine you had a Profile document and you wanted
-every Profile to have a profile image:
+The ``ImageMetadata`` class can be any embedded document:


It makes sense to say that the metadata document can be any embedded document class, but it seems odd to say that about ImageMetadata. ImageMetadata either is an embedded document class (can be used) or it isn't (cannot be used).

Reworded to "must be an embedded document".

docs/en/reference/storing-files-with-mongogridfs.rst

-    $image->setName('Test image');
-    $image->setFile('/path/to/image.png');
+    $repository = $documentManager->getRepository(Documents\Image::class);
+    $file = $repository->uploadFromFile('image.jpg', '/tmp/path/to/image', new Documents\ImageMetadata('image/jpeg'));


jmikola · 2018-07-31T19:22:25Z

docs/en/reference/storing-files-with-mongogridfs.rst

-    $dm->flush();
+When reading GridFS files, they behave like all other documents. You can query
+for them using the ``find*`` methods in the repository, create query or
+aggregation pipeline builders and also use them as ``targetDocument`` in


Oxford comma: "builders, and"

lib/Doctrine/ODM/MongoDB/Configuration.php

+     */
+    public function setDefaultGridFSRepositoryClassName(string $className): void
+    {
+        $reflectionClass = new \ReflectionClass($className);


lib/Doctrine/ODM/MongoDB/Mapping/Annotations/File.php

+    public $db;
+
+    /** @var string */
+    public $bucketName = 'fs';


jmikola · 2018-07-31T19:30:12Z

lib/Doctrine/ODM/MongoDB/Repository/DefaultGridFSRepository.php

+    /**
+     * @see Bucket::downloadToStream
+     */
+    public function downloadToStream($id, $destination): void


Did you still intend to create an openDownloadStream() method? If not in this PR, should there be a separate ticket to do so before 2.0?

See: https://github.com/doctrine/mongodb-odm/pull/1790/files#r203132136

That must have slipped through, I added it now.

…n type hints

This removes the dual-use from the collection property in ClassMetadata, storing the bucket name for GridFS files in the bucketName property. For compatibility, the collection name is initialized to the fs.files collection for the corresponding bucket.

jmikola · 2018-08-02T14:27:33Z

docs/en/reference/storing-files-with-mongogridfs.rst

-        private $name;
+If you want to pass options, such as a metadata object to the uploaded file, you
+If you want to add metadata to the uploaded file, you can pass it as the last
+can pass an ``UploadOptions`` object as the last argument to the


"can pass it as the last" on the line above should be deleted.

docs/en/reference/storing-files-with-mongogridfs.rst


-        /** @Field */
-        private $name;
+If you want to pass options, such as a metadata object to the uploaded file, you


jmikola · 2018-08-02T14:27:51Z

docs/en/reference/storing-files-with-mongogridfs.rst

+If you want to pass options, such as a metadata object to the uploaded file, you
+If you want to add metadata to the uploaded file, you can pass it as the last
+can pass an ``UploadOptions`` object as the last argument to the
+``uploadFromFile``, ``uploadFromStream`` or ``openUploadStream`` method call:


Oxford comma after uploadFromStream.

alcaeus added Feature BC Break labels May 13, 2018

alcaeus added this to the 2.0.0 milestone May 13, 2018

alcaeus self-assigned this May 13, 2018

alcaeus force-pushed the gridfs-2.0 branch 2 times, most recently from 2efc50b to d852a68 Compare May 13, 2018 18:53

malarzm reviewed May 14, 2018

View reviewed changes

jwage reviewed May 14, 2018

View reviewed changes

jmikola reviewed May 14, 2018

View reviewed changes

SenseException reviewed May 14, 2018

View reviewed changes

alcaeus force-pushed the gridfs-2.0 branch from d852a68 to 6b270f4 Compare May 18, 2018 05:43

alcaeus mentioned this pull request May 18, 2018

[2.0] Clean up docblocks in Configuration class #1796

Closed

This comment has been minimized.

Sign in to view

malarzm reviewed May 19, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

alcaeus force-pushed the gridfs-2.0 branch from 6b270f4 to 646f1c1 Compare June 4, 2018 05:33

alcaeus mentioned this pull request Jun 5, 2018

[2.0] Separate ID mapping from fields in XML driver #1807

Merged

alcaeus force-pushed the gridfs-2.0 branch 2 times, most recently from 5033ad0 to 35be2ef Compare June 8, 2018 05:16

jwage reviewed Jun 9, 2018

View reviewed changes

alcaeus force-pushed the gridfs-2.0 branch from 23d2808 to ac02a3b Compare June 15, 2018 06:15

alcaeus changed the title ~~[WIP] [2.0] Add GridFS implementation on top of mongodb/mongodb~~ [2.0] Add GridFS implementation on top of mongodb/mongodb Jun 15, 2018

alcaeus requested review from jwage, jmikola and malarzm June 15, 2018 06:15

alcaeus added 3 commits July 31, 2018 20:15

Only allow special fields in GridFS mappings

193ebec

Add tests for GridFS file mappings

39d31c8

Update SchemaManager to handle GridFS collections

fb19ba7

alcaeus force-pushed the gridfs-2.0 branch 2 times, most recently from 37243a5 to 11ba674 Compare July 31, 2018 19:02

malarzm reviewed Jul 31, 2018

View reviewed changes

malarzm approved these changes Jul 31, 2018

View reviewed changes

jmikola reviewed Jul 31, 2018

View reviewed changes

alcaeus added 11 commits August 1, 2018 18:05

Handle persisting and updating GridFS files in UnitOfWork

f54dbd5

Move DocumentRepository to Repository namespace

682aa79

Introduce separate repository for GridFS

119ff63

Ensure file deletion also deletes chunks

b01b8f1

Bump dependency to doctrine/common to fix proxy generation with retur…

4eecb1f

…n type hints

Add annotation reference for GridFS annotations

aca68a8

Add usage documentation for GridFS

2521ef7

Consistently use inline code for identifiers and options

49bd3c5

Add chunkSizeBytes option to GridFS file mapping

6f44159

Add openDownloadStream method to GridFSRepository

c943ec4

alcaeus force-pushed the gridfs-2.0 branch 2 times, most recently from 3b04df8 to d75dccc Compare August 2, 2018 06:24

jmikola reviewed Aug 2, 2018

View reviewed changes

alcaeus force-pushed the gridfs-2.0 branch from d75dccc to 44974ab Compare August 2, 2018 21:46

Replace GridFS option arguments with value object

f00a3c9

alcaeus force-pushed the gridfs-2.0 branch from 44974ab to f00a3c9 Compare August 3, 2018 04:59

alcaeus merged commit 13ea997 into doctrine:master Aug 3, 2018

alcaeus deleted the gridfs-2.0 branch August 3, 2018 05:18

alcaeus removed the Needs 1.3 Compat Layer label Feb 4, 2019

webdevilopers mentioned this pull request Dec 24, 2019

Custom repository class on GridFS File XML mapping is not used #2141

Closed


		$id = $this->getDocumentBucket()->uploadFromStream($filename, $source, $options);

		// TODO: apply primary read preference


		.. _file_chunksize:

		@File\ChunkSize

[2.0] Add GridFS implementation on top of mongodb/mongodb #1790

[2.0] Add GridFS implementation on top of mongodb/mongodb #1790

Conversation

alcaeus commented May 13, 2018 • edited Loading

Summary

Mapping

Files and objects

GridFS repository

BC breaks

Todo

malarzm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

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

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

Choose a reason for hiding this comment

jmikola May 14, 2018 • edited Loading

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

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment has been minimized.

This comment has been minimized.

alcaeus commented Jun 4, 2018

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

malarzm left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

alcaeus commented May 13, 2018 •

edited

Loading

jmikola May 14, 2018 •

edited

Loading