PHPLIB-712: Allow hint for unacknowledged writes using OP_MSG when supported by the server #859
Conversation
| * to check the original error. */ | ||
| if ($e instanceof BulkWriteException && $e->getPrevious() !== null) { | ||
| $e = $e->getPrevious(); | ||
| } |
There was a problem hiding this comment.
libmongoc already raises an error if hint and w:0 are mixed and hint is not supported by the server (see: _mongoc_write_command_execute_idl); however, that manifests as a BulkWriteException.
I still plan to add explicitly throw UnsupportedException from PHPLIB's operation classes. In the meantime, this change was necessary to ensure tests pass as-is. And in light of PHPC-1960, this change is just generally correct since not all BulkWriteExceptions originate from server-side errors.
There was a problem hiding this comment.
Explicit UnsupportedExceptions are now implemented and tested for Update, Delete, and FindAndModify operations.
There was a problem hiding this comment.
This addresses PHPLIB-711.
| @@ -55,7 +55,10 @@ class Update implements Executable, Explainable | |||
| private static $wireVersionForDocumentLevelValidation = 4; | |||
|
|
|||
| /** @var integer */ | |||
| private static $wireVersionForHintServerSideError = 5; | |||
| private static $wireVersionForHint = 8; | |||
There was a problem hiding this comment.
This will ultimately be used for detecting whether hint and w:0 are being used together on an unsupported server.
I also plan to add a method that detects an unacknowledged write concern. Since FindAndModify will also need to be addressed by this PR, I'll likely end up fixing PHPLIB-709 in the process.
There was a problem hiding this comment.
Implemented is_write_concern_acknowledged, which is now used in the Update, Delete, and FindAndModify operations. PHPLIB-709 has been fixed in the process.
jmikola
force-pushed
the
phplib-712
branch
2 times, most recently
from
a0d2d65 to
e0f2f9c
Compare
|
Note: mongodb/specifications#1057 (comment) explains the failing CI build. I think this will require removing |
| @@ -63,6 +64,46 @@ public function testDeleteMany(): void | |||
| $this->assertSameDocuments($expected, $this->collection->find()); | |||
| } | |||
|
|
|||
| public function testHintOptionUnsupportedClientSideError(): void | |||
There was a problem hiding this comment.
Are these scenarios not covered by the automated spec tests, or did you want to ensure that the correct exception thrown?
There was a problem hiding this comment.
Had I not implemented PHPLIB-711, we could rely on the spec tests to differentiate between PHPLIB's UnsupportedException and the BulkWriteException thrown from PHPC encountering libmongoc's client-side error. However, I think unwrapping BulkWriteExceptions is necessary to properly differentiate cilent-side and server-side errors.
With PHPLIB-711 implemented, the unwrapped BulkWriteException (in this particular case) and UnsupportedException are both considered client-side errors so the spec tests really cannot differentiate. That's where these tests come in, to explicitly assert that PHPLIB raises its own internal exception.
jmikola
force-pushed
the
phplib-712
branch
2 times, most recently
from
399ca90 to
003c6d1
Compare
Synced with mongodb/specifications@59a5dad Note: aggregate tests are intentionally excluded since those will be synced by mongodb#854
The CRUD spec requires a client-side error if hint is used and the server would not otherwise raise an error for an unsupported option. This was already being done, but this commit renames an internal constant and adds functional tests. A client-side error is also required if hint is used with an unacknowledged write concern and the server does not support hint for the operation (regardless of error reporting for unsupported options). This was previously done only for FindAndModify; however, it failed to detect some acknowledged write concerns (PHPLIB-712). That issue has been fixed and the commit additionally adds this behavior for update and delete operations.
https://jira.mongodb.org/browse/PHPLIB-712
POC for mongodb/specifications#1057 (DRIVERS-1340)