Skip to content

Add GetOptions for controlling offline get behaviour #655

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

Merged
merged 12 commits into from
Jan 19, 2018
Merged

Add GetOptions for controlling offline get behaviour #655

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
80a40bd
GetOptions for controlling offline behaviour.
rsgowman Jan 10, 2018
6d29de2
GetOptions for controlling offline behaviour (pt2)
rsgowman Jan 11, 2018
30a513f
Address (easy) review feedback.
rsgowman Jan 16, 2018
c2395cc
Remove mistaken changes to unittests.
rsgowman Jan 12, 2018
76d0ae8
Prevent clang-format from breaking file
rsgowman Jan 16, 2018
2a32091
Run style.sh
rsgowman Jan 16, 2018
ec55882
Ensure swift bindings are correct
rsgowman Jan 16, 2018
61876ad
Alter getDocumentsFromLocalCache to be more like FSTSyncEngine.listen…
rsgowman Jan 17, 2018
f5024fb
Fix error messages.
rsgowman Jan 18, 2018
3abd449
Fix doc comments.
rsgowman Jan 18, 2018
d7b9e60
FIRSource -> FIRGetSource
rsgowman Jan 18, 2018
893e56a
Review feedback
rsgowman Jan 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions Firestore/Example/Tests/Integration/API/FIRGetOptionsTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ - (void)testGetDocumentWhileOnlineWithDefaultGetOptions {
FIRDocumentSnapshot *result = [self readDocumentForRef:doc];
XCTAssertTrue(result.exists);
XCTAssertFalse(result.metadata.fromCache);
XCTAssertFalse(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(result.data, initialData);
}

Expand All @@ -57,9 +58,15 @@ - (void)testGetCollectionWhileOnlineWithDefaultGetOptions {
// initialDocs.
FIRQuerySnapshot *result = [self readDocumentSetForRef:col];
XCTAssertFalse(result.metadata.fromCache);
XCTAssertFalse(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(
FIRQuerySnapshotGetData(result),
(@[ @{@"key1" : @"value1"}, @{@"key2" : @"value2"}, @{@"key3" : @"value3"} ]));
XCTAssertEqualObjects(FIRQuerySnapshotGetDocChangesData(result), (@[
@[ @(FIRDocumentChangeTypeAdded), @"doc1", @{@"key1" : @"value1"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc2", @{@"key2" : @"value2"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc3", @{@"key3" : @"value3"} ]
]));
}

- (void)testGetDocumentWhileOfflineWithDefaultGetOptions {
Expand All @@ -86,6 +93,7 @@ - (void)testGetDocumentWhileOfflineWithDefaultGetOptions {
FIRDocumentSnapshot *result = [self readDocumentForRef:doc];
XCTAssertTrue(result.exists);
XCTAssertTrue(result.metadata.fromCache);
XCTAssertTrue(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(result.data, newData);
}

Expand Down Expand Up @@ -113,10 +121,18 @@ - (void)testGetCollectionWhileOfflineWithDefaultGetOptions {
// get docs and ensure they *are* from the cache, and matches the updated data.
FIRQuerySnapshot *result = [self readDocumentSetForRef:col];
XCTAssertTrue(result.metadata.fromCache);
XCTAssertTrue(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(FIRQuerySnapshotGetData(result), (@[
@{@"key1" : @"value1"}, @{@"key2" : @"value2", @"key2b" : @"value2b"},
@{@"key3b" : @"value3b"}, @{@"key4" : @"value4"}
]));
XCTAssertEqualObjects(
FIRQuerySnapshotGetDocChangesData(result), (@[
@[ @(FIRDocumentChangeTypeAdded), @"doc1", @{@"key1" : @"value1"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc2", @{@"key2" : @"value2", @"key2b" : @"value2b"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc3", @{@"key3b" : @"value3b"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc4", @{@"key4" : @"value4"} ]
]));
}

- (void)testGetDocumentWhileOnlineCacheOnly {
Expand All @@ -132,6 +148,7 @@ - (void)testGetDocumentWhileOnlineCacheOnly {
[self readDocumentForRef:doc options:[[FIRGetOptions alloc] initWithSource:FIRSourceCache]];
XCTAssertTrue(result.exists);
XCTAssertTrue(result.metadata.fromCache);
XCTAssertFalse(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(result.data, initialData);
}

Expand All @@ -152,11 +169,17 @@ - (void)testGetCollectionWhileOnlineCacheOnly {
[self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRSourceCache]];
XCTAssertTrue(result.metadata.fromCache);
XCTAssertFalse(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(FIRQuerySnapshotGetData(result), (@[
@{@"key1" : @"value1"},
@{@"key2" : @"value2"},
@{@"key3" : @"value3"},
]));
XCTAssertEqualObjects(FIRQuerySnapshotGetDocChangesData(result), (@[
@[ @(FIRDocumentChangeTypeAdded), @"doc1", @{@"key1" : @"value1"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc2", @{@"key2" : @"value2"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc3", @{@"key3" : @"value3"} ]
]));
}

- (void)testGetDocumentWhileOfflineCacheOnly {
Expand Down Expand Up @@ -184,6 +207,7 @@ - (void)testGetDocumentWhileOfflineCacheOnly {
[self readDocumentForRef:doc options:[[FIRGetOptions alloc] initWithSource:FIRSourceCache]];
XCTAssertTrue(result.exists);
XCTAssertTrue(result.metadata.fromCache);
XCTAssertTrue(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(result.data, newData);
}

Expand Down Expand Up @@ -214,10 +238,18 @@ - (void)testGetCollectionWhileOfflineCacheOnly {
[self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRSourceCache]];
XCTAssertTrue(result.metadata.fromCache);
XCTAssertTrue(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(FIRQuerySnapshotGetData(result), (@[
@{@"key1" : @"value1"}, @{@"key2" : @"value2", @"key2b" : @"value2b"},
@{@"key3b" : @"value3b"}, @{@"key4" : @"value4"}
]));
XCTAssertEqualObjects(
FIRQuerySnapshotGetDocChangesData(result), (@[
@[ @(FIRDocumentChangeTypeAdded), @"doc1", @{@"key1" : @"value1"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc2", @{@"key2" : @"value2", @"key2b" : @"value2b"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc3", @{@"key3b" : @"value3b"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc4", @{@"key4" : @"value4"} ]
]));
}

- (void)testGetDocumentWhileOnlineServerOnly {
Expand All @@ -233,6 +265,7 @@ - (void)testGetDocumentWhileOnlineServerOnly {
[self readDocumentForRef:doc options:[[FIRGetOptions alloc] initWithSource:FIRSourceServer]];
XCTAssertTrue(result.exists);
XCTAssertFalse(result.metadata.fromCache);
XCTAssertFalse(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(result.data, initialData);
}

Expand All @@ -253,11 +286,17 @@ - (void)testGetCollectionWhileOnlineServerOnly {
[self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRSourceServer]];
XCTAssertFalse(result.metadata.fromCache);
XCTAssertFalse(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(FIRQuerySnapshotGetData(result), (@[
@{@"key1" : @"value1"},
@{@"key2" : @"value2"},
@{@"key3" : @"value3"},
]));
XCTAssertEqualObjects(FIRQuerySnapshotGetDocChangesData(result), (@[
@[ @(FIRDocumentChangeTypeAdded), @"doc1", @{@"key1" : @"value1"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc2", @{@"key2" : @"value2"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc3", @{@"key3" : @"value3"} ]
]));
}

- (void)testGetDocumentWhileOfflineServerOnly {
Expand Down Expand Up @@ -341,13 +380,15 @@ - (void)testGetDocumentWhileOfflineWithDifferentGetOptions {
[self readDocumentForRef:doc options:[[FIRGetOptions alloc] initWithSource:FIRSourceCache]];
XCTAssertTrue(result.exists);
XCTAssertTrue(result.metadata.fromCache);
XCTAssertTrue(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(result.data, newData);

// attempt to get doc (with default get options)
result =
[self readDocumentForRef:doc options:[[FIRGetOptions alloc] initWithSource:FIRSourceDefault]];
XCTAssertTrue(result.exists);
XCTAssertTrue(result.metadata.fromCache);
XCTAssertTrue(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(result.data, newData);

// attempt to get doc (from the server) and ensure it cannot be retreived
Expand Down Expand Up @@ -398,10 +439,18 @@ - (void)testGetCollectionWhileOfflineWithDifferentGetOptions {
[self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRSourceCache]];
XCTAssertTrue(result.metadata.fromCache);
XCTAssertTrue(result.metadata.hasPendingWrites);
XCTAssertEqualObjects(FIRQuerySnapshotGetData(result), (@[
@{@"key1" : @"value1"}, @{@"key2" : @"value2", @"key2b" : @"value2b"},
@{@"key3b" : @"value3b"}, @{@"key4" : @"value4"}
]));
XCTAssertEqualObjects(
FIRQuerySnapshotGetDocChangesData(result), (@[
@[ @(FIRDocumentChangeTypeAdded), @"doc1", @{@"key1" : @"value1"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc2", @{@"key2" : @"value2", @"key2b" : @"value2b"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc3", @{@"key3b" : @"value3b"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc4", @{@"key4" : @"value4"} ]
]));

// attempt to get docs (with default get options)
result = [self readDocumentSetForRef:col
Expand All @@ -411,6 +460,13 @@ - (void)testGetCollectionWhileOfflineWithDifferentGetOptions {
@{@"key1" : @"value1"}, @{@"key2" : @"value2", @"key2b" : @"value2b"},
@{@"key3b" : @"value3b"}, @{@"key4" : @"value4"}
]));
XCTAssertEqualObjects(
FIRQuerySnapshotGetDocChangesData(result), (@[
@[ @(FIRDocumentChangeTypeAdded), @"doc1", @{@"key1" : @"value1"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc2", @{@"key2" : @"value2", @"key2b" : @"value2b"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc3", @{@"key3b" : @"value3b"} ],
@[ @(FIRDocumentChangeTypeAdded), @"doc4", @{@"key4" : @"value4"} ]
]));

// attempt to get docs (from the server) and ensure they cannot be retreived
XCTestExpectation *failedGetDocsCompletion = [self expectationWithDescription:@"failedGetDocs"];
Expand All @@ -431,6 +487,7 @@ - (void)testGetNonExistingDocWhileOnlineWithDefaultGetOptions {
FIRDocumentSnapshot *snapshot = [self readDocumentForRef:doc];
XCTAssertFalse(snapshot.exists);
XCTAssertFalse(snapshot.metadata.fromCache);
XCTAssertFalse(snapshot.metadata.hasPendingWrites);
}

- (void)testGetNonExistingCollectionWhileOnlineWithDefaultGetOptions {
Expand All @@ -439,7 +496,9 @@ - (void)testGetNonExistingCollectionWhileOnlineWithDefaultGetOptions {
// get collection and ensure it's empty and that it's *not* from the cache.
FIRQuerySnapshot *snapshot = [self readDocumentSetForRef:col];
XCTAssertEqual(snapshot.count, 0);
XCTAssertEqual(snapshot.documentChanges.count, 0);
XCTAssertFalse(snapshot.metadata.fromCache);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also be empty, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (throughout)

XCTAssertFalse(snapshot.metadata.hasPendingWrites);
}

- (void)testGetNonExistingDocWhileOfflineWithDefaultGetOptions {
Expand Down Expand Up @@ -471,7 +530,9 @@ - (void)testGetNonExistingCollectionWhileOfflineWithDefaultGetOptions {
// get collection and ensure it's empty and that it *is* from the cache.
FIRQuerySnapshot *snapshot = [self readDocumentSetForRef:col];
XCTAssertEqual(snapshot.count, 0);
XCTAssertEqual(snapshot.documentChanges.count, 0);
XCTAssertTrue(snapshot.metadata.fromCache);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also check empty.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Done.)

XCTAssertFalse(snapshot.metadata.hasPendingWrites);
}

- (void)testGetNonExistingDocWhileOnlineCacheOnly {
Expand Down Expand Up @@ -500,7 +561,9 @@ - (void)testGetNonExistingCollectionWhileOnlineCacheOnly {
[self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRSourceCache]];
XCTAssertEqual(snapshot.count, 0);
XCTAssertEqual(snapshot.documentChanges.count, 0);
XCTAssertTrue(snapshot.metadata.fromCache);
XCTAssertFalse(snapshot.metadata.hasPendingWrites);
}

- (void)testGetNonExistingDocWhileOfflineCacheOnly {
Expand Down Expand Up @@ -535,7 +598,9 @@ - (void)testGetNonExistingCollectionWhileOfflineCacheOnly {
[self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRSourceCache]];
XCTAssertEqual(snapshot.count, 0);
XCTAssertEqual(snapshot.documentChanges.count, 0);
XCTAssertTrue(snapshot.metadata.fromCache);
XCTAssertFalse(snapshot.metadata.hasPendingWrites);
}

- (void)testGetNonExistingDocWhileOnlineServerOnly {
Expand All @@ -546,6 +611,7 @@ - (void)testGetNonExistingDocWhileOnlineServerOnly {
[self readDocumentForRef:doc options:[[FIRGetOptions alloc] initWithSource:FIRSourceServer]];
XCTAssertFalse(snapshot.exists);
XCTAssertFalse(snapshot.metadata.fromCache);
XCTAssertFalse(snapshot.metadata.hasPendingWrites);
}

- (void)testGetNonExistingCollectionWhileOnlineServerOnly {
Expand All @@ -556,7 +622,9 @@ - (void)testGetNonExistingCollectionWhileOnlineServerOnly {
[self readDocumentSetForRef:col
options:[[FIRGetOptions alloc] initWithSource:FIRSourceServer]];
XCTAssertEqual(snapshot.count, 0);
XCTAssertEqual(snapshot.documentChanges.count, 0);
XCTAssertFalse(snapshot.metadata.fromCache);
XCTAssertFalse(snapshot.metadata.hasPendingWrites);
}

- (void)testGetNonExistingDocWhileOfflineServerOnly {
Expand Down
4 changes: 4 additions & 0 deletions Firestore/Example/Tests/Util/FSTIntegrationTestCase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,10 @@ NSArray<NSDictionary<NSString *, id> *> *FIRQuerySnapshotGetData(FIRQuerySnapsho
/** Converts the FIRQuerySnapshot to an NSArray containing the document IDs in order. */
NSArray<NSString *> *FIRQuerySnapshotGetIDs(FIRQuerySnapshot *docs);

/** Converts the FIRQuerySnapshot to an NSArray containing an NSArray containing the doc change data
* in order of { type, doc title, doc data }. */
NSArray<NSArray<id> *> *FIRQuerySnapshotGetDocChangesData(FIRQuerySnapshot *docs);

#if __cplusplus
} // extern "C"
#endif
Expand Down
12 changes: 12 additions & 0 deletions Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,18 @@ - (void)waitUntil:(BOOL (^)())predicate {
return result;
}

extern "C" NSArray<NSArray<id> *> *FIRQuerySnapshotGetDocChangesData(FIRQuerySnapshot *docs) {
NSMutableArray<NSMutableArray<id> *> *result = [NSMutableArray array];
for (FIRDocumentChange *docChange in docs.documentChanges) {
NSMutableArray<id> *docChangeData = [NSMutableArray array];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two bits of feedback here:

  1. You can create an array of known contents with far less typing by using an array literal, like so:
NSArray<id> *docChangeData = @[
  @(docChange.type), docChange.document.documentID, docChange.document.data
];
  1. I'm not wild about making this untyped and unstructured. Historically the way we've done this is to make a test helper for concisely creating the expected result, usually something like what's in FSTHelpers.h and then defining isEqual/describe on it such that we can XCTAssertEqualObjects and have the right thing happen.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

[docChangeData addObject:@(docChange.type)];
[docChangeData addObject:docChange.document.documentID];
[docChangeData addObject:docChange.document.data];
[result addObject:docChangeData];
}
return result;
}

@end

NS_ASSUME_NONNULL_END
27 changes: 12 additions & 15 deletions Firestore/Source/Core/FSTFirestoreClient.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#import "Firestore/Source/Core/FSTQuery.h"
#import "Firestore/Source/Core/FSTSyncEngine.h"
#import "Firestore/Source/Core/FSTTransaction.h"
#import "Firestore/Source/Core/FSTView.h"
#import "Firestore/Source/Local/FSTEagerGarbageCollector.h"
#import "Firestore/Source/Local/FSTLevelDB.h"
#import "Firestore/Source/Local/FSTLocalSerializer.h"
Expand Down Expand Up @@ -287,27 +288,23 @@ - (void)getDocumentsFromLocalCache:(FIRQuery *)query
completion:(void (^)(FIRQuerySnapshot *_Nullable query,
NSError *_Nullable error))completion {
[self.workerDispatchQueue dispatchAsync:^{

FSTDocumentDictionary *docs = [self.localStore executeQuery:query.query];
FSTDocumentKeySet *remoteKeys = [FSTDocumentKeySet keySet];

__block FSTDocumentSet *documents =
[FSTDocumentSet documentSetWithComparator:query.query.comparator];
FSTDocumentSet *oldDocuments = documents;
[docs enumerateKeysAndObjectsUsingBlock:^(FSTDocumentKey *key, FSTDocument *value, BOOL *stop) {
documents = [documents documentSetByAddingDocument:value];
}];
FSTView *view = [[FSTView alloc] initWithQuery:query.query remoteDocuments:remoteKeys];
FSTViewDocumentChanges *viewDocChanges = [view computeChangesWithDocuments:docs];
FSTViewChange *viewChange = [view applyChangesToDocuments:viewDocChanges];
FSTAssert(viewChange.limboChanges.count == 0,
@"View returned limbo docs before target ack from the server.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion text makes no sense in this context (there won't be a target ack from the server since we're not starting a listen with the server).

Rather this should be @"View returned limbo documents during local-only query execution".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


FSTViewSnapshot *snapshot = [[FSTViewSnapshot alloc] initWithQuery:query.query
documents:documents
oldDocuments:oldDocuments
documentChanges:@[]
fromCache:YES
hasPendingWrites:NO
syncStateChanged:NO];
FSTViewSnapshot *snapshot = viewChange.snapshot;
FIRSnapshotMetadata *metadata =
[FIRSnapshotMetadata snapshotMetadataWithPendingWrites:NO fromCache:YES];
[FIRSnapshotMetadata snapshotMetadataWithPendingWrites:snapshot.hasPendingWrites
fromCache:snapshot.fromCache];

completion([FIRQuerySnapshot snapshotWithFirestore:query.firestore
originalQuery:query.query
originalQuery:query
snapshot:snapshot
metadata:metadata],
nil);
Expand Down