GetOptions for controlling offline behaviour #463

rsgowman · 2018-01-23T16:54:52Z

Add option to allow the user to control where DocumentReference.get()
and Query.get() 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).

ts port of firebase/firebase-ios-sdk#655

Add option to allow the user to control where DocumentReference.get() and Query.get() 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).

mikelehen

I haven't quite finished reviewing this, but sending this over with initial feedback in case you're waiting...

mikelehen · 2018-01-24T18:21:55Z

packages/firestore-types/index.d.ts

@@ -394,6 +394,35 @@ export interface SetOptions {
  readonly merge?: boolean;
 }

+/**
+ * An options object that configures the behavior of `get()` calls in


nit: "in" => "on" ?

mikelehen · 2018-01-24T18:23:40Z

packages/firestore-types/index.d.ts

+ * An options object that configures the behavior of `get()` calls in
+ * `DocumentReference` and `Query`. By providing a `GetOptions` object, these
+ * methods can be configured to fetch results only from the server, only from
+ * the local cache or attempt the server and fall back to the cache (which is


nit: "or attempt ..." => ", or attempt to fetch results from the server and fall back ..."

mikelehen · 2018-01-24T18:25:27Z

packages/firestore-types/index.d.ts

+  /**
+   * Describes whether we should get from server or cache.
+   *
+   * Setting to 'default' (or not setting at all), if online, causes Firestore


I would drop "if online" since we /try/ regardless of whether you are online.

mikelehen · 2018-01-24T18:28:58Z

packages/firestore-types/index.d.ts

+   * Describes whether we should get from server or cache.
+   *
+   * Setting to 'default' (or not setting at all), if online, causes Firestore
+   * to try to give a consistent (server-retrieved) snapshot, or else revert to


nits:
"give" => "get" or "retrieve" ?
"a consistent" => "an up-to-date"
", or else revert to ..." => ", but fall back to returning cached data if the server can't be reached."

mikelehen · 2018-01-24T18:34:47Z

packages/firestore-types/index.d.ts

+   * error if a value cannot be retrieved from the server). The cache will be
+   * updated if the RPC succeeds. Latency compensation still occurs (implying
+   * that if the cache is more up to date, then it's values will be merged into
+   * the results).


I'd maybe rephrase as:

Setting to 'server' causes Firestore to avoid the cache, generating an error if the server cannot be reached. Note that the cache will still be updated if the server request succeeds. Also note that latency-compensation still takes effect, so any pending write operations will be visible in the returned data (merged into the server-provided data).

mikelehen · 2018-01-24T19:44:30Z

packages/firestore/src/api/database.ts

+            .ensureClientConfigured()
+            .getDocumentsFromLocalCache(this, {
+              next: (snap: firestore.QuerySnapshot) => resolve(snap),
+              error: reject


I think we want to resolve with an empty snapshot here?

Actually, it looks like as-implemented, error can't be called so this is fine. I think this will be clearer if we implement it with a Promise return rather than Observer in any case...

ack (+ implemented as a promise. Note that the private helper is still an observer since it essentially just wraps onSnapshotInternal.)

mikelehen · 2018-01-24T19:47:21Z

packages/firestore/src/core/firestore_client.ts

+  DocumentSnapshot,
+  Query,
+  QuerySnapshot
+} from '../api/database';


I think we want to avoid api/ dependencies here (CredentialsProvider is kind of an exception). The api/ layer should be responsible to converting to/from model types.

FWIW, I think as-written this may be impossible to port to Android, since you can't have circular dependencies in java.

mikelehen · 2018-01-24T19:49:36Z

packages/firestore/src/core/firestore_client.ts

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


We have a FirestoreError class we use for all of our public-facing errors.

... which I was curiously using in api/database.ts but for some reason didn't do so here. Fixed.

mikelehen · 2018-01-24T19:50:07Z

packages/firestore/src/core/firestore_client.ts

@@ -357,6 +367,57 @@ export class FirestoreClient {
    });
  }

+  getDocumentFromLocalCache(
+    doc: DocumentReference,


Per my comment elsewhere, this should probably take a DocumentKey.

mikelehen · 2018-01-24T19:50:30Z

packages/firestore/src/core/firestore_client.ts

+      return this.localStore.readDocument(doc._key).then(maybeDoc => {
+        if (maybeDoc instanceof Document) {
+          observer.next(
+            new DocumentSnapshot(doc.firestore, doc._key, maybeDoc, true)


Per my comments elsewhere, this should return Document instead of creating a DocumentSnapshot.

I am a big fan of commenting non-obvious literals in function calls, e.g.:

new DocumentSnapshot(doc.firestore, doc._key, maybeDoc, /*fromCache=*/true)

