PHPLIB-722: Ignore writeConcern option for read-only aggregations

[GromNaN]

Fix PHPLIB-722

This does not change anything functionally, but cleanup $options in the constructor instead of picking the writeOption conditionally in executeCommand(). There is no impact on getCommandDocument() which already ignores writeConcern.

[GromNaN]

Failing test:

1) MongoDB\Tests\Collection\CollectionFunctionalTest::testMethodInTransactionWithWriteConcernOption with data set #0 (Closure Object (...), 'rw')
Failed asserting that exception of type "MongoDB\Exception\UnsupportedException" is thrown.

Ignoring the writeConcern in the constructor skips the exception thrown in Aggregate::execute()

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

Lines 264 to 273 in 316d6d6

	$inTransaction = isset($this->options['session']) && $this->options['session']->isInTransaction();
	if ($inTransaction) {
	if (isset($this->options['readConcern'])) {
	throw UnsupportedException::readConcernNotSupportedInTransaction();
	}
	if (isset($this->options['writeConcern'])) {
	throw UnsupportedException::writeConcernNotSupportedInTransaction();
	}
	}

[jmikola]

collectionMethodClosures() includes the following aggregate() call, which is classified as a read and write:

[
    function ($collection, $session, $options = []): void {
        $collection->aggregate(
            [['$match' => ['_id' => ['$lt' => 3]]]],
            ['session' => $session] + $options
        );
    }, 'rw',
],

Can you try changing "rw" there to "r" and creating a second copy with an actual write stage (e.g. $out) with "rw" changed to "w"? That might overcome the unsetting of writeConcern and allow the actual exception to be triggered.

[GromNaN]

Fixed a typo

[GromNaN]

$out and $merge operators aren't supported in transaction. They throw a server error. I have a doubt on the best solution to exclude the new "read-write aggregate" method:

1. This solution: skip the test in case of exception. But unlike tests skipped due to a temporary incompatiblity with the running environment, this test will never pass.
2. Ignores the error and still run the assertions on command options. Even if that does not make sense to test a failing command. That could hide other errors and make debugging more difficult.
3. exclude this case from the test cases by adding a collectionTransactionMethodClosures data provider and add a new char t in $rw to exclude methods incompatible with transactions.

[jmikola]

1. This solution: skip the test in case of exception. But unlike tests skipped due to a temporary incompatiblity with the running environment, this test will never pass.

2. Ignores the error and still run the assertions on command options. Even if that does not make sense to test a failing command. That could hide other errors and make debugging more difficult.

I don't think conditionally skipping the test makes sense, given that the test only cares about asserting that readConcern and writeConcern are omitted on the outgoing command.

Catching and ignoring OperationNotSupportedInTransaction seems preferable. For consistency, I would suggest defining a private constant for its numeric code (263).

That said, I don't think that's a permanent solution. Read on.

3. exclude this case from the test cases by adding a collectionTransactionMethodClosures data provider and add a new char t in $rw to exclude methods incompatible with transactions.

I don't think this is necessary. collectionMethodClosures() already comments out various methods that cannot be run within a transaction. I was going to suggest removing all of those comments after adding logic to ignore OperationNotSupportedInTransaction, but then I realized that would only fix testMethodDoesNotInheritReadWriteConcernInTransaction.

testMethodInTransactionWithWriteConcernOption and testMethodInTransactionWithReadConcernOption use derivative data providers that filter for strings containing "w" and "r", respectively. Those tests would never work with the unsupported operations because we never added logic to check for active transactions and throw UnsupportedException.

Aggregate is a special case because pipelines are generally supported in transactions and only write operators are unsupported.

---

With all of that in mind, I think the most sensible solution is removing the pipeline with a write stage and just leaving a comment above read-only pipeline to explain that we're intentionally not testing a pipeline with a write stage since those are unsupported.

That would mean we have no test coverage for the following logic near the top of Aggregate::execute():

if (isset($this->options['writeConcern'])) {
   throw UnsupportedException::writeConcernNotSupportedInTransaction();
}

If you'd like to cover that, I'd suggest doing one of the following:

• Create a second data provider for testMethodDoesNotInheritReadWriteConcernInTransaction() that only includes the write pipeline. The test only takes one parameter so there'd be no reason to include "rw" in the data provider. This would still require modifying the test to catch and ignore OperationNotSupportedInTransaction errors.
• Create a separate test similar to testMethodDoesNotInheritReadWriteConcernInTransaction() that tests Aggregate directly with the write pipeline. A doc block for that test would be a good opportunity to explain why we couldn't do this with the common data provider.

Alternatively, just remove the write pipeline and leave this untested. I'm not too concerned about doing so since we know this code path will lead to an exception anyway.

[GromNaN]

You're right, there's no point in testing behavior on a command that's going to fail anyway.

[GromNaN]

Debugging is simplified when the data provider uses names. Even more so because of the filter applied in collectionReadMethodClosures and collectionWriteMethodClosures.

[GromNaN]

Small refactoring to use the PHP 8 function, provided by the polyfill for older versions.

[GromNaN]

This pipeline is actually read-only. I added a write pipeline as @jmikola recommended.