PHPC-1438: Expose session state #1061
Conversation
| case MONGOC_TRANSACTION_ABORTED: | ||
| return "aborted"; | ||
| default: | ||
| phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Invalid transaction state %d given", (int) state); |
There was a problem hiding this comment.
Considering that libmongoc exposes transaction states as defined in the spec, I've decided to throw an exception in this case. While this requires us to pay close attention to any new transaction states, I'm not sure what other alternative would be feasible (besides returning the integer value)
There was a problem hiding this comment.
I think an exception is fine here. Based on how CDRIVER-3364 was implemented, we know that libmongoc would abort before we ever reach this point. Also, any new transaction state would likely entail a spec change, which would trickle down as CDRIVER and PHPC tickets, so I'm not worried about missing such a change.
alcaeus
force-pushed
the
phpc-1438
branch
2 times, most recently
from
8c54d8b to
ce7084c
Compare
src/MongoDB/Session.c
Outdated
| { | ||
| switch (state) { | ||
| case MONGOC_TRANSACTION_NONE: | ||
| return "none"; |
There was a problem hiding this comment.
I've decided against exposing the MONGOC_TRANSACTION_ constants directly since we're trying to get rid of that for read preference.
Totally understandable.
I've also opted against introducing constants for those strings for now, but we may consider doing that to allow users to use constants instead of having to write out the strings.
I do think there's value in introducing constants for the strings we return here. The only problem with ReadPreference was exposing the raw numeric values (technically bit flags), which have no meaning outside of libmongoc. I have no qualms about having constants for meaningful string values, as they are useful to avoid having applications hard-code magic string literals.
As for naming, we have a few options:
TRANSACTION_STATE_NONETRANSACTION_NONETXN_STATE_NONETXN_NONE
I can go either way with including "state". I can't imagine what else we might need Session constants for, so I'm not too worried about naming conflicts. "TXN" is a common abbreviation for "Transaction", so maybe that's a no-brainer -- and "TXN_STATE_" is still shorter than "TRANSACTION_".
There was a problem hiding this comment.
As for including "state", I normally would opt for including it to avoid future naming conflicts. However, TXN_STARTING (or TRANSACTION_STARTING) reads better than the other options, so I'd omit "state" for now.
As for TXN versus TRANSACTION, I normally opt against abbreviations but I'll defer to your decision here. I can live with either.
There was a problem hiding this comment.
If we're skipping STATE, let's use TRANSACTION. TRANSACTION_STATE_ was really the only one that gave me pause due to its length.
| case MONGOC_TRANSACTION_ABORTED: | ||
| return "aborted"; | ||
| default: | ||
| phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Invalid transaction state %d given", (int) state); |
There was a problem hiding this comment.
I think an exception is fine here. Based on how CDRIVER-3364 was implemented, we know that libmongoc would abort before we ever reach this point. Also, any new transaction state would likely entail a spec change, which would trickle down as CDRIVER and PHPC tickets, so I'm not worried about missing such a change.
|
|
| zend_declare_class_constant_string(php_phongo_session_ce, ZEND_STRL("TRANSACTION_STARTING"), PHONGO_TRANSACTION_STARTING TSRMLS_CC); | ||
| zend_declare_class_constant_string(php_phongo_session_ce, ZEND_STRL("TRANSACTION_IN_PROGRESS"), PHONGO_TRANSACTION_IN_PROGRESS TSRMLS_CC); | ||
| zend_declare_class_constant_string(php_phongo_session_ce, ZEND_STRL("TRANSACTION_COMMITTED"), PHONGO_TRANSACTION_COMMITTED TSRMLS_CC); | ||
| zend_declare_class_constant_string(php_phongo_session_ce, ZEND_STRL("TRANSACTION_ABORTED"), PHONGO_TRANSACTION_ABORTED TSRMLS_CC); |
There was a problem hiding this comment.
I suggest adding one more test for these constants, similar to https://github.com/mongodb/mongo-php-driver/blob/master/tests/readConcern/readconcern-constants.phpt.
src/MongoDB/Session.c
Outdated
|
|
||
| state = mongoc_client_session_get_transaction_state(intern->client_session); | ||
|
|
||
| PHONGO_RETURN_STRING(php_phongo_get_transaction_state_string(state TSRMLS_CC)); |
There was a problem hiding this comment.
Similar to my concerns in #1062 (comment), I think we should only call PHONGO_RETURN_STRING if php_phongo_get_transaction_state_string returns a non-null value; otherwise we risk passing null to strlen(). Consider:
const char *state;
// ...
// Or keep state and add a state_str var if this is too long for your liking
state = php_phongo_get_transaction_state_string(mongoc_client_session_get_transaction_state(intern->client_session) TSRMLS_CC);
if (!state) {
/* Exception already thrown */
return;
}
PHONGO_RETURN_STRING(state);We won't be able to write a test for this (you can do so manually if you like), but it provides some peace of mind.
There was a problem hiding this comment.
Logic added. The joy of working with pointers :)
https://jira.mongodb.org/browse/PHPC-1438
Contains the libmongoc update from #1060.
I've decided against exposing the
MONGOC_TRANSACTION_constants directly since we're trying to get rid of that for read preference. I've also opted against introducing constants for those strings for now, but we may consider doing that to allow users to use constants instead of having to write out the strings.