Fix docs unnecessarily undergo limbo resolution while resuming query bug #7740

milaGGL · 2023-10-31T19:49:23Z

While resuming a query and there is an existence filter mismatch detected, SDK updated the limbo docs without waiting for a full re-query result.

Add a new flag to check if we should send docs to limbo
Add missing .watchCurrent for some spec tests
add a new spec test dedicated for this bug
renamed updateLimboDocuments in applyChanges to limboResolutionEnabled for better semantic meaning

Ported to android in firebase/firebase-android-sdk#5506 and ios in firebase/firebase-ios-sdk#12044.

changeset-bot · 2023-10-31T19:49:28Z

🦋 Changeset detected

Latest commit: 2df6a61

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@firebase/firestore	Patch
firebase	Patch
@firebase/firestore-compat	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

google-oss-bot · 2023-10-31T20:16:14Z

Size Report ¹

Affected Products

@firebase/firestore
Type Base (00235ba) Merge (a927106) Diff
browser 374 kB 374 kB +71 B (+0.0%)
esm5 359 kB 360 kB +73 B (+0.0%)
main 576 kB 576 kB +298 B (+0.1%)
module 374 kB 374 kB +71 B (+0.0%)
react-native 374 kB 374 kB +71 B (+0.0%)

`bundle`

Type	Base (`00235ba`)	Merge (`a927106`)	Diff
firestore (Persistence)	302 kB	302 kB	+71 B (+0.0%)
firestore (Query Cursors)	238 kB	238 kB	+71 B (+0.0%)
firestore (Query)	236 kB	236 kB	+71 B (+0.0%)
firestore (Read data once)	224 kB	224 kB	+71 B (+0.0%)
firestore (Realtime updates)	226 kB	226 kB	+71 B (+0.0%)

firebase
Type Base (00235ba) Merge (a927106) Diff
firebase-compat.js 778 kB 778 kB +71 B (+0.0%)
firebase-firestore-compat.js 340 kB 340 kB +71 B (+0.0%)
firebase-firestore.js 434 kB 434 kB +71 B (+0.0%)

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/mlXAogmDtv.html

google-oss-bot · 2023-10-31T20:25:16Z

Size Analysis Report ¹

Affected Products

@firebase/firestore
- enableMultiTabIndexedDbPersistence
  Size
  Type Base (00235ba) Merge (a927106) Diff
  size 221 kB 221 kB +71 B (+0.0%)
  size-with-ext-deps 293 kB 293 kB +71 B (+0.0%)
- getDoc
  Size
  Type Base (00235ba) Merge (a927106) Diff
  size 143 kB 143 kB +71 B (+0.0%)
  size-with-ext-deps 213 kB 213 kB +71 B (+0.0%)
- getDocFromServer
  Size
  Type Base (00235ba) Merge (a927106) Diff
  size 143 kB 143 kB +71 B (+0.0%)
  size-with-ext-deps 213 kB 213 kB +71 B (+0.0%)
- getDocs
  Size
  Type Base (00235ba) Merge (a927106) Diff
  size 145 kB 145 kB +71 B (+0.0%)
  size-with-ext-deps 215 kB 215 kB +71 B (+0.0%)
- getDocsFromCache
  Size
  Type Base (00235ba) Merge (a927106) Diff
  size 102 kB 102 kB +23 B (+0.0%)
  size-with-ext-deps 172 kB 172 kB +23 B (+0.0%)
- getDocsFromServer
  Size
  Type Base (00235ba) Merge (a927106) Diff
  size 144 kB 145 kB +71 B (+0.0%)
  size-with-ext-deps 215 kB 215 kB +71 B (+0.0%)
- onSnapshot
  Size
  Type Base (00235ba) Merge (a927106) Diff
  size 145 kB 145 kB +71 B (+0.0%)
  size-with-ext-deps 216 kB 216 kB +71 B (+0.0%)
- onSnapshotsInSync
  Size
  Type Base (00235ba) Merge (a927106) Diff
  size 135 kB 135 kB +71 B (+0.1%)
  size-with-ext-deps 205 kB 205 kB +71 B (+0.0%)
- persistentMultipleTabManager
  Size
  Type Base (00235ba) Merge (a927106) Diff
  size 218 kB 218 kB +71 B (+0.0%)
  size-with-ext-deps 290 kB 290 kB +71 B (+0.0%)

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/VLld896AKJ.html

ehsannas

looks good. I think it'd be better to add spec tests rather than modify the existing one. because the old test and the new exercise different series of events

milaGGL · 2023-11-06T18:41:22Z

looks good. I think it'd be better to add spec tests rather than modify the existing one. because the old test and the new exercise different series of events

Correct me if I am wrong @dconeybe. The existing one is missing a ".watchCurrent" when resuming a query, adding it on corrects the behaviour, which by the way checks the bug fix.

I will add a new spec test dedicated for this bug fix.

dconeybe · 2023-11-08T16:52:17Z

Correct me if I am wrong @dconeybe. The existing one is missing a ".watchCurrent" when resuming a query, adding it on corrects the behaviour, which by the way checks the bug fix.

I agree with @milaGGL. The fact that the spec tests were missing the watchCurrents() call actually made them invalid. This went unnoticed until this bug fix, which caused the test to fail, uncovering their invalidity. So the tests needed to be "fixed" to properly emulate Watch's behavior. I mean, the tests were, technically, still "valid", but the expected result was incorrect. So this PR needs to change to either (a) more accurately emulate Watch's behavior or (b) update the expected result to correctly represent what the client should do if such a sequence of messages from Watch were to ever be received. I think option A has the most value in this case.

dconeybe

