Skip to content
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.

Sign up for GitHub

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

Jump to bottom

PHPLIB-1182: Support codec option in operation classes #1140

Merged
merged 37 commits into from
Aug 25, 2023
Merged

PHPLIB-1182: Support codec option in operation classes #1140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
7b10f77
Add Codec support to find operations
alcaeus Jul 17, 2023
2ceed92
Add codec support to insert operations
alcaeus Jul 17, 2023
db998c3
Add codec support to replace operation
alcaeus Jul 17, 2023
d30c8b8
Add codec support to aggregate operation
alcaeus Jul 17, 2023
5320217
Add codec support to BulkWrite operation
alcaeus Jul 17, 2023
dead64b
Add codec support to findAndModify operations
alcaeus Jul 17, 2023
274d50a
Add codec support to Watch operation
alcaeus Jul 17, 2023
c67ff64
Support passing default codec to collection class
alcaeus Jul 21, 2023
97c6035
Remove temporary variable usage in operations
alcaeus Jul 25, 2023
a3f463a
Enforce iterator type when creating ChangeStreamIterator
alcaeus Jul 25, 2023
4d32785
Remove unnecessary null-coalesce
alcaeus Jul 25, 2023
4a0d944
Ensure operation-level type map gets precedence over collection-level…
alcaeus Jul 25, 2023
8ce7221
Reference psalm issue regarding unions and assert-if type annotations
alcaeus Jul 27, 2023
45e5a77
Add iterator type in ReadableStream
alcaeus Jul 27, 2023
6ee86c4
Assume _id property will be present in change stream result document.
alcaeus Jul 27, 2023
21edb06
Prohibit specifying typeMap and codec options for operations
alcaeus Jul 27, 2023
53f9fbf
Defer server selection until executing operation
alcaeus Jul 27, 2023
f484bd7
Remove useless assertion
alcaeus Jul 27, 2023
f06b40d
Assert iterator type in ChangeStreamIterator::getInnerIterator
alcaeus Jul 31, 2023
1f23d34
Make assertions on encoded result conditional
alcaeus Jul 31, 2023
18a0ed1
Extract factory to created decoded fixtures
alcaeus Jul 31, 2023
38aaf71
Split document creation for legibility
alcaeus Jul 31, 2023
1d9699f
Insert fixture through factory in test object
alcaeus Jul 31, 2023
ec53fe7
Trigger warning when calling CodecCursor::setTypeMap
alcaeus Jul 31, 2023
c4d46b8
Encode documents before type checks
alcaeus Aug 2, 2023
4eceaf6
Use more descriptive value in failing tests
alcaeus Aug 4, 2023
1cb170c
Add clarifying comment on skipping validation
alcaeus Aug 4, 2023
56fc1ef
Simplify double isset check
alcaeus Aug 4, 2023
003f73b
Split chained calls when inheriting options
alcaeus Aug 4, 2023
66b9308
Defer server selection in Collection::drop()
alcaeus Aug 4, 2023
69c32c8
Split inheritReadOptions method
alcaeus Aug 4, 2023
6f8cdea
Use decode() instead of decodeIfSupported()
alcaeus Aug 4, 2023
ed56c09
Add explanatory comment for ID filling behaviour
alcaeus Aug 16, 2023
e3d68a7
Handle null values in findAndModify responses
alcaeus Aug 22, 2023
0ed7f67
Update comment regarding missing identifiers in tests
alcaeus Aug 22, 2023
9cdb66a
Use intersection type for change streams
alcaeus Aug 23, 2023
f532119
Prohibit specifying codec and typeMap options to Watch
alcaeus Aug 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions psalm-baseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@
<code><![CDATA[$reply->cursor->nextBatch]]></code>
</MixedArgument>
<MixedAssignment>
<code>$resumeToken</code>
<code><![CDATA[$this->postBatchResumeToken]]></code>
</MixedAssignment>
<MixedPropertyFetch>
Expand Down Expand Up @@ -170,6 +171,7 @@
</file>
<file src="src/Operation/Aggregate.php">
<MixedArgument>
<code><![CDATA[$this->options['codec']]]></code>
<code><![CDATA[$this->options['typeMap']]]></code>
</MixedArgument>
<MixedAssignment>
Expand All @@ -191,6 +193,8 @@
<code>$args[0]</code>
<code>$args[0]</code>
<code>$args[0]</code>
<code>$args[0]</code>
<code>$args[1]</code>
<code>$args[1]</code>
<code>$args[1]</code>
<code>$args[1]</code>
Expand All @@ -205,6 +209,8 @@
<code>$args[0]</code>
<code>$args[0]</code>
<code>$args[0]</code>
<code>$args[0]</code>
<code>$args[1]</code>
<code>$args[1]</code>
<code>$args[1]</code>
<code>$args[1]</code>
Expand Down Expand Up @@ -244,6 +250,8 @@
<code>$args[2]</code>
<code><![CDATA[$args[2]['multi']]]></code>
<code><![CDATA[$args[2]['multi']]]></code>
<code>$operations[$i][$type][0]</code>
<code>$operations[$i][$type][1]</code>
<code>$operations[$i][$type][1]</code>
<code>$operations[$i][$type][2]</code>
<code>$operations[$i][$type][2]</code>
Expand Down Expand Up @@ -401,6 +409,7 @@
</file>
<file src="src/Operation/Find.php">
<MixedArgument>
<code><![CDATA[$this->options['codec']]]></code>
<code><![CDATA[$this->options['typeMap']]]></code>
</MixedArgument>
<MixedArrayAccess>
Expand All @@ -427,18 +436,34 @@
<code><![CDATA[$cmd['upsert']]]></code>
<code><![CDATA[$options['session']]]></code>
<code><![CDATA[$options['writeConcern']]]></code>
<code>$value</code>
</MixedAssignment>
<MixedInferredReturnType>
<code>array|object|null</code>
</MixedInferredReturnType>
<MixedMethodCall>
<code>decode</code>
<code>isInTransaction</code>
</MixedMethodCall>
<MixedReturnStatement>
<code><![CDATA[$value === null ? $value : $this->options['codec']->decode($value)]]></code>
<code><![CDATA[$value === null ? $value : $this->options['codec']->decode($value)]]></code>
<code><![CDATA[is_object($result) ? ($result->value ?? null) : null]]></code>
<code><![CDATA[is_object($result) ? ($result->value ?? null) : null]]></code>
</MixedReturnStatement>
</file>
<file src="src/Operation/FindOne.php">
<MixedAssignment>
<code>$document</code>
</MixedAssignment>
<MixedInferredReturnType>
<code>array|object|null</code>
</MixedInferredReturnType>
<MixedReturnStatement>
<code>$document === false ? null : $document</code>
<code>$document === false ? null : $document</code>
</MixedReturnStatement>
</file>
<file src="src/Operation/FindOneAndDelete.php">
<MixedAssignment>
<code><![CDATA[$options['fields']]]></code>
Expand All @@ -448,6 +473,9 @@
<MixedAssignment>
<code><![CDATA[$options['fields']]]></code>
</MixedAssignment>
<PossiblyInvalidArgument>
<code>$replacement</code>
</PossiblyInvalidArgument>
</file>
<file src="src/Operation/FindOneAndUpdate.php">
<MixedAssignment>
Expand All @@ -464,6 +492,9 @@
<MixedMethodCall>
<code>isInTransaction</code>
</MixedMethodCall>
<PossiblyInvalidArgument>
<code>$document</code>
</PossiblyInvalidArgument>
</file>
<file src="src/Operation/InsertOne.php">
<MixedAssignment>
Expand All @@ -475,6 +506,9 @@
<MixedMethodCall>
<code>isInTransaction</code>
</MixedMethodCall>
<PossiblyInvalidArgument>
<code>$document</code>
</PossiblyInvalidArgument>
</file>
<file src="src/Operation/ListIndexes.php">
<MixedAssignment>
Expand Down Expand Up @@ -522,6 +556,11 @@
<code>isInTransaction</code>
</MixedMethodCall>
</file>
<file src="src/Operation/ReplaceOne.php">
<PossiblyInvalidArgument>
<code>$replacement</code>
</PossiblyInvalidArgument>
</file>
<file src="src/Operation/Update.php">
<MixedArgument>
<code><![CDATA[$this->options['writeConcern']]]></code>
Expand Down
26 changes: 24 additions & 2 deletions src/ChangeStream.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
namespace MongoDB;

