PHPC-1231: Add Session::isInTransaction to indicate whether a transaction is active #882
Conversation
| @@ -390,6 +390,24 @@ static PHP_METHOD(Session, endSession) | |||
| mongoc_client_session_destroy(intern->client_session); | |||
| } /* }}} */ | |||
|
|
|||
| /* {{{ proto void MongoDB\Driver\Session::isInTransaction(void) | |||
| Returns whether a multi-document transaction is in progress */ | |||
| static PHP_METHOD(Session, isInTransaction) | |||
There was a problem hiding this comment.
This method name looks strange to me, but I think it's preferable to the more verbose option of isTransactionInProgress() or the like.
There was a problem hiding this comment.
Change it to inTransaction ? Don't like that much either...
There was a problem hiding this comment.
I didn't want to suggest inTransaction because it's not consistent with prefixing boolean-returning methods with "is". Totally OK with sticking with isInTransaction().
src/MongoDB/Session.c
Outdated
| return; | ||
| } | ||
|
|
||
| RETURN_BOOL(!!mongoc_client_session_in_transaction(intern->client_session)); |
There was a problem hiding this comment.
Is the quick bool cast with !! necessary? The function returns a bool and _mongoc_client_session_in_txn() uses true and false return values.
I would imagine there are a few other places where we trust libmongoc's bool return values.
src/MongoDB/Session.c
Outdated
| PHP_ME(Session, advanceClusterTime, ai_Session_advanceClusterTime, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL) | ||
| PHP_ME(Session, advanceOperationTime, ai_Session_advanceOperationTime, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL) | ||
| PHP_ME(Session, commitTransaction, ai_Session_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL) | ||
| PHP_ME(Session, endSession, ai_Session_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL) | ||
| PHP_ME(Session, isInTransaction, ai_Session_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL) |
There was a problem hiding this comment.
Since it looks like you're alphabetizing these, isInTransaction should go after the get methods.
There was a problem hiding this comment.
Yeah, I had alphabetised them in the first commit, ... and then put the new one in the wrong place :-)
| @@ -0,0 +1,36 @@ | |||
| --TEST-- | |||
| MongoDB\Driver\Session::isInTransaction() | |||
There was a problem hiding this comment.
I think this test would do well to cover both the "started" and "in progress" transaction states. At the moment, I believe it only covers "started", which is before the first command is sent. See test_in_transaction() for a good example. Ideally, we should just port that to a PHP test.
There was a problem hiding this comment.
Our test was basically that test already, but I added two sections that actually have an operation in the transaction too.
derickr
force-pushed
the
PHPC-1231-session-is-in-transaction
branch
from
52fc511 to
3493de9
Compare
| @@ -390,6 +390,24 @@ static PHP_METHOD(Session, endSession) | |||
| mongoc_client_session_destroy(intern->client_session); | |||
| } /* }}} */ | |||
|
|
|||
| /* {{{ proto void MongoDB\Driver\Session::isInTransaction(void) | |||
| Returns whether a multi-document transaction is in progress */ | |||
| static PHP_METHOD(Session, isInTransaction) | |||
There was a problem hiding this comment.
Change it to inTransaction ? Don't like that much either...
src/MongoDB/Session.c
Outdated
| return; | ||
| } | ||
|
|
||
| RETURN_BOOL(!!mongoc_client_session_in_transaction(intern->client_session)); |
src/MongoDB/Session.c
Outdated
| PHP_ME(Session, advanceClusterTime, ai_Session_advanceClusterTime, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL) | ||
| PHP_ME(Session, advanceOperationTime, ai_Session_advanceOperationTime, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL) | ||
| PHP_ME(Session, commitTransaction, ai_Session_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL) | ||
| PHP_ME(Session, endSession, ai_Session_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL) | ||
| PHP_ME(Session, isInTransaction, ai_Session_void, ZEND_ACC_PUBLIC | ZEND_ACC_FINAL) |
There was a problem hiding this comment.
Yeah, I had alphabetised them in the first commit, ... and then put the new one in the wrong place :-)
| @@ -0,0 +1,36 @@ | |||
| --TEST-- | |||
| MongoDB\Driver\Session::isInTransaction() | |||
There was a problem hiding this comment.
Our test was basically that test already, but I added two sections that actually have an operation in the transaction too.
| $session->startTransaction(); | ||
| var_dump($session->isInTransaction()); | ||
| $session->commitTransaction(); | ||
| var_dump($session->isInTransaction()); |
There was a problem hiding this comment.
Given that transactions are started lazily during the first command, how does this interact when the only operation is an abort or commit? I assume the driver is still sending something in this case and the server just registers it as a no-op?
Perhaps a question for @ShaneHarvey.
derickr
force-pushed
the
PHPC-1231-session-is-in-transaction
branch
from
3493de9 to
a917c35
Compare
https://jira.mongodb.org/browse/PHPC-1231