PHPC-1761: Snapshot Reads #1243
Conversation
jmikola
force-pushed
the
phpc-1761
branch
2 times, most recently
from
a26be88 to
64c4499
Compare
| } | ||
|
|
||
| if (cs_opts && mongoc_session_opts_get_causal_consistency(cs_opts) && mongoc_session_opts_get_snapshot(cs_opts)) { | ||
| phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT, "Only one of \"causalConsistency\" and \"snapshot\" can be enabled"); |
There was a problem hiding this comment.
libmongoc also raises this error through mongoc_client_start_session; however, the code/domain used for that would lead to a PHPC throwing a RuntimeException. I handled this manually so we can ensure an InvalidArgumentException is thrown, which makes more sense since this is just option validation.
| <?php require __DIR__ . "/../utils/basic-skipif.inc"; ?> | ||
| <?php skip_if_not_libmongoc_crypto(); ?> | ||
| <?php skip_if_not_live(); ?> | ||
| <?php skip_if_server_version('<', '3.6'); ?> |
There was a problem hiding this comment.
The previous tests were missing a SKIPIF entirely.
| @@ -1,24 +0,0 @@ | |||
| --TEST-- | |||
| MongoDB\Driver\Session with wrong defaultTransactionOptions | |||
There was a problem hiding this comment.
I explain why I deleted this in the commit message:
The deleted error test was redundant. It was originally created in ca4c52e to work around changes in PHP 8, but 60febd0 subsequently reintroduced a portable version of the test case.
This test was already covered by session_error-001.phpt, which this PR renamed to manager-startSession_error-001.phpt.
| } else { | ||
| ADD_ASSOC_NULL_EX(&retval, "causalConsistency"); | ||
| ADD_ASSOC_NULL_EX(&retval, "snapshot"); |
There was a problem hiding this comment.
Note: Session.c doesn't actually have getters for the options passed to Manager::startSession(). Session::getTransactionOptions() doesn't directly correlate with the defaultTransactionOptions session option, since it's (a) the combined options from starting a transaction and merging those options with the session's own defaults and (b) only relevant when a session is in an active transaction.
I forget if we considered adding an API for the startSession() options in the past, but I don't feel strongly about it.
|
@tanlisu Do you have any additional questions about this PR? If not, can you complete your review? |
| @@ -1 +1 @@ | |||
| 1.18.0 | |||
| 1.11.1-20210804+git230369cd07 | |||
There was a problem hiding this comment.
Disregard "1.11.1" here, as it's related to an outstanding issue with calc_release_version.py (CDRIVER-3315).
Sorry for the delay, LGTM |
Includes various tickets from the Snapshot Reads epic:
Unified spec tests will be handled separately by PHPLIB-694, although a couple of spec tests are incorporated in this PR for PHPC.
This PR bumps the libmongoc submodule to pull in changes from mongodb/mongo-c-driver#838 for CDRIVER-4110.