use Iterator;
use MongoDB\BSON\Document;
use MongoDB\Codec\DocumentCodec;
use MongoDB\Driver\CursorId;
use MongoDB\Driver\Exception\ConnectionException;
use MongoDB\Driver\Exception\RuntimeException;
Expand All @@ -27,6 +29,7 @@
use MongoDB\Model\ChangeStreamIterator;
use ReturnTypeWillChange;

use function assert;
use function call_user_func;
use function in_array;

Expand Down Expand Up @@ -82,15 +85,22 @@ class ChangeStream implements Iterator
*/
private bool $hasAdvanced = false;

private ?DocumentCodec $codec;

/**
* @internal
*
* @param ResumeCallable $resumeCallable
*/
public function __construct(ChangeStreamIterator $iterator, callable $resumeCallable)
public function __construct(ChangeStreamIterator $iterator, callable $resumeCallable, ?DocumentCodec $codec = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Watch constructs the ChangeStreamIterator and has access to its internal cursor, even though it currently sends it as-is directly from the private executeAggregate() method. Watch also constructs the $resumeCallable here.

I was going to ask why ChangeStream bothers with calling setTypeMap(['root' => 'bson']) when that could otherwise be done in Watch; however, even if we did move that logic to Watch, ChangeStream would still need to know about the codec in order to process return values for current().

And you previously explained that Watch cannot apply the codec itself since doing so might interfere with resume token collection.

I think this is fine as-is but just want to confirm with you before signing off. Feel free to resolve after reading if my understanding is correct.

Another reason for asking was that ChangeStream is in the public API (although undocumented), so I'm paying extra attention the signature change above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since ChangeStream needs to be in control of the codec used in the cursor (as it needs to be sure about the return types), I figured it would make more sense for ChangeStream to apply its codec to the underlying cursor instead of relying on that being set correctly. As is, Watch is only accepting the codec option on behalf of ChangeStream, and passing it on without needing to know about internal workings of ChangeStream.

The main problem with applying the codec in the original Aggregate operation is that while the aggregation pipeline results will always have an _id field to be used as resume token, we cannot make such guarantees after the codec has been applied. After that, the _id field may have been discarded or assigned to an object property we don't know about.

Also note that while ChangeStream and ChangeStreamIterator are part of the public API, both their constructors are marked as internal; indicating that while we expect users to interact with these classes, we don't expect them to instantiate them on their own. The signature change here is backward compatible, as the $codec parameter defaults to null, so any previously working instantiations of that class will continue to function as expected.

{
$this->iterator = $iterator;
$this->resumeCallable = $resumeCallable;
$this->codec = $codec;

if ($codec) {
$this->iterator->getInnerIterator()->setTypeMap(['root' => 'bson']);
}
}

/**
Expand All @@ -100,7 +110,15 @@ public function __construct(ChangeStreamIterator $iterator, callable $resumeCall
#[ReturnTypeWillChange]
public function current()
{
return $this->iterator->current();
$value = $this->iterator->current();

if (! $this->codec) {
return $value;
}

assert($value instanceof Document);

return $this->codec->decode($value);
}

/** @return CursorId */
Expand Down Expand Up @@ -252,6 +270,10 @@ private function resume(): void

$this->iterator->rewind();

if ($this->codec) {
$this->iterator->getInnerIterator()->setTypeMap(['root' => 'bson']);
}

$this->onIteration($this->hasAdvanced);
}

Expand Down
Loading