Fix Firestore failing to return empty results from the local cache #6624

milaGGL · 2022-09-22T20:06:57Z

Fix a bug where Firestore fails to return results from the local cache if the result set was empty.

This bug is due to the following logic in event_manager.ts:

firebase-js-sdk/packages/firestore/src/core/event_manager.ts

Lines 374 to 375 in 4ddf556

    
           // Raise data from cache if we have any documents or we are offline 
        
           return !snap.docs.isEmpty() || onlineState === OnlineState.Offline;

which assumes that if the result set from the local cache is empty that there is no cached result; however, if the result set of the query is indeed empty then it should be returning that empty result set from the cache.

This fix improves the logic to use the presence of a hasCachedResults flag to indicate that the empty result set is cached data and should be raised to the client.

Fixed #5873

changeset-bot · 2022-09-22T20:06:59Z

🦋 Changeset detected

Latest commit: 3e1495b

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 · 2022-09-22T20:31:27Z

Size Report ¹

Affected Products

@firebase/app
Type Base (de32c24) Merge (47ac5da) Diff
browser 14.4 kB 14.4 kB -1 B (-0.0%)
esm5 18.7 kB 18.7 kB -1 B (-0.0%)
main 19.6 kB 19.6 kB -1 B (-0.0%)
module 14.4 kB 14.4 kB -1 B (-0.0%)
@firebase/firestore
Type Base (de32c24) Merge (47ac5da) Diff
browser 265 kB 266 kB +301 B (+0.1%)
esm5 330 kB 330 kB +305 B (+0.1%)
main 531 kB 531 kB +464 B (+0.1%)
module 265 kB 266 kB +301 B (+0.1%)
react-native 266 kB 266 kB +301 B (+0.1%)
@firebase/storage
Type Base (de32c24) Merge (47ac5da) Diff
browser 54.7 kB 55.6 kB +892 B (+1.6%)
esm5 60.7 kB 61.8 kB +1.12 kB (+1.8%)
main 57.6 kB 58.4 kB +892 B (+1.5%)
module 54.7 kB 55.6 kB +892 B (+1.6%)
@firebase/webchannel-wrapper
Type Base (de32c24) Merge (47ac5da) Diff
esm5 44.6 kB 44.7 kB +75 B (+0.2%)
main 54.6 kB 54.6 kB +75 B (+0.1%)
module 42.3 kB 42.4 kB +75 B (+0.2%)

`bundle`

43 size changes

