-
Notifications
You must be signed in to change notification settings - Fork 266
PHPLIB-1022: Codec implementation for better BSON decoding/encoding #1059
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
163f05a
684ca2c
dff423c
28bea34
cf85d8c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
<?php | ||
|
||
namespace MongoDB\Codec; | ||
|
||
/** | ||
* @api | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this imply the class is something we want documented? I know we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be superfluous, in which case I'd go through and remove instances of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
* | ||
* @psalm-template B | ||
* @psalm-template T | ||
* @template-extends Decoder<B, T> | ||
* @template-extends Encoder<B, T> | ||
*/ | ||
interface Codec extends Decoder, Encoder | ||
{ | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,137 @@ | ||||||||||||||||||
<?php | ||||||||||||||||||
|
||||||||||||||||||
namespace MongoDB\Codec; | ||||||||||||||||||
|
||||||||||||||||||
use MongoDB\Exception\UnexpectedValueException; | ||||||||||||||||||
|
||||||||||||||||||
use function array_map; | ||||||||||||||||||
use function get_debug_type; | ||||||||||||||||||
use function sprintf; | ||||||||||||||||||
|
||||||||||||||||||
class CodecLibrary implements Codec | ||||||||||||||||||
{ | ||||||||||||||||||
/** @var array<Decoder> */ | ||||||||||||||||||
private $decoders = []; | ||||||||||||||||||
|
||||||||||||||||||
/** @var array<Encoder> */ | ||||||||||||||||||
private $encoders = []; | ||||||||||||||||||
|
||||||||||||||||||
/** @param Decoder|Encoder $items */ | ||||||||||||||||||
public function __construct(...$items) | ||||||||||||||||||
{ | ||||||||||||||||||
array_map( | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason to use this over |
||||||||||||||||||
function ($item) { | ||||||||||||||||||
if ($item instanceof Decoder) { | ||||||||||||||||||
$this->attachDecoder($item); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
if ($item instanceof Encoder) { | ||||||||||||||||||
$this->attachEncoder($item); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// Yes, we'll silently discard everything. Please let me already have union types... | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may want to replace this with an |
||||||||||||||||||
}, | ||||||||||||||||||
$items | ||||||||||||||||||
); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/** @return static */ | ||||||||||||||||||
final public function attachCodec(Codec $codec): self | ||||||||||||||||||
{ | ||||||||||||||||||
$this->decoders[] = $codec; | ||||||||||||||||||
$this->encoders[] = $codec; | ||||||||||||||||||
if ($codec instanceof KnowsCodecLibrary) { | ||||||||||||||||||
$codec->attachLibrary($this); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
return $this; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/** @return static */ | ||||||||||||||||||
final public function attachDecoder(Decoder $decoder): self | ||||||||||||||||||
{ | ||||||||||||||||||
$this->decoders[] = $decoder; | ||||||||||||||||||
if ($decoder instanceof KnowsCodecLibrary) { | ||||||||||||||||||
$decoder->attachLibrary($this); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
return $this; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/** @return static */ | ||||||||||||||||||
final public function attachEncoder(Encoder $encoder): self | ||||||||||||||||||
{ | ||||||||||||||||||
$this->encoders[] = $encoder; | ||||||||||||||||||
if ($encoder instanceof KnowsCodecLibrary) { | ||||||||||||||||||
$encoder->attachLibrary($this); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
return $this; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/** @param mixed $value */ | ||||||||||||||||||
final public function canDecode($value): bool | ||||||||||||||||||
{ | ||||||||||||||||||
foreach ($this->decoders as $decoder) { | ||||||||||||||||||
if ($decoder->canDecode($value)) { | ||||||||||||||||||
return true; | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
return $value === null; | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea was for each codec to decide how to handle a On second thought, the special handling for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/** @param mixed $value */ | ||||||||||||||||||
final public function canEncode($value): bool | ||||||||||||||||||
{ | ||||||||||||||||||
foreach ($this->encoders as $encoder) { | ||||||||||||||||||
if ($encoder->canEncode($value)) { | ||||||||||||||||||
return true; | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
return $value === null; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* @param mixed $value | ||||||||||||||||||
* @return mixed | ||||||||||||||||||
*/ | ||||||||||||||||||
final public function decode($value) | ||||||||||||||||||
{ | ||||||||||||||||||
foreach ($this->decoders as $decoder) { | ||||||||||||||||||
if (! $decoder->canDecode($value)) { | ||||||||||||||||||
continue; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
return $decoder->decode($value); | ||||||||||||||||||
Comment on lines
+103
to
+107
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
IMO, the guard statement makes sense if we were doing something else here that would need a |
||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
if ($value === null) { | ||||||||||||||||||
return null; | ||||||||||||||||||
} | ||||||||||||||||||
Comment on lines
+110
to
+112
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this intentionally after the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is correct - it was thought as a fallback. I might end up refactoring this though, see my comment above. |
||||||||||||||||||
|
||||||||||||||||||
throw new UnexpectedValueException(sprintf('No decoder found for value of type "%s"', get_debug_type($value))); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
* @param mixed $value | ||||||||||||||||||
* @return mixed | ||||||||||||||||||
*/ | ||||||||||||||||||
final public function encode($value) | ||||||||||||||||||
{ | ||||||||||||||||||
foreach ($this->encoders as $encoder) { | ||||||||||||||||||
if (! $encoder->canEncode($value)) { | ||||||||||||||||||
continue; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
return $encoder->encode($value); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
if ($value === null) { | ||||||||||||||||||
return null; | ||||||||||||||||||
} | ||||||||||||||||||
Comment on lines
+123
to
+133
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comments as I left on |
||||||||||||||||||
|
||||||||||||||||||
throw new UnexpectedValueException(sprintf('No encoder found for value of type "%s"', get_debug_type($value))); | ||||||||||||||||||
} | ||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
<?php | ||
|
||
namespace MongoDB\Codec; | ||
|
||
/** | ||
* @api | ||
* | ||
* @psalm-template B | ||
* @psalm-template T | ||
Comment on lines
+8
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
*/ | ||
interface Decoder | ||
{ | ||
/** | ||
* @param mixed $value | ||
* @psalm-assert-if-true B $value | ||
*/ | ||
public function canDecode($value): bool; | ||
|
||
/** | ||
* @param mixed $value | ||
* @psalm-param B $value | ||
* @return mixed | ||
* @psalm-return T | ||
*/ | ||
public function decode($value); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
<?php | ||
|
||
namespace MongoDB\Codec; | ||
|
||
/** | ||
* @api | ||
* | ||
* @psalm-template B | ||
* @psalm-template T | ||
*/ | ||
interface Encoder | ||
{ | ||
/** | ||
* @param mixed $value | ||
* @psalm-assert-if-true T $value | ||
jmikola marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*/ | ||
public function canEncode($value): bool; | ||
|
||
/** | ||
* @param mixed $value | ||
* @psalm-param T $value | ||
* @return mixed | ||
* @psalm-return B | ||
*/ | ||
public function encode($value); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
<?php | ||
|
||
namespace MongoDB\Codec; | ||
|
||
interface KnowsCodecLibrary | ||
{ | ||
public function attachLibrary(CodecLibrary $library): void; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
<?php | ||
|
||
namespace MongoDB\Codec; | ||
|
||
use MongoDB\BSON\PackedArray; | ||
use MongoDB\Exception\UnexpectedValueException; | ||
use MongoDB\Model\LazyBSONArray; | ||
|
||
use function array_values; | ||
use function sprintf; | ||
|
||
/** | ||
* Codec for lazy decoding of BSON PackedArray instances | ||
* | ||
* @template-implements Codec<PackedArray, LazyBSONArray> | ||
*/ | ||
class LazyBSONArrayCodec implements Codec, KnowsCodecLibrary | ||
{ | ||
/** @var CodecLibrary|null */ | ||
private $library = null; | ||
|
||
public function attachLibrary(CodecLibrary $library): void | ||
{ | ||
$this->library = $library; | ||
} | ||
|
||
/** @inheritDoc */ | ||
jmikola marked this conversation as resolved.
Show resolved
Hide resolved
|
||
public function canDecode($value): bool | ||
{ | ||
return $value instanceof PackedArray; | ||
} | ||
|
||
/** @inheritDoc */ | ||
public function canEncode($value): bool | ||
{ | ||
return $value instanceof LazyBSONArray; | ||
} | ||
|
||
/** @inheritDoc */ | ||
public function decode($value): LazyBSONArray | ||
{ | ||
if (! $value instanceof PackedArray) { | ||
throw new UnexpectedValueException(sprintf('"%s" can only decode from "%s" instances', self::class, PackedArray::class)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you adamant about using a RuntimeException here? If not, I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reading the documentation, |
||
} | ||
|
||
return new LazyBSONArray($value, $this->getLibrary()); | ||
} | ||
|
||
/** @inheritDoc */ | ||
public function encode($value): PackedArray | ||
{ | ||
if (! $value instanceof LazyBSONArray) { | ||
throw new UnexpectedValueException(sprintf('"%s" can only encode "%s" instances', self::class, LazyBSONArray::class)); | ||
} | ||
|
||
$return = []; | ||
foreach ($value as $offset => $offsetValue) { | ||
$return[$offset] = $this->getLibrary()->canEncode($offsetValue) ? $this->getLibrary()->encode($offsetValue) : $offsetValue; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently codecs are designed to throw an exception if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
|
||
return PackedArray::fromPHP(array_values($return)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than call As I'm writing this, I've yet to read through There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤦♂️ Yes, that seems like an easier way to accomplish the same thing. And yes, |
||
} | ||
|
||
private function getLibrary(): CodecLibrary | ||
{ | ||
return $this->library ?? $this->library = new CodecLibrary(new LazyBSONDocumentCodec(), $this); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this written as such to avoid constructing a CodecLibrary that might get discarded if Code style nit: I'm in favor of rewriting this with an explicit if statement to initialize the null property before an unconditional return. Full disclosure: Avoiding one-liners in PHP from @localheinz was fresh on my mind. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Originally I wanted to use a null-coalescing assignment, but those aren't available in 7.2. Your assumption however is correct: in the absence of a codec library (which we assume will be attached to the codec earlier) we create our own codec library consisting of the two codecs necessary to handle the lazy objects. I can refactor this if you'd like. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The assignment within a I wouldn't object to:
Once we require PHP 7.4+, I expect the CS tools would suggest a refactoring to use |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
<?php | ||
|
||
namespace MongoDB\Codec; | ||
|
||
use MongoDB\BSON\Document; | ||
use MongoDB\Exception\UnexpectedValueException; | ||
use MongoDB\Model\LazyBSONDocument; | ||
|
||
use function sprintf; | ||
|
||
/** | ||
* Codec for lazy decoding of BSON Document instances | ||
* | ||
* @template-implements Codec<Document, LazyBSONDocument> | ||
*/ | ||
class LazyBSONDocumentCodec implements Codec, KnowsCodecLibrary | ||
{ | ||
/** @var CodecLibrary|null */ | ||
private $library = null; | ||
|
||
public function attachLibrary(CodecLibrary $library): void | ||
{ | ||
$this->library = $library; | ||
} | ||
|
||
/** @inheritDoc */ | ||
public function canDecode($value): bool | ||
{ | ||
return $value instanceof Document; | ||
} | ||
|
||
/** @inheritDoc */ | ||
public function canEncode($value): bool | ||
{ | ||
return $value instanceof LazyBSONDocument; | ||
} | ||
|
||
/** @inheritDoc */ | ||
public function decode($value): LazyBSONDocument | ||
{ | ||
if (! $value instanceof Document) { | ||
throw new UnexpectedValueException(sprintf('"%s" can only decode from "%s" instances', self::class, Document::class)); | ||
} | ||
|
||
return new LazyBSONDocument($value, $this->getLibrary()); | ||
} | ||
|
||
/** @inheritDoc */ | ||
public function encode($value): Document | ||
{ | ||
if (! $value instanceof LazyBSONDocument) { | ||
throw new UnexpectedValueException(sprintf('"%s" can only encode "%s" instances', self::class, LazyBSONDocument::class)); | ||
} | ||
|
||
$return = []; | ||
foreach ($value as $field => $fieldValue) { | ||
$return[$field] = $this->getLibrary()->canEncode($fieldValue) ? $this->getLibrary()->encode($fieldValue) : $fieldValue; | ||
} | ||
Comment on lines
+55
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I recall that Also, I noted that I imagine there's a clear reason you're not doing this in both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd appreciate the opportunity to discuss this over video, so please make a note to bring it up in our next 1:1. |
||
|
||
return Document::fromPHP($return); | ||
} | ||
|
||
private function getLibrary(): CodecLibrary | ||
{ | ||
return $this->library ?? $this->library = new CodecLibrary($this, new LazyBSONArrayCodec()); | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.