PHPLIB-1022: Codec implementation for better BSON decoding/encoding #1059

alcaeus · 2023-04-12T11:28:04Z

This pull request adds the concept of codecs to the library. A codec consists of an encoder and decoder (currently separated, but we could consolidate these into a single interface).

A decoder is responsible for decoding BSON data into PHP data, while the encoder converts PHP data into BSON data. This PR comes with two default codecs, LazyBSONDocumentCodec and LazyBSONArrayCodec. These work with new lazy classes that mimic the existing BSONArray and BSONDocument, but are backed by the new BSON Document and PackedArray classes provided in the new extension version. This allows us to defer reading and decoding BSON until data is actually accessed. Furthermore, only accessed fields are ever read, meaning that any BSON data that isn't actively used is never decoded and thus does not have a considerable memory or time impact.

For the time being, the new lazy classes do not extend the existing BSONArray and BSONDocument classes, but for backward compatibility it might be preferable to do so: this would allow us to enable this lazy behaviour by default. As is, this functionality is opt-in due to the BC break involved in returning classes that don't extend the current classes.

The next step is to change the Collection class to take a codec option, which would allow said class to decode and encode data in the various methods that would return modelled data. Apart from decoding BSON documents into specific PHP objects lazily, the codec functionality would also allow users to change the way BSON types are returned. For example, a DateTimeCodec could decode any MongoDB\BSON\UTCDateTime instances to a DateTime instance and vice-versa, with the collection class automatically converting those instances to UTCDateTime.

This is a first draft, and as such there are a number of items left to do:

Figure out naming (Array vs. PackedArray)
Support passing a codec to a Collection instance
(optional, desired) Make lazy document/array classes extend existing classes
(optional) Make Client aware of a codec library containing codecs that should always be used, e.g. to transform BSON types
(optional) Provide a codec map to associate a codec with a specific collection, avoiding the need to pass a codec option whenever fetching a Collection instance.

psalm.xml.dist

alcaeus · 2023-04-12T11:30:10Z

src/Codec/CodecLibrary.php

+                    $this->attachEncoder($item);
+                }
+
+                // Yes, we'll silently discard everything. Please let me already have union types...


We may want to replace this with an InvalidArgumentException - for now this is fine.

alcaeus · 2023-04-12T11:31:48Z

src/Codec/LazyBSONArrayCodec.php

+
+        $return = [];
+        foreach ($value as $offset => $offsetValue) {
+            $return[$offset] = $this->getLibrary()->canEncode($offsetValue) ? $this->getLibrary()->encode($offsetValue) : $offsetValue;


Currently codecs are designed to throw an exception if encode is called with an unsupported object. However, we may want to consider a encodeOrReturn kind of method that contains this logic to avoid repeating this all over the place.

encodeOrReturn() seems sensible.

alcaeus · 2023-04-12T11:33:12Z

src/Model/AsListIterator.php

+ *
+ * @template-extends IteratorIterator<TKey, TValue, TIterator>
+ */
+final class AsListIterator extends IteratorIterator


This iterator is used when serialising LazyBSONArray instances to ensure that we return a packed array. The psalm template annotations may need some work (or silencing of errors), but for the time being this works.

alcaeus · 2023-04-12T11:34:17Z

src/Model/CallbackIterator.php

@@ -49,7 +49,7 @@ public function __construct(Traversable $traversable, Closure $callback)
    #[ReturnTypeWillChange]
    public function current()
    {
-        return ($this->callback)($this->iterator->current());
+        return ($this->callback)($this->iterator->current(), $this->iterator->key());


I noticed that our CallbackIterator only passes the value to the callback, but not the key. This change brings the iterator in line with CallbackFilterIterator which also passes both. Note that the latter also passes the whole iterator object as well, which I can add for the sake of feature parity, but it isn't required for this PR to work properly.

Historically, CallbackIterator has only ever been used by ListCollectionNames (2e39482), which had no need of key processing.

Is it feasible to document the callback type using Psalm annotations?

Also, I would be in favor of relaxing its type from Closure to callable and using call_user_func() here if you think that's more consistent with what we do elsewhere. I believe this class is the only place in the library we actually use Closure directly.

alcaeus · 2023-04-12T11:35:23Z

src/Model/LazyBSONArray.php

+        $codec = new LazyBSONArrayCodec();
+        $codec->attachLibrary($this->getLibrary());
+
+        // @psalm-suppress InvalidReturnStatement


toPHP returns array|object, so we specifically have to ignore psalm errors here as we know that a PackedArray::toPHP() will always return an array in the absence of a type map.

alcaeus · 2023-04-12T11:36:41Z

src/Model/LazyBSONArray.php

+        if ($offset === null) {
+            $this->readEntirePackedArray();
+
+            $offset = max(array_merge(
+                array_keys($this->read),
+                array_keys($this->set),
+            )) + 1;


This piece of logic is necessary to allow appending to the list, in which case $offset will be null. In that case, we grab the maximum offset that we have read or written and write the next offset. This is consistent with how PHP handles arrays with missing offsets. Note that for this to work we have to ensure that the entire BSON packed array has been read.

Did you test this for appending to an empty array? In that case, I'd expect $this->read and $this->set might both be empty, in which case you pass an empty array to max(), which is an error.

jmikola

Took a first pass through the source files but not tests.

jmikola · 2023-04-17T02:56:15Z

src/Codec/Decoder.php

+ * @psalm-template B
+ * @psalm-template T


I assume "T" refers to a PHP value and "B" is for BSON. Can we use more descriptive names (e.g. TBson) like you did in AsListIterator?

jmikola · 2023-04-17T02:57:36Z

src/Codec/Codec.php

+namespace MongoDB\Codec;
+
+/**
+ * @api


Does this imply the class is something we want documented?

I know we use @api elsewhere, but I've forgotten the motivation behind that. When we're thinking about preserving BC, I know everything in the library is implicitly part of the API unless we've annotated it with @internal, so that might lead one to think @api is redundant.

It may be superfluous, in which case I'd go through and remove instances of @api across the board. 👍

Opened https://jira.mongodb.org/browse/PHPLIB-1117.

jmikola · 2023-04-17T02:59:45Z

src/Codec/CodecLibrary.php

+    /** @param Decoder|Encoder $items */
+    public function __construct(...$items)
+    {
+        array_map(


Any reason to use this over foreach? You're not actually modifying the array elements, so the callback doesn't seem necessary.

jmikola · 2023-04-17T03:03:41Z

src/Codec/CodecLibrary.php

+            }
+        }
+
+        return $value === null;


Why does null get special treatment? Likewise for canEncode().

The idea was for each codec to decide how to handle a null value. For example, an ODM may decide that for a list of embedded documents it may not want to encode null as an empty array. If a single codec can't deal with a null value, it should return false in its canEncode method.

On second thought, the special handling for null seems misguided. If null deserves special treatment, other scalar values should probably enjoy the same. By that logic, it would make more sense to include a separate ScalarValueCodec that handles all scalar values that PHP offers, including null and returns them as-is. This would also do away with the encodeOrReturn topic in other places. Same goes for array, where a separate codec should support any array and traverse the array to encode all items within the array. I think that would make it more explicit.

Would ScalarValueCodec require additional changes in PHPC? If so, this may be a good opportunity to try and provide a way to differentiate 32-bit and 64-bit integers.

jmikola · 2023-04-17T03:05:47Z

src/Codec/CodecLibrary.php

+            if (! $decoder->canDecode($value)) {
+                continue;
+            }
+
+            return $decoder->decode($value);


Suggested change

if (! $decoder->canDecode($value)) {

continue;

}

return $decoder->decode($value);

if ($decoder->canDecode($value)) {

return $decoder->decode($value);

}

IMO, the guard statement makes sense if we were doing something else here that would need a break afterwards, but that's not the case with return. This just seems more concise unless you expect the body of this loop to get more complicated down the line.

jmikola · 2023-04-17T04:58:40Z

src/Model/LazyBSONArray.php

+        $value = $this->bson->get($offset);
+
+        // Decode value using the codec library if a codec exists
+        $this->read[$offset] = $this->decodeBSONValue($value);


Based on the logic in offsetGet(), $this->set is preferred over $this->read.

When offsetSet() is called, it clears out the corresponding offset in $this->unset and $this->notFound. Would it make sense for it to also clear out $this->read as well? As-is, you're leaving behind a decoded value that won't be accessed.

jmikola · 2023-04-17T04:59:28Z

src/Model/LazyBSONArray.php

+
+    private function getLibrary(): CodecLibrary
+    {
+        return $this->library ?? $this->library = new CodecLibrary(new LazyBSONDocumentCodec(), new LazyBSONArrayCodec());


Similar to my earlier comment, I think this is more readable with a conditional to initialize $this->library followed by an unconditional return.

jmikola · 2023-04-17T05:01:19Z

src/Model/LazyBSONArray.php

+            return;
+        }
+
+        $this->entirePackedArrayRead = true;


Would it be safer to defer this until after iteration? I'm considering some edge case where decoding BSON fails (e.g. during a call to offsetSet()) but the user catches and ignores the exception. That might leave LazyBSONArray in an inconsistent state.

jmikola · 2023-04-17T05:03:08Z

src/Model/LazyBSONArray.php

+        $this->entirePackedArrayRead = true;
+
+        foreach ($this->bson as $key => $value) {
+            if (isset($this->read[$key])) {


Should you use array_key_exists() here? $this->read stores decoded BSON values, and null is a valid field value.

If this was an oversight, it'd be worth adding a regression test.

Although, if this is just an optimization to cut down on additional calls to decodeBSONValue() maybe it isn't worth the trouble, since most things won't decode to null. In that case, a comment explaining the logic here would still be helpful.

jmikola · 2023-04-17T05:05:58Z

src/Model/LazyBSONDocument.php

+            if (is_object($value)) {
+                $this->set[$key] = clone $value;
+            }


Similar to my earlier comment in LazyBSONArray, I think you may want recursive_copy() here. And perhaps a regression test for both that fails with this original code.

alcaeus · 2023-07-07T08:54:53Z

Closing this prototype PR in favour of actual work: #1125

alcaeus self-assigned this Apr 12, 2023

alcaeus commented Apr 12, 2023

View reviewed changes

alcaeus force-pushed the phplib-1022-codec-implementation branch from 8e27a3c to d2d950d Compare April 12, 2023 11:46

alcaeus added 5 commits April 13, 2023 09:32

Introduce interfaces for codecs

163f05a

TEMP: Add stubs for new BSON classes

684ca2c

Add CodecLibrary to keep track of multiple codecs

dff423c

Add lazy array and document models with codecs

28bea34

Allow providing a codec to collection

cf85d8c

alcaeus force-pushed the phplib-1022-codec-implementation branch from d2d950d to cf85d8c Compare April 13, 2023 07:33

jmikola reviewed Apr 17, 2023

View reviewed changes

alcaeus mentioned this pull request Jul 7, 2023

PHPLIB-1180: Add basic infrastructure for codecs #1125

Merged

alcaeus closed this Jul 7, 2023

alcaeus deleted the phplib-1022-codec-implementation branch July 7, 2023 08:55

alcaeus mentioned this pull request Jul 7, 2023

PHPLIB-1184: Improve CallbackIterator #1126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPLIB-1022: Codec implementation for better BSON decoding/encoding #1059

PHPLIB-1022: Codec implementation for better BSON decoding/encoding #1059

alcaeus commented Apr 12, 2023 •

edited

Loading

alcaeus Apr 12, 2023

alcaeus Apr 12, 2023

jmikola Apr 17, 2023

alcaeus Apr 12, 2023

alcaeus Apr 12, 2023

jmikola Apr 17, 2023

alcaeus Apr 12, 2023

alcaeus Apr 12, 2023

jmikola Apr 17, 2023

jmikola left a comment

jmikola Apr 17, 2023

jmikola Apr 17, 2023

alcaeus Apr 17, 2023

jmikola Apr 18, 2023

jmikola Apr 17, 2023

jmikola Apr 17, 2023

alcaeus Apr 17, 2023

jmikola Apr 18, 2023

jmikola Apr 17, 2023

jmikola Apr 17, 2023

jmikola Apr 17, 2023

jmikola Apr 17, 2023

jmikola Apr 17, 2023

jmikola Apr 17, 2023

alcaeus commented Jul 7, 2023

PHPLIB-1022: Codec implementation for better BSON decoding/encoding #1059

PHPLIB-1022: Codec implementation for better BSON decoding/encoding #1059

Conversation

alcaeus commented Apr 12, 2023 • 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

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

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

alcaeus commented Jul 7, 2023

alcaeus commented Apr 12, 2023 •

edited

Loading