Persisting mutation batch state in LocalStorage #584

schmidt-sebastian · 2018-03-21T21:19:38Z

This PR allows pending mutation batches to be persisted in LocalStorage. In order to keep things focussed, this only adds the serialization and deserialization in SharedClientState. The next PR will add the calls from LocalStorage that invoke SharedClientState, and in a further follow up I will add the wiring between SyncEngine and SharedClientState.

This PR also paves the way for SyncEngine to gain two pieces of functionality:

Apply pending batches by Batch ID to all Local Views
Apply successful mutations by Batch ID

Note: The changes in fromLocalStorageEntry are formatting only as I wrapped the logic in a if (value !== null) block.

mikelehen

Generally looks good but lots of readability nits and I think we may need to revisit encoding uids in local storage keys.

mikelehen · 2018-03-21T23:05:25Z

packages/firestore/src/local/shared_client_delegate.ts

+ * Callback methods invoked by SharedClientState for notifications received
+ * from Local Storage.
+ */
+export interface SharedClientDelegate {


"Delegate" seems very iOS-y and I'd expect the delegate name to exactly match the class it's used with (i.e. SharedClientStateDelegate not SharedClientDelegate). I think the most JS-idiomatic way to do this would be for these to be events that you register for with sharedClientState.onNewPendingbatch(...) or similar. But I'm not opposed to wrapping them up together like this...

So naming options:

SharedClientStateChangeHandler
SharedClientStateCallbacks

You could also kinda' invert the way this works and have a set of change classes similar to WatchChange, and have consuming code like:

sharedClientState.onChange(change => { if (change instanceof NewPendingBatchChange) { ... } else if (change instanceof ...) { ... } })

That kind of appeals to me, but either way is fine.

The problem with naming this interface is that it will soon gain getActiveClients. It will then neither be a true delegate, and most certainly not a change handler or a list of callbacks. I went with SharedClientStateSyncer. I personally think 'Syncer' is a pretty awkward name, but at least it conveys that there is back and forth communication.

I also really like the way your .onChange API looks, but while I do agree that is appealing, it doesn't seem like the most straightforward way and will likely add some complexity.

mikelehen · 2018-03-21T23:07:12Z

packages/firestore/src/local/shared_client_delegate.ts

+  /**
+   * Loads a pending batch from the persistence layer and updates all views.
+   */
+  loadPendingBatch(batchId: BatchId): Promise<void>;


I think these method names and descriptions are breaking the abstraction that this interface appears to be trying to create (to allow SharedClientState to generate notifications / callbacks without knowing how the consumer will make use of them). So the method should be handleNewPendingBatch() or similar, and leave the implementer to decide what to do with new batches.

Alternatively we could go the route of RemoteStore/SyncEngine/RemoteSyncer and not try to provide the illusion of an abstraction. I think then we'd call this SharedClientStateSyncer and have SharedClientState have a "syncEngine: SharedClientStateSyncer" property and rework the method names / comments with the expectation that SyncEngine will be the one and only implementer. I still think the method names need tweaking though. loadPendingBatch() should try to encompass the view updates, etc... Maybe just applyNewPendingBatch() or something.

The main reason for this callbacks is to make testing feasible. My goal was not to create an abstraction layer on top of SyncEngine. It seems feasible to follow your suggestion here and mostly just look at this as an interface to SyncEngine. I renamed the internal members to reflect this.

I also went ahead and renamed this method to applyPendingBatch. I don't want to rely on the fact that this batch will always be unknown to SyncEngine.

mikelehen · 2018-03-21T23:13:34Z

packages/firestore/src/local/shared_client_state.ts

@@ -31,6 +33,10 @@ const LOG_TAG = 'SharedClientState';
 //     fs_clients_<persistence_prefix>_<instance_key>
 const CLIENT_STATE_KEY_PREFIX = 'fs_clients';

+// The format of the LocalStorage key that stores the mutation state is:
+//     fs_mutations_<persistence_prefix>_<instance_key>


nit but it's usually nice to organize data hierarchically so that related data is close together, which means we'd put "mutations" and "clients" after the <persistence_prefix> so that all of the data for a particular persistence prefix is together...

This doesn't really matter (in general data locality can be important for performance but I don't expect it to be measurable here) but it might at least make debugging slightly easier / less error-prone.

