Add GetOptions for controlling offline get behaviour #655

rsgowman · 2018-01-12T00:12:29Z

Add option to allow the user to control where DocumentReference.getDocument() and Query.getDocuments() fetches from. By default, they fetch from the server (if possible) and fall back to the local cache. It's now possible to alternatively fetch from the local cache only, or to
fetch from the server only (though in the server only case, latency compensation is still enabled).

Add option to allow the user to control where DocumentReference.getDocument() fetches from. By default, it fetches from the server (if possible) and falls back to the local cache. It's now possible to alternatively fetch from the local cache only, or to fetch from the server only (though in the server only case, latency compensation is still enabled). Still TODO: Also enable this for CollectionReferenece.getDocuments().

Enable GetOptions for CollectionReference / Query .getDocuments().

rsgowman · 2018-01-12T00:14:33Z

@wilhuff Pay particular attention to Firestore/Source/Core/FSTFirestoreClient.m since I'm not sure what I'm doing there is at all valid.

wilhuff

This mostly looks good except for the manual computation of views. These views are highly unlikely to conform to all the invariants we expect and will make the Cache-only snapshot substantially different form the first snapshot with default options.

wilhuff · 2018-01-16T01:44:22Z

Firestore/Example/Tests/Core/FSTQueryListenerTests.m

-  FSTListenOptions *options = [[FSTListenOptions alloc] initWithIncludeQueryMetadataChanges:YES
-                                                             includeDocumentMetadataChanges:NO
-                                                                      waitForSyncWhenOnline:NO];
+  FSTListenOptions *options =


All the changes in this file look like whitespace-only changes. Are you on clang 6?

$ clang --version Apple LLVM version 8.1.0 (clang-802.0.42) Target: x86_64-apple-darwin16.7.0 Thread model: posix InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

I think I see what happened though. I (mistakenly) changed this file in 80a40bd and then backed out those changes in 58ccdba. However, the run of style.sh was in between.

If you like, I can revert this file completely. OTOH, style.sh thinks this is better, and since it's already fixed it...

Hmm.. thinking about this further, I'll just back this out.

wilhuff · 2018-01-16T01:45:13Z

Firestore/Example/Tests/Integration/API/FIRGetOptionsTests.m

+
+@import FirebaseFirestore;
+
+#import <FirebaseFirestore/FIRFirestore.h>


This should be covered by importing the module above.

wilhuff · 2018-01-16T01:46:42Z

Firestore/Example/Tests/Integration/API/FIRGetOptionsTests.m

