Skip to content

PHPLIB-418: Add the ability to specify a pipeline to an update command #670

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
Aug 27, 2019
Merged

PHPLIB-418: Add the ability to specify a pipeline to an update command #670

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
2628c09
PHPLIB-418: Add the ability to specify a pipeline to an update command
alcaeus Aug 12, 2019
fc87768
PHPLIB-418: Fix wrong runOn syntax in crud/updateWithPiplines spec tests
alcaeus Aug 22, 2019
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
4 changes: 3 additions & 1 deletion docs/includes/apiargs-MongoDBCollection-common-param.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ type: array|object
description: |
Specifies the field and value combinations to update and any relevant update
operators. ``$update`` uses MongoDB's :method:`update operators
</reference/operator/update>`.
</reference/operator/update>`. Starting with MongoDB 4.2, an `aggregation
pipeline <https://docs.mongodb.com/master/reference/command/update/#update-with-an-aggregation-pipeline>`_
can be passed as this parameter.
interface: phpmethod
operation: ~
optional: false
Expand Down
9 changes: 8 additions & 1 deletion src/Operation/FindAndModify.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
use function is_integer;
use function is_object;
use function MongoDB\create_field_path_type_map;
use function MongoDB\is_pipeline;
use function MongoDB\server_supports_feature;

/**
Expand Down Expand Up @@ -255,12 +256,18 @@ private function createCommandDocument(Server $server)
$cmd['upsert'] = $this->options['upsert'];
}

foreach (['collation', 'fields', 'query', 'sort', 'update'] as $option) {
foreach (['collation', 'fields', 'query', 'sort'] as $option) {
if (isset($this->options[$option])) {
$cmd[$option] = (object) $this->options[$option];
}
}

if (isset($this->options['update'])) {
$cmd['update'] = is_pipeline($this->options['update'])
? $this->options['update']
: (object) $this->options['update'];
}

if (isset($this->options['arrayFilters'])) {
$cmd['arrayFilters'] = $this->options['arrayFilters'];
}
Expand Down
5 changes: 3 additions & 2 deletions src/Operation/FindOneAndUpdate.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use function is_integer;
use function is_object;
use function MongoDB\is_first_key_operator;
use function MongoDB\is_pipeline;

/**
* Operation for updating a document with the findAndModify command.
Expand Down Expand Up @@ -105,8 +106,8 @@ public function __construct($databaseName, $collectionName, $filter, $update, ar
throw InvalidArgumentException::invalidType('$update', $update, 'array or object');
}

if (! is_first_key_operator($update)) {
throw new InvalidArgumentException('First key in $update argument is not an update operator');
if (! is_first_key_operator($update) && ! is_pipeline($update)) {
throw new InvalidArgumentException('Expected an update document with operator as first key or a pipeline');
}

$options += [
Expand Down
5 changes: 5 additions & 0 deletions src/Operation/ReplaceOne.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use function is_array;
use function is_object;
use function MongoDB\is_first_key_operator;
use function MongoDB\is_pipeline;

/**
* Operation for replacing a single document with the update command.
Expand Down Expand Up @@ -79,6 +80,10 @@ public function __construct($databaseName, $collectionName, $filter, $replacemen
throw new InvalidArgumentException('First key in $replacement argument is an update operator');
}

if (is_pipeline($replacement)) {
throw new InvalidArgumentException('$replacement argument is a pipeline');
}

$this->update = new Update(
$databaseName,
$collectionName,
Expand Down
3 changes: 2 additions & 1 deletion src/Operation/Update.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
use function is_bool;
use function is_object;
use function MongoDB\is_first_key_operator;
use function MongoDB\is_pipeline;
use function MongoDB\server_supports_feature;

/**
Expand Down Expand Up @@ -126,7 +127,7 @@ public function __construct($databaseName, $collectionName, $filter, $update, ar
throw InvalidArgumentException::invalidType('"multi" option', $options['multi'], 'boolean');
}

if ($options['multi'] && ! is_first_key_operator($update)) {
if ($options['multi'] && ! is_first_key_operator($update) && ! is_pipeline($update)) {
throw new InvalidArgumentException('"multi" option cannot be true if $update is a replacement document');
}

Expand Down
5 changes: 3 additions & 2 deletions src/Operation/UpdateMany.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use function is_array;
use function is_object;
use function MongoDB\is_first_key_operator;
use function MongoDB\is_pipeline;

/**
* Operation for updating multiple documents with the update command.
Expand Down Expand Up @@ -81,8 +82,8 @@ public function __construct($databaseName, $collectionName, $filter, $update, ar
throw InvalidArgumentException::invalidType('$update', $update, 'array or object');
}

if (! is_first_key_operator($update)) {
throw new InvalidArgumentException('First key in $update argument is not an update operator');
if (! is_first_key_operator($update) && ! is_pipeline($update)) {
throw new InvalidArgumentException('Expected an update document with operator as first key or a pipeline');
}

$this->update = new Update(
Expand Down
5 changes: 3 additions & 2 deletions src/Operation/UpdateOne.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use function is_array;
use function is_object;
use function MongoDB\is_first_key_operator;
use function MongoDB\is_pipeline;

/**
* Operation for updating a single document with the update command.
Expand Down Expand Up @@ -81,8 +82,8 @@ public function __construct($databaseName, $collectionName, $filter, $update, ar
throw InvalidArgumentException::invalidType('$update', $update, 'array or object');
}

if (! is_first_key_operator($update)) {
throw new InvalidArgumentException('First key in $update argument is not an update operator');
if (! is_first_key_operator($update) && ! is_pipeline($update)) {
throw new InvalidArgumentException('Expected an update document with operator as first key or a pipeline');
}

$this->update = new Update(
Expand Down
41 changes: 41 additions & 0 deletions src/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,47 @@ function is_first_key_operator($document)
return isset($firstKey[0]) && $firstKey[0] === '$';
}

/**
* Returns whether an update specification is a valid aggregation pipeline.
*
* @internal
* @param mixed $pipeline
* @return boolean
*/
function is_pipeline($pipeline)
{
if (! is_array($pipeline)) {
return false;
}

if ($pipeline === []) {
Copy link
Member

Choose a reason for hiding this comment

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

Continuing from my previous shell testing, it looks like MongoDB 4.2 accepts an empty pipeline and simply NOPs:

rs0:PRIMARY> db.runCommand({update:"foo",updates:[{q:{},u:[]}]});
{
	"n" : 1,
	"nModified" : 0,
        ...
}

Contrast this with an empty replacement document:

rs0:PRIMARY> db.runCommand({update:"foo",updates:[{q:{},u:{}}]});
{
	"n" : 1,
	"nModified" : 1,
	...
}

Is this function based on libmongoc's _mongoc_document_is_pipeline()? I see that also returns false if the argument has no elements.

I'm not arguing that this is incorrect. I actually think it's the most intuitive behavior, especially for PHP where users would most commonly pass [] as an empty replacement document; however, I'm curious if this was dictated by the spec. If not, perhaps we should mention it in the SPEC ticket I proposed opening in my earlier comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this function based on libmongoc's _mongoc_document_is_pipeline()? I see that also returns false if the argument has no elements.

That is correct. I assumed that it'd be wrong to treat an empty array as an aggregation pipeline, but I also specifically remember one test failing because of that, which is why I added the check after all.

Copy link
Member

Choose a reason for hiding this comment

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

I made a note of this in SPEC-1430.

return false;
}

$expectedKey = 0;

foreach ($pipeline as $key => $stage) {
if (! is_array($stage) && ! is_object($stage)) {
return false;
}

if ($expectedKey !== $key) {
return false;
}

$expectedKey++;
$stage = (array) $stage;
reset($stage);
$key = key($stage);

if (! isset($key[0]) || $key[0] !== '$') {
return false;
}
}

return true;
}

/**
* Returns whether we are currently in a transaction.
*
Expand Down
30 changes: 30 additions & 0 deletions tests/FunctionsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use function MongoDB\generate_index_name;
use function MongoDB\is_first_key_operator;
use function MongoDB\is_mapreduce_output_inline;
use function MongoDB\is_pipeline;

/**
* Unit tests for utility functions.
Expand Down Expand Up @@ -224,4 +225,33 @@ public function provideTypeMapValues()
],
];
}

/**
* @dataProvider providePipelines
*/
public function testIsPipeline($expected, $pipeline)
{
$this->assertSame($expected, is_pipeline($pipeline));
}