Feel free to ignore if you don't want to bother. Like I said, it doesn't really matter.

I'm gonna ignore this advice for now as it invalidates the concept of "prefix", which makes the code a bit simpler. I also went further in the wrong direction here when I addressed the underscore issue for mutation batches...

mikelehen · 2018-03-22T14:59:19Z

packages/firestore/src/local/shared_client_state.ts

+ * LocalStorage serialization. The UserId and BatchId is omitted as it is
+ * encoded as part of the key.
+ */
+interface MutationBatchSchema {


I think we should probably name this slightly differently since it's not actually storing the mutation batch (it's basically just a pointer to it). PendingMutationBatchSchema maybe?

Renamed to MutationMetadataSchema.

mikelehen · 2018-03-22T15:00:03Z

packages/firestore/src/local/shared_client_state.ts

+}
+
+// Visible for testing
+export class RemoteMutationBatch {


Isn't this used locally as well? (and there's no corresponding LocalMutationBatch.) So perhaps this should be PendingMutationBatch?

Also, perhaps worth a class comment?

Renamed to MutationMetadata, as this will also store information for non-pending mutations.

mikelehen · 2018-03-22T16:26:03Z

packages/firestore/test/unit/local/persistence_test_helpers.ts

+    batchId: BatchId,
+    err: FirestoreError
+  ): Promise<void> {}
+}


Hrm. Will prettier let you put a newline after this?

Let me try. I am going to guess yes.

mikelehen · 2018-03-22T16:39:28Z

packages/firestore/test/unit/local/web_storage_shared_client_state.test.ts

+      const updatedState = new LocalClientState();
+      updatedState.addQueryTarget(5);
+      updatedState.addQueryTarget(6);
+      updatedState.addPendingMutation(0);


I don't fully understand why you added this addPendingMutation(0) call to this test... It seems like the old code was broken (it was returning 0 for getMinimumGlobalPendingMutation(), but it probably should have been returning -1 (BATCHID_UNKNOWN)... or maybe null).

It was broken, and I do want to verify that the new pending mutation is lower. So I added this extra call here.

mikelehen · 2018-03-22T16:39:59Z

packages/firestore/test/unit/local/web_storage_shared_client_state.test.ts

+        .testWebStorageSharedClientState(ownerId, clientDelegate)
+        .then(nc => {
+          sharedClientState = nc;
+          expect(storageCallback).to.not.be.undefined;


Is .to.be.defined not a thing?

It's not - but .to.exist is.

mikelehen · 2018-03-22T16:40:28Z

packages/firestore/test/unit/local/web_storage_shared_client_state.test.ts

+
+      return persistenceHelpers
+        .testWebStorageSharedClientState(ownerId, clientDelegate)
+        .then(nc => {


what does nc mean?

NotificationChannel :) Changed to clientState.

mikelehen · 2018-03-22T16:42:46Z

packages/firestore/test/unit/local/web_storage_shared_client_state.test.ts

+          'rejected',
+          new FirestoreError('internal', 'Test Error')
+        ).toLocalStorageJSON()
+      });


consider a helper method to reduce test boilerplate...

Done. Note that I never rely on parsing the oldValue, so I removed it from this test.

mikelehen

Mostly LGTM, but a couple small things left.

mikelehen · 2018-03-26T16:15:47Z

packages/firestore/src/local/shared_client_state.ts

@@ -31,6 +33,11 @@ const LOG_TAG = 'SharedClientState';
 //     fs_clients_<persistence_prefix>_<instance_key>
 const CLIENT_STATE_KEY_PREFIX = 'fs_clients';

