Skip to content

Commit d8d44f0

Browse files
jmikolajmikola
committed
PHPC-1050: Command cursor should not invoke getMore at execution
1 parent 4cdf895 commit d8d44f0

File tree

6 files changed

+178
-14
lines changed

6 files changed

+178
-14
lines changed

‎php_phongo.c‎

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,7 @@ static void phongo_cursor_init(zval *return_value, mongoc_client_t *client, mong
248248
intern->cursor = cursor;
249249
intern->server_id = mongoc_cursor_get_hint(cursor);
250250
intern->client = client;
251+
intern->advanced = false;
251252

252253
if (readPreference) {
253254
#if PHP_VERSION_ID >= 70000
@@ -286,6 +287,10 @@ static void phongo_cursor_init_for_query(zval *return_value, mongoc_client_t *cl
286287
/* namespace has already been validated by phongo_execute_query() */
287288
phongo_split_namespace(namespace, &intern->database, &intern->collection);
288289

290+
/* cursor has already been advanced by phongo_execute_query() calling
291+
* phongo_cursor_advance_and_check_for_error() */
292+
intern->advanced = true;
293+
289294
#if PHP_VERSION_ID >= 70000
290295
ZVAL_ZVAL(&intern->query, query, 1, 0);
291296
#else
@@ -709,9 +714,9 @@ bool phongo_execute_bulk_write(mongoc_client_t *client, const char *namespace, p
709714
return success;
710715
} /* }}} */
711716

712-
/* Advance the cursor and return whether there is an error. On error, the cursor
713-
* will be destroyed and an exception will be thrown. */
714-
static bool phongo_advance_cursor_and_check_for_error(mongoc_cursor_t *cursor TSRMLS_DC)
717+
/* Advance the cursor and return whether there is an error. On error, false is
718+
* returned and an exception is thrown. */
719+
bool phongo_cursor_advance_and_check_for_error(mongoc_cursor_t *cursor TSRMLS_DC) /* {{{ */
715720
{
716721
const bson_t *doc;
717722

@@ -720,20 +725,18 @@ static bool phongo_advance_cursor_and_check_for_error(mongoc_cursor_t *cursor TS
720725

721726
/* Check for connection related exceptions */
722727
if (EG(exception)) {
723-
mongoc_cursor_destroy(cursor);
724728
return false;
725729
}
726730

727731
/* Could simply be no docs, which is not an error */
728732
if (mongoc_cursor_error(cursor, &error)) {
729733
phongo_throw_exception_from_bson_error_t(&error TSRMLS_CC);
730-
mongoc_cursor_destroy(cursor);
731734
return false;
732735
}
733736
}
734737

735738
return true;
736-
}
739+
} /* }}} */
737740

738741
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) /* {{{ */
739742
{
@@ -784,7 +787,8 @@ int phongo_execute_query(mongoc_client_t *client, const char *namespace, zval *z
784787
mongoc_cursor_set_max_await_time_ms(cursor, query->max_await_time_ms);
785788
}
786789

787-
if (!phongo_advance_cursor_and_check_for_error(cursor TSRMLS_CC)) {
790+
if (!phongo_cursor_advance_and_check_for_error(cursor TSRMLS_CC)) {
791+
mongoc_cursor_destroy(cursor);
788792
return false;
789793
}
790794

@@ -912,15 +916,9 @@ int phongo_execute_command(mongoc_client_t *client, php_phongo_command_type_t ty
912916
bson_destroy(&reply);
913917
}
914918

915-
if (!phongo_advance_cursor_and_check_for_error(cmd_cursor TSRMLS_CC)) {
916-
/* If an error is found, the cmd_cursor is destroyed already */
917-
return false;
918-
}
919-
920919
phongo_cursor_init_for_command(return_value, client, cmd_cursor, db, zcommand, zreadPreference TSRMLS_CC);
921920
return true;
922921
} /* }}} */
923-
924922
/* }}} */
925923

926924
/* {{{ mongoc types from from_zval */

‎php_phongo.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,8 @@ bool phongo_execute_bulk_write (mongoc_client_t *client, c
137137
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);
138138
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);
139139

140+
bool phongo_cursor_advance_and_check_for_error(mongoc_cursor_t *cursor TSRMLS_DC);
141+
140142
const mongoc_read_concern_t* phongo_read_concern_from_zval (zval *zread_concern TSRMLS_DC);
141143
const mongoc_read_prefs_t* phongo_read_preference_from_zval(zval *zread_preference TSRMLS_DC);
142144
const mongoc_write_concern_t* phongo_write_concern_from_zval (zval *zwrite_concern TSRMLS_DC);

‎php_phongo_structs.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ typedef struct {
5555
mongoc_cursor_t *cursor;
5656
mongoc_client_t *client;
5757
uint32_t server_id;
58+
bool advanced;
5859
php_phongo_bson_state visitor_data;
5960
int got_iterator;
6061
long current;

‎src/MongoDB/Cursor.c‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,15 @@ static void php_phongo_cursor_iterator_move_forward(zend_object_iterator *iter T
9696
const bson_t *doc;
9797

9898
php_phongo_cursor_free_current(cursor);
99-
cursor->current++;
99+
100+
/* If the cursor has already advanced, increment its position. Otherwise,
101+
* the first call to mongoc_cursor_next() will be made below and we should
102+
* leave its position at zero. */
103+
if (cursor->advanced) {
104+
cursor->current++;
105+
} else {
106+
cursor->advanced = true;
107+
}
100108

101109
if (mongoc_cursor_next(cursor->cursor, &doc)) {
102110
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_
117125
php_phongo_cursor_t *cursor = cursor_it->cursor;
118126
const bson_t *doc;
119127

128+
/* If the cursor was never advanced (e.g. command cursor), do so now */
129+
if (!cursor->advanced) {
130+
cursor->advanced = true;
131+
132+
if (!phongo_cursor_advance_and_check_for_error(cursor->cursor TSRMLS_CC)) {
133+
/* Exception should already have been thrown */
134+
return;
135+
}
136+
}
137+
120138
if (cursor->current > 0) {
121139
phongo_throw_exception(PHONGO_ERROR_LOGIC TSRMLS_CC, "Cursors cannot rewind after starting iteration");
122140
return;

‎tests/cursor/bug1050-001.phpt‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
--TEST--
2+
PHPC-1050: Command cursor should not invoke getMore at execution
3+
--SKIPIF--
4+
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
5+
<?php NEEDS('REPLICASET'); CLEANUP(REPLICASET); ?>
6+
--FILE--
7+
<?php
8+
require_once __DIR__ . "/../utils/basic.inc";
9+
10+
$manager = new MongoDB\Driver\Manager(REPLICASET);
11+
12+
$cmd = new MongoDB\Driver\Command(
13+
[
14+
'aggregate' => COLLECTION_NAME,
15+
'pipeline' => [
16+
['$changeStream' => (object) []],
17+
],
18+
'cursor' => (object) [],
19+
],
20+
[
21+
'maxAwaitTimeMS' => 1000,
22+
]
23+
);
24+
25+
$start = microtime(true);
26+
$cursor = $manager->executeReadCommand(DATABASE_NAME, $cmd);
27+
printf("Executing command took %0.6f seconds\n", microtime(true) - $start);
28+
29+
$it = new IteratorIterator($cursor);
30+
31+
$start = microtime(true);
32+
$it->rewind();
33+
printf("Rewinding cursor took %0.6f seconds\n", microtime(true) - $start);
34+
printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no');
35+
36+
$bulk = new MongoDB\Driver\BulkWrite;
37+
$bulk->insert(['x' => 1]);
38+
$manager->executeBulkWrite(NS, $bulk);
39+
40+
$start = microtime(true);
41+
$it->next();
42+
printf("Advancing cursor took %0.6f seconds\n", microtime(true) - $start);
43+
printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no');
44+
45+
$document = $it->current();
46+
47+
if (isset($document)) {
48+
printf("Operation type: %s\n", $document->operationType);
49+
var_dump($document->fullDocument);
50+
}
51+
52+
?>
53+
===DONE===
54+
<?php exit(0); ?>
55+
--EXPECTF--
56+
Executing command took 0.%d seconds
57+
Rewinding cursor took 1.%d seconds
58+
Current position is valid: no
59+
Advancing cursor took %d.%d seconds
60+
Current position is valid: yes
61+
Operation type: insert
62+
object(stdClass)#%d (%d) {
63+
["_id"]=>
64+
object(MongoDB\BSON\ObjectId)#%d (%d) {
65+
["oid"]=>
66+
string(24) "%x"
67+
}
68+
["x"]=>
69+
int(1)
70+
}
71+
===DONE===

‎tests/cursor/bug1050-002.phpt‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
--TEST--
2+
PHPC-1050: Command cursor should not invoke getMore at execution (rewind omitted)
3+
--SKIPIF--
4+
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
5+
<?php NEEDS('REPLICASET'); CLEANUP(REPLICASET); ?>
6+
--FILE--
7+
<?php
8+
require_once __DIR__ . "/../utils/basic.inc";
9+
10+
$manager = new MongoDB\Driver\Manager(REPLICASET);
11+
12+
$cmd = new MongoDB\Driver\Command(
13+
[
14+
'aggregate' => COLLECTION_NAME,
15+
'pipeline' => [
16+
['$changeStream' => (object) []],
17+
],
18+
'cursor' => (object) [],
19+
],
20+
[
21+
'maxAwaitTimeMS' => 1000,
22+
]
23+
);
24+
25+
$start = microtime(true);
26+
$cursor = $manager->executeReadCommand(DATABASE_NAME, $cmd);
27+
printf("Executing command took %0.6f seconds\n", microtime(true) - $start);
28+
29+
$it = new IteratorIterator($cursor);
30+
31+
printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no');
32+
33+
$start = microtime(true);
34+
$it->next();
35+
printf("Advancing cursor took %0.6f seconds\n", microtime(true) - $start);
36+
printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no');
37+
38+
$bulk = new MongoDB\Driver\BulkWrite;
39+
$bulk->insert(['x' => 1]);
40+
$manager->executeBulkWrite(NS, $bulk);
41+
42+
$start = microtime(true);
43+
$it->next();
44+
printf("Advancing cursor took %0.6f seconds\n", microtime(true) - $start);
45+
printf("Current position is valid: %s\n", $it->valid() ? 'yes' : 'no');
46+
47+
$document = $it->current();
48+
49+
if (isset($document)) {
50+
printf("Operation type: %s\n", $document->operationType);
51+
var_dump($document->fullDocument);
52+
}
53+
54+
?>
55+
===DONE===
56+
<?php exit(0); ?>
57+
--EXPECTF--
58+
Executing command took 0.%d seconds
59+
Current position is valid: no
60+
Advancing cursor took 1.%d seconds
61+
Current position is valid: no
62+
Advancing cursor took %d.%d seconds
63+
Current position is valid: yes
64+
Operation type: insert
65+
object(stdClass)#%d (%d) {
66+
["_id"]=>
67+
object(MongoDB\BSON\ObjectId)#%d (%d) {
68+
["oid"]=>
69+
string(24) "%x"
70+
}
71+
["x"]=>
72+
int(1)
73+
}
74+
===DONE===

0 commit comments

Comments
 (0)