Fix startAfter/endBefore for orderByKeys queries (#7403)

jmwski · web-flow · commit 6db2e0d856e2 · 2021-02-10T09:42:36.000-08:00
diff --git a/FirebaseDatabase/CHANGELOG.md b/FirebaseDatabase/CHANGELOG.md
@@ -2,6 +2,7 @@
 - [fixed] Fix variable length array diagnostics warning (#7460).
 # v7.5.1
 - [changed] Optimize `FIRDatabaseQuery#getDataWithCompletionBlock` when in-memory active listener cache exists (#7312).
+- [fixed] Fixed an issue with `FIRDatabaseQuery#{queryStartingAfterValue,queryEndingBeforeValue} when used in `queryOrderedByKey` queries (#7403).
 
 # v7.5.0
 - [added] Implmement `queryStartingAfterValue` and `queryEndingBeforeValue` for FirebaseDatabase query pagination.
diff --git a/FirebaseDatabase/Sources/Api/FIRDatabaseQuery.m b/FirebaseDatabase/Sources/Api/FIRDatabaseQuery.m
@@ -95,32 +95,35 @@ - (void)validateQueryEndpointsForParams:(FQueryParams *)params {
             if (params.indexStartKey != [FUtilities minName] &&
                 params.indexStartKey != [FUtilities maxName]) {
                 [NSException raise:INVALID_QUERY_PARAM_ERROR
-                            format:@"Can't use queryStartingAtValue:childKey: "
+                            format:@"Can't use queryStartingAtValue:childKey:, "
+                                   @"queryStartingAfterValue:childKey:, "
                                    @"or queryEqualTo:andChildKey: in "
                                    @"combination with queryOrderedByKey"];
             }
             if (![params.indexStartValue.val isKindOfClass:[NSString class]]) {
-                [NSException
-                     raise:INVALID_QUERY_PARAM_ERROR
-                    format:
-                        @"Can't use queryStartingAtValue: with other types "
-                        @"than string in combination with queryOrderedByKey"];
+                [NSException raise:INVALID_QUERY_PARAM_ERROR
+                            format:@"Can't use queryStartingAtValue: or "
+                                   @"queryStartingAfterValue: "
+                                   @"with non-string types when used with "
+                                   @"queryOrderedByKey"];
             }
         }
         if ([params hasEnd]) {
             if (params.indexEndKey != [FUtilities maxName] &&
                 params.indexEndKey != [FUtilities minName]) {
                 [NSException raise:INVALID_QUERY_PARAM_ERROR
                             format:@"Can't use queryEndingAtValue:childKey: or "
+                                   @"queryEndingBeforeValue:childKey: "
                                    @"queryEqualToValue:childKey: in "
                                    @"combination with queryOrderedByKey"];
             }
             if (![params.indexEndValue.val isKindOfClass:[NSString class]]) {
                 [NSException
                      raise:INVALID_QUERY_PARAM_ERROR
-                    format:
-                        @"Can't use queryEndingAtValue: with other types than "
-                        @"string in combination with queryOrderedByKey"];
+                    format:@"Can't use queryEndingAtValue: or "
+                           @"queryEndingBeforeValue: "
+                           @"with other types than string in combination with "
+                           @"queryOrderedByKey"];
             }
         }
     } else if ([params.index isEqual:[FPriorityIndex priorityIndex]]) {
@@ -131,7 +134,8 @@ - (void)validateQueryEndpointsForParams:(FQueryParams *)params {
             [NSException
                  raise:INVALID_QUERY_PARAM_ERROR
                 format:@"When using queryOrderedByPriority, values provided to "
-                       @"queryStartingAtValue:, queryEndingAtValue:, or "
+                       @"queryStartingAtValue:, queryStartingAfterValue:, "
+                       @"queryEndingAtValue:, queryEndingBeforeValue:, or "
                        @"queryEqualToValue: must be valid priorities."];
         }
     }
@@ -142,13 +146,14 @@ - (void)validateEqualToCall {
         [NSException
              raise:INVALID_QUERY_PARAM_ERROR
             format:
-                @"Cannot combine queryEqualToValue: and queryStartingAtValue:"];
+                @"Cannot combine queryEqualToValue: and queryStartingAtValue: "
+                @"or queryStartingAfterValue:"];
     }
     if ([self.queryParams hasEnd]) {
         [NSException
              raise:INVALID_QUERY_PARAM_ERROR
-            format:
-                @"Cannot combine queryEqualToValue: and queryEndingAtValue:"];
+            format:@"Cannot combine queryEqualToValue: and queryEndingAtValue: "
+                   @"or queryEndingBeforeValue:"];
     }
 }
 
@@ -202,20 +207,25 @@ - (FIRDatabaseQuery *)queryStartingAfterValue:(id)startAfterValue {
 
 - (FIRDatabaseQuery *)queryStartingAfterValue:(id)startAfterValue
                                      childKey:(NSString *)childKey {
-    if ([self.queryParams.index isEqual:[FKeyIndex keyIndex]] &&
-        childKey != nil) {
-        @throw [[NSException alloc]
-            initWithName:INVALID_QUERY_PARAM_ERROR
-                  reason:@"You must use queryStartingAfterValue: instead of "
-                         @"queryStartingAfterValue:childKey: when using "
-                         @"queryOrderedByKey:"
-                userInfo:nil];
-    }
-    if (childKey == nil) {
-        childKey = [FUtilities maxName];
+    if ([self.queryParams.index isEqual:[FKeyIndex keyIndex]]) {
+        if (childKey != nil) {
+            @throw [[NSException alloc]
+                initWithName:INVALID_QUERY_PARAM_ERROR
+                      reason:
+                          @"You must use queryStartingAfterValue: instead of "
+                          @"queryStartingAfterValue:childKey: when using "
+                          @"queryOrderedByKey:"
+                    userInfo:nil];
+        }
+        if ([startAfterValue isKindOfClass:[NSString class]]) {
+            startAfterValue = [FNextPushId successor:startAfterValue];
+        }
     } else {
-        childKey = [FNextPushId successor:childKey];
-        NSLog(@"successor of child key %@", childKey);
+        if (childKey == nil) {
+            childKey = [FUtilities maxName];
+        } else {
+            childKey = [FNextPushId successor:childKey];
+        }
     }
     NSString *methodName = @"queryStartingAfterValue:childKey:";
     if (childKey != nil && ![childKey isEqual:[FUtilities maxName]]) {
@@ -234,7 +244,8 @@ - (FIRDatabaseQuery *)queryStartingAtInternal:(id<FNode>)startValue
     [self validateIndexValueType:startValue fromMethod:methodName];
     if ([self.queryParams hasStart]) {
         [NSException raise:INVALID_QUERY_PARAM_ERROR
-                    format:@"Can't call %@ after queryStartingAtValue or "
+                    format:@"Can't call %@ after queryStartingAtValue, "
+                           @"queryStartingAfterValue, or "
                            @"queryEqualToValue was previously called",
                            methodName];
     }
@@ -283,20 +294,24 @@ - (FIRDatabaseQuery *)queryEndingBeforeValue:(id)endValue {
 
 - (FIRDatabaseQuery *)queryEndingBeforeValue:(id)endValue
                                     childKey:(NSString *)childKey {
-    if ([self.queryParams.index isEqual:[FKeyIndex keyIndex]] &&
-        childKey != nil) {
-        @throw [[NSException alloc]
-            initWithName:INVALID_QUERY_PARAM_ERROR
-                  reason:@"You must use queryEndingBeforeValue: instead of "
-                         @"queryEndingBeforeValue:childKey: when using "
-                         @"queryOrderedByKey:"
-                userInfo:nil];
-    }
-
-    if (childKey == nil) {
-        childKey = [FUtilities minName];
+    if ([self.queryParams.index isEqual:[FKeyIndex keyIndex]]) {
+        if (childKey != nil) {
+            @throw [[NSException alloc]
+                initWithName:INVALID_QUERY_PARAM_ERROR
+                      reason:@"You must use queryEndingBeforeValue: instead of "
+                             @"queryEndingBeforeValue:childKey: when using "
+                             @"queryOrderedByKey:"
+                    userInfo:nil];
+        }
+        if ([endValue isKindOfClass:[NSString class]]) {
+            endValue = [FNextPushId predecessor:endValue];
+        }
     } else {
-        childKey = [FNextPushId predecessor:childKey];
+        if (childKey == nil) {
+            childKey = [FUtilities minName];
+        } else {
+            childKey = [FNextPushId predecessor:childKey];
+        }
     }
     NSString *methodName = @"queryEndingBeforeValue:childKey:";
     if (childKey != nil && ![childKey isEqual:[FUtilities minName]]) {
@@ -361,12 +376,12 @@ - (FIRDatabaseQuery *)queryEqualToInternal:(id)value
         [FValidation validateFrom:methodName validKey:childKey];
     }
     if ([self.queryParams hasEnd] || [self.queryParams hasStart]) {
-        [NSException
-             raise:INVALID_QUERY_PARAM_ERROR
-            format:
-                @"Can't call %@ after queryStartingAtValue, queryEndingAtValue "
-                @"or queryEqualToValue was previously called",
-                methodName];
+        [NSException raise:INVALID_QUERY_PARAM_ERROR
+                    format:@"Can't call %@ after queryStartingAtValue, "
+                           @"queryStartingAfterValue, queryEndingAtValue, "
+                           @"queryEndingBeforeValue or queryEqualToValue "
+                           @"was previously called",
+                           methodName];
     }
     id<FNode> node = [FSnapshotUtilities nodeFrom:value];
     FQueryParams *params = [[self.queryParams startAt:node
diff --git a/FirebaseDatabase/Sources/Core/FRepo.m b/FirebaseDatabase/Sources/Core/FRepo.m
@@ -518,10 +518,8 @@ - (void)getData:(FIRDatabaseQuery *)query
         (void (^_Nonnull)(NSError *__nullable error,
                           FIRDataSnapshot *__nullable snapshot))block {
     FQuerySpec *querySpec = [query querySpec];
-    id<FNode> node =
-        [self.serverSyncTree calcCompleteEventCacheAtPath:querySpec.path
-                                          excludeWriteIds:@[]];
-    if (![node isEmpty]) {
+    id<FNode> node = [self.serverSyncTree getServerValue:[query querySpec]];
+    if (node != nil) {
         block(nil, [[FIRDataSnapshot alloc]
                        initWithRef:query.ref
                        indexedNode:[FIndexedNode
diff --git a/FirebaseDatabase/Sources/Core/FSyncPoint.h b/FirebaseDatabase/Sources/Core/FSyncPoint.h
@@ -41,6 +41,10 @@
                 writesCache:(FWriteTreeRef *)writesCache
                 serverCache:(id<FNode>)optCompleteServerCache;
 
+- (FView *)getView:(FQuerySpec *)query
+       writesCache:(FWriteTreeRef *)writesCache
+       serverCache:(FCacheNode *)serverCache;
+
 /**
  * Returns array of FEvent
  */
@@ -61,6 +65,7 @@
  */
 - (NSArray *)queryViews;
 - (id<FNode>)completeServerCacheAtPath:(FPath *)path;
+- (id<FNode>)completeEventCacheAtPath:(FPath *)path;
 - (FView *)viewForQuery:(FQuerySpec *)query;
 - (BOOL)viewExistsForQuery:(FQuerySpec *)query;
 - (BOOL)hasCompleteView;
diff --git a/FirebaseDatabase/Sources/Core/FSyncPoint.m b/FirebaseDatabase/Sources/Core/FSyncPoint.m
@@ -128,17 +128,13 @@ - (NSArray *)applyOperation:(id<FOperation>)operation
     }
 }
 
-/**
- * Add an event callback for the specified query
- * Returns Array of FEvent events to raise.
- */
-- (NSArray *)addEventRegistration:(id<FEventRegistration>)eventRegistration
-       forNonExistingViewForQuery:(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
+- (FView *)getView:(FQuerySpec *)query
+       writesCache:(FWriteTreeRef *)writesCache
+       serverCache:(FCacheNode *)serverCache {
+    FView *view = self.views[query.params];
+    if (view != nil) {
+        return view;
+    }
     id<FNode> eventCache = [writesCache
         calculateCompleteEventCacheWithCompleteServerCache:
             serverCache.isFullyInitialized ? serverCache.node : nil];
@@ -147,8 +143,9 @@ - (NSArray *)addEventRegistration:(id<FEventRegistration>)eventRegistration
         eventCacheComplete = YES;
     } else {
         eventCache = [writesCache
-            calculateCompleteEventChildrenWithCompleteServerChildren:serverCache
-                                                                         .node];
+            calculateCompleteEventChildrenWithCompleteServerChildren:
+                serverCache.node != nil ? serverCache.node
+                                        : [FEmptyNode emptyNode]];
         eventCacheComplete = NO;
     }
 
@@ -161,8 +158,24 @@ - (NSArray *)addEventRegistration:(id<FEventRegistration>)eventRegistration
     FViewCache *viewCache =
         [[FViewCache alloc] initWithEventCache:eventCacheNode
                                    serverCache:serverCache];
-    FView *view = [[FView alloc] initWithQuery:query
-                              initialViewCache:viewCache];
+    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 {
+    NSAssert(self.views[query.params] == nil, @"Found view for query: %@",
+             query.params);
+    // TODO: make writesCache take flag for complete server node
+    FView *view = [self getView:query
+                    writesCache:writesCache
+                    serverCache:serverCache];
+
     // If this is a non-default query we need to tell persistence our current
     // view of the data
     if (!query.loadsAllData) {
@@ -273,6 +286,16 @@ - (NSArray *)queryViews {
     return serverCache;
 }
 
+- (id<FNode>)completeEventCacheAtPath:(FPath *)path {
+    __block id<FNode> eventCache = nil;
+    [self.views enumerateKeysAndObjectsUsingBlock:^(FQueryParams *key,
+                                                    FView *view, BOOL *stop) {
+      eventCache = [view completeEventCacheFor:path];
+      *stop = (eventCache != nil);
+    }];
+    return eventCache;
+}
+
 - (FView *)viewForQuery:(FQuerySpec *)query {
     return [self.views objectForKey:query.params];
 }
diff --git a/FirebaseDatabase/Sources/Core/FSyncTree.h b/FirebaseDatabase/Sources/Core/FSyncTree.h
@@ -78,6 +78,7 @@
 - (NSArray *)removeAllWrites;
 
 - (FIndexedNode *)persistenceServerCache:(FQuerySpec *)querySpec;
+- (id<FNode>)getServerValue:(FQuerySpec *)query;
 - (id<FNode>)calcCompleteEventCacheAtPath:(FPath *)path
                           excludeWriteIds:(NSArray *)writeIdsToExclude;
 
diff --git a/FirebaseDatabase/Sources/Core/FSyncTree.m b/FirebaseDatabase/Sources/Core/FSyncTree.m
@@ -734,6 +734,47 @@ - (FIndexedNode *)persistenceServerCache:(FQuerySpec *)querySpec {
     return cacheNode.node;
 }
 
+- (id<FNode>)getServerValue:(FQuerySpec *)query {
+    __block id<FNode> serverCacheNode = nil;
+    __block FSyncPoint *targetSyncPoint = nil;
+    [self.syncPointTree
+        forEachOnPath:query.path
+           whileBlock:^BOOL(FPath *pathToSyncPoint, FSyncPoint *syncPoint) {
+             FPath *relativePath = [FPath relativePathFrom:pathToSyncPoint
+                                                        to:query.path];
+             serverCacheNode =
+                 [syncPoint completeEventCacheAtPath:relativePath];
+             targetSyncPoint = syncPoint;
+             return serverCacheNode == nil;
+           }];
+
+    if (targetSyncPoint == nil) {
+        targetSyncPoint = [[FSyncPoint alloc]
+            initWithPersistenceManager:self.persistenceManager];
+        self.syncPointTree = [self.syncPointTree setValue:targetSyncPoint
+                                                   atPath:[query path]];
+    } else {
+        serverCacheNode =
+            serverCacheNode != nil
+                ? serverCacheNode
+                : [targetSyncPoint completeServerCacheAtPath:[FPath empty]];
+    }
+
+    FIndexedNode *indexed = [FIndexedNode
+        indexedNodeWithNode:serverCacheNode != nil ? serverCacheNode
+                                                   : [FEmptyNode emptyNode]
+                      index:query.index];
+    FCacheNode *serverCache =
+        [[FCacheNode alloc] initWithIndexedNode:indexed
+                             isFullyInitialized:serverCacheNode != nil
+                                     isFiltered:NO];
+    FView *view = [targetSyncPoint
+            getView:query
+        writesCache:[_pendingWriteTree childWritesForPath:[query path]]
+        serverCache:serverCache];
+    return [view completeEventCache];
+}
+
 /**
  * Returns a complete cache, if we have one, of the data at a particular path.
  * The location must have a listener above it, but as this is only used by
diff --git a/FirebaseDatabase/Sources/Core/View/FView.h b/FirebaseDatabase/Sources/Core/View/FView.h
@@ -41,7 +41,9 @@
 
 - (id<FNode>)eventCache;
 - (id<FNode>)serverCache;
+- (id<FNode>)completeEventCache;
 - (id<FNode>)completeServerCacheFor:(FPath *)path;
+- (id<FNode>)completeEventCacheFor:(FPath *)path;
 - (BOOL)isEmpty;
 
 - (void)addEventRegistration:(id<FEventRegistration>)eventRegistration;
diff --git a/FirebaseDatabase/Sources/Core/View/FView.m b/FirebaseDatabase/Sources/Core/View/FView.m
@@ -130,6 +130,10 @@ - (id)initWithQuery:(FQuerySpec *)query
     return self.viewCache.cachedEventSnap.node;
 }
 
+- (id<FNode>)completeEventCache {
+    return self.viewCache.completeEventSnap;
+}
+
 - (id<FNode>)completeServerCacheFor:(FPath *)path {
     id<FNode> cache = self.viewCache.completeServerSnap;
     if (cache) {
@@ -145,6 +149,21 @@ - (id)initWithQuery:(FQuerySpec *)query
     return nil;
 }
 
+- (id<FNode>)completeEventCacheFor:(FPath *)path {
+    id<FNode> cache = self.viewCache.completeEventSnap;
+    if (cache) {
+        // If this isn't a "loadsAllData" view, then cache isn't actually a
+        // complete cache and we need to see if it contains the child we're
+        // interested in.
+        if ([self.query loadsAllData] ||
+            (!path.isEmpty &&
+             ![cache getImmediateChild:path.getFront].isEmpty)) {
+            return [cache getChild:path];
+        }
+    }
+    return nil;
+}
+
 - (BOOL)isEmpty {
     return self.eventRegistrations.count == 0;
 }
diff --git a/FirebaseDatabase/Tests/Integration/FIRDatabaseQueryTests.m b/FirebaseDatabase/Tests/Integration/FIRDatabaseQueryTests.m
