Forbid queries endAt an uncommitted server timestamp. (#2382)

Without this, it still fails, but: a) not until serializing the query, and b) the error is an internal error, and c) the error message is quite cryptic and has nothing to do with the problem. Port of firebase/firebase-android-sdk#138
firebase · Feb 13, 2019 · 0f32878 · 0f32878
1 parent 571c825
commit 0f32878
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 2 deletions.
diff --git a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm
@@ -420,6 +420,55 @@ - (void)testQueryCannotBeCreatedFromDocumentsMissingSortValues {
   FSTAssertThrows([query queryEndingAtDocument:snapshot], reason);
 }
 
+- (void)testQueriesCannotBeSortedByAnUncommittedServerTimestamp {
+  __weak FIRCollectionReference *collection = [self collectionRef];
+  FIRFirestore *db = [self firestore];
+
+  [db disableNetworkWithCompletion:[self completionForExpectationWithName:@"Disable network"]];
+  [self awaitExpectations];
+
+  XCTestExpectation *offlineCallbackDone =
+      [self expectationWithDescription:@"offline callback done"];
+  XCTestExpectation *onlineCallbackDone = [self expectationWithDescription:@"online callback done"];
+
+  [collection addSnapshotListener:^(FIRQuerySnapshot *snapshot, NSError *error) {
+    XCTAssertNil(error);
+
+    // Skip the initial empty snapshot.
+    if (snapshot.empty) return;
+
+    XCTAssertEqual(snapshot.count, 1);
+    FIRQueryDocumentSnapshot *docSnap = snapshot.documents[0];
+
+    if (snapshot.metadata.pendingWrites) {
+      // Offline snapshot. Since the server timestamp is uncommitted, we
+      // shouldn't be able to query by it.
+      NSString *reason =
+          @"Invalid query. You are trying to start or end a query using a document for which the "
+          @"field 'timestamp' is an uncommitted server timestamp. (Since the value of this field "
+          @"is unknown, you cannot start/end a query with it.)";
+      FSTAssertThrows([[[collection queryOrderedByField:@"timestamp"] queryEndingAtDocument:docSnap]
+                          addSnapshotListener:^(FIRQuerySnapshot *, NSError *){
+                          }],
+                      reason);
+      [offlineCallbackDone fulfill];
+    } else {
+      // Online snapshot. Since the server timestamp is committed, we should be able to query by it.
+      [[[collection queryOrderedByField:@"timestamp"] queryEndingAtDocument:docSnap]
+          addSnapshotListener:^(FIRQuerySnapshot *, NSError *){
+          }];
+      [onlineCallbackDone fulfill];
+    }
+  }];
+
+  FIRDocumentReference *document = [collection documentWithAutoID];
+  [document setData:@{@"timestamp" : [FIRFieldValue fieldValueForServerTimestamp]}];
+  [self awaitExpectations];
+
+  [db enableNetworkWithCompletion:[self completionForExpectationWithName:@"Enable network"]];
+  [self awaitExpectations];
+}
+
 - (void)testQueryBoundMustNotHaveMoreComponentsThanSortOrders {
   FIRCollectionReference *testCollection = [self collectionRef];
   FIRQuery *query = [testCollection queryOrderedByField:@"foo"];

diff --git a/Firestore/Source/API/FIRQuery.mm b/Firestore/Source/API/FIRQuery.mm
@@ -22,6 +22,7 @@
 #import "Firestore/Source/API/FIRDocumentReference+Internal.h"
 #import "Firestore/Source/API/FIRDocumentSnapshot+Internal.h"
 #import "Firestore/Source/API/FIRFieldPath+Internal.h"
+#import "Firestore/Source/API/FIRFieldValue+Internal.h"
 #import "Firestore/Source/API/FIRFirestore+Internal.h"
 #import "Firestore/Source/API/FIRListenerRegistration+Internal.h"
 #import "Firestore/Source/API/FIRQuery+Internal.h"
@@ -550,7 +551,9 @@ - (void)validateOrderByField:(const FieldPath &)orderByField
  * Note that the FSTBound will always include the key of the document and the position will be
  * unambiguous.
  *
- * Will throw if the document does not contain all fields of the order by of the query.
+ * Will throw if the document does not contain all fields of the order by of
+ * the query or if any of the fields in the order by are an uncommitted server
+ * timestamp.
  */
 - (FSTBound *)boundFromSnapshot:(FIRDocumentSnapshot *)snapshot isBefore:(BOOL)isBefore {
   if (![snapshot exists]) {
@@ -572,7 +575,15 @@ - (FSTBound *)boundFromSnapshot:(FIRDocumentSnapshot *)snapshot isBefore:(BOOL)i
                                                    databaseID:self.firestore.databaseID]];
     } else {
       FSTFieldValue *value = [document fieldForPath:sortOrder.field];
-      if (value != nil) {
+
+      if ([value isKindOfClass:[FSTServerTimestampValue class]]) {
+        FSTThrowInvalidUsage(@"InvalidQueryException",
+                             @"Invalid query. You are trying to start or end a query using a "
+                              "document for which the field '%s' is an uncommitted server "
+                              "timestamp. (Since the value of this field is unknown, you cannot "
+                              "start/end a query with it.)",
+                             sortOrder.field.CanonicalString().c_str());
+      } else if (value != nil) {
         [components addObject:value];
       } else {
         FSTThrowInvalidUsage(@"InvalidQueryException",