Added VariantGetChild and VariantUpdateChild. These functions more closely

replicate the behavior of Node.GetChild and Node.UpdateChild in the Android
implementation to ensure that the behavior is the same as the mobile SDKs.

Also went through and updated a large number of places that were using
Set/GetInternalVariant or MakeVariantAtPath in a more ad hoc way to simulate
the logic in VariantGetChild or VariantUpdateChild.

Finally, fixed a number of broken or incorrect tests.

PiperOrigin-RevId: 249716604

Author: amablue

database/src/desktop/core/child_event_registration.cc

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 #include "database/src/desktop/core/child_event_registration.h"
+
 #include "database/src/desktop/data_snapshot_desktop.h"
 #include "database/src/desktop/view/event.h"
 #include "database/src/desktop/view/event_type.h"
@@ -34,11 +35,17 @@ bool ChildEventRegistration::RespondsTo(EventType event_type) {
 
 Event ChildEventRegistration::GenerateEvent(const Change& change,
                                             const QuerySpec& query_spec) {
-  return Event(change.event_type, this,
-               DataSnapshotInternal(database_,
-                                    query_spec.path.GetChild(change.child_key),
-                                    change.indexed_variant.variant()),
-               change.prev_name);
+  // return Event(change.event_type, this,
+  //              DataSnapshotInternal(database_,
+  //                                   query_spec.path.GetChild(change.child_key),
+  //                                   change.indexed_variant.variant()),
+  //              change.prev_name);
+  return Event(
+      change.event_type, this,
+      DataSnapshotInternal(database_, change.indexed_variant.variant(),
+                           QuerySpec(query_spec.path.GetChild(change.child_key),
+                                     change.indexed_variant.query_params())),
+      change.prev_name);
 }
 
 void ChildEventRegistration::FireEvent(const Event& event) {

database/src/desktop/core/compound_write.cc

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 #include "database/src/desktop/core/compound_write.h"
+
 #include "app/src/assert.h"
 #include "database/src/desktop/core/tree.h"
 #include "database/src/desktop/util_desktop.h"
@@ -70,19 +71,18 @@ CompoundWrite CompoundWrite::AddWrite(const Path& path,
       // TODO(amablue): Consider making FindRootMostPathWithValue also return
       // the remainder and not just the root most path.
       Optional<Path> relative_path = Path::GetRelative(*root_most_path, path);
-      const Variant* value = write_tree_.GetValueAt(root_most_path.value());
+      const Variant* value = write_tree_.GetValueAt(*root_most_path);
       std::vector<std::string> directories = relative_path->GetDirectories();
       std::string back = directories.empty() ? "" : directories.back();
-      const Variant* internal_variant =
-          GetInternalVariant(value, relative_path->GetParent());
-      if (!relative_path->empty() && back == ".priority" &&
-          (internal_variant == nullptr || internal_variant->is_null())) {
+
+      if (!relative_path->empty() && IsPriorityKey(back) &&
+          VariantIsEmpty(VariantGetChild(value, relative_path->GetParent()))) {
         // Ignore priority updates on empty variants
         return *this;
       } else {
         CompoundWrite result = *this;
         Variant updated_variant = *value;
-        *MakeVariantAtPath(&updated_variant, *relative_path) = *variant;
+        VariantUpdateChild(&updated_variant, *relative_path, *variant);
         result.write_tree_.SetValueAt(*root_most_path, updated_variant);
         return result;
       }
@@ -144,11 +144,9 @@ const Optional<Variant>& CompoundWrite::GetRootWrite() const {
 Optional<Variant> CompoundWrite::GetCompleteVariant(const Path& path) const {
   Optional<Path> root_most = write_tree_.FindRootMostPathWithValue(path);
   if (root_most.has_value()) {
-    const Path& root_most_path = root_most.value();
-    const Variant* root_most_value = write_tree_.GetValueAt(root_most_path);
-    Optional<Path> remaining_path = Path::GetRelative(root_most_path, path);
-    return OptionalFromPointer(
-        GetInternalVariant(root_most_value, *remaining_path));
+    const Variant* root_most_value = write_tree_.GetValueAt(*root_most);
+    Optional<Path> remaining_path = Path::GetRelative(*root_most, path);
+    return Optional<Variant>(VariantGetChild(root_most_value, *remaining_path));
   } else {
     return Optional<Variant>();
   }
@@ -213,14 +211,13 @@ Variant CompoundWrite::ApplySubtreeWrite(const Path& relative_path,
                                          Variant variant) const {
   if (write_tree->value().has_value()) {
     // Since there a write is always a leaf, we're done here
-    *MakeVariantAtPath(&variant, relative_path) = write_tree->value().value();
-    return variant;
+    VariantUpdateChild(&variant, relative_path, write_tree->value().value());
   } else {
     Optional<Variant> priority_write;
     for (auto& key_tree_pair : write_tree->children()) {
       const std::string& child_key = key_tree_pair.first;
       const Tree<Variant>& child_tree = key_tree_pair.second;
-      if (child_key == ".priority") {
+      if (IsPriorityKey(child_key)) {
         // Apply priorities at the end so we don't update priorities for
         // either empty variants or forget to apply priorities to empty
         // variants that are later filled
@@ -235,13 +232,13 @@ Variant CompoundWrite::ApplySubtreeWrite(const Path& relative_path,
     }
     // If there was a priority write, we only apply it if the variant is not
     // empty.
-    if (GetInternalVariant(&variant, relative_path) != nullptr &&
+    if (!VariantIsEmpty(VariantGetChild(&variant, relative_path)) &&
         priority_write.has_value()) {
-      *MakeVariantAtPath(&variant, relative_path.GetChild(".priority")) =
-          priority_write.value();
+      VariantUpdateChild(&variant, relative_path.GetChild(kPriorityKey),
+                         priority_write.value());
     }
-    return variant;
   }
+  return variant;
 }
 
 }  // namespace internal

database/src/desktop/core/indexed_variant.cc

@@ -95,7 +95,7 @@ const Variant* IndexedVariant::GetOrderByVariant(const Variant& key,
                                                  const Variant& value) {
   switch (query_params_.order_by) {
     case QueryParams::kOrderByPriority: {
-      return GetVariantPriority(value);
+      return &GetVariantPriority(value);
     }
     case QueryParams::kOrderByChild: {
       if (value.is_map()) {
@@ -118,16 +118,14 @@ const Variant* IndexedVariant::GetOrderByVariant(const Variant& key,
 static bool IsDefinedOn(const Variant& variant, const QueryParams& params) {
   switch (params.order_by) {
     case QueryParams::kOrderByPriority: {
-      const Variant* priority = GetVariantPriority(variant);
-      return priority && !VariantIsEmpty(*priority);
+      return !VariantIsEmpty(GetVariantPriority(variant));
     }
     case QueryParams::kOrderByKey: {
       return true;
     }
     case QueryParams::kOrderByChild: {
-      const Variant* child =
-          GetInternalVariant(&variant, Path(params.order_by_child));
-      return child && !VariantIsEmpty(*child);
+      return !VariantIsEmpty(
+          VariantGetChild(&variant, Path(params.order_by_child)));
     }
     case QueryParams::kOrderByValue: {
       return true;

database/src/desktop/core/operation.cc

@@ -13,7 +13,9 @@
 // limitations under the License.
 
 #include "database/src/desktop/core/operation.h"
+
 #include <iostream>
+
 #include "app/src/assert.h"
 #include "app/src/path.h"
 #include "database/src/desktop/util_desktop.h"
@@ -58,11 +60,8 @@ Operation Operation::ListenComplete(const OperationSource& source,
 static Optional<Operation> OperationForChildOverwrite(
     const Operation& op, const std::string& child_key) {
   if (op.path.empty()) {
-    const Variant* child_snapshot_ptr =
-        GetInternalVariant(&op.snapshot, child_key);
     return Optional<Operation>(Operation::Overwrite(
-        op.source, Path(),
-        child_snapshot_ptr ? *child_snapshot_ptr : Variant::Null()));
+        op.source, Path(), VariantGetChild(&op.snapshot, child_key)));
   } else {
     std::vector<std::string> directories = op.path.GetDirectories();
     Path child_path(std::next(directories.begin()), directories.end());

database/src/desktop/core/repo.cc

@@ -810,8 +810,8 @@ void Repo::HandleTransactionResponse(const connection::ResponsePtr& ptr) {
       TransactionDataPtr& transaction = future_to_complete.transaction;
       const Variant& node = future_to_complete.node;
 
-      DataSnapshot snapshot(
-          new DataSnapshotInternal(database_, transaction->path, node));
+      DataSnapshot snapshot(new DataSnapshotInternal(
+          database_, node, QuerySpec(transaction->path)));
       transaction->ref_future->CompleteWithResult(transaction->future_handle,
                                                   kErrorNone, snapshot);
     }
@@ -962,8 +962,8 @@ void Repo::RerunTransactionQueue(const std::vector<TransactionDataPtr>& queue,
     TransactionDataPtr& transaction = future_to_complete.transaction;
     Error& abort_reason = future_to_complete.abort_reason;
     Variant& node = future_to_complete.node;
-    DataSnapshot snapshot(
-        new DataSnapshotInternal(database_, transaction->path, node));
+    DataSnapshot snapshot(new DataSnapshotInternal(
+        database_, node, QuerySpec(transaction->path)));
     transaction->ref_future->CompleteWithResult(transaction->future_handle,
                                                 abort_reason, snapshot);
   }

database/src/desktop/core/server_values.cc

@@ -13,7 +13,9 @@
 // limitations under the License.
 
 #include "database/src/desktop/core/server_values.h"
+
 #include <ctime>
+
 #include "app/src/include/firebase/variant.h"
 #include "database/src/desktop/core/compound_write.h"
 #include "database/src/desktop/util_desktop.h"
@@ -69,12 +71,11 @@ static void ResolveDeferredValueSnapshotHelper(Variant* data,
 
 Variant ResolveDeferredValueSnapshot(const Variant& data,
                                      const Variant& server_values) {
-  const Variant* priority = GetVariantPriority(data);
-  priority = priority ? &ResolveDeferredValue(*priority, server_values)
-                      : &kNullVariant;
+  Variant priority = GetVariantPriority(data);
+  priority = ResolveDeferredValue(priority, server_values);
   Variant new_data = data;
   ResolveDeferredValueSnapshotHelper(&new_data, server_values);
-  return CombineValueAndPriority(new_data, *priority);
+  return CombineValueAndPriority(new_data, priority);
 }
 
 CompoundWrite ResolveDeferredValueMerge(const CompoundWrite& merge,

database/src/desktop/core/sync_tree.cc

@@ -13,7 +13,9 @@
 // limitations under the License.
 
 #include "database/src/desktop/core/sync_tree.h"
+
 #include <vector>
+
 #include "app/memory/unique_ptr.h"
 #include "app/src/assert.h"
 #include "app/src/callback.h"
@@ -172,10 +174,10 @@ std::vector<Event> SyncTree::AddEventRegistration(
         auto& variant = persistent_server_cache.indexed_variant().variant();
         if (variant.is_map()) {
           for (auto& key_value_pair : variant.map()) {
-            Path key(key_value_pair.first.AsString().string_value());
+            const char* key = key_value_pair.first.AsString().string_value();
+            const Variant& value = key_value_pair.second;
             if (GetInternalVariant(&*server_cache_variant, key) == nullptr) {
-              SetVariantAtPath(&*server_cache_variant, key,
-                               key_value_pair.second);
+              VariantUpdateChild(&server_cache_variant.value(), key, value);
             }
           }
         }
@@ -270,7 +272,7 @@ std::vector<Event> SyncTree::ApplyOperationHelper(
     Tree<SyncPoint>* child_tree = sync_point_tree->GetChild(child_key);
     if (child_tree && child_operation.has_value()) {
       const Variant* child_server_cache =
-          server_cache ? GetInternalVariant(server_cache, child_key) : nullptr;
+          server_cache ? &VariantGetChild(server_cache, child_key) : nullptr;
       WriteTreeRef child_writes_cache = writes_cache->Child(child_key);
       events = ApplyOperationHelper(*child_operation, child_tree,
                                     child_server_cache, &child_writes_cache);

database/src/desktop/core/value_event_registration.cc

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 #include "database/src/desktop/core/value_event_registration.h"
+
 #include "database/src/desktop/data_snapshot_desktop.h"
 #include "database/src/include/firebase/database/common.h"
 #include "firebase/database/data_snapshot.h"
@@ -29,10 +30,15 @@ bool ValueEventRegistration::RespondsTo(EventType event_type) {
 
 Event ValueEventRegistration::GenerateEvent(const Change& change,
                                             const QuerySpec& query_spec) {
-  return Event(kEventTypeValue, this,
-               DataSnapshotInternal(database_,
-                                    query_spec.path.GetChild(change.child_key),
-                                    change.indexed_variant.variant()));
+  // return Event(kEventTypeValue, this,
+  //              DataSnapshotInternal(database_,
+  //                                   query_spec.path.GetChild(change.child_key),
+  //                                   change.indexed_variant.variant()));
+  return Event(
+      kEventTypeValue, this,
+      DataSnapshotInternal(database_, change.indexed_variant.variant(),
+                           QuerySpec(query_spec.path.GetChild(change.child_key),
+                                     change.indexed_variant.query_params())));
 }
 
 void ValueEventRegistration::FireEvent(const Event& event) {

database/src/desktop/core/write_tree.cc

@@ -13,7 +13,9 @@
 // limitations under the License.
 
 #include "database/src/desktop/core/write_tree.h"
+
 #include <algorithm>
+
 #include "app/src/assert.h"
 #include "database/src/desktop/core/compound_write.h"
 #include "database/src/desktop/query_params_comparator.h"
@@ -273,8 +275,8 @@ Optional<Variant> WriteTree::CalcEventCacheAfterServerOverwrite(
     CompoundWrite child_merge = visible_writes_.ChildCompoundWrite(path);
     if (child_merge.IsEmpty()) {
       // We're not shadowing at all. Case 1.
-      return OptionalFromPointer(
-          GetInternalVariant(existing_server_snap, child_path));
+      return Optional<Variant>(
+          VariantGetChild(existing_server_snap, child_path));
     } else {
       // This could be more efficient if the server_node + updates doesn't
       // change the local_snap However this is tricky to find out, since user
@@ -284,8 +286,8 @@ Optional<Variant> WriteTree::CalcEventCacheAfterServerOverwrite(
       // therefore not enough to only check if the updates change the
       // server_node. Maybe check if the merge tree contains these special
       // cases and only do a full overwrite in that case?
-      return Optional<Variant>(child_merge.Apply(
-          *GetInternalVariant(existing_server_snap, child_path)));
+      return Optional<Variant>(
+          child_merge.Apply(VariantGetChild(existing_server_snap, child_path)));
     }
   }
 }
@@ -301,11 +303,9 @@ Optional<Variant> WriteTree::CalcCompleteChild(
   } else {
     if (existing_server_snap.IsCompleteForChild(child_key)) {
       CompoundWrite child_merge = visible_writes_.ChildCompoundWrite(path);
-      const Variant* child =
-          GetInternalVariant(&existing_server_snap.variant(), child_key);
-      if (child) {
-        return Optional<Variant>(child_merge.Apply(*child));
-      }
+      const Variant& child =
+          VariantGetChild(&existing_server_snap.variant(), child_key);
+      return Optional<Variant>(child_merge.Apply(child));
     }
     return Optional<Variant>();
   }
@@ -402,8 +402,8 @@ CompoundWrite WriteTree::LayerTree(const std::vector<UserWriteRecord>& writes,
         } else if (write_path.StartsWith(tree_root)) {
           compound_write = compound_write.AddWrite(
               Path(),
-              *GetInternalVariant(&write.overwrite,
-                                  *Path::GetRelative(write_path, tree_root)));
+              VariantGetChild(&write.overwrite,
+                              *Path::GetRelative(write_path, tree_root)));
         } else {
           // There is no overlap between root path and write path, ignore
           // write