[go: nahoru, domu]
Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 20 commits into from
Oct 7, 2022
Merged

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

merged 20 commits into from
Oct 7, 2022

Conversation

milaGGL
Copy link
Contributor
@milaGGL milaGGL commented Sep 22, 2022 •
edited
Loading

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
Copy link
changeset-bot bot commented Sep 22, 2022 •
edited
Loading

🦋 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
Copy link
Contributor
google-oss-bot commented Sep 22, 2022 •
edited
Loading

Size Report 1

Affected Products

  • @firebase/app

    TypeBase (de32c24)Merge (47ac5da)Diff
    browser14.4 kB14.4 kB-1 B (-0.0%)
    esm518.7 kB18.7 kB-1 B (-0.0%)
    main19.6 kB19.6 kB-1 B (-0.0%)
    module14.4 kB14.4 kB-1 B (-0.0%)

  • @firebase/firestore

    TypeBase (de32c24)Merge (47ac5da)Diff
    browser265 kB266 kB+301 B (+0.1%)
    esm5330 kB330 kB+305 B (+0.1%)
    main531 kB531 kB+464 B (+0.1%)
    module265 kB266 kB+301 B (+0.1%)
    react-native266 kB266 kB+301 B (+0.1%)

  • @firebase/storage

    TypeBase (de32c24)Merge (47ac5da)Diff
    browser54.7 kB55.6 kB+892 B (+1.6%)
    esm560.7 kB61.8 kB+1.12 kB (+1.8%)
    main57.6 kB58.4 kB+892 B (+1.5%)
    module54.7 kB55.6 kB+892 B (+1.6%)

  • @firebase/webchannel-wrapper

    TypeBase (de32c24)Merge (47ac5da)Diff
    esm544.6 kB44.7 kB+75 B (+0.2%)
    main54.6 kB54.6 kB+75 B (+0.1%)
    module42.3 kB42.4 kB+75 B (+0.2%)

  • bundle

    43 size changes

    TypeBase (de32c24)Merge (47ac5da)Diff
    analytics (logEvent)43.2 kB43.2 kB-1 B (-0.0%)
    app-check (CustomProvider)36.7 kB36.7 kB-1 B (-0.0%)
    app-check (ReCaptchaEnterpriseProvider)39.1 kB39.1 kB-1 B (-0.0%)
    app-check (ReCaptchaV3Provider)39.0 kB39.0 kB-1 B (-0.0%)
    auth (Anonymous)67.7 kB67.7 kB-1 B (-0.0%)
    auth (EmailAndPassword)71.8 kB71.8 kB-1 B (-0.0%)
    auth (GoogleFBTwitterGitHubPopup)94.1 kB94.1 kB-1 B (-0.0%)
    auth (GooglePopup)91.4 kB91.4 kB-1 B (-0.0%)
    auth (GoogleRedirect)91.6 kB91.6 kB-1 B (-0.0%)
    auth (Phone)77.9 kB77.8 kB-1 B (-0.0%)
    database (Append to a list of data)148 kB148 kB-1 B (-0.0%)
    database (Filtering data)147 kB147 kB-1 B (-0.0%)
    database (Listen for child events)163 kB163 kB-1 B (-0.0%)
    database (Listen for value events + Detach listeners)163 kB163 kB-1 B (-0.0%)
    database (Listen for value events)163 kB163 kB-1 B (-0.0%)
    database (Read data once)162 kB162 kB-1 B (-0.0%)
    database (Save data as transactions)165 kB165 kB-1 B (-0.0%)
    database (Sort data)148 kB148 kB-1 B (-0.0%)
    database (Write data)147 kB147 kB-1 B (-0.0%)
    firestore (Persistence)276 kB276 kB+306 B (+0.1%)
    firestore (Query Cursors)213 kB213 kB+329 B (+0.2%)
    firestore (Query)214 kB214 kB+329 B (+0.2%)
    firestore (Read data once)202 kB202 kB+329 B (+0.2%)
    firestore (Realtime updates)204 kB205 kB+329 B (+0.2%)
    firestore (Transaction)186 kB186 kB+102 B (+0.1%)
    firestore (Write data)186 kB186 kB+102 B (+0.1%)
    firestore-lite (Query Cursors)71.3 kB71.3 kB-1 B (-0.0%)
    firestore-lite (Query)74.5 kB74.5 kB-1 B (-0.0%)
    firestore-lite (Read data once)58.9 kB58.9 kB-1 B (-0.0%)
    firestore-lite (Transaction)83.5 kB83.5 kB-1 B (-0.0%)
    firestore-lite (Write data)68.7 kB68.7 kB-1 B (-0.0%)
    functions (call)30.9 kB30.9 kB-1 B (-0.0%)
    messaging (send + receive)46.6 kB46.6 kB-1 B (-0.0%)
    performance (trace)51.0 kB51.0 kB-1 B (-0.0%)
    remote-config (getAndFetch)45.7 kB45.6 kB-1 B (-0.0%)
    storage (getBytes)39.9 kB40.0 kB+132 B (+0.3%)
    storage (getDownloadURL)42.0 kB42.1 kB+132 B (+0.3%)
    storage (getMetadata)41.4 kB41.5 kB+132 B (+0.3%)
    storage (list + listAll)40.8 kB41.0 kB+132 B (+0.3%)
    storage (updateMetadata)41.7 kB41.8 kB+132 B (+0.3%)
    storage (uploadBytes)46.2 kB46.4 kB+132 B (+0.3%)
    storage (uploadBytesResumable)55.6 kB56.2 kB+563 B (+1.0%)
    storage (uploadString)46.4 kB46.6 kB+132 B (+0.3%)

  • firebase

    TypeBase (de32c24)Merge (47ac5da)Diff
    firebase-app-compat.js29.0 kB29.0 kB-2 B (-0.0%)
    firebase-app.js91.7 kB91.7 kB-1 B (-0.0%)
    firebase-compat.js739 kB740 kB+944 B (+0.1%)
    firebase-firestore-compat.js313 kB314 kB+384 B (+0.1%)
    firebase-firestore.js314 kB314 kB+382 B (+0.1%)
    firebase-performance-standalone-compat.es2017.js90.2 kB90.2 kB-1 B (-0.0%)
    firebase-performance-standalone-compat.js66.7 kB66.7 kB-2 B (-0.0%)
    firebase-storage-compat.js38.2 kB38.8 kB+562 B (+1.5%)
    firebase-storage.js37.8 kB38.4 kB+567 B (+1.5%)

