Skip to content

ValueUtil changes for FieldValue migration #7991

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 3 commits into from
May 14, 2021
Merged

ValueUtil changes for FieldValue migration #7991

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc4d0f1
ValueUtil changes for FieldValue migration
schmidt-sebastian Apr 28, 2021
3441ee3
Review
schmidt-sebastian May 14, 2021
be5467a
Update value_util.h
schmidt-sebastian May 14, 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
2 changes: 1 addition & 1 deletion Firestore/core/src/model/server_timestamp_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const char kPreviousValueKey[] = "__previous_value__";
const char kServerTimestampSentinel[] = "server_timestamp";

google_firestore_v1_Value EncodeServerTimestamp(
google_protobuf_Timestamp local_write_time,
const Timestamp& local_write_time,
absl::optional<google_firestore_v1_Value> previous_value) {
google_firestore_v1_Value result{};
result.which_value_type = google_firestore_v1_Value_map_value_tag;
Expand Down
7 changes: 3 additions & 4 deletions Firestore/core/src/model/server_timestamp_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#define FIRESTORE_CORE_SRC_MODEL_SERVER_TIMESTAMP_UTIL_H_

#include "Firestore/Protos/nanopb/google/firestore/v1/document.nanopb.h"
#include "Firestore/core/include/firebase/firestore/timestamp.h"
#include "absl/types/optional.h"

namespace firebase {
Expand All @@ -27,11 +28,9 @@ namespace model {
// Utility methods to handle ServerTimestamps, which are stored using special
// sentinel fields in MapValues.

/**
* Encodes the backing data for a server timestamp in a Value proto.
*/
/** Encodes the backing data for a server timestamp in a Value proto. */
google_firestore_v1_Value EncodeServerTimestamp(
google_protobuf_Timestamp local_write_time,
const Timestamp& local_write_time,
absl::optional<google_firestore_v1_Value> previous_value);
/**
* Returns whether the provided value is a field map that contains the
Expand Down
139 changes: 102 additions & 37 deletions Firestore/core/src/model/value_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,13 @@
#include "Firestore/core/src/model/value_util.h"

#include <algorithm>
#include <cmath>
#include <limits>
#include <memory>
#include <vector>

#include "Firestore/core/src/model/database_id.h"
#include "Firestore/core/src/model/document_key.h"
#include "Firestore/core/src/model/server_timestamp_util.h"
#include "Firestore/core/src/nanopb/nanopb_util.h"
#include "Firestore/core/src/util/comparison.h"
Expand Down Expand Up @@ -123,9 +127,10 @@ ComparisonResult CompareBlobs(const google_firestore_v1_Value& left,
? util::ComparisonResultFromInt(cmp)
: util::Compare(left.bytes_value->size, right.bytes_value->size);
} else {
// An empty blob is represented by a nullptr
return util::Compare(left.bytes_value != nullptr,
right.bytes_value != nullptr);
// An empty blob is represented by a nullptr (or an empty byte array)
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: this seems to be a behavior change from below? Can you explain why? Not that I am against..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not meant to be a behavior change but rather a bug fix. When we create an empty bytes array directly with NanoPB, it uses nullptr and does not actually allocate memory. When we receive documents from the backend, these empty byte arrays are represented using a zero-length array, but one with an actual address in memory. We need these two representations to yield the same comparison behavior so our users don't see any behavior change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, thanks.

return util::Compare(
!(left.bytes_value == nullptr || left.bytes_value->size == 0),
!(right.bytes_value == nullptr || right.bytes_value->size == 0));
}
}

Expand Down Expand Up @@ -247,8 +252,8 @@ ComparisonResult Compare(const google_firestore_v1_Value& left,
}
}

bool NumberEquals(const firebase::firestore::google_firestore_v1_Value& left,
const firebase::firestore::google_firestore_v1_Value& right) {
bool NumberEquals(const google_firestore_v1_Value& left,
const google_firestore_v1_Value& right) {
if (left.which_value_type == google_firestore_v1_Value_integer_value_tag &&
right.which_value_type == google_firestore_v1_Value_integer_value_tag) {
return left.integer_value == right.integer_value;
Expand All @@ -261,42 +266,36 @@ bool NumberEquals(const firebase::firestore::google_firestore_v1_Value& left,
return false;
}

bool ArrayEquals(const firebase::firestore::google_firestore_v1_Value& left,
const firebase::firestore::google_firestore_v1_Value& right) {
const google_firestore_v1_ArrayValue& left_array = left.array_value;
const google_firestore_v1_ArrayValue& right_array = right.array_value;

if (left_array.values_count != right_array.values_count) {
bool ArrayEquals(const google_firestore_v1_ArrayValue& left,
const google_firestore_v1_ArrayValue& right) {
if (left.values_count != right.values_count) {
return false;
}

for (size_t i = 0; i < left_array.values_count; ++i) {
if (left_array.values[i] != right_array.values[i]) {
for (size_t i = 0; i < left.values_count; ++i) {
if (left.values[i] != right.values[i]) {
return false;
}
}

return true;
}

bool ObjectEquals(const firebase::firestore::google_firestore_v1_Value& left,
const firebase::firestore::google_firestore_v1_Value& right) {
google_firestore_v1_MapValue left_map = left.map_value;
google_firestore_v1_MapValue right_map = right.map_value;

if (left_map.fields_count != right_map.fields_count) {
bool ObjectEquals(const google_firestore_v1_MapValue& left,
const google_firestore_v1_MapValue& right) {
if (left.fields_count != right.fields_count) {
return false;
}

// Porting Note: MapValues in iOS are always kept in sorted order. We
// therefore do no need to sort them before comparing.
for (size_t i = 0; i < right_map.fields_count; ++i) {
if (nanopb::MakeStringView(left_map.fields[i].key) !=
nanopb::MakeStringView(right_map.fields[i].key)) {
for (size_t i = 0; i < right.fields_count; ++i) {
if (nanopb::MakeStringView(left.fields[i].key) !=
nanopb::MakeStringView(right.fields[i].key)) {
return false;
}

if (left_map.fields[i].value != right_map.fields[i].value) {
if (left.fields[i].value != right.fields[i].value) {
return false;
}
}
Expand Down Expand Up @@ -349,16 +348,21 @@ bool Equals(const google_firestore_v1_Value& lhs,
lhs.geo_point_value.longitude == rhs.geo_point_value.longitude;

case TypeOrder::kArray:
return ArrayEquals(lhs, rhs);
return ArrayEquals(lhs.array_value, rhs.array_value);

case TypeOrder::kMap:
return ObjectEquals(lhs, rhs);
return ObjectEquals(lhs.map_value, rhs.map_value);

default:
HARD_FAIL("Invalid type value: %s", left_type);
}
}

bool Equals(const google_firestore_v1_ArrayValue& lhs,
const google_firestore_v1_ArrayValue& rhs) {
return ArrayEquals(lhs, rhs);
}

std::string CanonifyTimestamp(const google_firestore_v1_Value& value) {
return absl::StrFormat("time(%d,%d)", value.timestamp_value.seconds,
value.timestamp_value.nanos);
Expand All @@ -381,13 +385,11 @@ std::string CanonifyGeoPoint(const google_firestore_v1_Value& value) {
value.geo_point_value.longitude);
}

std::string CanonifyArray(const google_firestore_v1_Value& value) {
const auto& array = value.array_value;

std::string CanonifyArray(const google_firestore_v1_ArrayValue& array_value) {
std::string result = "[";
for (size_t i = 0; i < array.values_count; ++i) {
absl::StrAppend(&result, CanonicalId(array.values[i]));
if (i != array.values_count - 1) {
for (size_t i = 0; i < array_value.values_count; ++i) {
absl::StrAppend(&result, CanonicalId(array_value.values[i]));
if (i != array_value.values_count - 1) {
absl::StrAppend(&result, ",");
}
}
Expand Down Expand Up @@ -444,7 +446,7 @@ std::string CanonicalId(const google_firestore_v1_Value& value) {
return CanonifyGeoPoint(value);

case google_firestore_v1_Value_array_value_tag:
return CanonifyArray(value);
return CanonifyArray(value.array_value);

case google_firestore_v1_Value_map_value_tag: {
return CanonifyObject(value);
Expand All @@ -455,12 +457,61 @@ std::string CanonicalId(const google_firestore_v1_Value& value) {
}
}

google_firestore_v1_Value DeepClone(google_firestore_v1_Value source) {
std::string CanonicalId(const google_firestore_v1_ArrayValue& value) {
return CanonifyArray(value);
}

bool Contains(google_firestore_v1_ArrayValue haystack,
google_firestore_v1_Value needle) {
for (pb_size_t i = 0; i < haystack.values_count; ++i) {
if (Equals(haystack.values[i], needle)) {
return true;
}
}
return false;
}

google_firestore_v1_Value NullValue() {
google_firestore_v1_Value null_value{};
null_value.which_value_type = google_firestore_v1_Value_null_value_tag;
return null_value;
}

bool IsNullValue(const google_firestore_v1_Value& value) {
return value.which_value_type == google_firestore_v1_Value_null_value_tag;
}

google_firestore_v1_Value NaNValue() {
google_firestore_v1_Value nan_value{};
nan_value.which_value_type = google_firestore_v1_Value_double_value_tag;
nan_value.double_value = std::numeric_limits<double>::quiet_NaN();
return nan_value;
}

bool IsNaNValue(const google_firestore_v1_Value& value) {
return value.which_value_type == google_firestore_v1_Value_double_value_tag &&
std::isnan(value.double_value);
}

google_firestore_v1_Value RefValue(const model::DatabaseId& database_id,
const model::DocumentKey& document_key) {
google_firestore_v1_Value result{};
result.which_value_type = google_firestore_v1_Value_reference_value_tag;
result.string_value = nanopb::MakeBytesArray(util::StringFormat(
"projects/%s/databases/%s/documents/%s", database_id.project_id(),
database_id.database_id(), document_key.ToString()));
return result;
}

google_firestore_v1_Value DeepClone(const google_firestore_v1_Value& source) {
google_firestore_v1_Value target = source;
switch (source.which_value_type) {
case google_firestore_v1_Value_string_value_tag:
target.string_value = nanopb::MakeBytesArray(source.string_value->bytes,
source.string_value->size);
target.string_value =
source.string_value
? nanopb::MakeBytesArray(source.string_value->bytes,
source.string_value->size)
: nullptr;
break;

case google_firestore_v1_Value_reference_value_tag:
Expand All @@ -469,8 +520,10 @@ google_firestore_v1_Value DeepClone(google_firestore_v1_Value source) {
break;

case google_firestore_v1_Value_bytes_value_tag:
target.bytes_value = nanopb::MakeBytesArray(source.bytes_value->bytes,
source.bytes_value->size);
target.bytes_value =
source.bytes_value ? nanopb::MakeBytesArray(source.bytes_value->bytes,
source.bytes_value->size)
: nullptr;
break;

case google_firestore_v1_Value_array_value_tag:
Expand Down Expand Up @@ -499,6 +552,18 @@ google_firestore_v1_Value DeepClone(google_firestore_v1_Value source) {
return target;
}

google_firestore_v1_ArrayValue DeepClone(
const google_firestore_v1_ArrayValue& source) {
google_firestore_v1_ArrayValue target = source;
target.values_count = source.values_count;
target.values =
nanopb::MakeArray<google_firestore_v1_Value>(source.values_count);
for (pb_size_t i = 0; i < source.values_count; ++i) {
target.values[i] = DeepClone(source.values[i]);
}
return target;
}

} // namespace model
} // namespace firestore
} // namespace firebase
73 changes: 72 additions & 1 deletion Firestore/core/src/model/value_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@

#include <ostream>
#include <string>
#include <vector>

#include "Firestore/Protos/nanopb/google/firestore/v1/document.nanopb.h"
#include "absl/types/optional.h"

namespace firebase {
namespace firestore {
Expand All @@ -31,6 +33,9 @@ enum class ComparisonResult;

namespace model {

class DocumentKey;
class DatabaseId;

/**
* The order of types in Firestore. This order is based on the backend's
* ordering, but modified to support server timestamps.
Expand Down Expand Up @@ -58,14 +63,70 @@ util::ComparisonResult Compare(const google_firestore_v1_Value& left,
bool Equals(const google_firestore_v1_Value& left,
const google_firestore_v1_Value& right);

bool Equals(const google_firestore_v1_ArrayValue& left,
const google_firestore_v1_ArrayValue& right);

/**
* Generate the canonical ID for the provided field value (as used in Target
* serialization).
*/
std::string CanonicalId(const google_firestore_v1_Value& value);

/**
* Generate the canonical ID for the provided array value (as used in Target
Copy link
Contributor

Choose a reason for hiding this comment

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

"Generates"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* serialization).
*/
std::string CanonicalId(const google_firestore_v1_ArrayValue& value);

/** Returns true if the array value contains the specified element. */
bool Contains(google_firestore_v1_ArrayValue haystack,
google_firestore_v1_Value needle);

/** Returns a null Protobuf value. */
google_firestore_v1_Value NullValue();

/** Returns `true` if `value` is null in its Protobuf representation. */
bool IsNullValue(const google_firestore_v1_Value& value);

/** Returns `NaN` in its Protobuf representation. */
google_firestore_v1_Value NaNValue();

/** Returns `true` if `value` is `NaN` in its Protobuf representation. */
bool IsNaNValue(const google_firestore_v1_Value& value);

/** Returns a Protobuf reference value representing the given location. */
google_firestore_v1_Value RefValue(const DatabaseId& database_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

const DocumentKey& document_key);

/** Creates a copy of the contents of the Value proto. */
google_firestore_v1_Value DeepClone(google_firestore_v1_Value source);
google_firestore_v1_Value DeepClone(const google_firestore_v1_Value& source);

/** Creates a copy of the contents of the ArrayValue proto. */
google_firestore_v1_ArrayValue DeepClone(
const google_firestore_v1_ArrayValue& source);

/** Returns true if `value` is a INTEGER_VALUE. */
inline bool IsInteger(const absl::optional<google_firestore_v1_Value>& value) {
return value &&
value->which_value_type == google_firestore_v1_Value_integer_value_tag;
}

/** Returns true if `value` is a DOUBLE_VALUE. */
inline bool IsDouble(const absl::optional<google_firestore_v1_Value>& value) {
return value &&
value->which_value_type == google_firestore_v1_Value_double_value_tag;
}

/** Returns true if `value` is either a INTEGER_VALUE or a DOUBLE_VALUE. */
inline bool IsNumber(const absl::optional<google_firestore_v1_Value>& value) {
return IsInteger(value) || IsDouble(value);
}

/** Returns true if `value` is an ARRAY_VALUE. */
inline bool IsArray(const absl::optional<google_firestore_v1_Value>& value) {
return value &&
value->which_value_type == google_firestore_v1_Value_array_value_tag;
}

} // namespace model

Expand All @@ -79,6 +140,16 @@ inline bool operator!=(const google_firestore_v1_Value& lhs,
return !model::Equals(lhs, rhs);
}

inline bool operator==(const google_firestore_v1_ArrayValue& lhs,
const google_firestore_v1_ArrayValue& rhs) {
return model::Equals(lhs, rhs);
}

inline bool operator!=(const google_firestore_v1_ArrayValue& lhs,
const google_firestore_v1_ArrayValue& rhs) {
return !model::Equals(lhs, rhs);
}

inline std::ostream& operator<<(std::ostream& out,
const google_firestore_v1_Value& value) {
return out << model::CanonicalId(value);
Expand Down