Thanks for the fix! Nice job finding the root cause and an elegant solution. Technically, it all LGTM. My comments are merely nitpicks about style and some minor tweaks to improve readability.

packages/firestore/src/core/sync_engine_impl.ts

packages/firestore/src/core/view.ts

packages/firestore/test/unit/specs/limbo_spec.test.ts

dconeybe

Looks great! Please add a changeset so there will be a release notes entry for this fix. Make sure to word it in such a way that a customer who is unfamiliar with implementation details like "limbo resolution" will have some idea of what was fixed.

…bug (#5506) Ported from : firebase/firebase-js-sdk#7740

milaGGL added 3 commits October 31, 2023 15:03

initial code fix

3b2f2c7

remove parameter default value

78d92d2

update spec tests

49f3a95

milaGGL requested review from a team as code owners October 31, 2023 19:49

rename the flag

b41f528

milaGGL self-assigned this Oct 31, 2023

milaGGL requested a review from dconeybe October 31, 2023 20:04

milaGGL assigned dconeybe and unassigned milaGGL Nov 2, 2023

This was referenced Nov 2, 2023

Fix docs unnecessarily undergo limbo resolution while resuming query bug firebase/firebase-android-sdk#5506

Merged

Fix docs unnecessarily undergo limbo resolution while resuming query bug firebase/firebase-ios-sdk#12044

Merged

format code

c2e6aaa

ehsannas reviewed Nov 3, 2023

View reviewed changes

Update limbo_spec.test.ts

a0a933e

dconeybe requested changes Nov 8, 2023

View reviewed changes

dconeybe assigned milaGGL and unassigned dconeybe Nov 8, 2023

milaGGL added 2 commits November 13, 2023 10:55

resolve comments

d927609

format

5a917ee

milaGGL requested a review from dconeybe November 13, 2023 17:27

milaGGL assigned dconeybe and unassigned milaGGL Nov 13, 2023

dconeybe requested changes Nov 16, 2023

View reviewed changes

dconeybe assigned milaGGL Nov 16, 2023

dconeybe removed their assignment Nov 16, 2023

add changeset

04aed32

milaGGL requested a review from a team as a code owner November 20, 2023 16:12

Merge branch 'master' into mila/fix-limbo-resolution-bug

2df6a61

dconeybe approved these changes Nov 21, 2023

View reviewed changes

milaGGL added a commit to firebase/firebase-android-sdk that referenced this pull request Nov 21, 2023

Fix docs unnecessarily undergo limbo resolution while resuming query …

a014114

…bug (#5506) Ported from : firebase/firebase-js-sdk#7740

milaGGL merged commit 0d29adc into master Nov 22, 2023
40 of 41 checks passed

milaGGL deleted the mila/fix-limbo-resolution-bug branch November 22, 2023 00:24

google-oss-bot mentioned this pull request Nov 22, 2023

Version Packages #7777

Merged

firebase locked and limited conversation to collaborators Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix docs unnecessarily undergo limbo resolution while resuming query bug #7740

Fix docs unnecessarily undergo limbo resolution while resuming query bug #7740

milaGGL commented Oct 31, 2023 •

edited by dconeybe

Loading

changeset-bot bot commented Oct 31, 2023 •

edited

Loading

google-oss-bot commented Oct 31, 2023 •

edited

Loading

`@firebase/firestore`

`bundle`

`firebase`

google-oss-bot commented Oct 31, 2023 •

edited

Loading

`@firebase/firestore`

`enableMultiTabIndexedDbPersistence`

`getDoc`

`getDocFromServer`

`getDocs`

`getDocsFromCache`

`getDocsFromServer`

`onSnapshot`

`onSnapshotsInSync`

`persistentMultipleTabManager`

ehsannas left a comment

milaGGL commented Nov 6, 2023 •

edited

Loading

dconeybe commented Nov 8, 2023

dconeybe left a comment

dconeybe left a comment

Fix docs unnecessarily undergo limbo resolution while resuming query bug #7740

Fix docs unnecessarily undergo limbo resolution while resuming query bug #7740

Conversation

milaGGL commented Oct 31, 2023 • edited by dconeybe Loading

changeset-bot bot commented Oct 31, 2023 • edited Loading

🦋 Changeset detected

google-oss-bot commented Oct 31, 2023 • edited Loading

Size Report 1

Affected Products

@firebase/firestore

bundle

firebase

Test Logs

google-oss-bot commented Oct 31, 2023 • edited Loading

Size Analysis Report 1

Affected Products

@firebase/firestore

enableMultiTabIndexedDbPersistence

getDoc

getDocFromServer

getDocs

getDocsFromCache

getDocsFromServer

onSnapshot

onSnapshotsInSync

persistentMultipleTabManager

Test Logs

ehsannas left a comment

Choose a reason for hiding this comment

milaGGL commented Nov 6, 2023 • edited Loading

dconeybe commented Nov 8, 2023

dconeybe left a comment

Choose a reason for hiding this comment

dconeybe left a comment

Choose a reason for hiding this comment

milaGGL commented Oct 31, 2023 •

edited by dconeybe

Loading

changeset-bot bot commented Oct 31, 2023 •

edited

Loading

google-oss-bot commented Oct 31, 2023 •

edited

Loading

Size Report ¹

`@firebase/firestore`

`bundle`

`firebase`

google-oss-bot commented Oct 31, 2023 •

edited

Loading

Size Analysis Report ¹

`@firebase/firestore`

`enableMultiTabIndexedDbPersistence`

`getDoc`

`getDocFromServer`

`getDocs`

`getDocsFromCache`

`getDocsFromServer`

`onSnapshot`

`onSnapshotsInSync`

`persistentMultipleTabManager`

milaGGL commented Nov 6, 2023 •

edited

Loading