PHPLIB-1220: Support codec option for GridFS buckets #1149

alcaeus · 2023-09-05T12:20:15Z

Note that the Bucket class does not forward the codec option to the underlying collections, as they can't both work with the same codec (it's only applicable to file documents). Setting a codec on the file collection also wouldn't work, as the collection would expect to apply the codec when inserting a file document. To work around these issues, the codec is manually applied in the findOne, find, and getFileDocumentForStream methods.

src/GridFS/WritableStream.php

GromNaN

The test codec is a good simple example of object mapping 👏🏻.

LGTM. It's up to you to allow null to unset the codec option.

src/GridFS/Bucket.php

src/GridFS/WritableStream.php

GromNaN

LGTM

src/GridFS/Bucket.php

jmikola · 2023-09-06T21:22:25Z

src/GridFS/WritableStream.php

@@ -136,6 +136,8 @@ public function __construct(CollectionWrapper $collectionWrapper, string $filena
            '_id' => $options['_id'],
            'chunkSize' => $this->chunkSize,
            'filename' => $filename,
+            'length' => 0,


This is not initialized until fileCollectionInsert(). Is there any reason to use 0 here instead of null like you do for uploadDate below? null more clearly communicates that it's unset vs. being a potentially empty file, just as a null upload date infers that we're in a pre-upload state.

The only reason I can think otherwise might be if you were defining an array shape on this, but I don't see any evidence of that.

I discovered this when calling getFileDocumentForStream, which tried to operate on the partial data available. Adding null values here allowed me to call $document->get('...') in the codec. While GridFS files are expected to always have a length and uploadDate, during the state of uploading (i.e. when the file hasn't been persisted to the database yet) this data is missing entirely, making the previous assumption false. I'm not sure what the better course of action is: ensuring that the data always has all fields, or making the codec accept partial documents that have some fields missing.

I noted that you changed length to null, which is actually all I was curious about.

The explanation is very helpful, though ✌️

jmikola · 2023-09-07T14:56:32Z

src/GridFS/Bucket.php

@@ -166,6 +175,7 @@ public function __construct(Manager $manager, string $databaseName, array $optio
        $this->databaseName = $databaseName;
        $this->bucketName = $options['bucketName'];
        $this->chunkSizeBytes = $options['chunkSizeBytes'];
+        $this->codec = $options['codec'] ?? null;


I understand your explanation for not injecting codec into $collectionOptions and only applying on-demand in specific methods, but now I'm curious if we're handling typeMap incorrectly. That is set directly into $collectionOptions and ends up applying to most calls on the files and chunks collections.

Bucket uses getRawFileDocumentForStream() internally as needed to access a raw document (without the type map). Does that not also cover us for a codec? Various CollectionWrapper methods explicitly set a simple typeMap when we need consistent access to a document, and that should avoid a codec being inherited.

The main problem with the codec is that it also applies on incoming data for operations like insertOne, while typeMap only applies to outgoing data. Applying a codec on the collection broke inserting GridFS files, which is when I noticed that something was up.

In an ideal world, we'd be handling everything as raw BSON data internally and only decoding it using typeMap or codec when necessary to do so. However, I believe this goes beyond the scope of what we want to do here. I'd be in favour of revisiting this when we make changes to our GridFS API to accommodate the recent request to make the stream protocol available to consumers. The refactoring necessary to accommodate this would also allow us to change the Bucket to internally always work with BSON documents and removing this special handling entirely. The collections would then receive a ['root' => 'bson'] type map, and all handling regarding codecs and type maps for the file document would be handled by the Bucket class.

src/GridFS/Bucket.php

jmikola · 2023-09-07T14:58:32Z

tests/Fixtures/Codec/TestFileCodec.php

+        $fileObject->filename = $value->get('filename');
+
+        $uploadDate = $value->get('uploadDate');
+        $fileObject->uploadDate = $uploadDate ? DateTimeImmutable::createFromMutable($uploadDate->toDateTime()) : null;


Did this prompt PHPC-2286?

Prompted by #1154 (comment).

jmikola · 2023-09-07T14:58:41Z

tests/Fixtures/Codec/TestFileCodec.php

+        $fileObject = new TestFile();
+        $fileObject->id = $value->get('_id');
+        $fileObject->length = (int) $value->get('length');
+        $fileObject->chunkSize = (int) $value->get('chunkSize');


What is the purpose of the int casts here? I notice you don't do casts for filename, but perhaps that's handled by PHP enforcing TestFile's property type.

Since we're operating on raw BSON, we may get Int64 instances, however unlikely they may be (especially for chunkSize). I decided to be prudent and cast these to int to avoid that issue.

That was my assumption. Thanks for confirming!

jmikola · 2023-09-07T14:59:39Z

tests/Fixtures/Codec/TestFileCodec.php

+        $fileObject->uploadDate = $uploadDate ? DateTimeImmutable::createFromMutable($uploadDate->toDateTime()) : null;
+
+        if ($value->has('metadata')) {
+            $fileObject->metadata = $value->get('metadata');


Should this assert that metadata is a document? I don't think any other type is expected.

Or is that possibly a can of worms because then we'd want a codec for that as well?

Assertion added so I can call toPHP() on it, see comment below.

jmikola · 2023-09-07T14:59:50Z

tests/Fixtures/Document/TestFile.php

+    public int $chunkSize;
+    public ?DateTimeImmutable $uploadDate = null;
+    public string $filename;
+    public $metadata;


Per my earlier question, should this be typed?

Added a ?stdClass doctype and called toPHP on the metadata document to show that the "old" way of decoding data is still available.

jmikola · 2023-09-07T15:00:36Z

tests/Fixtures/Document/TestFile.php

+
+final class TestFile
+{
+    public $id;


Noted that GridFS permits custom _id values, so we'd be wrong to force ObjectId here.

GromNaN · 2023-09-11T07:13:06Z

src/GridFS/Bucket.php

@@ -381,6 +404,10 @@ public function getFileDocumentForStream($stream)
    {
        $file = $this->getRawFileDocumentForStream($stream);

+        if ($this->codec) {
+            return $this->codec->decode(Document::fromPHP($file));


Here you re-create a BSON document that have already been decoded into PHP?

Unfortunately, yes: the document may not have come from the database but may be stored by the WritableStream instance as a hash.

alcaeus requested review from jmikola and GromNaN September 5, 2023 12:20

alcaeus self-assigned this Sep 5, 2023

alcaeus commented Sep 5, 2023

View reviewed changes

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

GromNaN approved these changes Sep 6, 2023

View reviewed changes

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

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

alcaeus requested a review from GromNaN September 6, 2023 08:57

GromNaN approved these changes Sep 6, 2023

View reviewed changes

alcaeus force-pushed the phplib-1220-gridfs-codec branch from c8dd067 to d6dce41 Compare September 6, 2023 10:15

GromNaN reviewed Sep 6, 2023

View reviewed changes

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

jmikola reviewed Sep 7, 2023

View reviewed changes

alcaeus force-pushed the phplib-1220-gridfs-codec branch 2 times, most recently from fca2389 to a326ea4 Compare September 8, 2023 08:56

jmikola approved these changes Sep 8, 2023

View reviewed changes

alcaeus added 7 commits September 11, 2023 08:39

PHPLIB-1220: Support codec option for GridFS buckets

6651e3b

Support null as an uploadDate in test codec

66ac79b

Add test to ensure bucket codec can be overridden

d4cfa08

Omit instanceof check for codec

f1d4d29

Forbid specifying codec and typeMap in bucket options

bf8e887

Use null as default value for length

ed114f1

Convert embedded metadata document to stdClass

f3a3be3

alcaeus force-pushed the phplib-1220-gridfs-codec branch from a326ea4 to f3a3be3 Compare September 11, 2023 06:40

GromNaN reviewed Sep 11, 2023

View reviewed changes

alcaeus merged commit 7da7372 into mongodb:master Sep 11, 2023

alcaeus deleted the phplib-1220-gridfs-codec branch September 11, 2023 07:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPLIB-1220: Support codec option for GridFS buckets #1149

PHPLIB-1220: Support codec option for GridFS buckets #1149

alcaeus commented Sep 5, 2023

GromNaN left a comment

GromNaN left a comment

jmikola Sep 6, 2023

alcaeus Sep 8, 2023

jmikola Sep 8, 2023

jmikola Sep 7, 2023

alcaeus Sep 8, 2023

jmikola Sep 7, 2023

GromNaN Sep 7, 2023

jmikola Sep 7, 2023

alcaeus Sep 8, 2023

jmikola Sep 8, 2023

jmikola Sep 7, 2023

alcaeus Sep 8, 2023

jmikola Sep 7, 2023

alcaeus Sep 8, 2023

jmikola Sep 7, 2023

GromNaN Sep 11, 2023

alcaeus Sep 11, 2023

PHPLIB-1220: Support codec option for GridFS buckets #1149

PHPLIB-1220: Support codec option for GridFS buckets #1149

Conversation

alcaeus commented Sep 5, 2023

GromNaN left a comment

Choose a reason for hiding this comment

GromNaN 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

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