PHPC-980: Sessions API #680

jmikola · 2017-11-28T23:27:07Z

https://jira.mongodb.org/browse/PHPC-980

I reached a road block supporting options for phongo_execute_query() and ended up continuing that work in #707 while refactoring the option parsing for execute methods for PHPC-1057.

This PR is limited just to relevant session commits. All included spec tests should be passing.

derickr

It doesn't compile on PHP 5: https://travis-ci.org/mongodb/mongo-php-driver/jobs/316783642#L1424-1432

It segfaults on PHP 7: https://travis-ci.org/mongodb/mongo-php-driver/jobs/316783639#L1934

derickr · 2017-12-15T11:01:45Z

php_phongo.c

+	const mongoc_client_session_t *client_session;
+
+	if (Z_TYPE_P(option) != IS_OBJECT || !instanceof_function(Z_OBJCE_P(option), php_phongo_session_ce TSRMLS_CC)) {
+		phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected \"session\" option to be %s, %s given", ZSTR_VAL(php_phongo_session_ce->name), zend_get_type_by_const(Z_TYPE_P(option)));


This should use the newly introduce macro from your other PR to show the class names.

Same answer as #707 (comment).

This will be updated to use macros in #705 after that PR is merged and this is rebased.

derickr · 2017-12-15T11:23:50Z

php_phongo.c

+	}
+
+	if (!mongoc_client_session_append(Z_SESSION_OBJ_P(option)->client_session, mongoc_opts, NULL)) {
+		phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Error appending \"session\" option");


Is this likely to happen? If so, we should improve our error message to suggest how users can fix it.

Very unlikely. BSON append for primitive types only fails if the buffer cannot grow or the document length exceeds a 32-bit integer range. Similar to other places where we check for failure (e.g. appending "serverId"), I'm just being a good libmongoc citizen here.

derickr · 2017-12-15T11:24:30Z

php_phongo.c

@@ -606,6 +649,10 @@ static int phongo_execute_parse_options(mongoc_client_t* client, int server_id,
 				if (!process_read_preference(*driver_option, mongoc_opts, zreadPreference, client, server_id TSRMLS_CC)) {
 					return false;
 				}
+			} else if ((!strcmp(ZSTR_VAL(string_key), "session", zsession, client))) {


strcmp here, but strcasecmp below?

Knowing that we were not going to allow case-insensitive options with PHPC-1057, I opted not to use strcasecmp() when I implemented this. That said, this code no longer exists after the refactoring in #705, since we know use the php_array functions to access it.

derickr · 2017-12-15T11:25:35Z

src/MongoDB/Manager.c

+		return;
+	}
+
+	if (options && php_array_exists(options, "causalConsistency")) {


Do you want to extract option parsing into its own function like we do for many other options arrays?

We can consider that in the future if the options list grows a bit. I believe I modeled this after the ReadPreference constructor, which also parses a single option ("maxStalenessSeconds") at present.

derickr · 2017-12-15T11:27:08Z

src/MongoDB/Session.c

+
+	intern = Z_SESSION_OBJ_P(getThis());
+
+	if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "A", &zcluster_time) == FAILURE) {


I presume we shouldn't need to verify the structure of this cluster time object?

Good question. libmongoc has a private _mongoc_parse_cluster_time() function used to validate its structure. The driver only cares about the timestamp and doesn't validate the signature (only used server-side AFAIK).

mongoc_client_session_advance_cluster_time() only assigns it if it parses successfully, and the function is void so we don't even get an error if it's malformed. I'm curious if that was intentional. Perhaps @ajdavis can chime in.

On a related note, the spec doesn't refer to cluster time as an "opaque value" like we do for session and transaction IDs in retryable writes. Therefore, users can certainly access it and poke around with it. The spec also doesn't say anything about reporting errors if users try to advance an invalid value (see here).

Right, we only care that it has a timestamp, we don't know or care about other fields. I didn't think it worthwhile to implement elaborate error handling. Applications should simply save a cluster time from a session, then set it on another session, without modification. If you make a mistake there's an error log.

derickr · 2017-12-15T11:29:15Z

src/MongoDB/Session.c

+		php_phongo_bson_state state = PHONGO_BSON_STATE_INITIALIZER;
+		/* Use native arrays for debugging output */
+		state.map.root_type = PHONGO_TYPEMAP_NATIVE_ARRAY;
+		state.map.document_type = PHONGO_TYPEMAP_NATIVE_ARRAY;


You'd almost want to pull this often done conversion with NATIVE/NATIVE out into its own function.

Or perhaps a separate initializer struct like PHONGO_BSON_STATE_INITIALIZER but used just for debugging? Either way, that's a good idea. I've opened PHPC-1074 to track it.

jmikola · 2017-12-15T17:28:45Z

Rebased on master and using the new PHONGO_ZVAL_CLASS_OR_TYPE_NAME() macro.

This introduces a Session class, which wraps libmongoc's client session object. Additionally, we support a "session" option on Manager and Server execute methods to allow operations to be associated with an explicit session (mostly relevant for causal consistency).

derickr

LGTM

jmikola force-pushed the phpc-980 branch 6 times, most recently from 67344a9 to 1420a64 Compare December 8, 2017 22:59

jmikola force-pushed the phpc-980 branch 6 times, most recently from ed78252 to 8be214f Compare December 15, 2017 06:23

jmikola mentioned this pull request Dec 15, 2017

PHPC-1057: Refactor option parsing for execute methods #707

Merged

jmikola force-pushed the phpc-980 branch from 8be214f to e802f2c Compare December 15, 2017 06:30

jmikola requested a review from derickr December 15, 2017 06:33

jmikola changed the title ~~[WIP] PHPC-980: Sessions API~~ PHPC-980: Sessions API Dec 15, 2017

derickr suggested changes Dec 15, 2017

View reviewed changes

jmikola force-pushed the phpc-980 branch 2 times, most recently from cf946fe to 8eb75a6 Compare December 15, 2017 17:17

jmikola force-pushed the phpc-980 branch from 8eb75a6 to 636a385 Compare December 15, 2017 21:19

jmikola added 2 commits December 15, 2017 16:25

PHPC-980: Driver session spec tests

b2d85bf

jmikola force-pushed the phpc-980 branch from 636a385 to b2d85bf Compare December 15, 2017 21:26

derickr approved these changes Dec 18, 2017

View reviewed changes

jmikola merged commit b2d85bf into mongodb:master Dec 18, 2017

jmikola added a commit that referenced this pull request Dec 18, 2017

Merge pull request #680

4f8c20b

jmikola deleted the phpc-980 branch December 18, 2017 17:10


		intern = Z_SESSION_OBJ_P(getThis());

		if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "A", &zcluster_time) == FAILURE) {

PHPC-980: Sessions API #680

PHPC-980: Sessions API #680

Uh oh!

Conversation

jmikola commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola commented Dec 15, 2017

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmikola commented Nov 28, 2017 •

edited

Loading