PHPLIB-1129: Improve pipeline checks for replace operations

[jmikola]

https://jira.mongodb.org/browse/PHPLIB-1129

Consolidates data providers in base TestCase and FunctionalTestCase classes.

Change is_pipeline() to conditionally consider empty arrays as pipelines. This was needed for improvements to replace/update parameter validation, and will prove useful for consolidated pipeline validation in PHPLIB-881.

Prohibit update documents and pipelines (both empty and non-empty) for replacement operations.

Prohibit replacement documents and empty pipeines for update operations. Empty pipelines must be prohibited because libmongoc does not support them for update commands (they're indistinguishable from empty replacement documents). For consistency, we also prohibit them for FindOneAndUpdate even though PHPLIB could work around that.

Prohibit update pipelines for replace operations (both empty and non-empty). Previously ReplaceOne only prohibited non-empty pipelines and BulkWrite replaceOne and FindOneAndReplace has no validation. A notable exception is empty arrays, which are treated as replacement documents for BC.

Add tests for valid update/replacement arguments for BulkWrite replaceOne, updateMany, and updateOne operations. Also adds more comprehensive tests for valid $update arguments for UpdateMany and UpdateOne operations (update documents and non-empty pipelines).

---

This depends on #1096 and can be rebased after that is merged.

[GromNaN]

LGTM. My PR added the same argument to is_pipeline for this same issue; I'll rebase it once this one get merged. #1078 (comment)

[GromNaN]

I don't need such comment with phpstorm. It may be replaced by named argument when the lib is upgraded to PHP 8+.

[jmikola]

Interesting. How does PhpStorm decide to show the second parameter name but not the first ($args[1])? It looks like it shows the first parameter for sprintf() on the second line.

[GromNaN]

The hint is not shown when a variable is passed, it's shown for static values only.

[GromNaN]

I don't get why you need to convert the empty array into an empty object. Replacement documents can be array, isn't it?

[jmikola]

Without this, is_pipeline() with allowEmpty: true is going to consider the argument a pipeline and throw an exception (since pipelines are only acceptable for update operations).

Since is_pipeline() historically returned false for empty arrays, an empty array was always accepted for ReplaceOne and would be encoded as a BSON object in the update command (per libmongoc's internal logic discussed in CDRIVER-4658).

Since this PR is adding logic to reject empty pipelines for both update and replace operations, we need to explicitly allow empty arrays for BC.

BulkWrite replaceOne operations historically has never used is_pipeline(), so pipelines could have snuck through there. Empty PHP arrays would still be encoded as a replacement document, though (same libmongoc logic).

FindOneAndReplace also never used is_pipeline() during constructor validation. The only relevant code path was FindAndModify::createCommandDocument() conditionally casting the update option to an object if is_pipeline() returned false (see: this change from PHPLIB-418). I actually think there was a bug there, as an empty array would have been encoded as a BSON array in the findAndModify command -- and thus treated as an empty pipeline (i.e. NOP) in MongoDB 4.2+.

By prohibiting empty pipelines for both updates and replacements, we resolve the last issue with FindOneAndReplace and also enable users to express empty replacement documents as [] like they already could for ReplaceOne and BulkWrite replaceOne (where they previously might have had to pass (object) [] explicitly).

[jmikola]

I actually think there was a bug there, as an empty array would have been encoded as a BSON array in the findAndModify command -- and thus treated as an empty pipeline (i.e. NOP) in MongoDB 4.2+.

Correction. Since FindOneAndReplace previously never validated its $replacement parameter with is_pipeline(), an empty BSON array would still have correctly encoded as a BSON document due to this logic:

mongo-php-library/src/Operation/FindAndModify.php

Lines 295 to 299 in 3a681a3

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

is_pipeline() returns false and the parameter gets cast to an object.

So there was no bug.

Also, I can confirm that the cast in FindAndModify::createCommandDocument() is still necessary. If we execute FindAndModify directly (as is done in the test suite), then we rely on that code to ensure an empty array encodes properly.

Technically, we could also change the constructor logic in FindOneAndReplace to cast all $replacement arrays to objects is_pipeline($replacement) is false, but it's that just adds additional overhead and wouldn't cover the case where FindAndModify is executed directly. The empty array check is cheap and plays nicely with the subsequent validation that rejects any pipeline (empty and non-empty alike).

[GromNaN]

Thanks for this insider comment.

[jmikola]

I wanted to add a comment explaining this conditional cast after diving into it in #1098 (comment).

[jmikola]

I previously added this test case to UpdateFunctionalTest so we had a record of CDRIVER-4658. FindAndModify behaves correctly (unaffected by libmongoc's code path for update commands) and I wanted to make a record of that in the test suite.