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 #10437

Merged
merged 23 commits into from
Nov 9, 2022
Merged

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

merged 23 commits into from
Nov 9, 2022

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Nov 1, 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 query_listener.cc:

firebase-ios-sdk/Firestore/core/src/core/query_listener.cc

Lines 150 to 151 in a1b78b2

// Raise data from cache if we have any documents or we are offline
return !snapshot.documents().empty() || online_state == 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.

This bug fix is ported from firebase/firebase-js-sdk#6624 and firebase/firebase-android-sdk#4207, which fixed firebase/firebase-js-sdk#5873.
@dconeybe dconeybe self-assigned this Nov 1, 2022
@dconeybe dconeybe changed the title CHANGELOG.md: added entry Nov 1, 2022
@dconeybe dconeybe marked this pull request as draft November 1, 2022 16:00
@google-oss-bot
Copy link

google-oss-bot commented Nov 1, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • FirebaseFirestore-iOS-FirebaseFirestore.framework

    Overall coverage changed from 88.21% (80940c0) to 88.21% (242c2d3) by +0.00%.

    FilenameBase (80940c0)Merge (242c2d3)Diff
    leveldb_remote_document_cache.cc96.25%94.38%-1.88%
    sync_engine.cc95.24%95.25%+0.01%
    view.cc98.37%98.39%+0.02%
    view_snapshot.cc78.63%78.99%+0.36%
    write_stream.cc91.55%94.37%+2.82%

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/wTvVLG3MtX.html
@dconeybe dconeybe mentioned this pull request Nov 2, 2022
Merged
@dconeybe dconeybe changed the base branch from master to dconeybe/SpecTestUpdate November 2, 2022 18:19
Base automatically changed from dconeybe/SpecTestUpdate to master November 2, 2022 19:19
@dconeybe dconeybe removed api: crashlytics api: inappmessaging Firebase In App Messaging labels Nov 2, 2022
@dconeybe dconeybe marked this pull request as ready for review November 3, 2022 22:59
@google-oss-bot
Copy link

Size Report 1

Affected Products

  • FirebaseFirestore

    TypeBase (80940c0)Merge (242c2d3)Diff
    CocoaPods?-51.5 kB? (?)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/KzPagDE3p9.html
@dconeybe dconeybe requested a review from ehsannas November 5, 2022 02:44
@ehsannas ehsannas assigned ehsannas and unassigned dconeybe Nov 8, 2022
ehsannas
ehsannas approved these changes Nov 8, 2022
View reviewed changes
Copy link
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

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

LGTM!
Firestore/Example/Tests/SpecTests/json/listen_spec_test.json Show resolved Hide resolved
Firestore/core/src/core/query_listener.cc Show resolved Hide resolved
@dconeybe
Copy link
Contributor Author

dconeybe commented Nov 9, 2022

LGTM!

Thanks for the review, @ehsannas!
@dconeybe dconeybe merged commit 92e269c into master Nov 9, 2022
@dconeybe dconeybe deleted the dconeybe/FixCacheEmptyResultBug branch November 9, 2022 01:50
visumickey pushed a commit that referenced this pull request Nov 9, 2022
@dconeybe @visumickey
Fix Firestore failing to return empty results from the local cache (#…
8ba074c 
…10437)
@dconeybe
Copy link
Contributor Author

FYI This fix has been incorporated into version 10.2.0 released November 15, 2022 https://firebase.google.com/support/release-notes/ios#version_1020_-_november_15_2022
@paulb777
Copy link
Member

@dconeybe Note that you can mark the fix version in the milestone field of the PR - I'll do this one now ...
@paulb777 paulb777 added this to the 10.2.0 - M124 milestone Nov 28, 2022
@dconeybe
Copy link
Contributor Author

@paulb777 Ahh good to know. I'll use that in the future.
@firebase firebase locked and limited conversation to collaborators Dec 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: firestore
5 participants
@dconeybe @google-oss-bot @paulb777 @ehsannas @milaGGL