Skip to content

PHPLIB-1192: Remove useCursor option in aggregate #1130

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

Merged
merged 2 commits into from
Jul 17, 2023
Merged

PHPLIB-1192: Remove useCursor option in aggregate #1130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a6a74ea
PHPLIB-1192: Remove useCursor option in aggregate
alcaeus Jul 14, 2023
5631bd8
Remove unnecessary property for isExplain
alcaeus Jul 17, 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
16 changes: 0 additions & 16 deletions docs/includes/apiargs-MongoDBCollection-method-aggregate-option.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,22 +59,6 @@ source:
file: apiargs-MongoDBCollection-common-option.yaml
ref: typeMap
---
arg_name: option
name: useCursor
type: boolean
description: |
Indicates whether the command will request that the server provide results
using a cursor. The default is ``true``.

.. note::

MongoDB 3.6+ no longer supports returning results without a cursor (excluding
when the explain option is used) and specifying false for this option will
result in a server error.
interface: phpmethod
operation: ~
optional: true
---
source:
file: apiargs-MongoDBCollection-common-option.yaml
ref: writeConcern
Expand Down
9 changes: 2 additions & 7 deletions docs/reference/method/MongoDBCollection-aggregate.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,8 @@ Errors/Exceptions
Behavior
--------

:phpmethod:`MongoDB\\Collection::aggregate()`'s return value depends on the
MongoDB server version and whether the ``useCursor`` option is specified. If
``useCursor`` is ``true``, a :php:`MongoDB\\Driver\\Cursor
<class.mongodb-driver-cursor>` object is returned. If ``useCursor`` is
``false``, an :php:`ArrayIterator <arrayiterator>` is returned that wraps the
``result`` array from the command response document. In both cases, the return
value will be :php:`Traversable <traversable>`.
:phpmethod:`MongoDB\\Collection::aggregate()`'s returns a
:php:`MongoDB\\Driver\\Cursor <class.mongodb-driver-cursor>` object.
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this applies regardless of whether explain: true is used. SGTM.


Examples
--------
Expand Down
8 changes: 1 addition & 7 deletions src/Collection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
use MongoDB\Operation\UpdateMany;
use MongoDB\Operation\UpdateOne;
use MongoDB\Operation\Watch;
use Traversable;