+ * acknowledge the writes. (This is expected to be called primarily when
+ * offline, so waiting for the server doesn't make sense.)
+ */
+- (void)updateData {


Perhaps name this in a way that indicates it doesn't wait? Perhaps updateDataNoWait?

However, see above: it's not clear after calling this when a test makes an assertion about _testDoc but not _testCol why not. These tests would be clearer if you updated the things you cared about in the test.

No longer relevant.

wilhuff · 2018-01-16T02:03:56Z

Firestore/Example/Tests/Integration/API/FIRGetOptionsTests.m

+- (void)setUp {
+  [super setUp];
+
+  _emptyDoc = [self documentRef];


Factoring out these common parts is a good thing from a DRY perspective but it has the effect of making these tests tightly coupled, brittle, hard to read, and ultimately do more work than they need to do.

For example, in testGetDocumentWhileOnlineWithDefaultGetOptions, it's not obvious why [self readDocumentForRef:_testDoc] would necessarily satisfy the requirement that _testDoc "exists, is not from the cache, and matches the initial data".

This was the subject TotT 258 (from 2012), available externally here: https://testing.googleblog.com/2017/01/testing-on-toilet-keep-cause-and-effect.html

Please set up what documents you need per test.

Undone the refactor commit.

wilhuff · 2018-01-16T02:08:34Z

Firestore/Example/Tests/Integration/API/FIRGetOptionsTests.m

+  XCTAssertEqualObjects(result.data, _testDocInitialData);
+}
+
+- (void)testGetDocumentsWhileOnlineWithDefaultGetOptions {


This test name differs from the above by a single 's'. Something like testGetCollection... would make the distinction clearer.

So does the api. :) (getDocumentWithCompletion vs getDocumentsWithCompletion. Though admittedly, they're at least on different objects.)

Done (throughout.)

wilhuff · 2018-01-16T03:00:47Z

Firestore/Source/API/FIRQuery.m

+                                          code:FIRFirestoreErrorCodeUnavailable
+                                      userInfo:@{
+                                        NSLocalizedDescriptionKey :
+                                            @"Failed to get documents from server."


More error text to explain that this is a user-requested error condition (as above).

"Failed to get documents from server. (However, these documents may exist in the local cache. Run again without setting FIRServer in the GetOptions to retrieve the cached documents.)"

wilhuff · 2018-01-16T03:03:11Z

Firestore/Source/Core/FSTFirestoreClient.m

+          nil, [NSError errorWithDomain:FIRFirestoreErrorDomain
+                                   code:FIRFirestoreErrorCodeUnavailable
+                               userInfo:@{
+                                 NSLocalizedDescriptionKey : @"Failed to get document from server.",


More error text to explain that this is a user-requested error condition (as above).

Also in this case, this is a cache-only request so the server has nothing to do with it, right?

Re cache vs server: Oops.

Updated text:

"Failed to get document from cache. (However, this document may exist on the server. Run again without setting FIRCache in the GetOptions to attempt to retrieve the document from the server.)"

wilhuff · 2018-01-16T03:09:59Z

Firestore/Source/Core/FSTFirestoreClient.m

+                                                          oldDocuments:oldDocuments
+                                                       documentChanges:@[]
+                                                             fromCache:YES
+                                                      hasPendingWrites:NO


This seems like the wrong value. The local store will apply local mutations if present.

Also the documentChanges array being empty seems suspicious: the view processing code will normally make all the documents in the initial snapshot adds.

These are both examples of reasons why you shouldn't compose the view snapshots yourself.

Done; ptal.

wilhuff · 2018-01-16T03:10:50Z

Firestore/Source/Public/FIRDocumentReference.h

@@ -195,6 +196,11 @@ NS_SWIFT_NAME(DocumentReference)
 - (void)getDocumentWithCompletion:(FIRDocumentSnapshotBlock)completion
    NS_SWIFT_NAME(getDocument(completion:));

+// clang-format off
+- (void)getDocumentWithOptions:(FIRGetOptions *)options completion:(FIRDocumentSnapshotBlock)completion
+    NS_SWIFT_NAME(getDocument(options:completion:));


Please add a swift-language test that verifies this looks correct from swift.

wilhuff · 2018-01-16T03:13:39Z

Firestore/Source/Public/FIRGetOptions.h

+ * stale with respect to the value on the server.) For a single document, the
+ * get will fail if the document doesn't exist.
+ */
+typedef NS_ENUM(NSUInteger, FIRSource) { FIRDefault, FIRServer, FIRCache } NS_SWIFT_NAME(Source);


In order for swift bridging to work correctly the enumeration names need to all be suffixes of the type.

That means these need to be FIRSourceDefault, FIRSourceServer, etc.

I'm a little worried that "FIRSource" is too generic a term: we share the FIR namespace with the rest of Firebase, unfortunately. We should flag this as a potential issue in the API proposal.

Done. (Also flagged in the api review doc.)

Still have a few (more in depth) comments to address.

Artifacts of an earlier attempt at this. :(

wilhuff

updates LGTM after nitpicking.

Still need to use FSTViewChange to actually compose the snapshot, but I know you're still working on that.

wilhuff · 2018-01-17T17:16:10Z

Firestore/Source/Public/FIRGetOptions.h

+ * try to give a consistent (server-retrieved) snapshot, or else revert to the
+ * cache to provide a value.
+ *
+ * FIRServer causes Firestore to avoid the cache (generating an error if a


Comments should match the names: s/FIRServer/FIRSourceServer/, here and throughout.

wilhuff · 2018-01-17T17:17:34Z

Firestore/Source/API/FIRDocumentReference.m

+                                        NSLocalizedDescriptionKey :
+                                            @"Failed to get document from server. (However, this "
+                                            @"document does exist in the local cache. Run again "
+                                            @"without setting FIRServer in the GetOptions to "


This line mixes FIR-prefixed Server (which should be FIRSourceServer, incidentally) with non-prefixed GetOptions. These should be consistent (and probably prefixed).

…ToQuery

wilhuff

I think the view computation you've added is sound. A few final nitpicks and we're good to go.

wilhuff · 2018-01-18T18:07:08Z

Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm

+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];


Two bits of feedback here:

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 ];

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.

wilhuff · 2018-01-18T18:08:58Z

Firestore/Source/Core/FSTFirestoreClient.m

+    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.");


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".

wilhuff · 2018-01-18T18:11:56Z

Firestore/Example/SwiftBuildTest/main.swift

@@ -228,11 +228,11 @@ func readDocument(at docRef: DocumentReference) {
 func readDocumentWithOptions(at docRef: DocumentReference) {
  docRef.getDocument(options:GetOptions.defaultOptions()) { document, error in
  }
-  docRef.getDocument(options:GetOptions.init(source:Source.default)) { document, error in
+  docRef.getDocument(options:GetOptions.init(source:GetSource.default)) { document, error in


Try making one of these use the unqualified form:

docRef.getDocument(options:GetOptions(source:.default)) { // ...

Also, explicitly calling init is not required. Using a constructor-style invocation calls the init function.

Swift api limbo error message

wilhuff

That last commit is a cure worse than the disease. I can see now that there's no easy way to construct a FIRDocumentChange in a test that would compare equal to the generated values so let's just drop the final commit.

wilhuff · 2018-01-19T18:08:47Z

Also, this API hasn't been formally approved yet, so let's squash and merge into the firestore-api-changes branch.

* Convert FIRGetOptions.initWithSource to factory method. * source -> optionsWithSource * Swift: GetOptions.source(source:) -> source(_:)

Add option to allow the user to control where DocumentReference.getDocument() and CollectionReference.getDocuments() fetches from. By default, it fetches from the server (if possible) and falls back to the local cache. It's now possible to alternatively fetch from the local cache only, or to fetch from the server only (though in the server only case, latency compensation is still enabled).

rsgowman added 2 commits January 11, 2018 18:59

GetOptions for controlling offline behaviour (pt2)

6d29de2

Enable GetOptions for CollectionReference / Query .getDocuments().

googlebot added the cla: yes label Jan 12, 2018

rsgowman requested a review from wilhuff January 12, 2018 00:12

rsgowman assigned wilhuff Jan 12, 2018

wilhuff suggested changes Jan 16, 2018

View reviewed changes

rsgowman added 4 commits January 16, 2018 13:43

Address (easy) review feedback.

30a513f

Still have a few (more in depth) comments to address.

Remove mistaken changes to unittests.

c2395cc

Artifacts of an earlier attempt at this. :(

Prevent clang-format from breaking file

76d0ae8

Run style.sh

2a32091

rsgowman force-pushed the rsgowman/get_options_2 branch from 58ccdba to 2a32091 Compare January 16, 2018 20:20

Ensure swift bindings are correct

ec55882

wilhuff reviewed Jan 17, 2018

View reviewed changes

wilhuff assigned rsgowman and unassigned wilhuff Jan 17, 2018

rsgowman added 4 commits January 17, 2018 18:54

Alter getDocumentsFromLocalCache to be more like FSTSyncEngine.listen…

61876ad

…ToQuery

Fix error messages.

f5024fb

Fix doc comments.

3abd449

FIRSource -> FIRGetSource

d7b9e60

wilhuff reviewed Jan 18, 2018

View reviewed changes

Review feedback

893e56a

Swift api limbo error message

rsgowman assigned wilhuff Jan 18, 2018

wilhuff approved these changes Jan 19, 2018

View reviewed changes

rsgowman force-pushed the rsgowman/get_options_2 branch from 36fc2da to 893e56a Compare January 19, 2018 18:33

rsgowman changed the base branch from master to firestore-api-changes January 19, 2018 18:37

rsgowman merged commit 3440fef into firestore-api-changes Jan 19, 2018

rsgowman mentioned this pull request Jan 23, 2018

GetOptions for controlling offline behaviour firebase/firebase-js-sdk#463

Merged

wilhuff mentioned this pull request Jan 26, 2018

Firestore "no internet connection" error #622

Closed

rsgowman added a commit that referenced this pull request Feb 1, 2018

Convert FIRGetOptions.initWithSource to factory method. (#655) (#711)

5a68eda

* Convert FIRGetOptions.initWithSource to factory method. * source -> optionsWithSource * Swift: GetOptions.source(source:) -> source(_:)

rsgowman deleted the rsgowman/get_options_2 branch February 21, 2018 20:57

mrbonezy mentioned this pull request Apr 25, 2018

FR: Be able to invalidate offline data #1180

Closed

firebase locked and limited conversation to collaborators Nov 9, 2019


		@import FirebaseFirestore;

		#import <FirebaseFirestore/FIRFirestore.h>

Add GetOptions for controlling offline get behaviour #655

Add GetOptions for controlling offline get behaviour #655

Conversation

rsgowman commented Jan 12, 2018

rsgowman commented Jan 12, 2018

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment

wilhuff commented Jan 19, 2018