From d8d44f0acf6d7050caadc523567a707720519e17 Mon Sep 17 00:00:00 2001 From: Jeremy Mikola Date: Thu, 4 Jan 2018 18:22:27 -0500 Subject: [PATCH] PHPC-1050: Command cursor should not invoke getMore at execution --- php_phongo.c | 24 ++++++------ php_phongo.h | 2 + php_phongo_structs.h | 1 + src/MongoDB/Cursor.c | 20 +++++++++- tests/cursor/bug1050-001.phpt | 71 +++++++++++++++++++++++++++++++++ tests/cursor/bug1050-002.phpt | 74 +++++++++++++++++++++++++++++++++++ 6 files changed, 178 insertions(+), 14 deletions(-) create mode 100644 tests/cursor/bug1050-001.phpt create mode 100644 tests/cursor/bug1050-002.phpt diff --git a/php_phongo.c b/php_phongo.c index da21e85f4..7c89f8ae3 100644 --- a/php_phongo.c +++ b/php_phongo.c @@ -248,6 +248,7 @@ static void phongo_cursor_init(zval *return_value, mongoc_client_t *client, mong intern->cursor = cursor; intern->server_id = mongoc_cursor_get_hint(cursor); intern->client = client; + intern->advanced = false; if (readPreference) { #if PHP_VERSION_ID >= 70000 @@ -286,6 +287,10 @@ static void phongo_cursor_init_for_query(zval *return_value, mongoc_client_t *cl /* namespace has already been validated by phongo_execute_query() */ phongo_split_namespace(namespace, &intern->database, &intern->collection); + /* cursor has already been advanced by phongo_execute_query() calling + * phongo_cursor_advance_and_check_for_error() */ + intern->advanced = true; + #if PHP_VERSION_ID >= 70000 ZVAL_ZVAL(&intern->query, query, 1, 0); #else @@ -709,9 +714,9 @@ bool phongo_execute_bulk_write(mongoc_client_t *client, const char *namespace, p return success; } /* }}} */ -/* Advance the cursor and return whether there is an error. On error, the cursor - * will be destroyed and an exception will be thrown. */ -static bool phongo_advance_cursor_and_check_for_error(mongoc_cursor_t *cursor TSRMLS_DC) +/* Advance the cursor and return whether there is an error. On error, false is + * returned and an exception is thrown. */ +bool phongo_cursor_advance_and_check_for_error(mongoc_cursor_t *cursor TSRMLS_DC) /* {{{ */ { const bson_t *doc; @@ -720,20 +725,18 @@ static bool phongo_advance_cursor_and_check_for_error(mongoc_cursor_t *cursor TS /* Check for connection related exceptions */ if (EG(exception)) { - mongoc_cursor_destroy(cursor); return false; } /* Could simply be no docs, which is not an error */ if (mongoc_cursor_error(cursor, &error)) { phongo_throw_exception_from_bson_error_t(&error TSRMLS_CC); - mongoc_cursor_destroy(cursor); return false; } } return true; -} +} /* }}} */ int phongo_execute_query(mongoc_client_t *client, const char *namespace, zval *zquery, zval *options, uint32_t server_id, zval *return_value, int return_value_used TSRMLS_DC) /* {{{ */ { @@ -784,7 +787,8 @@ int phongo_execute_query(mongoc_client_t *client, const char *namespace, zval *z mongoc_cursor_set_max_await_time_ms(cursor, query->max_await_time_ms); } - if (!phongo_advance_cursor_and_check_for_error(cursor TSRMLS_CC)) { + if (!phongo_cursor_advance_and_check_for_error(cursor TSRMLS_CC)) { + mongoc_cursor_destroy(cursor); return false; } @@ -912,15 +916,9 @@ int phongo_execute_command(mongoc_client_t *client, php_phongo_command_type_t ty bson_destroy(&reply); } - if (!phongo_advance_cursor_and_check_for_error(cmd_cursor TSRMLS_CC)) { - /* If an error is found, the cmd_cursor is destroyed already */ - return false; - } - phongo_cursor_init_for_command(return_value, client, cmd_cursor, db, zcommand, zreadPreference TSRMLS_CC); return true; } /* }}} */ - /* }}} */ /* {{{ mongoc types from from_zval */ diff --git a/php_phongo.h b/php_phongo.h index fb39554a3..399d9e782 100644 --- a/php_phongo.h +++ b/php_phongo.h @@ -137,6 +137,8 @@ bool phongo_execute_bulk_write (mongoc_client_t *client, c int phongo_execute_command (mongoc_client_t *client, php_phongo_command_type_t type, const char *db, zval *zcommand, zval *zreadPreference, uint32_t server_id, zval *return_value, int return_value_used TSRMLS_DC); int phongo_execute_query (mongoc_client_t *client, const char *namespace, zval *zquery, zval *zreadPreference, uint32_t server_id, zval *return_value, int return_value_used TSRMLS_DC); +bool phongo_cursor_advance_and_check_for_error(mongoc_cursor_t *cursor TSRMLS_DC); + const mongoc_read_concern_t* phongo_read_concern_from_zval (zval *zread_concern TSRMLS_DC); const mongoc_read_prefs_t* phongo_read_preference_from_zval(zval *zread_preference TSRMLS_DC); const mongoc_write_concern_t* phongo_write_concern_from_zval (zval *zwrite_concern TSRMLS_DC); diff --git a/php_phongo_structs.h b/php_phongo_structs.h index f914385a8..eecd97066 100644 --- a/php_phongo_structs.h +++ b/php_phongo_structs.h @@ -55,6 +55,7 @@ typedef struct { mongoc_cursor_t *cursor; mongoc_client_t *client; uint32_t server_id; + bool advanced; php_phongo_bson_state visitor_data; int got_iterator; long current; diff --git a/src/MongoDB/Cursor.c b/src/MongoDB/Cursor.c index 49cb25c78..68ecae4b9 100644 --- a/src/MongoDB/Cursor.c +++ b/src/MongoDB/Cursor.c @@ -96,7 +96,15 @@ static void php_phongo_cursor_iterator_move_forward(zend_object_iterator *iter T const bson_t *doc; php_phongo_cursor_free_current(cursor); - cursor->current++; + + /* If the cursor has already advanced, increment its position. Otherwise, + * the first call to mongoc_cursor_next() will be made below and we should + * leave its position at zero. */ + if (cursor->advanced) { + cursor->current++; + } else { + cursor->advanced = true; + } if (mongoc_cursor_next(cursor->cursor, &doc)) { php_phongo_bson_to_zval_ex(bson_get_data(doc), doc->len, &cursor->visitor_data); @@ -117,6 +125,16 @@ static void php_phongo_cursor_iterator_rewind(zend_object_iterator *iter TSRMLS_ php_phongo_cursor_t *cursor = cursor_it->cursor; const bson_t *doc; + /* If the cursor was never advanced (e.g. command cursor), do so now */ + if (!cursor->advanced) { + cursor->advanced = true; + + if (!phongo_cursor_advance_and_check_for_error(cursor->cursor TSRMLS_CC)) { + /* Exception should already have been thrown */ + return; + } + } + if (cursor->current > 0) { phongo_throw_exception(PHONGO_ERROR_LOGIC TSRMLS_CC, "Cursors cannot rewind after starting iteration"); return; diff --git a/tests/cursor/bug1050-001.phpt b/tests/cursor/bug1050-001.phpt new file mode 100644 index 000000000..4c45dae4c --- /dev/null +++ b/tests/cursor/bug1050-001.phpt @@ -0,0 +1,71 @@ +--TEST-- +PHPC-1050: Command cursor should not invoke getMore at execution +--SKIPIF-- + + +--FILE-- + COLLECTION_NAME, + 'pipeline' => [ + ['$changeStream' => (object) []], + ], + 'cursor' => (object) [], + ], + [ + 'maxAwaitTimeMS' => 1000, + ] +); + +$start = microtime(true); +$cursor = $manager->executeReadCommand(DATABASE_NAME, $cmd); +printf("Executing command took %0.6f seconds\n", microtime(true) - $start); + +$it = new IteratorIterator($cursor); + +$start = microtime(true); +$it->rewind(); +printf("Rewinding cursor took %0.6f seconds\n", microtime(true) - $start); +printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no'); + +$bulk = new MongoDB\Driver\BulkWrite; +$bulk->insert(['x' => 1]); +$manager->executeBulkWrite(NS, $bulk); + +$start = microtime(true); +$it->next(); +printf("Advancing cursor took %0.6f seconds\n", microtime(true) - $start); +printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no'); + +$document = $it->current(); + +if (isset($document)) { + printf("Operation type: %s\n", $document->operationType); + var_dump($document->fullDocument); +} + +?> +===DONE=== + +--EXPECTF-- +Executing command took 0.%d seconds +Rewinding cursor took 1.%d seconds +Current position is valid: no +Advancing cursor took %d.%d seconds +Current position is valid: yes +Operation type: insert +object(stdClass)#%d (%d) { + ["_id"]=> + object(MongoDB\BSON\ObjectId)#%d (%d) { + ["oid"]=> + string(24) "%x" + } + ["x"]=> + int(1) +} +===DONE=== diff --git a/tests/cursor/bug1050-002.phpt b/tests/cursor/bug1050-002.phpt new file mode 100644 index 000000000..664eb3720 --- /dev/null +++ b/tests/cursor/bug1050-002.phpt @@ -0,0 +1,74 @@ +--TEST-- +PHPC-1050: Command cursor should not invoke getMore at execution (rewind omitted) +--SKIPIF-- + + +--FILE-- + COLLECTION_NAME, + 'pipeline' => [ + ['$changeStream' => (object) []], + ], + 'cursor' => (object) [], + ], + [ + 'maxAwaitTimeMS' => 1000, + ] +); + +$start = microtime(true); +$cursor = $manager->executeReadCommand(DATABASE_NAME, $cmd); +printf("Executing command took %0.6f seconds\n", microtime(true) - $start); + +$it = new IteratorIterator($cursor); + +printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no'); + +$start = microtime(true); +$it->next(); +printf("Advancing cursor took %0.6f seconds\n", microtime(true) - $start); +printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no'); + +$bulk = new MongoDB\Driver\BulkWrite; +$bulk->insert(['x' => 1]); +$manager->executeBulkWrite(NS, $bulk); + +$start = microtime(true); +$it->next(); +printf("Advancing cursor took %0.6f seconds\n", microtime(true) - $start); +printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no'); + +$document = $it->current(); + +if (isset($document)) { + printf("Operation type: %s\n", $document->operationType); + var_dump($document->fullDocument); +} + +?> +===DONE=== + +--EXPECTF-- +Executing command took 0.%d seconds +Current position is valid: no +Advancing cursor took 1.%d seconds +Current position is valid: no +Advancing cursor took %d.%d seconds +Current position is valid: yes +Operation type: insert +object(stdClass)#%d (%d) { + ["_id"]=> + object(MongoDB\BSON\ObjectId)#%d (%d) { + ["oid"]=> + string(24) "%x" + } + ["x"]=> + int(1) +} +===DONE===