Skip to content

Commit acd9c37

Browse files
GromNaNGromNaN
committed
Consolidate validation of aggregation pipeline options
Empty pipelines are allowed Reword "list of pipeline operations" As used in the doc Makes is_pipeline accept empty list Keep forbidding empty replacement in ReplaceOne Update psalm-baseline Add argument to is_pipeline to set return value for empty pipeline Necessary to handle non-array values
1 parent b97cffb commit acd9c37

12 files changed

+24
-44
lines changed

‎psalm-baseline.xml

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -417,9 +417,8 @@
417417
</DocblockTypeContradiction>
418418
</file>
419419
<file src="src/Operation/CreateCollection.php">
420-
<MixedArgument occurrences="3">
421-
<code>$i</code>
422-
<code>$i</code>
420+
<MixedArgument occurrences="2">
421+
<code>$options['pipeline']</code>
423422
<code>$this-&gt;options['typeMap']</code>
424423
</MixedArgument>
425424
<MixedAssignment occurrences="3">

‎src/Client.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,7 @@ public function startSession(array $options = [])
352352
* Create a change stream for watching changes to the cluster.
353353
*
354354
* @see Watch::__construct() for supported options
355-
* @param array $pipeline List of pipeline operations
355+
* @param array $pipeline Aggregation pipeline
356356
* @param array $options Command options
357357
* @return ChangeStream
358358
* @throws InvalidArgumentException for parameter/option parsing errors

‎src/Collection.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ public function __toString()
199199
* "result" array from the command response document.
200200
*
201201
* @see Aggregate::__construct() for supported options
202-
* @param array $pipeline List of pipeline operations
202+
* @param array $pipeline Aggregation pipeline
203203
* @param array $options Command options
204204
* @return Traversable
205205
* @throws UnexpectedValueException if the command response was malformed
@@ -1105,7 +1105,7 @@ public function updateOne($filter, $update, array $options = [])
11051105
* Create a change stream for watching changes to the collection.
11061106
*
11071107
* @see Watch::__construct() for supported options
1108-
* @param array $pipeline List of pipeline operations
1108+
* @param array $pipeline Aggregation pipeline
11091109
* @param array $options Command options
11101110
* @return ChangeStream
11111111
* @throws InvalidArgumentException for parameter/option parsing errors

‎src/Database.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ public function __toString()
187187
* and $listLocalSessions. Requires MongoDB >= 3.6
188188
*
189189
* @see Aggregate::__construct() for supported options
190-
* @param array $pipeline List of pipeline operations
190+
* @param array $pipeline Aggregation pipeline
191191
* @param array $options Command options
192192
* @return Traversable
193193
* @throws UnexpectedValueException if the command response was malformed
@@ -598,7 +598,7 @@ public function selectGridFSBucket(array $options = [])
598598
* Create a change stream for watching changes to the database.
599599
*
600600
* @see Watch::__construct() for supported options
601-
* @param array $pipeline List of pipeline operations
601+
* @param array $pipeline Aggregation pipeline
602602
* @param array $options Command options
603603
* @return ChangeStream
604604
* @throws InvalidArgumentException for parameter/option parsing errors

‎src/Operation/Aggregate.php

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@
3939
use function is_string;
4040
use function MongoDB\create_field_path_type_map;
4141
use function MongoDB\is_last_pipeline_operator_write;
42-
use function sprintf;
42+
use function MongoDB\is_pipeline;
4343