Type	Base (`de32c24`)	Merge (`47ac5da`)	Diff
analytics (logEvent)	43.2 kB	43.2 kB	-1 B (-0.0%)
app-check (CustomProvider)	36.7 kB	36.7 kB	-1 B (-0.0%)
app-check (ReCaptchaEnterpriseProvider)	39.1 kB	39.1 kB	-1 B (-0.0%)
app-check (ReCaptchaV3Provider)	39.0 kB	39.0 kB	-1 B (-0.0%)
auth (Anonymous)	67.7 kB	67.7 kB	-1 B (-0.0%)
auth (EmailAndPassword)	71.8 kB	71.8 kB	-1 B (-0.0%)
auth (GoogleFBTwitterGitHubPopup)	94.1 kB	94.1 kB	-1 B (-0.0%)
auth (GooglePopup)	91.4 kB	91.4 kB	-1 B (-0.0%)
auth (GoogleRedirect)	91.6 kB	91.6 kB	-1 B (-0.0%)
auth (Phone)	77.9 kB	77.8 kB	-1 B (-0.0%)
database (Append to a list of data)	148 kB	148 kB	-1 B (-0.0%)
database (Filtering data)	147 kB	147 kB	-1 B (-0.0%)
database (Listen for child events)	163 kB	163 kB	-1 B (-0.0%)
database (Listen for value events + Detach listeners)	163 kB	163 kB	-1 B (-0.0%)
database (Listen for value events)	163 kB	163 kB	-1 B (-0.0%)
database (Read data once)	162 kB	162 kB	-1 B (-0.0%)
database (Save data as transactions)	165 kB	165 kB	-1 B (-0.0%)
database (Sort data)	148 kB	148 kB	-1 B (-0.0%)
database (Write data)	147 kB	147 kB	-1 B (-0.0%)
firestore (Persistence)	276 kB	276 kB	+306 B (+0.1%)
firestore (Query Cursors)	213 kB	213 kB	+329 B (+0.2%)
firestore (Query)	214 kB	214 kB	+329 B (+0.2%)
firestore (Read data once)	202 kB	202 kB	+329 B (+0.2%)
firestore (Realtime updates)	204 kB	205 kB	+329 B (+0.2%)
firestore (Transaction)	186 kB	186 kB	+102 B (+0.1%)
firestore (Write data)	186 kB	186 kB	+102 B (+0.1%)
firestore-lite (Query Cursors)	71.3 kB	71.3 kB	-1 B (-0.0%)
firestore-lite (Query)	74.5 kB	74.5 kB	-1 B (-0.0%)
firestore-lite (Read data once)	58.9 kB	58.9 kB	-1 B (-0.0%)
firestore-lite (Transaction)	83.5 kB	83.5 kB	-1 B (-0.0%)
firestore-lite (Write data)	68.7 kB	68.7 kB	-1 B (-0.0%)
functions (call)	30.9 kB	30.9 kB	-1 B (-0.0%)
messaging (send + receive)	46.6 kB	46.6 kB	-1 B (-0.0%)
performance (trace)	51.0 kB	51.0 kB	-1 B (-0.0%)
remote-config (getAndFetch)	45.7 kB	45.6 kB	-1 B (-0.0%)
storage (getBytes)	39.9 kB	40.0 kB	+132 B (+0.3%)
storage (getDownloadURL)	42.0 kB	42.1 kB	+132 B (+0.3%)
storage (getMetadata)	41.4 kB	41.5 kB	+132 B (+0.3%)
storage (list + listAll)	40.8 kB	41.0 kB	+132 B (+0.3%)
storage (updateMetadata)	41.7 kB	41.8 kB	+132 B (+0.3%)
storage (uploadBytes)	46.2 kB	46.4 kB	+132 B (+0.3%)
storage (uploadBytesResumable)	55.6 kB	56.2 kB	+563 B (+1.0%)
storage (uploadString)	46.4 kB	46.6 kB	+132 B (+0.3%)

`firebase`

Type	Base (`de32c24`)	Merge (`47ac5da`)	Diff
firebase-app-compat.js	29.0 kB	29.0 kB	-2 B (-0.0%)
firebase-app.js	91.7 kB	91.7 kB	-1 B (-0.0%)
firebase-compat.js	739 kB	740 kB	+944 B (+0.1%)
firebase-firestore-compat.js	313 kB	314 kB	+384 B (+0.1%)
firebase-firestore.js	314 kB	314 kB	+382 B (+0.1%)
firebase-performance-standalone-compat.es2017.js	90.2 kB	90.2 kB	-1 B (-0.0%)
firebase-performance-standalone-compat.js	66.7 kB	66.7 kB	-2 B (-0.0%)
firebase-storage-compat.js	38.2 kB	38.8 kB	+562 B (+1.5%)
firebase-storage.js	37.8 kB	38.4 kB	+567 B (+1.5%)

Test Logs

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

google-oss-bot · 2022-09-22T20:43:47Z

Size Analysis Report ¹

This report is too large (180,142 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

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

…y-result-bug

dconeybe · 2022-09-28T18:24:27Z

Please add a changeset so an entry will be included in the release notes.

…y-result-bug

dconeybe

A few small comments. Also, I added a couple of spec tests. I don't think you need to learn how to write spec tests at this point (you'll definitely have chances later!).

packages/firestore/src/core/event_manager.ts

packages/firestore/src/core/view_snapshot.ts

packages/firestore/test/integration/api/query.test.ts

packages/firestore/test/util/api_helpers.ts

dconeybe · 2022-09-30T17:00:10Z

I just realized that I had forgotten to push up my changes that add two spec tests. I've now pushed them up in f06eb5f

dconeybe · 2022-09-30T17:00:29Z

Please add a changeset so an entry will be included in the release notes.

packages/firestore/src/core/event_manager.ts

packages/firestore/test/integration/api/query.test.ts

packages/firestore/src/core/sync_engine_impl.ts

packages/firestore/src/remote/remote_event.ts

packages/firestore/test/unit/api/database.test.ts

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

dconeybe · 2022-10-05T15:55:43Z

Please add a changeset so an entry will be included in the release notes.

Bump

dconeybe

I've asked @wu-hui to look at the spec tests tomorrow. Other than that, mostly LGTM (other than my 2 other comments).

.changeset/cool-grapes-attend.md

wu-hui

The multi-tab spec tests look good. Also this seems to be a clever fix!

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

wu-hui · 2022-10-06T15:01:56Z

The multi-tab spec tests look good. Also this seems to be a clever fix!

I do wonder if we should avoid passing resumeToken all the way to snapshots/views however. It is an underlying detail that should be no concern of snapshots/views. Maybe we can replace it with a boolean, something like hasCachedResults?

packages/firestore/src/core/view.ts

packages/firestore/test/unit/api/database.test.ts

dconeybe

Awesome job! Thanks!

… fix for its issue isn't fixed yet (it will be fixed by #10437)

add resumeToken to viewSnapshot and raiseInitialEvent condition check

e5abb85

update test helper with resume token, remove it.only

7bb2cfa

milaGGL and others added 7 commits September 22, 2022 23:40

modify failing tests

0ae4afc

Merge remote-tracking branch 'origin/master' into mila/fix-cache-empt…

d18be98

…y-result-bug

fix spec tests in listen_spec.test.ts and recovery_spec.test.ts

2e388bc

format to pass lint test

7050b29

add more test cases

fdb8f4b

update tests

f9f6ff8

reformat to pass lint

4651a5b

milaGGL requested a review from dconeybe September 27, 2022 23:26

milaGGL assigned milaGGL and dconeybe Sep 27, 2022

Merge remote-tracking branch 'origin/master' into mila/fix-cache-empt…

016cc8f

…y-result-bug

dconeybe requested changes Sep 28, 2022

View reviewed changes

resolve comments

a1cf3d1

milaGGL requested a review from dconeybe September 30, 2022 01:17

Add two spec tests for raising initial empty snapshot from cache

f06eb5f

dconeybe requested changes Sep 30, 2022

View reviewed changes

resolve part of the comments

da6dcc0

milaGGL requested review from dwyfrequency and hsubox76 as code owners October 3, 2022 16:31

add spec test for multi-tab

ffe6711

milaGGL commented Oct 4, 2022

View reviewed changes

packages/firestore/test/unit/specs/listen_spec.test.ts Show resolved Hide resolved

milaGGL commented Oct 4, 2022

View reviewed changes

packages/firestore/test/unit/specs/listen_spec.test.ts Show resolved Hide resolved

milaGGL commented Oct 4, 2022

View reviewed changes

packages/firestore/test/unit/specs/listen_spec.test.ts Outdated Show resolved Hide resolved

dconeybe requested changes Oct 5, 2022

View reviewed changes

resolve comments

dc88dd5

milaGGL requested a review from egilmorez as a code owner October 5, 2022 18:47

dconeybe reviewed Oct 5, 2022

View reviewed changes

.changeset/cool-grapes-attend.md Show resolved Hide resolved

wu-hui reviewed Oct 6, 2022

View reviewed changes

packages/firestore/test/unit/specs/listen_spec.test.ts Outdated Show resolved Hide resolved

milaGGL added 4 commits October 6, 2022 09:34

add firebase to changeset, remove unnecessary resume token

67d0ee7

use hasCachedResults instead of resumeToken

9f25aab

add hasCachedResults to viewSnapshot equality

a9bee4e

remove unnecessary resumeToken test

91f720d

milaGGL requested review from dconeybe and wu-hui October 7, 2022 00:44

dconeybe requested changes Oct 7, 2022

View reviewed changes

packages/firestore/src/core/view.ts Outdated Show resolved Hide resolved

packages/firestore/test/unit/api/database.test.ts Outdated Show resolved Hide resolved

resolve comments

3e1495b

dconeybe approved these changes Oct 7, 2022

View reviewed changes

milaGGL merged commit 0a112bd into master Oct 7, 2022

milaGGL deleted the mila/fix-cache-empty-result-bug branch October 7, 2022 19:38

This was referenced Oct 7, 2022

Fix Firestore failing to return empty results from the local cache #5897

Closed

Collection snapshot queries with no documents don't store anything in cache. Always requires server snapshot. #5873

Closed

google-oss-bot mentioned this pull request Oct 10, 2022

Version Packages #6672

Merged

dconeybe mentioned this pull request Oct 25, 2022

Fix Firestore failing to return empty results from the local cache firebase/firebase-android-sdk#4207

Merged

dconeybe mentioned this pull request Nov 1, 2022

Fix Firestore failing to return empty results from the local cache firebase/firebase-ios-sdk#10437

Merged

dconeybe added a commit to firebase/firebase-ios-sdk that referenced this pull request Nov 2, 2022

Revert spec test changes from firebase/firebase-js-sdk#6624 since the…

60dce81

… fix for its issue isn't fixed yet (it will be fixed by #10437)

dconeybe mentioned this pull request Nov 2, 2022

Firestore: Update spec tests firebase/firebase-ios-sdk#10445

Merged

dconeybe added a commit to firebase/firebase-ios-sdk that referenced this pull request Nov 2, 2022

update spec test changes from firebase/firebase-js-sdk#6624

48b661a

firebase locked and limited conversation to collaborators Nov 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Firestore failing to return empty results from the local cache #6624

Fix Firestore failing to return empty results from the local cache #6624

milaGGL commented Sep 22, 2022 •

edited

Loading

changeset-bot bot commented Sep 22, 2022 •

edited

Loading

google-oss-bot commented Sep 22, 2022 •

edited

Loading

`@firebase/app`

`@firebase/firestore`

`@firebase/storage`

`@firebase/webchannel-wrapper`

`bundle`

`firebase`

google-oss-bot commented Sep 22, 2022 •

edited

Loading

dconeybe commented Sep 28, 2022

dconeybe left a comment

dconeybe commented Sep 30, 2022

dconeybe commented Sep 30, 2022

dconeybe commented Oct 5, 2022

dconeybe left a comment

wu-hui left a comment

wu-hui commented Oct 6, 2022

dconeybe left a comment

	// Raise data from cache if we have any documents or we are offline
	return !snap.docs.isEmpty() \|\| onlineState === OnlineState.Offline;

Fix Firestore failing to return empty results from the local cache #6624

Fix Firestore failing to return empty results from the local cache #6624

Conversation

milaGGL commented Sep 22, 2022 • edited Loading

changeset-bot bot commented Sep 22, 2022 • edited Loading

🦋 Changeset detected

google-oss-bot commented Sep 22, 2022 • edited Loading

Size Report 1

Affected Products

@firebase/app

@firebase/firestore

@firebase/storage

@firebase/webchannel-wrapper

bundle

firebase

Test Logs

google-oss-bot commented Sep 22, 2022 • edited Loading

Size Analysis Report 1

Test Logs

dconeybe commented Sep 28, 2022

dconeybe left a comment

Choose a reason for hiding this comment

dconeybe commented Sep 30, 2022

dconeybe commented Sep 30, 2022

dconeybe commented Oct 5, 2022

dconeybe left a comment

Choose a reason for hiding this comment

wu-hui left a comment

Choose a reason for hiding this comment

wu-hui commented Oct 6, 2022

dconeybe left a comment

Choose a reason for hiding this comment

milaGGL commented Sep 22, 2022 •

edited

Loading

changeset-bot bot commented Sep 22, 2022 •

edited

Loading

google-oss-bot commented Sep 22, 2022 •

edited

Loading

Size Report ¹

`@firebase/app`

`@firebase/firestore`

`@firebase/storage`

`@firebase/webchannel-wrapper`

`bundle`

`firebase`

google-oss-bot commented Sep 22, 2022 •

edited

Loading

Size Analysis Report ¹