Skip to content

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Apr 14, 2015

@jmikola jmikola changed the title [WIP] Cursor iteration issues PHPC-240: Cannot iterate beyond command cursor's first batch Apr 15, 2015
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested removing this because, after simplifying our iteration handlers, it would only be used in the Cursor's debug handler.

@bjori
Copy link
Contributor

bjori commented Apr 15, 2015

lgtm

@bjori
Copy link
Contributor

bjori commented Apr 19, 2015

After reading the cursor-iterator_handlers-001.phpt testcase, I think we have to throw an exception in rewind() if the cursor has been iterated over, or in the middle of iteration.
Repeated calls to rewind() without next() in between is fine (although doesn't make a lot of sense), but after iteration has started a call to rewind() really has to throw OutOfBoundsException.

It is really confusing if calling rewind() is somewhat of a NOOP and you can continue stepping through some parts of it.

rewind($cursor); $first = current($cursor); next($cursor);
rewind($cursor); $firstAgain = current($cursor);
$first must be == $firstAgain

And since cannot do that without re-executing the query, which we clearly don't want to do -- I think we must throw an exception.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are adding tests for this -- maybe add one test where the results fit nicely within the batchSize?

@derickr
Copy link
Contributor

derickr commented Apr 20, 2015

I think calling rewind more than once (even when no next() is called), should throw that exception.

jmikola added 5 commits April 21, 2015 14:23
libmongoc already provides a mechanism to upgrade a cursor with a single command result document into a command cursor, which will seamlessly iterate through the first batch and documents returned by successive getmore ops.

This allows us to remove our own "first batch" handling and simplify the move_forward and rewind iteration handlers.
php_phongo_cursor_iterator_dtor() should not need to free the current element, which is stored on our cursor struct. If the cursor is only used by one iterator, zval_ptr_dtor() on the iterator's intern.data property (i.e. our Cursor zval) will implicitly call the free_object handler, which will invoke php_phongo_cursor_free() and clear the current element.
@bjori
Copy link
Contributor

bjori commented Apr 21, 2015

lgtm -- see also https://jira.mongodb.org/browse/PHPC-255

jmikola added a commit to jmikola/mongo-php-driver that referenced this pull request Apr 21, 2015
@jmikola jmikola merged commit 70b7110 into mongodb:master Apr 21, 2015
@jmikola jmikola deleted the cursor-iteration branch April 21, 2015 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants