Merge pull request #392 from firebase/bugfix/database-sync-point-test-fixes

Fixed a number of minor issues with database.

Author: Alex Ames

database/src/desktop/core/indexed_variant.cc

@@ -145,19 +145,18 @@ void IndexedVariant::EnsureIndexed() {
   PruneNulls(&variant_);
 
   for (const auto& entry : variant_.map()) {
+    if (entry.first.is_string() && entry.first.string_value()[0] == '.') {
+      // Do not index pseudo-keys.
+      continue;
+    }
     index_.insert(entry);
   }
 }
 
 IndexedVariant IndexedVariant::UpdateChild(const std::string& key,
                                            const Variant& child) const {
-  // Remove element.
-  Variant result = variant_.is_map() ? variant_ : Variant::EmptyMap();
-  if (child.is_null()) {
-    result.map().erase(key);
-  } else {
-    result.map()[key] = child;
-  }
+  Variant result = variant_;
+  VariantUpdateChild(&result, key, child);
   return IndexedVariant(result, query_params_);
 }
 

database/src/desktop/view/child_change_accumulator.cc

@@ -60,7 +60,8 @@ void TrackChildChange(const Change& change,
     } else if (type == kEventTypeChildRemoved &&
                old_type == kEventTypeChildChanged) {
       // Change from kTypeChildChanged to kTypeChildRemoved => kTypeChildRemoved
-      iter->second = ChildRemovedChange(child_key, old_change.indexed_variant);
+      iter->second =
+          ChildRemovedChange(child_key, old_change.old_indexed_variant);
     } else if (type == kEventTypeChildChanged &&
                old_type == kEventTypeChildAdded) {
       // Change from kTypeChildAdded to kTypeChildChanged => kTypeChildAdded

database/src/desktop/view/indexed_filter.cc

@@ -86,6 +86,10 @@ IndexedVariant IndexedFilter::UpdateFullVariant(
     const Variant* new_value = GetVariantValue(&new_snap.variant());
     if (old_value->is_map()) {
       for (auto& key_value_pair : old_value->map()) {
+        if (key_value_pair.first.string_value()[0] == '.') {
+          // Do not create events for pseudo-keys.
+          continue;
+        }
         const Variant& key = key_value_pair.first;
         const Variant& value = key_value_pair.second;
         if (!GetInternalVariant(new_value, key)) {
@@ -97,6 +101,10 @@ IndexedVariant IndexedFilter::UpdateFullVariant(
     if (new_value->is_map()) {
       // Check which elements were changed or added.
       for (auto& key_value_pair : new_value->map()) {
+        if (key_value_pair.first.string_value()[0] == '.') {
+          // Do not create events for pseudo-keys.
+          continue;
+        }
         std::string key = key_value_pair.first.string_value();
         const Variant& value = key_value_pair.second;
         const Variant* old_child = GetInternalVariant(old_value, key);

database/src/desktop/view/limited_filter.cc

@@ -121,7 +121,9 @@ IndexedVariant LimitedFilter::UpdatePriority(
   return old_snap;
 }
 
-const VariantFilter* LimitedFilter::GetIndexedFilter() const { return this; }
+const VariantFilter* LimitedFilter::GetIndexedFilter() const {
+  return ranged_filter_->GetIndexedFilter();
+}
 
 bool LimitedFilter::FiltersVariants() const { return true; }
 

database/src/desktop/view/ranged_filter.cc

@@ -76,7 +76,9 @@ IndexedVariant RangedFilter::UpdatePriority(const IndexedVariant& old_snap,
   return old_snap;
 }
 
-const VariantFilter* RangedFilter::GetIndexedFilter() const { return this; }
+const VariantFilter* RangedFilter::GetIndexedFilter() const {
+  return indexed_filter_.get();
+}
 
 bool RangedFilter::FiltersVariants() const { return true; }
 

database/src/desktop/view/view.cc

@@ -152,7 +152,7 @@ std::vector<Event> View::ApplyOperation(
     const Variant* opt_complete_server_cache,
     std::vector<Change>* out_changes) {
   if (operation.type == Operation::kTypeMerge &&
-      !operation.source.query_params.has_value()) {
+      operation.source.query_params.has_value()) {
     FIREBASE_DEV_ASSERT_MESSAGE(
         view_cache_.GetCompleteServerSnap() != nullptr,
         "We should always have a full cache before handling merges");

database/src/desktop/view/view_processor.cc

@@ -384,9 +384,8 @@ ViewCache ViewProcessor::ApplyServerOverwrite(
     ChildChangeAccumulator* accumulator) {
   CacheNode old_server_snap = old_view_cache.server_snap();
   IndexedVariant new_server_cache;
-  IndexedFilter default_filter((QueryParams()));
-  VariantFilter* server_filter =
-      filter_server_node ? filter_.get() : &default_filter;
+  const VariantFilter* server_filter =
+      filter_server_node ? filter_.get() : filter_->GetIndexedFilter();
 
   if (change_path.empty()) {
     // If the path is empty, we can just apply the overwrite directly.
@@ -420,8 +419,8 @@ ViewCache ViewProcessor::ApplyServerOverwrite(
     VariantUpdateChild(&new_child_node, child_change_path, changed_snap);
     if (IsPriorityKey(child_key.str())) {
       // If this is a priority node, update the priority on the indexed node.
-      new_server_cache =
-          server_filter->UpdatePriority(new_server_cache, new_child_node);
+      new_server_cache = server_filter->UpdatePriority(
+          old_server_snap.indexed_variant(), new_child_node);
     } else {
       // If this is a regular update, the update through the filter to make sure
       // we get only the values that are not filtered by the query spec.

database/tests/desktop/core/sync_point_spec_test.cc

@@ -160,7 +160,6 @@ class TestEventRegistration : public EventRegistration {
   }
 
   bool MatchesListener(const void* listener_ptr) const override {
-    EXPECT_TRUE(false) << "Can't raise test events!";
     return static_cast<const void*>(this) == listener_ptr;
   }
 };
@@ -257,7 +256,8 @@ void SyncTreeTest::RunTest(const test_data::TestCase* test_spec,
         EventRegistration* event_registration = nullptr;
         int callback_id = spec->callbackId();
         if (callback_id != 0 && registrations.count(callback_id) != 0) {
-          event_registration = registrations[callback_id];
+          event_registration = new TestEventRegistration(
+              registrations[callback_id]->query_spec());
         } else {
           event_registration = new TestEventRegistration(query->query_spec());
           if (callback_id != 0) {
@@ -296,8 +296,9 @@ void SyncTreeTest::RunTest(const test_data::TestCase* test_spec,
         break;
       }
       case test_data::StepType_serverMerge: {
-        std::map<Path, Variant> merges =
-            VariantToPathMap(FlexbufferToVariant(spec->data_flexbuffer_root()));
+        std::map<Path, Variant> merges = VariantToPathMap(
+            spec->data() ? FlexbufferToVariant(spec->data_flexbuffer_root())
+                         : Variant::Null());
         std::vector<Event> actual;
         if (spec->tag()) {
           actual =
@@ -309,7 +310,9 @@ void SyncTreeTest::RunTest(const test_data::TestCase* test_spec,
         break;
       }
       case test_data::StepType_set: {
-        Variant to_set = FlexbufferToVariant(spec->data_flexbuffer_root());
+        Variant to_set = spec->data()
+                             ? FlexbufferToVariant(spec->data_flexbuffer_root())
+                             : Variant::Null();
         OverwriteVisibility visible =
             spec->visible() ? kOverwriteVisible : kOverwriteInvisible;
         // For now, assume anything visible should be persisted.
@@ -321,7 +324,8 @@ void SyncTreeTest::RunTest(const test_data::TestCase* test_spec,
       }
       case test_data::StepType_update: {
         CompoundWrite merges = CompoundWrite::FromVariantMerge(
-            FlexbufferToVariant(spec->data_flexbuffer_root()));
+            spec->data() ? FlexbufferToVariant(spec->data_flexbuffer_root())
+                         : Variant::Null());
         std::vector<Event> actual = sync_tree_->ApplyUserMerge(
             path, merges, merges, current_write_id++, kPersist);
         EXPECT_THAT(actual, UnorderedPointwise(EventEq(), expected));
@@ -420,12 +424,10 @@ TEST_F(SyncTreeTest, AQueryCanGetACompleteCacheThenAMerge) {
 }
 
 TEST_F(SyncTreeTest, ServerMergeOnListenerWithCompleteChildren) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("Server merge on listener with complete children");
 }
 
 TEST_F(SyncTreeTest, DeepMergeOnListenerWithCompleteChildren) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("Deep merge on listener with complete children");
 }
 
@@ -490,7 +492,6 @@ TEST_F(SyncTreeTest, RevertDeepSetWithNoServerData) {
 }
 
 TEST_F(SyncTreeTest, RevertSetCoveredByNonvisibleTransaction) {
-  GTEST_SKIP();  // Fails expectations.
   RunOne("Revert set covered by non-visible transaction");
 }
 
@@ -511,17 +512,14 @@ TEST_F(SyncTreeTest, CanSetAlongsideARemoteMerge) {
 }
 
 TEST_F(SyncTreeTest, SetPriorityOnALocationWithNoCache) {
-  // GTEST_SKIP();
   RunOne("setPriority on a location with no cache");
 }
 
 TEST_F(SyncTreeTest, DeepUpdateDeletesChildFromLimitWindowAndPullsInNewChild) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("deep update deletes child from limit window and pulls in new child");
 }
 
 TEST_F(SyncTreeTest, DeepSetDeletesChildFromLimitWindowAndPullsInNewChild) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("deep set deletes child from limit window and pulls in new child");
 }
 
@@ -534,7 +532,6 @@ TEST_F(SyncTreeTest, RevertSetInQueryWindow) {
 }
 
 TEST_F(SyncTreeTest, HandlesAServerValueMovingAChildOutOfAQueryWindow) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("Handles a server value moving a child out of a query window");
 }
 
@@ -551,12 +548,10 @@ TEST_F(SyncTreeTest, LimitIsRefilledFromServerDataAfterMerge) {
 }
 
 TEST_F(SyncTreeTest, HandleRepeatedListenWithMergeAsFirstUpdate) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("Handle repeated listen with merge as first update");
 }
 
 TEST_F(SyncTreeTest, LimitIsRefilledFromServerDataAfterSet) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("Limit is refilled from server data after set");
 }
 
@@ -615,7 +610,6 @@ TEST_F(SyncTreeTest, WriteLeafNodeOverwriteAtParentNode) {
 }
 
 TEST_F(SyncTreeTest, ConfirmCompleteChildrenFromTheServer) {
-  GTEST_SKIP();  // Fails expectations.
   RunOne("Confirm complete children from the server");
 }
 
@@ -634,7 +628,6 @@ TEST_F(SyncTreeTest, BasicKeyIndexSanityCheck) {
 }
 
 TEST_F(SyncTreeTest, CollectCorrectSubviewsToListenOn) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("Collect correct subviews to listen on");
 }
 
@@ -663,7 +656,6 @@ TEST_F(SyncTreeTest, ServerDataIsNotPurgedForNonServerIndexedQueries) {
 }
 
 TEST_F(SyncTreeTest, LimitWithCustomOrderByIsRefilledWithCorrectItem) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("Limit with custom orderBy is refilled with correct item");
 }
 
@@ -680,7 +672,6 @@ TEST_F(SyncTreeTest, LimitedQueryDoesntPullInOutOfRangeChild) {
 }
 
 TEST_F(SyncTreeTest, MergerForLocationWithDefaultAndLimitedListener) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("Merge for location with default and limited listener");
 }
 
@@ -716,7 +707,6 @@ TEST_F(SyncTreeTest, UserWriteWithDeepOverwrite) {
 TEST_F(SyncTreeTest, DeepServerMerge) { RunOne("Deep server merge"); }
 
 TEST_F(SyncTreeTest, ServerUpdatesPriority) {
-  GTEST_SKIP();  // Fails expectations.
   RunOne("Server updates priority");
 }
 
@@ -729,27 +719,22 @@ TEST_F(SyncTreeTest, UserChildOverwriteForNonexistentServerNode) {
 }
 
 TEST_F(SyncTreeTest, RevertUserOverwriteOfChildOnLeafNode) {
-  GTEST_SKIP();  // Fails expectations.
   RunOne("Revert user overwrite of child on leaf node");
 }
 
 TEST_F(SyncTreeTest, ServerOverwriteWithDeepUserDelete) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("Server overwrite with deep user delete");
 }
 
 TEST_F(SyncTreeTest, UserOverwritesLeafNodeWithPriority) {
-  GTEST_SKIP();  // Fails expectations.
   RunOne("User overwrites leaf node with priority");
 }
 
 TEST_F(SyncTreeTest, UserOverwritesInheritPriorityValuesFromLeafNodes) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("User overwrites inherit priority values from leaf nodes");
 }
 
 TEST_F(SyncTreeTest, UserUpdateOnUserSetLeafNodeWithPriorityAfterServerUpdate) {
-  GTEST_SKIP();  // Fails expectations.
   RunOne("User update on user set leaf node with priority after server update");
 }
 
@@ -762,32 +747,26 @@ TEST_F(SyncTreeTest, UserSetsRootPriority) {
 }
 
 TEST_F(SyncTreeTest, UserUpdatesPriorityOnEmptyRoot) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("User updates priority on empty root");
 }
 
 TEST_F(SyncTreeTest, RevertSetAtRootWithPriority) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("Revert set at root with priority");
 }
 
 TEST_F(SyncTreeTest, ServerUpdatesPriorityAfterUserSetsPriority) {
-  GTEST_SKIP();  // Fails expectations.
   RunOne("Server updates priority after user sets priority");
 }
 
 TEST_F(SyncTreeTest, EmptySetDoesntPreventServerUpdates) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("Empty set doesn't prevent server updates");
 }
 
 TEST_F(SyncTreeTest, UserUpdatesPriorityTwiceFirstIsReverted) {
-  GTEST_SKIP();  // Fails expectations.
   RunOne("User updates priority twice, first is reverted");
 }
 
 TEST_F(SyncTreeTest, ServerAcksRootPrioritySetAfterUserDeletesRootNode) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("Server acks root priority set after user deletes root node");
 }
 
@@ -796,7 +775,6 @@ TEST_F(SyncTreeTest, ADeleteInAMergeDoesntPushOutNodes) {
 }
 
 TEST_F(SyncTreeTest, ATaggedQueryFiresEventsEventually) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("A tagged query fires events eventually");
 }
 
@@ -817,13 +795,11 @@ TEST_F(SyncTreeTest, ClearParentShadowingServerValuesMergeWithServerChildren) {
 }
 
 TEST_F(SyncTreeTest, PrioritiesDontMakeMeSick) {
-  GTEST_SKIP();  // Fails assert.
   RunOne("Priorities don't make me sick");
 }
 
 TEST_F(SyncTreeTest,
        MergeThatMovesChildFromWindowToBoundaryDoesNotCauseChildToBeReadded) {
-  GTEST_SKIP();  // Fails expectations.
   RunOne(
       "Merge that moves child from window to boundary does not cause child "
       "to be readded");
@@ -865,7 +841,6 @@ TEST_F(SyncTreeTest, UnrelatedAckedUpdateIsNotCachedInTaggedListen) {
 }
 
 TEST_F(SyncTreeTest, DeepUpdateRaisesImmediateEventsOnlyIfHasCompleteData) {
-  GTEST_SKIP();  // Fails expectations.
   RunOne("Deep update raises immediate events only if has complete data");
 }
 

database/tests/desktop/view/child_change_accumulator_test.cc

@@ -121,15 +121,15 @@ TEST(ChildChangeAccumulator, TrackChildChangeAddedThenRemoved) {
 // in sequence.
 TEST(ChildChangeAccumulator, TrackChildChangeChangedThenRemoved) {
   ChildChangeAccumulator accumulator;
-  TrackChildChange(ChildChangedChange("ChildChangeThenRemove", "old", "order"),
+  TrackChildChange(ChildChangedChange("ChildChangeThenRemove", "old", "older"),
                    &accumulator);
   // Note: the removed value "new" does not need to match the value "old"
   //       changed previously.
   TrackChildChange(ChildRemovedChange("ChildChangeThenRemove", "new"),
                    &accumulator);
 
   // Expected result should be a ChildRemoved change from "old" value
-  Change expected = ChildRemovedChange("ChildChangeThenRemove", "old");
+  Change expected = ChildRemovedChange("ChildChangeThenRemove", "older");
 
   auto it = accumulator.find("ChildChangeThenRemove");
   ASSERT_NE(it, accumulator.end());

database/tests/desktop/view/indexed_filter_test.cc

@@ -175,7 +175,7 @@ TEST(IndexedFilter, UpdateChild_RemovedValue) {
   CompleteChildSource* source = nullptr;
   ChildChangeAccumulator change_accumulator;
 
-  IndexedVariant expected_result(Variant::EmptyMap());
+  IndexedVariant expected_result(Variant::Null());
   ChildChangeAccumulator expected_changes{
       std::make_pair(
           "aaa",