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

Merged
merged 8 commits into from
Oct 26, 2022
Merged

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

merged 8 commits into from
Oct 26, 2022

Conversation

milaGGL
Copy link
Contributor

@milaGGL milaGGL commented Oct 15, 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 QueryListener.java:

firebase-android-sdk/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java

Line 143 in ae3bdb8

return !snapshot.getDocuments().isEmpty() || onlineState.equals(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, which fixed #5873

@milaGGL milaGGL added the WIP label Oct 15, 2022
@milaGGL milaGGL self-assigned this Oct 15, 2022
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 15, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • firebase-firestore

    Overall coverage changed from ? (d939e63) to 44.46% (264d78a) by ?.

    315 individual files with coverage change

    FilenameBase (d939e63)Merge (264d78a)Diff
    AbstractStream.java?34.81%?
    ActivityScope.java?0.00%?
    AggregateQuery.java?15.00%?
    AggregateQuerySnapshot.java?0.00%?
    AggregateSource.java?0.00%?
    AggregationResult.java?0.00%?
    AggregationResultOrBuilder.java?0.00%?
    AggregationResultProto.java?0.00%?
    AndroidConnectivityMonitor.java?39.51%?
    ApiUtil.java?12.50%?
    ArrayContainsAnyFilter.java?100.00%?
    ArrayContainsFilter.java?100.00%?
    ArrayTransformOperation.java?86.49%?
    ArrayValue.java?48.60%?
    ArrayValueOrBuilder.java?0.00%?
    Assert.java?60.00%?
    AsyncEventListener.java?0.00%?
    AsyncQueue.java?70.65%?
    AutoValue_FieldIndex.java?58.33%?
    AutoValue_FieldIndex_IndexOffset.java?58.62%?
    AutoValue_FieldIndex_IndexState.java?59.09%?
    AutoValue_FieldIndex_Segment.java?54.17%?
    AutoValue_IndexEntry.java?34.21%?
    AutoValue_Overlay.java?54.55%?
    BackgroundQueue.java?81.25%?
    BasePath.java?86.54%?
    BatchGetDocumentsRequest.java?0.00%?
    BatchGetDocumentsRequestOrBuilder.java?0.00%?
    BatchGetDocumentsResponse.java?0.00%?
    BatchGetDocumentsResponseOrBuilder.java?0.00%?
    BeginTransactionRequest.java?0.00%?
    BeginTransactionRequestOrBuilder.java?0.00%?
    BeginTransactionResponse.java?0.00%?
    BeginTransactionResponseOrBuilder.java?0.00%?
    Blob.java?76.92%?
    Bound.java?39.29%?
    BundleCache.java?0.00%?
    BundleCallback.java?0.00%?
    BundledDocumentMetadata.java?0.00%?
    BundledDocumentMetadataOrBuilder.java?0.00%?
    BundleDocument.java?90.00%?
    BundledQuery.java?34.57%?
    BundledQueryOrBuilder.java?0.00%?
    BundleElement.java?0.00%?
    BundleElementOrBuilder.java?0.00%?
    BundleLoader.java?100.00%?
    BundleMetadata.java?0.00%?
    BundleMetadataOrBuilder.java?0.00%?
    BundleProto.java?0.00%?
    BundleReader.java?95.29%?
    BundleSerializer.java?89.47%?
    ByteBufferInputStream.java?83.33%?
    CollectionReference.java?13.64%?
    CommitRequest.java?0.00%?
    CommitRequestOrBuilder.java?0.00%?
    CommitResponse.java?0.00%?
    CommitResponseOrBuilder.java?0.00%?
    CommonProto.java?0.00%?
    ComponentProvider.java?100.00%?
    CompositeFilter.java?98.46%?
    ConnectivityMonitor.java?0.00%?
    Consumer.java?0.00%?
    CreateDocumentRequest.java?0.00%?
    CreateDocumentRequestOrBuilder.java?0.00%?
    CredentialsProvider.java?100.00%?
    Cursor.java?35.29%?
    CursorOrBuilder.java?0.00%?
    CustomClassMapper.java?84.25%?
    DatabaseId.java?70.37%?
    DatabaseInfo.java?90.91%?
    Datastore.java?23.28%?
    DatastoreTestTrace.java?0.00%?
    DeleteDocumentRequest.java?0.00%?
    DeleteDocumentRequestOrBuilder.java?0.00%?
    DeleteMutation.java?95.24%?
    DirectionalIndexByteEncoder.java?100.00%?
    Document.java?100.00%?
    DocumentChange.java?67.61%?
    DocumentChangeOrBuilder.java?0.00%?
    DocumentCollections.java?83.33%?
    DocumentDelete.java?34.62%?
    DocumentDeleteOrBuilder.java?0.00%?
    DocumentId.java?0.00%?
    DocumentKey.java?97.14%?
    DocumentMask.java?40.66%?
    DocumentMaskOrBuilder.java?0.00%?
    DocumentOrBuilder.java?0.00%?
    DocumentOverlayCache.java?0.00%?
    DocumentProto.java?0.00%?
    DocumentReference.java?13.24%?
    DocumentRemove.java?29.23%?
    DocumentRemoveOrBuilder.java?0.00%?
    DocumentSet.java?83.78%?
    DocumentSnapshot.java?37.50%?
    DocumentTransform.java?29.31%?
    DocumentTransformOrBuilder.java?0.00%?
    DocumentViewChange.java?90.91%?
    DocumentViewChangeSet.java?87.88%?
    EncodedPath.java?93.22%?
    EventListener.java?0.00%?
    EventManager.java?96.15%?
    Exclude.java?0.00%?
    Executors.java?100.00%?
    ExistenceFilter.java?80.00%?
    ExistenceFilterOrBuilder.java?0.00%?
    ExponentialBackoff.java?30.43%?
    FieldFilter.java?93.06%?
    FieldIndex.java?98.18%?
    FieldMask.java?57.89%?
    FieldPath.java?89.66%?
    FieldTransform.java?64.71%?
    FieldValue.java?82.14%?
    FileUtil.java?0.00%?
    Filter.java?0.00%?
    FirebaseAppCheckTokenProvider.java?93.75%?
    FirebaseAuthCredentialsProvider.java?100.00%?
    FirebaseClientGrpcMetadataProvider.java?44.00%?
    FirebaseFirestore.java?36.67%?
    FirebaseFirestoreException.java?83.72%?
    FirebaseFirestoreSettings.java?75.00%?
    FirestoreCallCredentials.java?18.60%?
    FirestoreChannel.java?18.10%?
    FirestoreClient.java?36.11%?
    FirestoreGrpc.java?2.46%?
    FirestoreIndexValueWriter.java?77.11%?
    FirestoreMultiDbComponent.java?100.00%?
    FirestoreProto.java?0.00%?
    FirestoreRegistrar.java?100.00%?
    Function.java?0.00%?
    GeoPoint.java?91.67%?
    GetDocumentRequest.java?0.00%?
    GetDocumentRequestOrBuilder.java?0.00%?
    GrpcCallProvider.java?69.41%?
    GrpcMetadataProvider.java?0.00%?
    IgnoreExtraProperties.java?0.00%?
    IncomingStreamObserver.java?0.00%?
    Index.java?30.02%?
    IndexBackfiller.java?100.00%?
    IndexByteEncoder.java?88.57%?
    IndexEntry.java?100.00%?
    IndexManager.java?100.00%?
    IndexOrBuilder.java?0.00%?
    IndexProto.java?0.00%?
    InFilter.java?100.00%?
    IntMath.java?41.67%?
    KeyFieldFilter.java?66.67%?
    KeyFieldInFilter.java?0.00%?
    KeyFieldNotInFilter.java?0.00%?
    LimboDocumentChange.java?70.59%?
    ListCollectionIdsRequest.java?0.00%?
    ListCollectionIdsRequestOrBuilder.java?0.00%?
    ListCollectionIdsResponse.java?0.00%?
    ListCollectionIdsResponseOrBuilder.java?0.00%?
    ListDocumentsRequest.java?0.00%?
    ListDocumentsRequestOrBuilder.java?0.00%?
    ListDocumentsResponse.java?0.00%?
    ListDocumentsResponseOrBuilder.java?0.00%?
    Listener.java?0.00%?
    ListenerRegistration.java?0.00%?
    ListenerRegistrationImpl.java?0.00%?
    ListenRequest.java?10.66%?
    ListenRequestOrBuilder.java?0.00%?
    ListenResponse.java?32.64%?
    ListenResponseOrBuilder.java?0.00%?
    ListenSequence.java?100.00%?
    LoadBundleTask.java?25.26%?
    LoadBundleTaskProgress.java?60.47%?
    LocalDocumentsResult.java?100.00%?
    LocalDocumentsView.java?100.00%?
    LocalSerializer.java?97.35%?
    LocalStore.java?100.00%?
    LocalViewChanges.java?100.00%?
    Logger.java?75.00%?
    LogicUtils.java?96.67%?
    LruDelegate.java?0.00%?
    LruGarbageCollector.java?97.20%?
    MapValue.java?53.00%?
    MapValueOrBuilder.java?0.00%?
    MemoryBundleCache.java?100.00%?
    MemoryComponentProvider.java?100.00%?
    MemoryDocumentOverlayCache.java?100.00%?
    MemoryEagerReferenceDelegate.java?100.00%?
    MemoryIndexManager.java?68.97%?
    MemoryLruReferenceDelegate.java?96.00%?
    MemoryMutationQueue.java?98.57%?
    MemoryOverlayMigrationManager.java?100.00%?
    MemoryPersistence.java?100.00%?
    MemoryRemoteDocumentCache.java?98.21%?
    MemoryTargetCache.java?100.00%?
    MetadataChanges.java?100.00%?
    MutableDocument.java?100.00%?
    Mutation.java?98.48%?
    MutationBatch.java?89.06%?
    MutationBatchResult.java?100.00%?
    MutationQueue.java?0.00%?
    MutationResult.java?100.00%?
    NamedQuery.java?0.00%?
    NamedQueryOrBuilder.java?0.00%?
    NotInFilter.java?85.71%?
    NumberComparisonHelper.java?100.00%?
    NumericIncrementTransformOperation.java?87.50%?
    ObjectValue.java?99.06%?
    OnlineState.java?100.00%?
    OnlineStateTracker.java?98.11%?
    OnProgressListener.java?0.00%?
    OrderBy.java?96.55%?
    OrderedCodeWriter.java?68.52%?
    Overlay.java?100.00%?
    OverlayedDocument.java?100.00%?
    OverlayMigrationManager.java?0.00%?
    PatchMutation.java?100.00%?
    Persistence.java?100.00%?
    Precondition.java?37.82%?
    PreconditionOrBuilder.java?0.00%?
    Preconditions.java?57.89%?
    PropertyName.java?0.00%?
    Query.java?3.97%?
    QueryDocumentSnapshot.java?64.71%?
    QueryEngine.java?98.63%?
    QueryListener.java?100.00%?
    QueryProto.java?0.00%?
    QueryPurpose.java?100.00%?
    QueryResult.java?100.00%?
    QuerySnapshot.java?76.36%?
    QueryView.java?100.00%?
    ReferenceDelegate.java?0.00%?
    ReferenceSet.java?92.45%?
    RemoteDocumentCache.java?0.00%?
    RemoteEvent.java?92.31%?
    RemoteSerializer.java?84.63%?
    RemoteStore.java?88.49%?
    ResourcePath.java?94.74%?
    RollbackRequest.java?0.00%?
    RollbackRequestOrBuilder.java?0.00%?
    RunAggregationQueryRequest.java?0.00%?
    RunAggregationQueryRequestOrBuilder.java?0.00%?
    RunAggregationQueryResponse.java?0.00%?
    RunAggregationQueryResponseOrBuilder.java?0.00%?
    RunQueryRequest.java?0.00%?
    RunQueryRequestOrBuilder.java?0.00%?
    RunQueryResponse.java?0.00%?
    RunQueryResponseOrBuilder.java?0.00%?
    Scheduler.java?0.00%?
    ServerTimestamp.java?0.00%?
    ServerTimestampOperation.java?100.00%?
    ServerTimestamps.java?79.17%?
    SetMutation.java?97.22%?
    SetOptions.java?0.00%?
    SnapshotMetadata.java?68.75%?
    SnapshotVersion.java?87.50%?
    Source.java?0.00%?
    SQLiteBundleCache.java?90.00%?
    SQLiteComponentProvider.java?100.00%?
    SQLiteDocumentOverlayCache.java?97.85%?
    SQLiteIndexManager.java?99.48%?
    SQLiteLruReferenceDelegate.java?98.67%?
    SQLiteMutationQueue.java?82.76%?
    SQLiteOverlayMigrationManager.java?90.48%?
    SQLitePersistence.java?86.34%?
    SQLiteRemoteDocumentCache.java?98.17%?
    SQLiteSchema.java?96.63%?
    SQLiteTargetCache.java?98.47%?
    Stream.java?100.00%?
    StructuredAggregationQuery.java?0.00%?
    StructuredAggregationQueryOrBuilder.java?0.00%?
    StructuredQuery.java?32.54%?
    StructuredQueryOrBuilder.java?0.00%?
    Supplier.java?0.00%?
    SyncEngine.java?93.31%?
    Target.java?95.79%?
    TargetCache.java?0.00%?
    TargetChange.java?80.00%?
    TargetChangeOrBuilder.java?0.00%?
    TargetData.java?77.50%?
    TargetIdGenerator.java?100.00%?
    TargetIndexMatcher.java?100.00%?
    TargetOrBuilder.java?0.00%?
    TargetState.java?97.78%?
    ThrottledForwardingExecutor.java?100.00%?
    ThrowOnExtraProperties.java?0.00%?
    Timestamp.java?80.39%?
    Token.java?0.00%?
    Transaction.java?0.00%?
    TransactionOptions.java?0.00%?
    TransactionOptionsOrBuilder.java?0.00%?
    TransactionRunner.java?0.00%?
    TransformOperation.java?0.00%?
    UpdateDocumentRequest.java?0.00%?
    UpdateDocumentRequestOrBuilder.java?0.00%?
    User.java?78.57%?
    UserData.java?66.00%?
    UserDataReader.java?70.48%?
    UserDataWriter.java?51.02%?
    Util.java?67.63%?
    Value.java?40.97%?
    ValueOrBuilder.java?0.00%?
    Values.java?95.65%?
    VerifyMutation.java?46.15%?
    View.java?96.82%?
    ViewChange.java?100.00%?
    ViewSnapshot.java?89.23%?
    WatchChange.java?70.51%?
    WatchChangeAggregator.java?98.21%?
    WatchStream.java?19.23%?
    Write.java?36.11%?
    WriteBatch.java?0.00%?
    WriteOrBuilder.java?0.00%?
    WriteProto.java?0.00%?
    WriteRequest.java?10.41%?
    WriteRequestOrBuilder.java?0.00%?
    WriteResponse.java?11.70%?
    WriteResponseOrBuilder.java?0.00%?
    WriteResult.java?0.00%?
    WriteResultOrBuilder.java?0.00%?
    WriteStream.java?31.82%?

  • firebase-firestore-ktx

    Overall coverage changed from ? (d939e63) to 26.67% (264d78a) by ?.

    FilenameBase (d939e63)Merge (264d78a)Diff
    Firestore.kt?26.67%?

Test Logs

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

@github-actions
Copy link
Contributor

github-actions bot commented Oct 15, 2022 •
edited
Loading

Unit Test Results

1 109 tests  +12   1 093 ✔️ +12   54s ⏱️ -3s
     79 suites ±  0        16 💤 ±  0 
     79 files   ±  0          0 ❌ ±  0 

Results for commit 0bd5820. ± Comparison against base commit ae3bdb8.

♻️ This comment has been updated with latest results.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Oct 15, 2022 •
edited
Loading

Size Report 1

Affected Products

  • firebase-firestore

    TypeBase (d939e63)Merge (264d78a)Diff
    aar1.30 MB1.30 MB+317 B (+0.0%)
    apk (release)3.32 MB3.32 MB+368 B (+0.0%)

  • firebase-firestore-ktx

    TypeBase (d939e63)Merge (264d78a)Diff
    apk (release)4.26 MB4.26 MB+296 B (+0.0%)

Test Logs

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

@milaGGL milaGGL changed the title add hasCachedResults flag Fix Firestore failing to return empty results from the local cache problem Oct 17, 2022
@milaGGL milaGGL marked this pull request as ready for review October 25, 2022 02:37
@milaGGL milaGGL removed the WIP label Oct 25, 2022
@milaGGL milaGGL requested a review from dconeybe October 25, 2022 04:12
@dconeybe
Copy link
Contributor

Please update the description of this PR. Feel free to (mostly) copy the description from firebase/firebase-js-sdk#6624. Also, mention that this is a port of firebase/firebase-js-sdk#6624. This really helps enabling cross linking between PRs.

@dconeybe dconeybe changed the title Fix Firestore failing to return empty results from the local cache problem Fix Firestore failing to return empty results from the local cache Oct 25, 2022
dconeybe
dconeybe requested changes Oct 25, 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.

Looks great! Just two very minor comments, plus changes to the PR's description.

firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java Outdated Show resolved Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java Outdated Show resolved Hide resolved
@dconeybe
Copy link
Contributor

Oh, and please add a change log entry

@milaGGL milaGGL requested a review from dconeybe October 25, 2022 19:08
dconeybe
dconeybe requested changes Oct 26, 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.

Just one tiny fix to the change log. Otherwise, LGTM.

firebase-firestore/CHANGELOG.md Outdated Show resolved Hide resolved
@milaGGL milaGGL requested a review from dconeybe October 26, 2022 18:26
dconeybe
dconeybe approved these changes Oct 26, 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.

Approved!

@milaGGL milaGGL merged commit 6498bd0 into master Oct 26, 2022
@milaGGL milaGGL deleted the mila/Fix-Firestore-failing-to-return-empty-results-from-the-local-cache branch October 26, 2022 22:07
@dconeybe dconeybe mentioned this pull request Nov 1, 2022
Merged
@firebase firebase locked and limited conversation to collaborators Nov 26, 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

Assignees

@milaGGL milaGGL

Labels
size/XXL
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.
3 participants
@milaGGL @google-oss-bot @dconeybe