Fix RTDB query get caching behavior

Author: jmwski

FirebaseDatabase/Sources/Core/FRepo.m

@@ -533,7 +533,7 @@ - (void)getData:(FIRDatabaseQuery *)query
         }];
         return;
     }
-    [self.persistenceManager setQueryActive:querySpec];
+    NSNumber *tag = [self.serverSyncTree registerQuery:[query querySpec]];
     [self.connection
         getDataAtPath:[query.path toString]
            withParams:querySpec.params.wireProtocolParams
@@ -569,10 +569,19 @@ - (void)getData:(FIRDatabaseQuery *)query
                }];
            } else {
                node = [FSnapshotUtilities nodeFrom:data];
-               [self.eventRaiser
-                   raiseEvents:[self.serverSyncTree
-                                   applyServerOverwriteAtPath:[query path]
-                                                      newData:node]];
+               if ([query.querySpec loadsAllData]) {
+                   [self.eventRaiser
+                       raiseEvents:[self.serverSyncTree
+                                       applyServerOverwriteAtPath:[query path]
+                                                          newData:node]];
+               } else {
+                   [self.eventRaiser
+                       raiseEvents:[self.serverSyncTree
+                                       applyTaggedQueryOverwriteAtPath:[query
+                                                                           path]
+                                                               newData:node
+                                                                 tagId:tag]];
+               }
                [self.eventRaiser raiseCallback:^{
                  block(
                      nil,

FirebaseDatabase/Sources/Core/FSyncPoint.h

@@ -70,5 +70,9 @@
 - (BOOL)viewExistsForQuery:(FQuerySpec *)query;
 - (BOOL)hasCompleteView;
 - (FView *)completeView;
+- (void)registerQuery:(FQuerySpec *)query
+          writesCache:(FWriteTreeRef *)writesCache
+          serverCache:(FCacheNode *)serverCache;
+- (BOOL)unregisterQuery:(FQuerySpec *)query;
 
 @end

FirebaseDatabase/Sources/Core/FSyncPoint.m

@@ -161,14 +161,9 @@ - (FView *)getView:(FQuerySpec *)query
     return [[FView alloc] initWithQuery:query initialViewCache:viewCache];
 }
 
-/**
- * Add an event callback for the specified query
- * Returns an array of events to raise.
- */
-- (NSArray *)addEventRegistration:(id<FEventRegistration>)eventRegistration
-       forNonExistingViewForQuery:(FQuerySpec *)query
-                      writesCache:(FWriteTreeRef *)writesCache
-                      serverCache:(FCacheNode *)serverCache {
+- (void)registerQuery:(FQuerySpec *)query
+          writesCache:(FWriteTreeRef *)writesCache
+          serverCache:(FCacheNode *)serverCache {
     NSAssert(self.views[query.params] == nil, @"Found view for query: %@",
              query.params);
     // TODO: make writesCache take flag for complete server node
@@ -187,6 +182,17 @@ - (NSArray *)addEventRegistration:(id<FEventRegistration>)eventRegistration
         [self.persistenceManager setTrackedQueryKeys:allKeys forQuery:query];
     }
     self.views[query.params] = view;
+}
+
+/**
+ * Add an event callback for the specified query
+ * Returns an array of events to raise.
+ */
+- (NSArray *)addEventRegistration:(id<FEventRegistration>)eventRegistration
+       forNonExistingViewForQuery:(FQuerySpec *)query
+                      writesCache:(FWriteTreeRef *)writesCache
+                      serverCache:(FCacheNode *)serverCache {
+    [self registerQuery:query writesCache:writesCache serverCache:serverCache];
     return [self addEventRegistration:eventRegistration
               forExistingViewForQuery:query];
 }
@@ -199,6 +205,15 @@ - (NSArray *)addEventRegistration:(id<FEventRegistration>)eventRegistration
     return [view initialEvents:eventRegistration];
 }
 
+- (BOOL)unregisterQuery:(FQuerySpec *)query {
+    FView *view = self.views[query.params];
+    NSAssert(view != nil, @"no view for query %@", query);
+    if ([view isEmpty]) {
+        [self.views removeObjectForKey:query.params];
+    }
+    return [view isEmpty];
+}
+
 /**
  * Remove event callback(s). Return cancelEvents if a cancelError is specified.
  *

FirebaseDatabase/Sources/Core/FSyncTree.h

@@ -81,5 +81,7 @@
 - (id<FNode>)getServerValue:(FQuerySpec *)query;
 - (id<FNode>)calcCompleteEventCacheAtPath:(FPath *)path
                           excludeWriteIds:(NSArray *)writeIdsToExclude;
+- (NSNumber *)registerQuery:(FQuerySpec *)querySpec;
+- (void)unregisterQuery:(FQuerySpec *)querySpec;
 
 @end

FirebaseDatabase/Sources/Core/FSyncTree.m

@@ -164,6 +164,15 @@ - (id)initWithPersistenceManager:(FPersistenceManager *)persistenceManager
     return self;
 }
 
+- (NSNumber *)tagQuery:(FQuerySpec *)query {
+    NSAssert(self.queryToTagMap[query] == nil,
+             @"View does not exist, but we have a tag");
+    NSNumber *tagId = [self.queryTagCounter getAndIncrement];
+    self.queryToTagMap[query] = tagId;
+    self.tagToQueryMap[tagId] = query;
+    return tagId;
+}
+
 #pragma mark -
 #pragma mark Apply Operations
 
@@ -448,6 +457,42 @@ - (NSArray *)applyTaggedServerRangeMergeAtPath:(FPath *)path
     }
 }
 
+- (NSNumber *)registerQuery:(FQuerySpec *)query {
+    FPath *path = query.path;
+
+    __block BOOL foundAncestorDefaultView = NO;
+    [self.syncPointTree
+        forEachOnPath:query.path
+           whileBlock:^BOOL(FPath *pathToSyncPoint, FSyncPoint *syncPoint) {
+             foundAncestorDefaultView =
+                 foundAncestorDefaultView || [syncPoint hasCompleteView];
+             return !foundAncestorDefaultView;
+           }];
+
+    [self.persistenceManager setQueryActive:query];
+
+    FSyncPoint *syncPoint = [self.syncPointTree valueAtPath:path];
+    if (syncPoint == nil) {
+        syncPoint = [[FSyncPoint alloc]
+            initWithPersistenceManager:self.persistenceManager];
+        self.syncPointTree = [self.syncPointTree setValue:syncPoint
+                                                   atPath:path];
+    }
+    NSNumber *tag = nil;
+    BOOL viewAlreadyExists = [syncPoint viewExistsForQuery:query];
+    if (!viewAlreadyExists) {
+        FWriteTreeRef *writesCache =
+            [self.pendingWriteTree childWritesForPath:path];
+        FCacheNode *serverCache = [self serverCacheForQuery:query];
+        [syncPoint registerQuery:query
+                     writesCache:writesCache
+                     serverCache:serverCache];
+    } else if (![query loadsAllData]) {
+        tag = [self tagQuery:query];
+    }
+    return tag;
+}
+
 /**
  * Add an event callback for the specified query
  * @return NSArray of FEvent to raise.
@@ -575,6 +620,23 @@ - (FCacheNode *)serverCacheForQuery:(FQuerySpec *)query {
     return serverCache;
 }
 
+- (void)unregisterQuery:(FQuerySpec *)query {
+    FPath *path = query.path;
+    FSyncPoint *maybeSyncPoint = [self.syncPointTree valueAtPath:path];
+
+    if (maybeSyncPoint &&
+        ([query isDefault] || [maybeSyncPoint viewExistsForQuery:query])) {
+        BOOL removed = [maybeSyncPoint unregisterQuery:query];
+        if ([maybeSyncPoint isEmpty]) {
+            self.syncPointTree = [self.syncPointTree removeValueAtPath:path];
+        }
+        if (removed) {
+            [self removeTags:@[ query ]];
+            [self.persistenceManager setQueryInactive:query];
+        }
+    }
+}
+
 /**
  * Remove event callback(s).
  *

FirebaseDatabase/Tests/Integration/FIRDatabaseGetTests.m

@@ -20,20 +20,49 @@
 #import "FirebaseDatabase/Sources/Constants/FConstants.h"
 #import "FirebaseDatabase/Sources/Core/FQuerySpec.h"
 #import "FirebaseDatabase/Sources/Utilities/FUtilities.h"
+#import "FirebaseDatabase/Tests/Helpers/FEventTester.h"
 #import "FirebaseDatabase/Tests/Helpers/FIRFakeApp.h"
 #import "FirebaseDatabase/Tests/Helpers/FTestExpectations.h"
+#import "FirebaseDatabase/Tests/Helpers/FTupleEventTypeString.h"
 
 @implementation FIRDatabaseGetTests
 
-- (void)testTest {
-    FIRDatabaseReference* ref = [FTestHelpers getRandomNode];
-    FIRDatabaseReference* root = [ref root];
-    
-    __block BOOL removeDone = NO;
-    [root removeValueWithCompletionBlock:^(NSError *error, FIRDatabaseReference *ref) {
-      removeDone = YES;
-    }];
-    WAIT_FOR(removeDone);
+- (void)testGetDoesntTriggerExtraListens {
+  FIRDatabaseReference* ref = [FTestHelpers getRandomNode];
+  FIRDatabaseReference* root = [ref root];
+  FIRDatabaseReference* list = [root child:@"list"];
+
+  __block BOOL removeDone = NO;
+  [root removeValueWithCompletionBlock:^(NSError* error, FIRDatabaseReference* ref) {
+    removeDone = YES;
+  }];
+  WAIT_FOR(removeDone);
+
+  [self waitForCompletionOf:[list childByAutoId] setValue:@{@"name" : @"child1"}];
+  [self waitForCompletionOf:[list childByAutoId] setValue:@{@"name" : @"child2"}];
+
+  // The original report of this issue makes a listen call first, and then
+  // performs a getData. However, in the testing environment, if the listen
+  // is made first, it will round trip and cache results before get has a
+  // chance to run, at which point it reads from cache.
+  // https://github.com/firebase/firebase-ios-sdk/issues/8286
+  __block BOOL getDone = NO;
+  [[[list queryOrderedByChild:@"name"] queryEqualToValue:@"child2"]
+      getDataWithCompletionBlock:^(NSError* error, FIRDataSnapshot* snapshot) {
+        XCTAssertNil(error);
+        XCTAssertEqual(snapshot.childrenCount, 1L);
+        getDone = YES;
+      }];
+  __block NSInteger numListenEvents = 0L;
+  FIRDatabaseHandle handle = [list observeEventType:FIRDataEventTypeValue
+                                          withBlock:^(FIRDataSnapshot* snapshot) {
+                                            XCTAssertEqual(snapshot.childrenCount, 2L);
+                                            numListenEvents += 1;
+                                          }];
+  WAIT_FOR(getDone);
+  [NSThread sleepForTimeInterval:1];
+  XCTAssertEqual(numListenEvents, 1);
+  [list removeObserverWithHandle:handle];
 }
 
 @end