Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query.next_page() should produce a null cursor if not set on response #1054

Closed
dhermes opened this issue Aug 11, 2015 · 9 comments
Closed

Query.next_page() should produce a null cursor if not set on response #1054

dhermes opened this issue Aug 11, 2015 · 9 comments
Assignees
Labels
api: datastore Issues related to the Datastore API. priority: p2 Moderately-important priority. Fix may not be included in next release.

Comments

@dhermes
Copy link
Contributor

dhermes commented Aug 11, 2015

Currently it returns an empty string.

@dhermes dhermes added the api: datastore Issues related to the Datastore API. label Aug 11, 2015
@dhermes dhermes self-assigned this Aug 11, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Aug 11, 2015

Noticed this during #1055

@dhermes
Copy link
Contributor Author

dhermes commented Aug 11, 2015

OK, checking response.batch.HasField('end_cursor') is always True, even when the cursor is ''.

@tseaver WDYT we should do here? I think returning '' and setting it as the _start_cursor value on an Iterator is not the right move.

@tseaver
Copy link
Contributor

tseaver commented Aug 12, 2015

@dhermes While investigating, it just seemed easier to make the change (to ensure I didn't break anything).

@dhermes
Copy link
Contributor Author

dhermes commented Aug 12, 2015

No worries.

tseaver added a commit that referenced this issue Aug 12, 2015
#1054: Set query cursor to 'None' when returned as empty string.
@dhermes
Copy link
Contributor Author

dhermes commented Aug 13, 2015

@tseaver Turns out this issue still open.

Remaining concern: Should Connection.run_query check if the end_cursor is set on the response.batch?

@dhermes
Copy link
Contributor Author

dhermes commented Aug 27, 2015

@tseaver Bump

@tseaver
Copy link
Contributor

tseaver commented Aug 31, 2015

I assume you are worried about this line. If the response protobuf doesn't have a value set, we get a None there, right? That seems like the right sentinel to me.

@dhermes
Copy link
Contributor Author

dhermes commented Aug 31, 2015

See what I said above:

checking response.batch.HasField('end_cursor') is always True, even when the cursor is ''.

@lukesneeringer lukesneeringer added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Apr 19, 2017
@lukesneeringer
Copy link
Contributor

Hello,
One of the challenges of maintaining a large open source project is that sometimes, you can bite off more than you can chew. As the lead maintainer of google-cloud-python, I can definitely say that I have let the issues here pile up.

As part of trying to get things under control (as well as to empower us to provide better customer service in the future), I am declaring a "bankruptcy" of sorts on many of the old issues, especially those likely to have been addressed or made obsolete by more recent updates.

My goal is to close stale issues whose relevance or solution is no longer immediately evident, and which appear to be of lower importance. I believe in good faith that this is one of those issues, but I am scanning quickly and may occasionally be wrong. If this is an issue of high importance, please comment here and we will reconsider. If this is an issue whose solution is trivial, please consider providing a pull request.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API. priority: p2 Moderately-important priority. Fix may not be included in next release.
Projects
None yet
Development

No branches or pull requests

3 participants