Skip to content

ObjectValue changes #8007

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 10, 2021
Merged

ObjectValue changes #8007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
128d885
ObjectValue changes
schmidt-sebastian Apr 29, 2021
7d9b183
Cleanup
schmidt-sebastian Apr 29, 2021
e19bb2d
Always sort
schmidt-sebastian Apr 29, 2021
3fd157d
Cleanup
schmidt-sebastian Apr 29, 2021
62237e0
Cleanup
schmidt-sebastian Apr 29, 2021
3a0eb06
Review
schmidt-sebastian May 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 86 additions & 13 deletions Firestore/core/src/model/object_value.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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.
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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_;
Expand All @@ -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());
Expand All @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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`.
Expand Down
58 changes: 36 additions & 22 deletions Firestore/core/src/model/object_value.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this class copy constructible but not copy assignable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would need this operator:

firebase-ios-sdk/Firestore/core/src/nanopb/message.h

Line 87 in a12b78e

Message& operator=(const Message&) = delete;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.


/**
* 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;
Expand All @@ -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.
*
Expand All @@ -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
Expand All @@ -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. */
Expand All @@ -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_ << ")";
}

Expand Down
Loading