Skip to content

Forbid queries endAt an uncommitted server timestamp. - #138

Merged
rsgowman merged 1 commit into
masterfrom
rsgowman/better_error
Nov 28, 2018
Merged

Forbid queries endAt an uncommitted server timestamp.#138
rsgowman merged 1 commit into
masterfrom
rsgowman/better_error

Conversation

@rsgowman

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@mikelehen mikelehen left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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