4444
/**
4545
* Operation for the aggregate command.
@@ -131,24 +131,14 @@ class Aggregate implements Executable, Explainable
131131
*
132132
* @param string $databaseName Database name
133133
* @param string|null $collectionName Collection name
134-
* @param array $pipeline List of pipeline operations
134+
* @param array $pipeline Aggregation pipeline
135135
* @param array $options Command options
136136
* @throws InvalidArgumentException for parameter/option parsing errors
137137
*/
138138
public function __construct(string $databaseName, ?string $collectionName, array $pipeline, array $options = [])
139139
{
140-
$expectedIndex = 0;
141-
142-
foreach ($pipeline as $i => $operation) {
143-
if ($i !== $expectedIndex) {
144-
throw new InvalidArgumentException(sprintf('$pipeline is not a list (unexpected index: "%s")', $i));
145-
}
146-
147-
if (! is_array($operation) && ! is_object($operation)) {
148-
throw InvalidArgumentException::invalidType(sprintf('$pipeline[%d]', $i), $operation, 'array or object');
149-
}
150-
151-
$expectedIndex += 1;
140+
if (! is_pipeline($pipeline, true /* allowEmpty */)) {
141+
throw new InvalidArgumentException('$pipeline is not a valid aggregation pipeline');
152142
}
153143

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

‎src/Operation/CreateCollection.php

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
use function is_integer;
3131
use function is_object;
3232
use function is_string;
33-
use function sprintf;
33+
use function MongoDB\is_pipeline;
3434
use function trigger_error;
3535

3636
use const E_USER_DEPRECATED;
@@ -240,20 +240,8 @@ public function __construct(string $databaseName, string $collectionName, array
240240
trigger_error('The "autoIndexId" option is deprecated and will be removed in a future release', E_USER_DEPRECATED);
241241
}
242242

243-
if (isset($options['pipeline'])) {
244-
$expectedIndex = 0;
245-
246-
foreach ($options['pipeline'] as $i => $operation) {
247-
if ($i !== $expectedIndex) {
248-
throw new InvalidArgumentException(sprintf('The "pipeline" option is not a list (unexpected index: "%s")', $i));
249-
}
250-
251-
if (! is_array($operation) && ! is_object($operation)) {
252-
throw InvalidArgumentException::invalidType(sprintf('$options["pipeline"][%d]', $i), $operation, 'array or object');
253-
}
254-
255-
$expectedIndex += 1;
256-
}
243+
if (isset($options['pipeline']) && ! is_pipeline($options['pipeline'], true /* allowEmpty */)) {
244+
throw new InvalidArgumentException('"pipeline" option is not a valid aggregation pipeline');
257245
}
258246

259247
$this->databaseName = $databaseName;

‎src/Operation/Watch.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,7 @@ class Watch implements Executable, /* @internal */ CommandSubscriber
196196
* @param Manager $manager Manager instance from the driver
197197
* @param string|null $databaseName Database name
198198
* @param string|null $collectionName Collection name
199-
* @param array $pipeline List of pipeline operations
199+
* @param array $pipeline Aggregation pipeline
200200
* @param array $options Command options
201201
* @throws InvalidArgumentException for parameter/option parsing errors
202202
*/

‎src/functions.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -303,7 +303,7 @@ function is_in_transaction(array $options): bool
303303
* executed against a primary server.
304304
*
305305
* @internal
306-
* @param array $pipeline List of pipeline operations
306+
* @param array $pipeline Aggregation pipeline
307307
*/
308308
function is_last_pipeline_operator_write(array $pipeline): bool
309309
{

‎tests/FunctionsTest.php

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -243,9 +243,9 @@ public function testIsLastPipelineOperatorWrite(callable $cast): void
243243
}
244244

245245
/** @dataProvider providePipelines */
246-
public function testIsPipeline($expected, $pipeline): void
246+
public function testIsPipeline($expected, $pipeline, $allowEmpty = false): void
247247
{
248-
$this->assertSame($expected, is_pipeline($pipeline));
248+
$this->assertSame($expected, is_pipeline($pipeline, $allowEmpty));
249249
}
250250

251251
public function providePipelines(): array
@@ -286,6 +286,9 @@ public function providePipelines(): array
286286
'invalid pipeline element type: Serializable' => [false, new BSONArray([new BSONArray([])])],
287287
'invalid pipeline element type: PackedArray' => [false, PackedArray::fromPHP([[]])],
288288
// Empty array has no pipeline stages
289+
'valid empty: array' => [true, [], true],
290+
'valid empty: Serializable' => [true, new BSONArray([]), true],
291+
'valid empty: PackedArray' => [true, PackedArray::fromPHP([]), true],
289292
'invalid empty: array' => [false, []],
290293
'invalid empty: Serializable' => [false, new BSONArray([])],
291294
'invalid empty: PackedArray' => [false, PackedArray::fromPHP([])],

‎tests/Operation/AggregateTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ class AggregateTest extends TestCase
1010
public function testConstructorPipelineArgumentMustBeAList(): void
1111
{
1212
$this->expectException(InvalidArgumentException::class);
13-
$this->expectExceptionMessage('$pipeline is not a list (unexpected index: "1")');
13+
$this->expectExceptionMessage('$pipeline is not a valid aggregation pipeline');
1414
new Aggregate($this->getDatabaseName(), $this->getCollectionName(), [1 => ['$match' => ['x' => 1]]]);
1515
}
1616

‎tests/Operation/CreateCollectionTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ class CreateCollectionTest extends TestCase
1010
public function testConstructorPipelineOptionMustBeAList(): void
1111
{
1212
$this->expectException(InvalidArgumentException::class);
13-
$this->expectExceptionMessage('The "pipeline" option is not a list (unexpected index: "1")');
13+
$this->expectExceptionMessage('"pipeline" option is not a valid aggregation pipeline');
1414
new CreateCollection($this->getDatabaseName(), $this->getCollectionName(), ['pipeline' => [1 => ['$match' => ['x' => 1]]]]);
1515
}
1616

‎tests/Operation/WatchTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ public function testConstructorCollectionNameShouldBeNullIfDatabaseNameIsNull():
2323
public function testConstructorPipelineArgumentMustBeAList(): void
2424
{
2525
$this->expectException(InvalidArgumentException::class);
26-
$this->expectExceptionMessage('$pipeline is not a list (unexpected index: "foo")');
26+
$this->expectExceptionMessage('$pipeline is not a valid aggregation pipeline');
2727

2828
/* Note: Watch uses array_unshift() to prepend the $changeStream stage
2929
* to the pipeline. Since array_unshift() reindexes numeric keys, we'll

0 commit comments

Comments
 (0)