Skip to content

Set next_page_token to none if there are no more result#4349

Merged
tseaver merged 2 commits into
googleapis:masterfrom
SpaceKnow:fix/ds-next-page-token
Nov 10, 2017
Merged

Set next_page_token to none if there are no more result#4349
tseaver merged 2 commits into
googleapis:masterfrom
SpaceKnow:fix/ds-next-page-token

Conversation

@mathewcohle

@mathewcohle mathewcohle commented Nov 6, 2017 •
edited by tseaver
Loading

Copy link
Copy Markdown

Depending on empty value of response_pb.batch.end_cursor is not reliable as the behaviour is not documented. It's better to check API response field moreResults, see
https://cloud.google.com/datastore/docs/reference/rpc/google.datastore.v1#google.datastore.v1.QueryResultBatch.

Closes #4347.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 6, 2017
@mathewcohle mathewcohle force-pushed the fix/ds-next-page-token branch 2 times, most recently from 1078441 to 05893ad Compare November 6, 2017 16:04
@mathewcohle mathewcohle changed the title Set next_page_token to none if there are no more result (#4347) Set next_page_token to None using MoreResults enum (#4347) Nov 6, 2017
@dhermes dhermes added the api: datastore Issues related to the Datastore API. label Nov 6, 2017
dhermes
dhermes reviewed Nov 6, 2017
View reviewed changes
Comment thread datastore/google/cloud/datastore/query.py
self._skipped_results = response_pb.batch.skipped_results

if response_pb.batch.end_cursor == b'': # Empty-value for bytes.
if response_pb.batch.more_results == _NO_MORE_RESULTS:

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@tseaver tseaver changed the title Set next_page_token to None using MoreResults enum (#4347) Set next_page_token to none if there are no more result Nov 6, 2017
@tseaver tseaver added the backend label Nov 6, 2017
@mathewcohle

mathewcohle commented Nov 7, 2017

Copy link
Copy Markdown
Author

FYI, I've tested the change against our tests which were previously failing. The patch has fixed the problem.

@mathewcohle mathewcohle force-pushed the fix/ds-next-page-token branch from 05893ad to 9c0d881 Compare November 7, 2017 11:15
mathewcohle added 2 commits November 9, 2017 13:10
Update _process_query_results_done Datastore test
eb2dfd0 
Response ``end_cursor`` might be non-empty as reported in googleapis#4347.
Set next_page_token to None using MoreResults enum (googleapis#4347)
622078d 
Depending on empty value of ``response_pb.batch.end_cursor`` is not
reliable as the behaviour is not documented. It's better to check API
response field ``moreResults``, see
https://cloud.google.com/datastore/docs/reference/rpc/google.datastore.v1#google.datastore.v1.QueryResultBatch.
@mathewcohle mathewcohle force-pushed the fix/ds-next-page-token branch from 9c0d881 to 622078d Compare November 9, 2017 12:12
@mathewcohle

mathewcohle commented Nov 9, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

@dhermes how does it look? This is currently blocker for migration to env: flex.

@dhermes

dhermes commented Nov 9, 2017

Copy link
Copy Markdown
Contributor

@mathewcohle RE: "how does it look?"

The code looks fine, but my previous sentiment is unchanged:

I'd prefer sign off from the backend team before making the change.

@tseaver tseaver merged commit 69a9840 into googleapis:master Nov 10, 2017
@tseaver

tseaver commented Nov 10, 2017

Copy link
Copy Markdown
Contributor

@mathewcohle Thank you for the patch!

@mathewcohle mathewcohle deleted the fix/ds-next-page-token branch November 10, 2017 17:26
@dhermes

dhermes commented Nov 10, 2017

Copy link
Copy Markdown
Contributor

@tseaver Sorry I didn't chime in. I wanted to actually reproduce #4347 before merging this. The build right after this broke, so we may have to revert this change.

dhermes added a commit to dhermes/google-cloud-python that referenced this pull request Nov 10, 2017
@dhermes
Update datastore doctests to reflect change in cursor behavior.
8df8887 
Follow up to googleapis#4349.
@dhermes dhermes mentioned this pull request Nov 10, 2017
Merged
@tseaver

tseaver commented Nov 11, 2017

Copy link
Copy Markdown
Contributor

Sorry: everything was green on the last commit in the PR, and you said you were waiting on signoff from the back-end team, which we got.

dhermes added a commit that referenced this pull request Nov 11, 2017
@dhermes
Update datastore doctests to reflect change in cursor behavior. (#4382)
9cb3d06 
Follow up to #4349.
parthea pushed a commit that referenced this pull request Nov 24, 2025
@tseaver
Set next_page_token to none if there are no more result (#4349)
5bdd49f 
* Update _process_query_results_done Datastore test

Response ``end_cursor`` might be non-empty as reported in #4347.

* Set next_page_token to None using MoreResults enum (#4347)

Depending on empty value of ``response_pb.batch.end_cursor`` is not
reliable as the behaviour is not documented. It's better to check API
response field ``moreResults``, see
https://cloud.google.com/datastore/docs/reference/rpc/google.datastore.v1#google.datastore.v1.QueryResultBatch.
parthea pushed a commit that referenced this pull request Nov 24, 2025
@dhermes
Update datastore doctests to reflect change in cursor behavior. (#4382)
66ad087 
Follow up to #4349.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

2 more reviewers

@dhermes dhermes dhermes left review comments

@wsh wsh wsh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: datastore Issues related to the Datastore API. backend cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mathewcohle @dhermes @tseaver @wsh @googlebot