Test Logs

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

@google-oss-bot
Copy link
Contributor
google-oss-bot commented Sep 22, 2022 •
edited
Loading

Size Analysis Report 1

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

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

@dconeybe
Copy link
Contributor

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

dconeybe
dconeybe requested changes Sep 28, 2022
View reviewed changes
Copy link
Contributor
@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
packages/firestore/src/core/view_snapshot.ts Outdated Show resolved Hide resolved
packages/firestore/test/integration/api/query.test.ts Outdated Show resolved Hide resolved
packages/firestore/test/integration/api/query.test.ts Outdated Show resolved Hide resolved
packages/firestore/test/integration/api/query.test.ts Outdated Show resolved Hide resolved
packages/firestore/test/integration/api/query.test.ts Outdated Show resolved Hide resolved
packages/firestore/test/integration/api/query.test.ts Outdated Show resolved Hide resolved
packages/firestore/test/util/api_helpers.ts Outdated Show resolved Hide resolved
@dconeybe
Copy link
Contributor

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

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

dconeybe
dconeybe requested changes Sep 30, 2022
View reviewed changes
packages/firestore/src/core/event_manager.ts Outdated Show resolved Hide resolved
packages/firestore/test/integration/api/query.test.ts Outdated Show resolved Hide resolved
packages/firestore/test/integration/api/query.test.ts Outdated Show resolved Hide resolved
packages/firestore/src/core/sync_engine_impl.ts Show resolved Hide resolved
packages/firestore/src/remote/remote_event.ts Outdated Show resolved Hide resolved
packages/firestore/test/unit/api/database.test.ts Outdated Show resolved Hide resolved
@dconeybe
Copy link
Contributor
dconeybe commented Oct 5, 2022

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

Bump

dconeybe
dconeybe requested changes Oct 5, 2022
View reviewed changes
Copy link
Contributor
@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

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

wu-hui
wu-hui reviewed Oct 6, 2022
View reviewed changes
Copy link
Contributor
@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

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

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

packages/firestore/test/unit/specs/listen_spec.test.ts Outdated Show resolved Hide resolved
@wu-hui
Copy link
Contributor
wu-hui commented Oct 6, 2022

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?

dconeybe
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
dconeybe
dconeybe approved these changes Oct 7, 2022
View reviewed changes
Copy link
Contributor
@dconeybe dconeybe left a comment

Choose a reason for hiding this comment

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

Awesome job! Thanks!

@milaGGL milaGGL merged commit 0a112bd into master Oct 7, 2022
@milaGGL milaGGL deleted the mila/fix-cache-empty-result-bug branch October 7, 2022 19:38
This was referenced Oct 7, 2022
Closed
Closed
@google-oss-bot google-oss-bot mentioned this pull request Oct 10, 2022
Merged
@dconeybe dconeybe mentioned this pull request Oct 25, 2022
Merged
@dconeybe dconeybe mentioned this pull request Nov 1, 2022
Merged
dconeybe added a commit to firebase/firebase-ios-sdk that referenced this pull request Nov 2, 2022
@dconeybe
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 dconeybe mentioned this pull request Nov 2, 2022
Merged
dconeybe added a commit to firebase/firebase-ios-sdk that referenced this pull request Nov 2, 2022
@dconeybe
update spec test changes from firebase/firebase-js-sdk#6624
48b661a
@firebase firebase locked and limited conversation to collaborators Nov 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dconeybe dconeybe dconeybe approved these changes

@dwyfrequency dwyfrequency Awaiting requested review from dwyfrequency

@hsubox76 hsubox76 Awaiting requested review from hsubox76

@egilmorez egilmorez Awaiting requested review from egilmorez

@wu-hui wu-hui Awaiting requested review from wu-hui

Assignees

@dconeybe dconeybe

@milaGGL milaGGL

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Collection snapshot queries with no documents don't store anything in cache. Always requires server snapshot.
4 participants
@milaGGL @google-oss-bot @dconeybe @wu-hui