public function providePipelines()
{
return [
'Not an array' => [false, (object) []],
'Empty array' => [false, []],
'Non-sequential indexes in array' => [false, [1 => ['$group' => []]]],
'Update document instead of pipeline' => [false, ['$set' => ['foo' => 'bar']]],
'Invalid pipeline stage' => [false, [['group' => []]]],
'Update with DbRef' => [false, ['x' => ['$ref' => 'foo', '$id' => 'bar']]],
'Valid pipeline' => [
true,
[
['$match' => ['foo' => 'bar']],
['$group' => ['_id' => 1]],
],
],
'False positive with DbRef in numeric field' => [true, ['0' => ['$ref' => 'foo', '$id' => 'bar']]],
'DbRef in numeric field as object' => [false, (object) ['0' => ['$ref' => 'foo', '$id' => 'bar']]],
];
}
}
4 changes: 2 additions & 2 deletions tests/Operation/FindOneAndUpdateTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ public function testConstructorUpdateArgumentTypeCheck($update)
new FindOneAndUpdate($this->getDatabaseName(), $this->getCollectionName(), [], $update);
}

public function testConstructorUpdateArgumentRequiresOperators()
public function testConstructorUpdateArgumentRequiresOperatorsOrPipeline()
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('First key in $update argument is not an update operator');
$this->expectExceptionMessage('Expected an update document with operator as first key or a pipeline');
new FindOneAndUpdate($this->getDatabaseName(), $this->getCollectionName(), [], []);
}

Expand Down
2 changes: 1 addition & 1 deletion tests/Operation/UpdateManyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public function testConstructorUpdateArgument($update)
public function testConstructorUpdateArgumentRequiresOperators($replacement)
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('First key in $update argument is not an update operator');
$this->expectExceptionMessage('Expected an update document with operator as first key or a pipeline');
new UpdateMany($this->getDatabaseName(), $this->getCollectionName(), ['x' => 1], $replacement);
}

Expand Down
2 changes: 1 addition & 1 deletion tests/Operation/UpdateOneTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public function testConstructorUpdateArgument($update)
public function testConstructorUpdateArgumentRequiresOperators($replacement)
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('First key in $update argument is not an update operator');
$this->expectExceptionMessage('Expected an update document with operator as first key or a pipeline');
new UpdateOne($this->getDatabaseName(), $this->getCollectionName(), ['x' => 1], $replacement);
}

Expand Down
13 changes: 0 additions & 13 deletions tests/SpecTests/CrudSpecTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,6 @@
*/
class CrudSpecTest extends FunctionalTestCase
{
/* These should all pass before the driver can be considered compatible with
* MongoDB 4.2. */
private static $incompleteTests = [
'bulkWrite-arrayFilters: BulkWrite with arrayFilters' => 'Fails due to command assertions',
'updateWithPipelines: UpdateOne using pipelines' => 'PHPLIB-418',
'updateWithPipelines: UpdateMany using pipelines' => 'PHPLIB-418',
'updateWithPipelines: FindOneAndUpdate using pipelines' => 'PHPLIB-418',
];

/**
* Assert that the expected and actual command documents match.
*
Expand All @@ -48,10 +39,6 @@ public static function assertCommandMatches(stdClass $expected, stdClass $actual
*/
public function testCrud(stdClass $test, array $runOn = null, array $data, $databaseName = null, $collectionName = null)
{
if (isset(self::$incompleteTests[$this->dataDescription()])) {
$this->markTestIncomplete(self::$incompleteTests[$this->dataDescription()]);
}

if (isset($runOn)) {
$this->checkServerRequirements($runOn);
}
Expand Down
6 changes: 5 additions & 1 deletion tests/SpecTests/crud/updateWithPipelines.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
{
"runOn": [
Copy link
Member Author

Choose a reason for hiding this comment

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

See mongodb/specifications#635.

{
"minServerVersion": "4.1.11"
}
],
"data": [
{
"_id": 1,
Expand All @@ -16,7 +21,6 @@
"y": 1
}
],
"minServerVersion": "4.1.11",
"collection_name": "test",
"database_name": "crud-tests",
"tests": [
Expand Down