Skip to content

Forbid queries endAt an uncommitted server timestamp. #138

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

Merged
merged 1 commit into from
Nov 28, 2018

Conversation

rsgowman
Copy link
Member

Without this, it still fails, but:
a) not until serializing the query, and
b) the error is an internal error, and
c) the error message is quite cryptic and has nothing to do with the problem.

@rsgowman rsgowman self-assigned this Nov 22, 2018
@googlebot googlebot added the cla: yes Override cla label Nov 22, 2018
@rsgowman rsgowman requested a review from var-const November 22, 2018 22:23
@rsgowman rsgowman assigned var-const and unassigned rsgowman Nov 22, 2018
collection
.orderBy("timestamp")
.endAt(docSnap)
.addSnapshotListener((snapshot2, error2) -> {}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is addSnapshotListener necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not strictly. It is necessary to trigger the internal error that would otherwise occur though. For that reason, I'd like to keep the snapshot listener in the 'online' case just a few lines down. And if I'm keeping it there, then keeping it here for symmetry seems to make some sense. (Though I don't mind removing it either.) wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

@var-const var-const assigned rsgowman and unassigned var-const Nov 22, 2018
@rsgowman rsgowman assigned mikelehen and unassigned rsgowman Nov 26, 2018
@rsgowman rsgowman requested a review from mikelehen November 26, 2018 18:30
Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@mikelehen mikelehen assigned rsgowman and unassigned mikelehen Nov 26, 2018
@rsgowman
Copy link
Member Author

/retest

@rsgowman rsgowman merged commit 4ae3f1b into master Nov 28, 2018
@rsgowman rsgowman deleted the rsgowman/better_error branch November 28, 2018 18:23
rsgowman added a commit to firebase/firebase-js-sdk that referenced this pull request Feb 11, 2019
Without this, it still fails, but:
a) not until serializing the query, and
b) the error is an internal error, and
c) the error message is quite cryptic and has nothing to do with the problem.

Port of firebase/firebase-android-sdk#138
rsgowman added a commit to firebase/firebase-ios-sdk that referenced this pull request Feb 12, 2019
Without this, it still fails, but:
a) not until serializing the query, and
b) the error is an internal error, and
c) the error message is quite cryptic and has nothing to do with the problem.

Port of firebase/firebase-android-sdk#138
rsgowman added a commit to firebase/firebase-js-sdk that referenced this pull request Feb 12, 2019
* Forbid queries endAt an uncommitted server timestamp.

Without this, it still fails, but:
a) not until serializing the query, and
b) the error is an internal error, and
c) the error message is quite cryptic and has nothing to do with the problem.

Port of firebase/firebase-android-sdk#138
rsgowman added a commit to firebase/firebase-ios-sdk that referenced this pull request Feb 13, 2019
Without this, it still fails, but:
a) not until serializing the query, and
b) the error is an internal error, and
c) the error message is quite cryptic and has nothing to do with the problem.

Port of firebase/firebase-android-sdk#138
@firebase firebase locked and limited conversation to collaborators Oct 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Override cla size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants