Refactor Remote Event #1379

schmidt-sebastian · 2018-06-05T20:48:05Z

This is a port of firebase/firebase-js-sdk#784. This PR also ports the test runner changes from firebase/firebase-js-sdk#812.

Spec test updates for this change are in a separate PR #1380 (to keep this PR somewhat sane).
Not that with the spec tests included in this PR the unit tests don't pass. They pass with #1380 merged in.

As this is my first larger PR after the C++ migration, there's likely some very general feedback that I should address before it makes sense to look at all the details.

wilhuff

First pass. I haven't gone through the tests in any detail yet.

wilhuff · 2018-06-06T15:50:27Z

Firestore/Example/Tests/Util/FSTHelpers.h

+ */
+ (instancetype)withEmptyResultAtKey:(DocumentKey)documentKey;
+
+- (instancetype)


init methods should be first.

I removed this initializer altogether.

wilhuff · 2018-06-06T15:50:47Z

Firestore/Example/Tests/Util/FSTHelpers.h

@@ -48,6 +49,9 @@
 @class FSTObjectValue;
 @protocol FSTFilter;

+using firebase::firestore::model::DocumentKey;


using statements shouldn't be in headers.

Make sense. Removed here and everywhere.

wilhuff · 2018-06-06T15:51:54Z

Firestore/Example/Tests/Util/FSTHelpers.mm

+      }];
+}
+
+- (instancetype)


Standard method order is init methods, NSObject other methods.

This has been removed. The other classes that I added already follow this pattern.

wilhuff · 2018-06-06T15:54:56Z

Firestore/Example/Tests/Util/FSTHelpers.h

+ * Creates an FSTTestTargetMetadataProvider that returns the given document for the existing key
+ * callback (via `remoteKeysForTarget`) and marks queries as active (via `queryDataForTarget`).
+ */
+ (instancetype)withSingleResultAtKey:(DocumentKey)documentKey;


This names should reflect the return type as the first part. Probably providerWith... This parallels e.g. [NSString stringWithFormat:].

Also, your comment indicates that it's a lookup "for the existing key". Perhaps this should be ...ForKey?

Renamed this and withEmptyResultAtKey as per your suggestion.

wilhuff · 2018-06-06T16:05:58Z

Firestore/Example/Tests/Util/FSTHelpers.mm