use function array_diff_key;
use function array_intersect_key;
Expand Down Expand Up @@ -186,15 +185,10 @@ public function __toString()
/**
* Executes an aggregation framework pipeline on the collection.
*
* Note: this method's return value depends on the MongoDB server version
* and the "useCursor" option. If "useCursor" is true, a Cursor will be
* returned; otherwise, an ArrayIterator is returned, which wraps the
* "result" array from the command response document.
*
* @see Aggregate::__construct() for supported options
* @param array $pipeline Aggregation pipeline
* @param array $options Command options
* @return Traversable
* @return Cursor
* @throws UnexpectedValueException if the command response was malformed
* @throws UnsupportedException if options are not supported by the selected server
* @throws InvalidArgumentException for parameter/option parsing errors
Expand Down
58 changes: 7 additions & 51 deletions src/Operation/Aggregate.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

namespace MongoDB\Operation;

use ArrayIterator;
use MongoDB\Driver\Command;
use MongoDB\Driver\Cursor;
use MongoDB\Driver\Exception\RuntimeException as DriverRuntimeException;
Expand All @@ -31,13 +30,11 @@
use MongoDB\Exception\UnsupportedException;
use stdClass;

use function current;
use function is_array;
use function is_bool;
use function is_integer;
use function is_object;
use function is_string;
use function MongoDB\create_field_path_type_map;
use function MongoDB\is_document;
use function MongoDB\is_last_pipeline_operator_write;
use function MongoDB\is_pipeline;
Expand All @@ -58,8 +55,6 @@ class Aggregate implements Executable, Explainable

private array $options;

private bool $isExplain;

private bool $isWrite;

/**
Expand Down Expand Up @@ -112,12 +107,6 @@ class Aggregate implements Executable, Explainable
* * typeMap (array): Type map for BSON deserialization. This will be
* applied to the returned Cursor (it is not sent to the server).
*
* * useCursor (boolean): Indicates whether the command will request that
* the server provide results using a cursor. The default is true.
*
* This option allows users to turn off cursors if necessary to aid in
* mongod/mongos upgrades.
*
* * writeConcern (MongoDB\Driver\WriteConcern): Write concern. This only
* applies when an $out or $merge stage is specified.
*
Expand All @@ -136,8 +125,6 @@ public function __construct(string $databaseName, ?string $collectionName, array
throw new InvalidArgumentException('$pipeline is not a valid aggregation pipeline');
}

$options += ['useCursor' => true];

if (isset($options['allowDiskUse']) && ! is_bool($options['allowDiskUse'])) {
throw InvalidArgumentException::invalidType('"allowDiskUse" option', $options['allowDiskUse'], 'boolean');
}
Expand Down Expand Up @@ -190,18 +177,10 @@ public function __construct(string $databaseName, ?string $collectionName, array
throw InvalidArgumentException::invalidType('"typeMap" option', $options['typeMap'], 'array');
}

if (! is_bool($options['useCursor'])) {
throw InvalidArgumentException::invalidType('"useCursor" option', $options['useCursor'], 'boolean');
}

if (isset($options['writeConcern']) && ! $options['writeConcern'] instanceof WriteConcern) {
throw InvalidArgumentException::invalidType('"writeConcern" option', $options['writeConcern'], WriteConcern::class);
}

if (isset($options['batchSize']) && ! $options['useCursor']) {
throw new InvalidArgumentException('"batchSize" option should not be used if "useCursor" is false');
}

if (isset($options['bypassDocumentValidation']) && ! $options['bypassDocumentValidation']) {
unset($options['bypassDocumentValidation']);
}
Expand All @@ -214,14 +193,7 @@ public function __construct(string $databaseName, ?string $collectionName, array
unset($options['writeConcern']);
}

$this->isExplain = ! empty($options['explain']);
$this->isWrite = is_last_pipeline_operator_write($pipeline) && ! $this->isExplain;

// Explain does not use a cursor
if ($this->isExplain) {
$options['useCursor'] = false;
unset($options['batchSize']);
}
$this->isWrite = is_last_pipeline_operator_write($pipeline) && ! ($options['explain'] ?? false);

if ($this->isWrite) {
/* Ignore batchSize for writes, since no documents are returned and
Expand All @@ -241,7 +213,7 @@ public function __construct(string $databaseName, ?string $collectionName, array
* Execute the operation.
*
* @see Executable::execute()
* @return ArrayIterator|Cursor
* @return Cursor
* @throws UnexpectedValueException if the command response was malformed
* @throws UnsupportedException if read concern or write concern is used and unsupported
* @throws DriverRuntimeException for other driver errors (e.g. connection errors)
Expand All @@ -266,25 +238,11 @@ public function execute(Server $server)

$cursor = $this->executeCommand($server, $command);

if ($this->options['useCursor'] || $this->isExplain) {
if (isset($this->options['typeMap'])) {
$cursor->setTypeMap($this->options['typeMap']);
}

return $cursor;
}

if (isset($this->options['typeMap'])) {
$cursor->setTypeMap(create_field_path_type_map($this->options['typeMap'], 'result.$'));
$cursor->setTypeMap($this->options['typeMap']);
}

$result = current($cursor->toArray());

if (! is_object($result) || ! isset($result->result) || ! is_array($result->result)) {
throw new UnexpectedValueException('aggregate command did not return a "result" array');
}

return new ArrayIterator($result->result);
return $cursor;
}

/**
Expand Down Expand Up @@ -331,11 +289,9 @@ private function createCommandDocument(): array
$cmd['hint'] = is_array($this->options['hint']) ? (object) $this->options['hint'] : $this->options['hint'];
}

if ($this->options['useCursor']) {
$cmd['cursor'] = isset($this->options["batchSize"])
? ['batchSize' => $this->options["batchSize"]]
: new stdClass();
}
$cmd['cursor'] = isset($this->options['batchSize'])
? ['batchSize' => $this->options['batchSize']]
: new stdClass();
Copy link
Member

Choose a reason for hiding this comment

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

I assume always supplying a cursor option for the explain option is kosher?

Quoting Aggregate: Example from the server docs:

MongoDB 3.6 removes the use of aggregate command without the cursor option unless the command includes the explain option. Unless you include the explain option, you must specify the cursor option.

It doesn't say anything about explain prohibiting a cursor option, but cursor does seem to be ignored. Do you think we should gate this with a ! $this->isExplain condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd leave the option in unless explain specifically prohibits it - this is consistent with other commands where we also send batchSize as part of an explain.


return $cmd;
}
Expand Down
3 changes: 0 additions & 3 deletions src/Operation/CountDocuments.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,13 @@

namespace MongoDB\Operation;

use MongoDB\Driver\Cursor;
use MongoDB\Driver\Exception\RuntimeException as DriverRuntimeException;
use MongoDB\Driver\Server;
use MongoDB\Exception\InvalidArgumentException;
use MongoDB\Exception\UnexpectedValueException;
use MongoDB\Exception\UnsupportedException;

use function array_intersect_key;
use function assert;
use function count;
use function current;
use function is_float;
Expand Down Expand Up @@ -125,7 +123,6 @@ public function __construct(string $databaseName, string $collectionName, $filte
public function execute(Server $server)
{
$cursor = $this->aggregate->execute($server);
assert($cursor instanceof Cursor);

$allResults = $cursor->toArray();

Expand Down
6 changes: 1 addition & 5 deletions src/Operation/Watch.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
use function array_intersect_key;
use function array_key_exists;
use function array_unshift;
use function assert;
use function count;
use function is_array;
use function is_bool;
Expand Down Expand Up @@ -355,10 +354,7 @@ private function executeAggregate(Server $server): Cursor
addSubscriber($this);

try {
$cursor = $this->aggregate->execute($server);
assert($cursor instanceof Cursor);

return $cursor;
return $this->aggregate->execute($server);
} finally {
removeSubscriber($this);
}
Expand Down
14 changes: 0 additions & 14 deletions tests/Operation/AggregateTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,23 +40,10 @@ public function provideInvalidConstructorOptions()
'readPreference' => $this->getInvalidReadPreferenceValues(),
'session' => $this->getInvalidSessionValues(),
'typeMap' => $this->getInvalidArrayValues(),
'useCursor' => $this->getInvalidBooleanValues(true),
'writeConcern' => $this->getInvalidWriteConcernValues(),
]);
}

public function testConstructorBatchSizeOptionRequiresUseCursor(): void
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('"batchSize" option should not be used if "useCursor" is false');
new Aggregate(
$this->getDatabaseName(),
$this->getCollectionName(),
[['$match' => ['x' => 1]]],
['batchSize' => 100, 'useCursor' => false],
);
}

public function testExplainableCommandDocument(): void
{
$options = [
Expand All @@ -69,7 +56,6 @@ public function testExplainableCommandDocument(): void
'let' => ['a' => 1],
'maxTimeMS' => 100,
'readConcern' => new ReadConcern(ReadConcern::LOCAL),
'useCursor' => true,
// Intentionally omitted options
// The "explain" option is illegal
'readPreference' => new ReadPreference(ReadPreference::SECONDARY_PREFERRED),
Expand Down
4 changes: 2 additions & 2 deletions tests/UnifiedSpecTests/Util.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ final class Util
'rewrapManyDataKey' => ['filter', 'opts'],
],
Database::class => [
'aggregate' => ['pipeline', 'session', 'useCursor', 'allowDiskUse', 'batchSize', 'bypassDocumentValidation', 'collation', 'comment', 'explain', 'hint', 'let', 'maxAwaitTimeMS', 'maxTimeMS'],
'aggregate' => ['pipeline', 'session', 'allowDiskUse', 'batchSize', 'bypassDocumentValidation', 'collation', 'comment', 'explain', 'hint', 'let', 'maxAwaitTimeMS', 'maxTimeMS'],
'createChangeStream' => ['pipeline', 'session', 'fullDocument', 'resumeAfter', 'startAfter', 'startAtOperationTime', 'batchSize', 'collation', 'maxAwaitTimeMS', 'showExpandedEvents'],
'createCollection' => ['collection', 'session', 'autoIndexId', 'capped', 'changeStreamPreAndPostImages', 'clusteredIndex', 'collation', 'expireAfterSeconds', 'flags', 'indexOptionDefaults', 'max', 'maxTimeMS', 'pipeline', 'size', 'storageEngine', 'timeseries', 'validationAction', 'validationLevel', 'validator', 'viewOn'],
'dropCollection' => ['collection', 'session'],
Expand All @@ -83,7 +83,7 @@ final class Util
'runCommand' => ['command', 'commandName', 'readPreference', 'session'],
],
Collection::class => [
'aggregate' => ['pipeline', 'session', 'useCursor', 'allowDiskUse', 'batchSize', 'bypassDocumentValidation', 'collation', 'comment', 'explain', 'hint', 'let', 'maxAwaitTimeMS', 'maxTimeMS'],
'aggregate' => ['pipeline', 'session', 'allowDiskUse', 'batchSize', 'bypassDocumentValidation', 'collation', 'comment', 'explain', 'hint', 'let', 'maxAwaitTimeMS', 'maxTimeMS'],
'bulkWrite' => ['let', 'requests', 'session', 'ordered', 'bypassDocumentValidation', 'comment'],
'createChangeStream' => ['pipeline', 'session', 'fullDocument', 'fullDocumentBeforeChange', 'resumeAfter', 'startAfter', 'startAtOperationTime', 'batchSize', 'collation', 'maxAwaitTimeMS', 'comment', 'showExpandedEvents'],
'createFindCursor' => ['filter', 'session', 'allowDiskUse', 'allowPartialResults', 'batchSize', 'collation', 'comment', 'cursorType', 'hint', 'limit', 'max', 'maxAwaitTimeMS', 'maxScan', 'maxTimeMS', 'min', 'modifiers', 'noCursorTimeout', 'oplogReplay', 'projection', 'returnKey', 'showRecordId', 'skip', 'snapshot', 'sort'],
Expand Down