We do this in most places (though probably not quite 100%).

Also: It occurs to me that it's possible that the user has an ongoing listener for this document already and so it's guaranteed to be up-to-date, and technically isFromCache should be false in that case. But to make that work, we'd need to route this into the SyncEngine and detect that there's an active View for this document query already, and use the snapshot from that View. We could do this but I'm not sure it's worth the trouble, so I'm fine with keeping this as-is for now. We could consider doing it later.

It's actually the EventManager that does the initial snapshot delivery if you relisten:

https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/core/event_manager.ts#L79

mikelehen

And a bit more feedback (mostly on your tests... thanks for being so exhaustive, by the way).

mikelehen · 2018-01-24T23:38:11Z

packages/firestore/test/integration/api/get_options.test.ts

+apiDescribe('GetOptions', persistence => {
+  it('get document while online with default get options', () => {
+    const initialData = { key: 'value' };
+    return withTestDoc(persistence, docRef => {


I think you could use withTestCollection() to write the initialData here (like you do in other tests). It's not a big deal, but I think this does draw a clearer split between the test setup code and implementation code. I don't feel super strongly though if you prefer this.

I like the split, but I'm less wild about calling "xCollection" in a test dealing solely with documents.

What about modifying withTestDoc to also accept an initialData parameter, similar to withTestCol? (See change)

To do this, I've added a second method (withTestDocAndInitialData) to keep the patch small. However, I think we could just adjust withTestDoc directly since existing uses of it seem to almost always immediately set some initial data before continuing with the test (just like this code used to do.)

mikelehen · 2018-01-24T23:39:30Z

packages/firestore/src/core/firestore_client.ts

+      return this.localStore.executeQuery(query._query).then(docs => {
+        const remoteKeys: DocumentKeySet = documentKeySet();
+
+        const view = new View(query._query, remoteKeys);


FWIW- Using View seems kind of overkill and confusing rather than just localStore.executeQuery()...

I think we're using it for two reasons:

localStore.executeQuery() intentionally doesn't enforce .limit().

It's a convenient way to generate a ViewSnapshot (but I suspect it wouldn't be that hard to generate manually).

If you feel like it, it might be worth seeing how hard it is to enforce the query limit and generate the ViewSnapshot manually here, so we don't have to pull in View.

It's also somewhat tempting to create a SyncEngine.getDocumentsFromLocalCache() method that this defers to, since SyncEngine could check if there's an existing view and just get the cached snapshot from it, which would avoid reading from persistence. But unless this simplifies implementation significantly (which I'm not convinced it would), we probably shouldn't bother with this sort of optimization unless we have a motivating use case in hand.

FYI- We should probably also have a test for a .limit() query. :-)

I was the one that recommended we use the view to compute the snapshot we're sending because they're hard to get right, considering the changes and metadata bits that need to be just so.

I still think that's a good idea: though your idea of defering to the sync engine (or event manager based on which is actually the right place to do this) to pick up the existing snapshot would be worthwhile if it's not too crazy.

It would also have the effect of keeping the FirestoreClient mostly logic free, which is important because it's fiendishly hard to test (today).

mikelehen · 2018-01-25T01:20:21Z

packages/firestore/src/core/firestore_client.ts

+        assert(
+          viewChange.limboChanges.length === 0,
+          'View returned limbo documents during local-only query execution.'
+        );


So I was actually surprised that this assert held because remoteKeys is empty, and so all the documents should be in limbo (they're in the local cache but not in the remote results). But it turns out we don't calculate limbo documents until you call applyChanges() with a TargetChange.

So this assert holds but not for the reason I would have expected. And if there were limbo documents, that wouldn't be a problem for us. So I'd argue this assert is confusing and not protecting us from anything, so I'd just remove it.

mikelehen · 2018-01-25T01:22:26Z

packages/firestore/test/integration/api/get_options.test.ts

+import { expect } from 'chai';
+import { apiDescribe, withTestDoc, withTestCollection } from '../util/helpers';
+
+apiDescribe('GetOptions', persistence => {


Thanks for all the tests. I love good test coverage! One thing I don't love though is boilerplate and lots of code to port though... So you might consider if:

Any of these tests are redundant

If you could simplify them (and reduce boilerplate) with helper method(s).

If you could get equivalent coverage but with less boilerplate by testing at a lower-level. [this probably doesn't apply here, but in general we try to do our more exhaustive testing as "unit" tests which run much more quickly and then only sanity-checking at the API layer]

Any of these tests are redundant

I don't believe so; at least not from a behaviour point of view. Obviously, some of the tests test very similar things, especially from a code coverage point of view. eg. fetching results from server only vs default while online yield the same results. But I think it's worth while to call that out via a test.

If you could simplify them (and reduce boilerplate) with helper method(s).

Done. See below.

mikelehen · 2018-01-25T01:24:42Z

packages/firestore/test/integration/api/get_options.test.ts

+        let docsData = {};
+        qrySnap.forEach(doc => {
+          docsData[doc.id] = doc.data();
+        });


We have a toDataArray() helper that does something similar already. Since it stores in an array, it drops the .id, but preserves ordering (which you lose here).

I guess it is convenient to be able to directly expect(...).to.deep.equal(initialDocs) though. So maybe keep this as-is, but consider creating a .toDataMap() helper since you do this in multiple tests?

Added toDataMap().

mikelehen · 2018-01-25T01:36:31Z

packages/firestore/src/core/firestore_client.ts

+      return this.localStore.readDocument(doc._key).then(maybeDoc => {
+        if (maybeDoc instanceof Document) {
+          observer.next(
+            new DocumentSnapshot(doc.firestore, doc._key, maybeDoc, true)


I am a big fan of commenting non-obvious literals in function calls, e.g.:

new DocumentSnapshot(doc.firestore, doc._key, maybeDoc, /*fromCache=*/true)

We do this in most places (though probably not quite 100%).

Also: It occurs to me that it's possible that the user has an ongoing listener for this document already and so it's guaranteed to be up-to-date, and technically isFromCache should be false in that case. But to make that work, we'd need to route this into the SyncEngine and detect that there's an active View for this document query already, and use the snapshot from that View. We could do this but I'm not sure it's worth the trouble, so I'm fine with keeping this as-is for now. We could consider doing it later.

mikelehen · 2018-01-25T01:42:23Z

packages/firestore/test/integration/api/get_options.test.ts

+        colRef
+          .get()
+          // now go offine. Note that if persistence is disabled, this will cause
+          // the initialDocs to be garbage collected.


I see that this (and the above test) combat the GC'ing by doing local .set()s instead... But this means that:

The initialDocs don't really matter (the test would still pass without them I believe).

This is really testing latency compensation instead of the cache.

Instead I think we should either:

Disable the test when persistence is not enabled (if (!persistence) { return; } or whatever)

Register an onSnapshot listener that includes the data. This will hold them in the cache for the duration of the listener.

Done (via onSnapshot. Good idea.)

mikelehen · 2018-01-25T01:44:08Z

packages/firestore/test/integration/api/get_options.test.ts

+        .set(initialData)
+        .then(() => docRef.get({ source: 'cache' }))
+        .then(doc => {
+          if (persistence) {


As mentioned earlier, you could register a listener to control whether the document is in the cache, rather than having conditional test logic.

mikelehen · 2018-01-25T01:51:03Z

packages/firestore/test/integration/api/get_options.test.ts

+    });
+  });
+
+  it('get collection while offline cache only', () => {


FWIW, to make it extra obvious what's being tested, perhaps say "with source=cache" instead of "cache only"

Done (throughout)

mikelehen · 2018-01-25T01:56:07Z

packages/firestore/src/api/database.ts

+          });
+          return;
+        }
+


And if I'm not confused, please figure out why the tests didn't catch this. :-)

mohshraim · 2018-03-14T17:23:30Z

any chance to have this feature in near beta?
i am watching this pull daily :)
this will give us a full control for better user experience..
thanks

rsgowman · 2018-03-14T17:49:32Z

Hi @mohshraim,

This feature is currently scheduled for an internal api review. (We want to make sure we get the api right before committing ourselves to it.) Once that's done, we should be able to get this out fairly quickly.

In the meantime, depending on your use case, you may be able to get similar behaviour by issuing a listen request rather than a get via DocumentReference.onSnapshot() (or CollectionReference.onSnapshot()) If you set the includeMetadataChanges option to true, your callback should be executed twice: the first time with the local data (if any), and the second time with the server data. (includeMetadataChanges ensures that your callback will be called for the server data, even if the data is the same in the local cache and the server.)

https://firebase.google.com/docs/reference/js/firebase.firestore.DocumentReference#onSnapshot

For instance, lets assume you suspect the local cached data is stale, and you only want to display the data from the server (rather than displaying stale data briefly while waiting for server data). Then you might do something like this (untested):

docref.onSnapshot( {includeMetadataChanges:true}, function(doc) {
  if (doc.metadata.fromCache) {
    // possibly stale cache data that we want to ignore
    return;
  } else {
    // up-to-date server data that we should display
    displayResultsToUser(doc);
  }
});

mohshraim · 2018-03-14T22:27:40Z

Hi @rsgowman

Thanks a lot for all the work and detailed response.

Almost read the firestore documentation 20 times. it's my favourite..
My case i have many users will login to system many times every day(20-50 time); they have shared item collection that changed once daily and contain around 10K doc. so my target is minimize the read count and bandwidth consumption from server to read it from cached data. and get fresh data a according to some conditions 2-3 times daily per user..
This feature according to my scenario calculation will save +90% bandwidth daily.. we was looking to save the data on another IndexedDB and read from it but we don't want to reinvent the wheel!

Waiting this amazing feature.
Thanks

mikelehen · 2018-04-08T15:08:01Z

Please assign back to me if this is ready for another pass. :-)

mikelehen · 2018-04-08T15:08:19Z

(oh, you have merge conflicts though, so please resolve them first)

mikelehen

Basically LGTM but a few last tweaks / suggestions.

mikelehen · 2018-04-10T22:22:23Z

packages/firestore/src/api/database.ts

    return new Promise(
      (resolve: Resolver<firestore.DocumentSnapshot>, reject: Rejecter) => {
+        if (options && options.source === 'cache') {
+          this._firestoreClient.getDocumentFromLocalCache(this, {


Hrm... I don't see the AsyncQueue change... I'm guessing it went away when you merged with master? :)

mikelehen · 2018-04-10T22:32:21Z

packages/firestore-types/index.d.ts

   * @return A Promise that will be resolved with the results of the Query.
   */
-  get(): Promise<QuerySnapshot>;
+  get(options?: GetOptions): Promise<QuerySnapshot>;


Great news! As it turns out our type definitions are duplicated in 3 places so you will also want to update them in

firebase-js-sdk/packages/firebase/app/index.d.ts

Line 625 in eeab13f

declare namespace firebase.firestore {

and

firebase-js-sdk/packages/firebase/index.d.ts

Line 626 in eeab13f

declare namespace firebase.firestore {

cc/ @jshcrowthe :)

Oh! Actually one of those may have just gone away (yay): c198f9a

That's horrifying. :(

(Done.)

mikelehen · 2018-04-10T22:39:59Z

packages/firestore/src/api/database.ts

+                Code.UNAVAILABLE,
+                'Failed to get document from server. (However, this ' +
+                  'document does exist in the local cache. Run again ' +
+                  'without setting source to FIRGetSourceServer to ' +


FIRGetSourceServer => "server" :)

mikelehen · 2018-04-10T22:43:23Z

packages/firestore/src/api/database.ts

+            // if the document doesn't exist.
+            reject(
+              new FirestoreError(
+                Code.ABORTED,


Hrm. I guess this is preexisting, but this should really be Code.UNAVAILABLE (that's what we do on iOS and Android). Can you fix?

mikelehen · 2018-04-10T23:01:34Z

packages/firestore/src/api/database.ts

+    );
+  }
+
+  private getViaSnapshotListener_(


I don't think we usually add _ to private methods.

We would ideally not use _ as a naming convention at all and just rely on private/public modifiers, etc but with our current build setup, any properties or public methods on classes end up showing up on the object at runtime and so end users could potentially discover them. So for any properties / public methods we don't want end users to see / use (because they're not part of our official API), we add a _ suffix which triggers the minification step in our build to obfuscate the name: https://github.com/firebase/firebase-js-sdk/blob/master/packages/firebase/webpack.config.js#L48

Anyway, private methods are completely hidden at runtime based on how TypeScript implements private methods and so we don't need / want to use the _ suffix (here and on the other getViaSnapshotListener_ method).

Done. (Was an instinct to add it. Blame scarring from too much javascript.)

mikelehen · 2018-04-10T23:03:40Z

packages/firestore/src/core/firestore_client.ts

@@ -41,7 +49,7 @@ import { debug } from '../util/log';
 import { Deferred } from '../util/promise';

 import { DatabaseId, DatabaseInfo } from './database_info';
-import { Query } from './query';
+import { Query as InternalQuery } from './query';


I think this type alias is no longer necessary? You can just import it as Query?

mikelehen · 2018-04-10T23:40:14Z

packages/firestore/test/integration/util/helpers.ts

+// fix existing usages of it. Then delete this function. This makes withTestDoc
+// more analogous to withTestCollection and eliminates the pattern of
+// `withTestDoc(..., docRef => { docRef.set(initialData) ...});` that otherwise is
+// quite common.


Why is this a TODO rather than just doing it? :-)

Because fixing existing uses of it seems big(ish) and otherwise orthogonal to this PR. But since you asked so nicely: #675

mikelehen

LGTM, thanks!

mohshraim · 2018-04-12T18:01:28Z

Hi @rsgowman

I would like to ask if this will be available in next beta?
or still need more internal API review?

thanks for great work, amazing feature.
and thanks @mikelehen for super fast reviews..

mikelehen

Thanks! I have some minor suggestions, but looks good.

mikelehen · 2018-04-12T20:52:20Z

packages/firestore/CHANGELOG.md

+- [feature] Added ability to control whether DocumentReference.get() and
+  Query.get() should fetch from server only, cache only, or attempt server and
+  fall back to the cache (which was the only option previously, and is now the
+  default.)


super nit: period should go after paren, I think?

mikelehen · 2018-04-12T20:53:21Z

packages/firestore/CHANGELOG.md

@@ -10,6 +10,10 @@
  `FirestoreSettings` to `true`. Note that the current behavior
  (`DocumentSnapshot`s returning JS Date objects) will be removed in a future
  release. `Timestamp` supports higher precision than JS Date.
+- [feature] Added ability to control whether DocumentReference.get() and
+  Query.get() should fetch from server only, cache only, or attempt server and


To make this maximally helpful (especially since there's generally no statement completion in JS and docs will probably not be updated immediately), I'd probably say:

... should fetch from server only (by passing { source: 'server' }, cache only (by passing { source: 'cache' }), or attempt ...

mikelehen · 2018-04-12T20:58:28Z

packages/firestore/CHANGELOG.md

@@ -10,6 +10,10 @@
  `FirestoreSettings` to `true`. Note that the current behavior
  (`DocumentSnapshot`s returning JS Date objects) will be removed in a future
  release. `Timestamp` supports higher precision than JS Date.
+- [feature] Added ability to control whether DocumentReference.get() and
+  Query.get() should fetch from server only, (by passing { source: 'server' },


missing close paren after "{ source: 'server' }" :)

Done. (sigh).

rsgowman · 2018-04-12T21:50:12Z

@mohshraim: I think this is now done. :)

If you want to give it a try immediately, you should be able to check out the code from master. Otherwise, it should be available in the next release. If you have any feedback on this feature, either positive or negative, please send it in; thanks!

rsgowman added 2 commits January 23, 2018 11:46

[AUTOMATED]: Prettier Code Styling

56b4b91

rsgowman self-assigned this Jan 23, 2018

rsgowman requested review from mikelehen, schmidt-sebastian and wilhuff as code owners January 23, 2018 16:54

google-oss-bot added the needs-triage label Jan 23, 2018

mikelehen self-assigned this Jan 24, 2018

mikelehen reviewed Jan 24, 2018

View reviewed changes

mikelehen suggested changes Jan 25, 2018

View reviewed changes

mikelehen removed their assignment Jan 31, 2018

mohshraim mentioned this pull request Mar 18, 2018

FR: Offline first support (Firestore) #262

Closed

review feedback

3cb151d

rsgowman added 3 commits April 9, 2018 17:57

Merge branch 'master' into rsgowman/get_options

ac63ead

Fixes resulting from merging master.

2c2b4d8

Merge remote-tracking branch 'origin/master' into rsgowman/get_options

78610b0

rsgowman assigned mikelehen Apr 10, 2018

mikelehen suggested changes Apr 10, 2018

View reviewed changes

mikelehen removed their assignment Apr 10, 2018

Merge remote-tracking branch 'origin/master' into rsgowman/get_options

e2b769a

rsgowman requested a review from jshcrowthe as a code owner April 11, 2018 21:47

mikelehen approved these changes Apr 11, 2018

View reviewed changes

review feedback

a5f73fb

rsgowman force-pushed the rsgowman/get_options branch from 44f77e6 to a5f73fb Compare April 11, 2018 22:10

Update changelog

e32c840

mikelehen approved these changes Apr 12, 2018

View reviewed changes

rsgowman added 2 commits April 12, 2018 16:56

review feedback

5a4576f

period after bracket

a1eb31c

mikelehen approved these changes Apr 12, 2018

View reviewed changes

close all the parens!

e807f00

rsgowman merged commit 8b29658 into master Apr 12, 2018

jshcrowthe deleted the rsgowman/get_options branch April 16, 2018 21:57

firebase locked and limited conversation to collaborators Oct 23, 2019

GetOptions for controlling offline behaviour #463

GetOptions for controlling offline behaviour #463

Uh oh!

Conversation

rsgowman commented Jan 23, 2018

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!