+// The format of the LocalStorage key that stores the mutation state is:
+//     fs_mutations_<persistence_prefix>_<batch_id> (for unauthenticated users)
+// or: fs_mutations_<persistence_prefix>_<batch_id>_<user_uid>


Perhaps add

// NOTE: user_uid is last to avoid needing to escape '_' characters that it might contain.

mikelehen · 2018-03-26T16:19:32Z

packages/firestore/src/local/shared_client_state.ts

  }

  /** Returns 'true' if LocalStorage is available in the current environment. */
  static isAvailable(): boolean {
    return typeof window !== 'undefined' && window.localStorage != null;
  }

-  start(knownClients: ClientKey[]): void {
+  subscribe(syncEngine: SharedClientStateSyncer) {


This method should be renamed or go away (RemoteStore just makes syncEngine a public mutable property; so that's probably easiest and most consistent).

Followed the semantics of RemoteStore.

mikelehen · 2018-03-26T16:20:48Z

packages/firestore/src/local/shared_client_state.ts

 }

+/**
+ * Holds the state of a mutation batch, including its user ID, batch ID annd


mikelehen · 2018-03-26T16:25:39Z

packages/firestore/src/local/shared_client_state.ts

  }

  toLocalStorageJSON(): string {
-    const batchState: MutationBatchSchema = {
-      lastUpdateTime: this.lastUpdateTime.getTime(),
+    const batchState: MutationMetadataSchema = {


batchState => batchMetadata ?

mikelehen · 2018-03-26T17:05:51Z

packages/firestore/src/local/shared_client_state.ts

@@ -399,7 +395,7 @@ export class WebStorageSharedClientState implements SharedClientState {
      `^${CLIENT_STATE_KEY_PREFIX}_${persistenceKey}_([^_]*)$`
    );
    this.mutationBatchKeyRe = new RegExp(
-      `^${MUTATION_BATCH_KEY_PREFIX}_${persistenceKey}_([^_]*)_(\\d*)$`
+      `^${MUTATION_BATCH_KEY_PREFIX}_${persistenceKey}_(\\d*)(?:_(.*))$`


I'm trying to figure out two things:

Why \\d* instead of \\d+?

Why (?:_(.*))$ instead of just _(.*)$?

Am I missing something or would it be more precise / simple to do:

`^${MUTATION_BATCH_KEY_PREFIX}_${persistenceKey}_(\\d+)_(.*)$`

Oh... It looks like you only add _blah for authenticated users. But then for this regex to work you'd need a ? after the non-capturing group. i.e. ...(?:_(.*))?$ right?

Should there be a test for unauthenticated mutation batches?

I have added tests for both unauthenticated users and "different" users that should verify all cases that is regular expression is trying to cover. As part of this, I also added the question mark that somehow went missing.

mikelehen · 2018-03-26T17:07:45Z

packages/firestore/src/local/shared_client_state.ts

  start(initialUser: User, knownClients: ClientKey[]): void {
    assert(!this.started, 'WebStorageSharedClientState already started');
    assert(
-      this.sharedClientDelegate !== null,
+      this.syncEngine !== null,
      'Start() called before subscribing to events'


"subscribing to events" is no longer accurate (especially with the getActiveClients change)

This was moved to handleMutationBatchEvent and reads: 'syncEngine property must be set in order to handle events'

mikelehen · 2018-03-26T17:14:30Z

packages/firestore/src/local/shared_client_state.ts

@@ -497,26 +498,27 @@ export class WebStorageSharedClientState implements SharedClientState {
      );

      if (event.newValue == null) {


minor, but I think this code would read better if you branch based on the key before inspecting the value. This probably means you'll need to add a newValue != null check in your mutationBatchKeyRe branch, but I think that will actually be clearer / more explicit.

Reworked based on your suggestion.

mikelehen · 2018-03-26T17:19:18Z

packages/firestore/src/local/shared_client_state.ts

-   * Parses a client state in LocalStorage. Returns null if the key does not
-   * match the expected key format.
+   * Parses a client state in LocalStorage. Returns 'null' if the value could
+   * not be parsed.


It looks like this never returns null. Update comment and return type?

This returns null as an error condition if the data read from WebStorage is invalid.

mikelehen · 2018-03-26T17:19:37Z

packages/firestore/src/local/shared_client_state.ts

-   * Parses a mutation batch state in LocalStorage. Returns null if the key does
-   * not match the expected key format.
+   * Parses a mutation batch state in LocalStorage. Returns 'null' if the value
+   * could not be parsed.


Ditto. Never returns null now.

This returns null as an error condition if the data read from WebStorage is invalid.

mikelehen · 2018-03-26T17:20:28Z

packages/firestore/src/local/shared_client_state_syncer.ts

@@ -18,24 +18,22 @@ import { BatchId } from '../core/types';
 import { FirestoreError } from '../util/error';

 /**
- * Callback methods invoked by SharedClientState for notifications received
- * from Local Storage.
+ * A interface that describes the actions the SharedClientState class needs to


"An interface"

Fixed here and in RemoteSyncer.

mikelehen

LGTM. Thanks!

schmidt-sebastian requested review from mikelehen and wilhuff as code owners March 21, 2018 21:19

google-oss-bot added the needs-triage label Mar 21, 2018

schmidt-sebastian force-pushed the multitab-mutationnotifications branch from fc0e813 to 83654e9 Compare March 21, 2018 21:38

schmidt-sebastian requested a review from jshcrowthe as a code owner March 21, 2018 21:38

schmidt-sebastian force-pushed the multitab-mutationnotifications branch from 83654e9 to 8c20d85 Compare March 21, 2018 21:48

Persisting mutation batch state in LocalStorage

756eb47

schmidt-sebastian force-pushed the multitab-mutationnotifications branch from 8c20d85 to 756eb47 Compare March 21, 2018 21:50

schmidt-sebastian assigned mikelehen Mar 21, 2018

mikelehen suggested changes Mar 22, 2018

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Mar 22, 2018

schmidt-sebastian force-pushed the multitab-mutationnotifications branch from 293bd11 to 2ef9c61 Compare March 23, 2018 02:11

Addressing review comments

2d172ff

schmidt-sebastian force-pushed the multitab-mutationnotifications branch from 2ef9c61 to 2d172ff Compare March 23, 2018 02:13

schmidt-sebastian assigned mikelehen and unassigned schmidt-sebastian Mar 23, 2018

schmidt-sebastian added 3 commits March 23, 2018 08:29

Merge branch 'firestore-multi-tab' into multitab-mutationnotifications

1cb975b

Renaming delegate variables

6826e49

[AUTOMATED]: Prettier Code Styling

84420b6

mikelehen suggested changes Mar 26, 2018

View reviewed changes

mikelehen assigned schmidt-sebastian and unassigned mikelehen Mar 26, 2018

Next round of review comments

efd2c2f

schmidt-sebastian force-pushed the multitab-mutationnotifications branch from a2ca679 to efd2c2f Compare March 26, 2018 22:16

schmidt-sebastian assigned mikelehen Mar 26, 2018

mikelehen approved these changes Mar 26, 2018

View reviewed changes

schmidt-sebastian merged commit 2d2ad36 into firestore-multi-tab Mar 26, 2018

jshcrowthe deleted the multitab-mutationnotifications branch April 16, 2018 21:58

firebase locked and limited conversation to collaborators Oct 22, 2019

		@@ -497,26 +498,27 @@ export class WebStorageSharedClientState implements SharedClientState {
		);

		if (event.newValue == null) {

Persisting mutation batch state in LocalStorage #584

Persisting mutation batch state in LocalStorage #584

Uh oh!

Conversation

schmidt-sebastian commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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

schmidt-sebastian commented Mar 21, 2018 •

edited

Loading