PHPLIB-881: Consolidate validation of aggregation pipeline options #1078
Conversation
GromNaN
force-pushed
the
PHPLIB-881
branch
2 times, most recently
from
b6bba88 to
4f70901
Compare
src/Operation/BulkWrite.php
Outdated
| @@ -229,7 +229,7 @@ public function __construct(string $databaseName, string $collectionName, array | |||
| throw InvalidArgumentException::invalidType(sprintf('$operations[%d]["%s"][1]', $i, $type), $args[1], 'array or object'); | |||
| } | |||
|
|
|||
| if (! is_first_key_operator($args[1]) && ! is_pipeline($args[1])) { | |||
| if ($args[1] === [] || ! is_first_key_operator($args[1]) && ! is_pipeline($args[1])) { | |||
There was a problem hiding this comment.
I need to check the spec to see if an empty array is accepted in the following cases:
- BulkWrite UPDATE_MANY and UPDATE_ONE
- FindOneAndUpdate
- ReplaceOne
- Update, UpdateMany, UpdateOne
There was a problem hiding this comment.
Outside of ReplaceOne and findOneAndReplace, an empty update array should be rejected, as it would entail a no-op on the server. In a replace operation, it would mean "replace the matched document with an empty document", which could be a valid use case (even though I fail to see how).
Replace currently allows an aggregation pipeline, but this will be removed in PHPLIB-1129.
There was a problem hiding this comment.
So I'll change the behavior for ReplaceOne that is currently rejecting empty array.
There was a problem hiding this comment.
Should be fine - just make sure it is covered by a test.
FWIW, the shell behaves the same way:
test> // Replace with empty replacement
test> db.test.replaceOne({_id: 1}, {})
{
acknowledged: true,
insertedId: null,
matchedCount: 1,
modifiedCount: 1,
upsertedCount: 0
}
test> // Update with empty document
test> db.test.updateOne({_id: 1}, {})
MongoInvalidArgumentError: Update document requires atomic operators
test> // Update with empty pipeline
test> db.test.updateOne({_id: 1}, [])
MongoInvalidArgumentError: Update document requires atomic operators
There was a problem hiding this comment.
Thanks for checking, I'll adopt this reflex.
Test case already added: https://github.com/mongodb/mongo-php-library/pull/1078/files#diff-6e13b3e2ae70bee5981233671f8a994c0111a3cf84e2f958e723bdd08c9c83d2R45
tests/Operation/ReplaceOneTest.php
Outdated
| ['y' => 1], | ||
| (object) ['y' => 1], | ||
| new BSONDocument(['y' => 1]), | ||
| ]); | ||
| } | ||
|
|
||
| public function provideUpdateDocuments() | ||
| public function provideInvalidReplacementDocuments() |
There was a problem hiding this comment.
Note: this is going to conflict with #1077. The changes in that PR are more involved, so I'd suggest waiting on that to be merged.
Consideration of PackedArray instances for pipeline values is also going to impact this PR.
|
@GromNaN: #1077 has been merged but I'm still expecting some conflicts due to PHPLIB-1129. I also just realized that PHPLIB has historically prohibited empty update pipelines despite those being valid (DRIVERS-2638). Please hold off on resuming this PR until I can sort that out next week. |
GromNaN
force-pushed
the
PHPLIB-881
branch
2 times, most recently
from
124bb58 to
41aad07
Compare
|
PR rebased.
The dumb comparison |
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
I copied the style introduced by @jmikola and discussed in review. Could be reverted. |
Ah, I missed that one. No worries then, perfectly fine with it 👍 |
Fix PHPLIB-881
Leverage
is_pipelineto validate$pipelineargument and"pipeline"option. Since this function returns a boolean, it does not provide context about the error. This change allows to factorise the code and to go further in the validation, but the message in the exception is less precise.