+      initWithRemoteKeysForTargetCallback:^DocumentKeySet(FSTTargetID targetID) {
+        return DocumentKeySet{};
+      }
+      queryDataCallback:^FSTQueryData *(FSTTargetID targetID) {


This doesn't seem like a very rigorous testing mechanism. It basically will create a target for any target id and that target id will unconditionally be a document listen. If the system under test is grossly misbehaving and issuing all kinds of random queries this isn't going to catch it.

Additionally, I'm a little concerned about all the magical callbacks involved here.

A much more straightforward implementation would be a "fixed" metadata provider, where you register which queries are active and their results. In pseudocode:

new fake provider

add listener to query X with results Y (with target id Z? not sure)

repeat for all active queries

Now you can test watch updates for queries that don't register as active and can avoid all this magical callback stuff.

I changed the way I use the FSTTestTargetMetadataProvider in the tests for RemoteEvent and also followed your suggestion to add a setter that can change the internal state. For the most part, the tests still follow the JS pattern and set up everything up front. All tests that only create a single remote event still set up the initial state in the aggregatorWith... helper method.
The test testDocumentUpdate has been updated to modify the existing keys after the first snapshot.

FWIW, I am mostly relying on spec tests to test the interactions between these callbacks and the watch change aggregator.

wilhuff · 2018-06-06T19:20:47Z

Firestore/Source/Remote/FSTRemoteEvent.mm

+ * Adds the provided document to the internal list of document updates and its document key to the
+ * given target's mapping.
+ */
+// Visible for testing.


Shouldn't this comment be in the @interface?

Hmpf. I meant to use this in my tests, but instead I am just using handleDocumentChange with a synthesized FSTDocumentWatchChange in this port. Removed the "visible for testing" comment.

wilhuff · 2018-06-06T19:22:01Z

Firestore/Source/Remote/FSTRemoteEvent.mm

@@ -605,44 +541,91 @@ - (void)recordResponseForTargetID:(FSTBoxedTargetID *)targetID {
 * preventing in preventing race conditions for a target where events arrive but the server hasn't
 * yet acknowledged the intended change in state.
 */
- (BOOL)isActiveTarget:(FSTBoxedTargetID *)targetID {
+- (BOOL)isActiveTarget:(FSTTargetID)targetID {


What's the point of this change? This forces a caller that has a boxed target ID to unbox so it can be reboxed here.

I am calling this method from three places (handleTargetChange, addDocument and removeDocument). Only handleTargetChange uses the boxed target ID. queryDataForActiveTarget uses both the boxed and the unboxed ID, so passing one or the other doesn't make a huge difference (if we ignore the object allocation).

I left this code as is for now.

wilhuff · 2018-06-06T19:22:38Z

Firestore/Source/Remote/FSTRemoteEvent.mm

-  FSTQueryData *queryData = self.listenTargets[targetID];
-  return (queryData && !self.pendingTargetResponses[targetID]) ? queryData : nil;
+- (FSTQueryData *_Nullable)queryDataForActiveTarget:(FSTTargetID)targetID {
+  const auto &targetState = _targetStates.find(targetID);


Again auto targetState.

In general, make the types of the locals match the types of the return type.

wilhuff · 2018-06-06T19:24:11Z

Firestore/Source/Remote/FSTRemoteEvent.mm

+        // document. This resolves the limbo state of the document, removing it from
+        // limboDocumentRefs.
+        //
+        // TODO(dimond): Ideally we would have an explicit lookup query instead resulting in an


We can probably kill this TODO. We're not going to tackle this separately.

Poor Jonny... Removed.

wilhuff · 2018-06-06T19:24:39Z

Firestore/Source/Remote/FSTRemoteEvent.mm

+        // TODO(dimond): Ideally we would have an explicit lookup query instead resulting in an
+        // explicit delete message and we could remove this special logic.
+        FSTDocumentKey *key = [FSTDocumentKey keyWithPath:queryData.query.path];
+        if (self->_pendingDocumentUpdates.find(key) == self->_pendingDocumentUpdates.end() &&


self-> is unnecessary here.

wilhuff · 2018-06-06T19:29:48Z

I think you should create a branch from master to merge this and #1380 into. Once both of these PRs land on that branch, the combination can be merged into master in another PR and we can observe travis passing before actually merging it.

schmidt-sebastian

I addressed all comments. I also changed the base branch of this PR and its friend to mrschmidt-eventaggregator-and-tests

schmidt-sebastian · 2018-06-06T22:54:04Z

Firestore/Example/Tests/Util/FSTHelpers.h

@@ -48,6 +49,9 @@
 @class FSTObjectValue;
 @protocol FSTFilter;

+using firebase::firestore::model::DocumentKey;


Make sense. Removed here and everywhere.

schmidt-sebastian · 2018-06-06T23:44:42Z

Firestore/Source/Remote/FSTRemoteEvent.h

-    (const firebase::firestore::model::DocumentKeySet &)existingKeys;
-
-@end
+using firebase::firestore::model::DocumentKey;


schmidt-sebastian · 2018-06-06T23:45:54Z

Firestore/Source/Remote/FSTRemoteEvent.h

 */
-+ (FSTUpdateMapping *)mappingWithAddedDocuments:(NSArray<FSTDocument *> *)added
-                               removedDocuments:(NSArray<FSTDocument *> *)removed;
+- (FSTQueryData *_Nullable)queryDataForTarget:(FSTBoxedTargetID *)targetID;


Addressed here and in the other method declarations.

schmidt-sebastian · 2018-06-06T23:49:00Z

Firestore/Example/Tests/Util/FSTHelpers.h

+ * Creates an FSTTestTargetMetadataProvider that returns the given document for the existing key
+ * callback (via `remoteKeysForTarget`) and marks queries as active (via `queryDataForTarget`).
+ */
+ (instancetype)withSingleResultAtKey:(DocumentKey)documentKey;


Renamed this and withEmptyResultAtKey as per your suggestion.

schmidt-sebastian · 2018-06-06T23:49:23Z

Firestore/Example/Tests/Util/FSTHelpers.h

+ */
+ (instancetype)withEmptyResultAtKey:(DocumentKey)documentKey;
+
+- (instancetype)


I removed this initializer altogether.

schmidt-sebastian · 2018-06-07T00:25:58Z

Firestore/Source/Remote/FSTRemoteEvent.mm

+ * Adds the provided document to the internal list of document updates and its document key to the
+ * given target's mapping.
+ */
+// Visible for testing.


Hmpf. I meant to use this in my tests, but instead I am just using handleDocumentChange with a synthesized FSTDocumentWatchChange in this port. Removed the "visible for testing" comment.

schmidt-sebastian · 2018-06-07T00:30:29Z

Firestore/Source/Remote/FSTRemoteEvent.mm

@@ -605,44 +541,91 @@ - (void)recordResponseForTargetID:(FSTBoxedTargetID *)targetID {
 * preventing in preventing race conditions for a target where events arrive but the server hasn't
 * yet acknowledged the intended change in state.
 */
- (BOOL)isActiveTarget:(FSTBoxedTargetID *)targetID {
+- (BOOL)isActiveTarget:(FSTTargetID)targetID {


I am calling this method from three places (handleTargetChange, addDocument and removeDocument). Only handleTargetChange uses the boxed target ID. queryDataForActiveTarget uses both the boxed and the unboxed ID, so passing one or the other doesn't make a huge difference (if we ignore the object allocation).

I left this code as is for now.

schmidt-sebastian · 2018-06-07T00:30:58Z

Firestore/Source/Remote/FSTRemoteEvent.mm

+        // document. This resolves the limbo state of the document, removing it from
+        // limboDocumentRefs.
+        //
+        // TODO(dimond): Ideally we would have an explicit lookup query instead resulting in an


Poor Jonny... Removed.

schmidt-sebastian · 2018-06-07T00:31:30Z

Firestore/Source/Remote/FSTRemoteEvent.mm

+        // TODO(dimond): Ideally we would have an explicit lookup query instead resulting in an
+        // explicit delete message and we could remove this special logic.
+        FSTDocumentKey *key = [FSTDocumentKey keyWithPath:queryData.query.path];
+        if (self->_pendingDocumentUpdates.find(key) == self->_pendingDocumentUpdates.end() &&


schmidt-sebastian · 2018-06-07T01:16:05Z

Firestore/Example/Firestore.xcodeproj/xcshareddata/xcschemes/Firestore_FuzzTests_iOS.xcscheme

@@ -5,6 +5,19 @@
   <BuildAction
      parallelizeBuildables = "YES"
      buildImplicitDependencies = "YES">
+      <BuildActionEntries>


Opened #1388 for this.

wilhuff · 2018-06-07T16:11:45Z

Firestore/Source/Remote/FSTRemoteEvent.mm

@@ -154,10 +144,10 @@ - (void)removeDocumentChangeForKey:(const DocumentKey &)documentKey;

 @implementation FSTTargetState {
  /**
-   * The number of pending responses (adds or removes) that we are waiting on. We only consider
+   * The number of outstanding responses (adds or removes) that we are waiting on. We only consider
   * targets active that have no pending responses.


s/pending/outstanding/

wilhuff · 2018-06-07T16:12:11Z

Firestore/Source/Remote/FSTRemoteEvent.mm

@@ -171,22 +161,22 @@ @implementation FSTTargetState {
 - (instancetype)init {
  if (self = [super init]) {
    _resumeToken = [NSData data];
-    _pendingResponses = 0;
+    _outstandingResponses = 0;

    // We initialize to 'true' so that newly-added targets are included in the next RemoteEvent.
    _hasPendingChanges = true;


BOOL values should get YES

wilhuff · 2018-06-07T16:25:15Z

Firestore/Example/Tests/Remote/FSTRemoteEventTests.mm

 - (void)testWillAccumulateDocumentAddedAndRemovedEvents {
-  FSTDocument *doc1 = FSTTestDoc("docs/1", 1, @{ @"value" : @1 }, NO);
-  FSTDocument *doc2 = FSTTestDoc("docs/2", 2, @{ @"value" : @2 }, NO);
+  NSDictionary<FSTBoxedTargetID *, FSTQueryData *> *targetMap =


There's now some pretty subtle interaction in here and it's not entirely clear to me how these pieces fit together.

Could you add some comments about how they're supposed to work? For example, on top of listenForTargets:, what does the resulting map represent? Listens previously established?

Another piece that's missing: what if you omit a target id from listensForTargets: and then create an FSTDocumentWatchChange referring to that target id. I think that would simulate an update to a previously listened-to target, but I'm not entirely sure today and I'll certainly have no idea in 6 months :-).

I renamed the listen and limboListen helpers to queryDataForTarget/queryDataForLimboTarget, which should make it make a bit more obvious what is going on.

If you use a target in the tests that is not included in the targetMap, then FSTTestTargetMetadataProvider will hit an assert telling you that the target is unknown.

I have added a bunch of method comments to the top of this test file and commented the first test case as an example. I hope that clears up some of the confusion.

wilhuff · 2018-06-07T16:34:25Z

Firestore/Example/Tests/Remote/FSTRemoteEventTests.mm

+                                                                                documentKey:doc1.key
+                                                                                   document:doc1];
+
+  FSTWatchTargetChange *markCurrent =


The order here seems mixed up. You've got

new change 1

new change 2

new aggregator

use change 1

use change 2

new change 3

use change 3

I suggest creating all the changes first then pushing them through the aggregator to make it obvious what the sequence of changes is. Alternatively, create and use right away as you've done below.

I have changed all test cases to declare the variables much closer to where they are used. The new "layout" is now "create doc, create change, apply change". That should be cleaner than what I had before.

wilhuff · 2018-06-07T16:41:52Z

Firestore/Example/Tests/Util/FSTHelpers.h

+/**
+ * Creates an FSTTestTargetMetadataProvider that returns the given document for the existing key
+ * callback (via `remoteKeysForTarget`) and additionally marks the given targets as active (via
+ * `queryDataForTarget`).


This comment is helpful but it assumes you understand everything about how the system works. It would be even more helpful if you framed this in higher-level terms of what this represents in a user visible way or in terms of what history of events we're simulating. I have no idea if it's accurate or not, but something like this:

Creates an FSTTestTargetMetadataProvider that behaves as if there's an established listen for each of the given targets, where each target has already seen query results containing just the given documentKey.

Internally this means that the remoteKeysForTarget callback for these targets will return the documentKey and that targets will be returned as active from the queryDataForTarget target.

Similar feedback applies below.

Done (I mostly went with your comment).

wilhuff · 2018-06-07T16:45:55Z

Firestore/Example/Tests/Util/FSTHelpers.mm

+  std::unordered_map<FSTTargetID, FSTQueryData *> _queryData;
+}
+
+ (instancetype)providerWithSingleResultForKey:(firebase::firestore::model::DocumentKey)documentKey


Nit: you can use using declarations in the .mm files.

wilhuff · 2018-06-07T16:51:13Z

Firestore/Source/Remote/FSTRemoteEvent.h

        documentUpdates:
-            (std::map<firebase::firestore::model::DocumentKey, FSTMaybeDocument *>)documentUpdates
+            (std::unordered_map<firebase::firestore::model::DocumentKey,


Optional: consider adding a using alias for this type so you don't have to repeat it throughout.

using DocumentUpdateMap = std::unordered_map< firebase::firestore::model::DocumentKey, FSTMaybeDocument *, firebase::firestore::model::DocumentKeyHash>;

I think this is a good optimization, but I opted not to do this, as this conflicts somewhat with the MaybeDocumentMap. Using the explicit type makes it really clear what is going on here.

wilhuff · 2018-06-07T16:56:45Z

Firestore/Source/Remote/FSTRemoteEvent.mm

+        removedDocuments = removedDocuments.insert(entry.first);
+        break;
+      default:
+        HARD_FAIL("Encountered invalid change type:  %s", entry.second);


Nit: extra spaces in the message.

wilhuff

LGTM

googlebot added the cla: yes label Jun 5, 2018

schmidt-sebastian force-pushed the mrschmidt-eventaggregator branch 2 times, most recently from 35a1a8b to c97b8bf Compare June 5, 2018 20:51

schmidt-sebastian mentioned this pull request Jun 5, 2018

Adding EventAggregator spec tests #1380

Merged

schmidt-sebastian force-pushed the mrschmidt-eventaggregator branch 2 times, most recently from dfe5c51 to 0e45a26 Compare June 5, 2018 22:09

Refactor Remote Event

df59590

schmidt-sebastian force-pushed the mrschmidt-eventaggregator branch from 0e45a26 to df59590 Compare June 5, 2018 22:33

schmidt-sebastian assigned wilhuff Jun 5, 2018

wilhuff reviewed Jun 6, 2018

View reviewed changes

schmidt-sebastian changed the base branch from master to mrschmidt-eventaggregator-and-tests June 6, 2018 22:43

Review comments

9d63bb7

schmidt-sebastian commented Jun 7, 2018

View reviewed changes

wilhuff reviewed Jun 7, 2018

View reviewed changes

Comment/renames

b9ff6c3

wilhuff approved these changes Jun 11, 2018

View reviewed changes

schmidt-sebastian merged commit 55889bb into mrschmidt-eventaggregator-and-tests Jun 11, 2018

schmidt-sebastian mentioned this pull request Jun 11, 2018

Merge: Refactor Remote Event #1396

Merged

schmidt-sebastian deleted the mrschmidt-eventaggregator branch October 15, 2018 17:26

firebase locked and limited conversation to collaborators Nov 1, 2019

Refactor Remote Event #1379

Refactor Remote Event #1379

Conversation

schmidt-sebastian commented Jun 5, 2018 • edited Loading

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

wilhuff commented Jun 6, 2018

schmidt-sebastian 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

schmidt-sebastian Jun 8, 2018 • edited Loading

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

schmidt-sebastian commented Jun 5, 2018 •

edited

Loading

schmidt-sebastian Jun 8, 2018 •

edited

Loading