From 3dd9c6a8156533ee6ac18eb7d266b11a0b4359a3 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Tue, 12 Feb 2019 16:24:50 -0500 Subject: [PATCH 1/2] Forbid queries endAt an uncommitted server timestamp. 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 https://github.com/firebase/firebase-android-sdk/pull/138 --- .../Integration/API/FIRValidationTests.mm | 49 +++++++++++++++++++ Firestore/Source/API/FIRQuery.mm | 15 +++++- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm index b3a72f43ae7..570c3f1e34d 100644 --- 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. Your 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 index e4d2f7066cd..bad8e64ff30 100644 --- 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. Your 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", From f28958398f702ae2cc4403d053dd265af30409f0 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Tue, 12 Feb 2019 17:13:41 -0500 Subject: [PATCH 2/2] review feedback --- .../Example/Tests/Integration/API/FIRValidationTests.mm | 6 +++--- Firestore/Source/API/FIRQuery.mm | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm index 570c3f1e34d..b1fec91d657 100644 --- a/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRValidationTests.mm @@ -438,13 +438,13 @@ - (void)testQueriesCannotBeSortedByAnUncommittedServerTimestamp { if (snapshot.empty) return; XCTAssertEqual(snapshot.count, 1); - FIRQueryDocumentSnapshot *docSnap = [snapshot documents][0]; + FIRQueryDocumentSnapshot *docSnap = snapshot.documents[0]; - if ([snapshot metadata].pendingWrites) { + 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. Your are trying to start or end a query using a document for which the " + @"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] diff --git a/Firestore/Source/API/FIRQuery.mm b/Firestore/Source/API/FIRQuery.mm index bad8e64ff30..3742359bd34 100644 --- a/Firestore/Source/API/FIRQuery.mm +++ b/Firestore/Source/API/FIRQuery.mm @@ -578,7 +578,7 @@ - (FSTBound *)boundFromSnapshot:(FIRDocumentSnapshot *)snapshot isBefore:(BOOL)i if ([value isKindOfClass:[FSTServerTimestampValue class]]) { FSTThrowInvalidUsage(@"InvalidQueryException", - @"Invalid query. Your are trying to start or end a query using a " + @"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.)",