ObjectValue changes (#8007)

Author: schmidt-sebastian

Firestore/core/src/model/object_value.cc

@@ -24,19 +24,24 @@
 #include "Firestore/core/src/nanopb/fields_array.h"
 #include "Firestore/core/src/nanopb/message.h"
 #include "Firestore/core/src/nanopb/nanopb_util.h"
+#include "Firestore/core/src/util/hashing.h"
+#include "absl/types/span.h"
 
 namespace firebase {
 namespace firestore {
 namespace model {
 
 namespace {
 
-using nanopb::FieldsArray;
+using nanopb::CheckedSize;
 using nanopb::FreeFieldsArray;
+using nanopb::FreeNanopbMessage;
 using nanopb::MakeArray;
 using nanopb::MakeBytesArray;
 using nanopb::MakeString;
 using nanopb::MakeStringView;
+using nanopb::ReleaseFieldOwnership;
+using nanopb::SetRepeatedField;
 
 struct MapEntryKeyCompare {
   bool operator()(const google_firestore_v1_MapValue_FieldsEntry& entry,
@@ -49,6 +54,28 @@ struct MapEntryKeyCompare {
   }
 };
 
+/** Traverses a Value proto and sorts all MapValues by key. */
+void SortFields(google_firestore_v1_Value& value) {
+  if (value.which_value_type == google_firestore_v1_Value_map_value_tag) {
+    google_firestore_v1_MapValue& map_value = value.map_value;
+    std::sort(map_value.fields, map_value.fields + map_value.fields_count,
+              [](const google_firestore_v1_MapValue_FieldsEntry& lhs,
+                 const google_firestore_v1_MapValue_FieldsEntry& rhs) {
+                return nanopb::MakeStringView(lhs.key) <
+                       nanopb::MakeStringView(rhs.key);
+              });
+
+    for (pb_size_t i = 0; i < map_value.fields_count; ++i) {
+      SortFields(map_value.fields[i].value);
+    }
+  } else if (value.which_value_type ==
+             google_firestore_v1_Value_array_value_tag) {
+    for (pb_size_t i = 0; i < value.array_value.values_count; ++i) {
+      SortFields(value.array_value.values[i]);
+    }
+  }
+}
+
 /**
  * Finds an entry by key in the provided map value. Returns `nullptr` if the
  * entry does not exist.
@@ -135,6 +162,7 @@ void ApplyChanges(
 
         target_entry.key = source_entry.key;
         target_entry.value = DeepClone(upsert_it->second);
+        SortFields(target_entry.value);
 
         ++upsert_it;
         ++source_index;
@@ -162,22 +190,57 @@ void ApplyChanges(
 
   free(parent->fields);
   parent->fields = target_fields;
-  parent->fields_count = target_count;
+  parent->fields_count = CheckedSize(target_count);
 }
 
 }  // namespace
 
-MutableObjectValue::MutableObjectValue() {
+ObjectValue::ObjectValue() {
   value_->which_value_type = google_firestore_v1_Value_map_value_tag;
   value_->map_value.fields_count = 0;
   value_->map_value.fields = nullptr;
 }
 
-FieldMask MutableObjectValue::ToFieldMask() const {
+ObjectValue::ObjectValue(const google_firestore_v1_Value& value)
+    : value_(value) {
+  HARD_ASSERT(value.which_value_type == google_firestore_v1_Value_map_value_tag,
+              "ObjectValues should be backed by a MapValue");
+  SortFields(*value_);
+}
+
+ObjectValue::ObjectValue(const ObjectValue& other)
+    : value_(DeepClone(*other.value_)) {
+}
+
+ObjectValue ObjectValue::FromMapValue(google_firestore_v1_MapValue map_value) {
+  google_firestore_v1_Value value{};
+  value.which_value_type = google_firestore_v1_Value_map_value_tag;
+  value.map_value = map_value;
+  return ObjectValue{value};
+}
+
+ObjectValue ObjectValue::FromFieldsEntry(
+    google_firestore_v1_Document_FieldsEntry* fields_entry, pb_size_t count) {
+  google_firestore_v1_Value value{};
+  value.which_value_type = google_firestore_v1_Value_map_value_tag;
+  SetRepeatedField(
+      &value.map_value.fields, &value.map_value.fields_count,
+      absl::Span<google_firestore_v1_Document_FieldsEntry>(fields_entry, count),
+      [](const google_firestore_v1_Document_FieldsEntry& entry) {
+        google_firestore_v1_MapValue_FieldsEntry result{};
+        result.key = entry.key;
+        result.value = entry.value;
+        return result;
+      });
+  ReleaseFieldOwnership(fields_entry, count);
+  return ObjectValue{value};
+}
+
+FieldMask ObjectValue::ToFieldMask() const {
   return ExtractFieldMask(value_->map_value);
 }
 
-FieldMask MutableObjectValue::ExtractFieldMask(
+FieldMask ObjectValue::ExtractFieldMask(
     const google_firestore_v1_MapValue& value) const {
   std::set<FieldPath> fields;
 
@@ -206,7 +269,7 @@ FieldMask MutableObjectValue::ExtractFieldMask(
   return FieldMask(std::move(fields));
 }
 
-absl::optional<google_firestore_v1_Value> MutableObjectValue::Get(
+absl::optional<google_firestore_v1_Value> ObjectValue::Get(
     const FieldPath& path) const {
   if (path.empty()) {
     return *value_;
@@ -222,8 +285,12 @@ absl::optional<google_firestore_v1_Value> MutableObjectValue::Get(
   return nested_value;
 }
 
-void MutableObjectValue::Set(const FieldPath& path,
-                             const google_firestore_v1_Value& value) {
+google_firestore_v1_Value ObjectValue::Get() const {
+  return *value_;
+}
+
+void ObjectValue::Set(const FieldPath& path,
+                      const google_firestore_v1_Value& value) {
   HARD_ASSERT(!path.empty(), "Cannot set field for empty path on ObjectValue");
 
   google_firestore_v1_MapValue* parent_map = ParentMap(path.PopLast());
@@ -235,8 +302,7 @@ void MutableObjectValue::Set(const FieldPath& path,
   ApplyChanges(parent_map, upserts, /*deletes=*/{});
 }
 
-void MutableObjectValue::SetAll(const FieldMask& field_mask,
-                                const MutableObjectValue& data) {
+void ObjectValue::SetAll(const FieldMask& field_mask, const ObjectValue& data) {
   FieldPath parent;
 
   std::map<std::string, google_firestore_v1_Value> upserts;
@@ -264,7 +330,7 @@ void MutableObjectValue::SetAll(const FieldMask& field_mask,
   ApplyChanges(parent_map, upserts, deletes);
 }
 
-void MutableObjectValue::Delete(const FieldPath& path) {
+void ObjectValue::Delete(const FieldPath& path) {
   HARD_ASSERT(!path.empty(), "Cannot delete field with empty path");
 
   google_firestore_v1_Value* nested_value = value_.get();
@@ -286,12 +352,19 @@ void MutableObjectValue::Delete(const FieldPath& path) {
   }
 }
 
+std::string ObjectValue::ToString() const {
+  return CanonicalId(*value_);
+}
+
+size_t ObjectValue::Hash() const {
+  return util::Hash(CanonicalId(*value_));
+}
+
 /**
  * Returns the map that contains the leaf element of `path`. If the parent
  * entry does not yet exist, or if it is not a map, a new map will be created.
  */
-google_firestore_v1_MapValue* MutableObjectValue::ParentMap(
-    const FieldPath& path) {
+google_firestore_v1_MapValue* ObjectValue::ParentMap(const FieldPath& path) {
   google_firestore_v1_Value* parent = value_.get();
 
   // Find a or create a parent map entry for `path`.

Firestore/core/src/model/object_value.h

@@ -37,26 +37,33 @@ namespace firestore {
 namespace model {
 
 /** A structured object value stored in Firestore. */
-// TODO(mutabledocuments): Rename to ObjectValue once other ObjectValue class
-// is removed
-class MutableObjectValue {
+class ObjectValue {
  public:
-  MutableObjectValue();
+  ObjectValue();
 
   /** Creates a new MutableObjectValue and takes ownership of `value`. */
-  explicit MutableObjectValue(const google_firestore_v1_Value& value)
-      : value_(value) {
-    HARD_ASSERT(
-        value.which_value_type == google_firestore_v1_Value_map_value_tag,
-        "ObjectValues should be backed by a MapValue");
-  }
+  explicit ObjectValue(const google_firestore_v1_Value& value);
 
-  MutableObjectValue(MutableObjectValue&& other) noexcept = default;
-  MutableObjectValue& operator=(MutableObjectValue&& other) = default;
+  ObjectValue(ObjectValue&& other) noexcept = default;
+  ObjectValue& operator=(ObjectValue&& other) noexcept = default;
+  ObjectValue(const ObjectValue& other);
 
-  /** `MutableObjectValue` models unique ownership. */
-  MutableObjectValue(const MutableObjectValue&) = delete;
-  MutableObjectValue& operator=(const MutableObjectValue&) = delete;
+  ObjectValue& operator=(const ObjectValue&) = delete;
+
+  /**
+   * Creates a new ObjectValue that is backed by the given `map_value`.
+   * ObjectValue takes on ownership of the data.
+   */
+  static ObjectValue FromMapValue(google_firestore_v1_MapValue map_value);
+
+  /**
+   * Creates a new ObjectValue that is backed by the provided document fields.
+   * ObjectValue takes on ownership of the data and zeroes out the pointers in
+   * `fields_entry`. This allows the callsite to destruct the Document proto
+   * without affecting the fields data.
+   */
+  static ObjectValue FromFieldsEntry(
+      google_firestore_v1_Document_FieldsEntry* fields_entry, pb_size_t count);
 
   /** Recursively extracts the FieldPaths that are set in this ObjectValue. */
   FieldMask ToFieldMask() const;
@@ -69,6 +76,11 @@ class MutableObjectValue {
    */
   absl::optional<google_firestore_v1_Value> Get(const FieldPath& path) const;
 
+  /**
+   * Returns the ObjectValue in its Protobuf representation.
+   */
+  google_firestore_v1_Value Get() const;
+
   /**
    * Sets the field to the provided value.
    *
@@ -85,7 +97,7 @@ class MutableObjectValue {
    * @param field_mask The field mask that controls which fields to modify.
    * @param data A MutableObjectValue that contains the field values.
    */
-  void SetAll(const FieldMask& field_mask, const MutableObjectValue& data);
+  void SetAll(const FieldMask& field_mask, const ObjectValue& data);
 
   /**
    * Removes the field at the specified path. If there is no field at the
@@ -95,10 +107,13 @@ class MutableObjectValue {
    */
   void Delete(const FieldPath& path);
 
-  friend bool operator==(const MutableObjectValue& lhs,
-                         const MutableObjectValue& rhs);
+  std::string ToString() const;
+
+  size_t Hash() const;
+
+  friend bool operator==(const ObjectValue& lhs, const ObjectValue& rhs);
   friend std::ostream& operator<<(std::ostream& out,
-                                  const MutableObjectValue& object_value);
+                                  const ObjectValue& object_value);
 
  private:
   /** Returns the field mask for the provided map value. */
@@ -113,13 +128,12 @@ class MutableObjectValue {
   nanopb::Message<google_firestore_v1_Value> value_;
 };
 
-inline bool operator==(const MutableObjectValue& lhs,
-                       const MutableObjectValue& rhs) {
+inline bool operator==(const ObjectValue& lhs, const ObjectValue& rhs) {
   return *lhs.value_ == *rhs.value_;
 }
 
 inline std::ostream& operator<<(std::ostream& out,
-                                const MutableObjectValue& object_value) {
+                                const ObjectValue& object_value) {
   return out << "ObjectValue(" << *object_value.value_ << ")";
 }