Skip to content

Eliminate TypedValue and serialize direct from FieldValue to bytes. #860

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 1 commit into from
Feb 27, 2018
Merged

Eliminate TypedValue and serialize direct from FieldValue to bytes. #860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
95 changes: 13 additions & 82 deletions Firestore/core/src/firebase/firestore/remote/serializer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,33 +102,7 @@ int64_t DecodeInteger(pb_istream_t* stream) {

using firebase::firestore::model::FieldValue;

Serializer::TypedValue Serializer::EncodeFieldValue(
const FieldValue& field_value) {
Serializer::TypedValue proto_value{
field_value.type(), google_firestore_v1beta1_Value_init_default};
switch (field_value.type()) {
case FieldValue::Type::Null:
proto_value.value.null_value = google_protobuf_NullValue_NULL_VALUE;
break;
case FieldValue::Type::Boolean:
if (field_value == FieldValue::TrueValue()) {
proto_value.value.boolean_value = true;
} else {
FIREBASE_DEV_ASSERT(field_value == FieldValue::FalseValue());
proto_value.value.boolean_value = false;
}
break;
case FieldValue::Type::Integer:
proto_value.value.integer_value = field_value.integer_value();
break;
default:
// TODO(rsgowman): implement the other types
abort();
}
return proto_value;
}

void Serializer::EncodeTypedValue(const TypedValue& value,
void Serializer::EncodeFieldValue(const FieldValue& field_value,
std::vector<uint8_t>* out_bytes) {
// TODO(rsgowman): how large should the output buffer be? Do some
// investigation to see if we can get nanopb to tell us how much space it's
Expand All @@ -139,7 +113,7 @@ void Serializer::EncodeTypedValue(const TypedValue& value,
// TODO(rsgowman): some refactoring is in order... but will wait until after a
// non-varint, non-fixed-size (i.e. string) type is present before doing so.
bool status = false;
switch (value.type) {
switch (field_value.type()) {
case FieldValue::Type::Null:
status = pb_encode_tag(&stream, PB_WT_VARINT,
google_firestore_v1beta1_Value_null_value_tag);
Expand All @@ -157,7 +131,12 @@ void Serializer::EncodeTypedValue(const TypedValue& value,
// TODO(rsgowman): figure out error handling
abort();
}
EncodeBool(&stream, value.value.boolean_value);
if (field_value == FieldValue::TrueValue()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not field_value.boolean_value()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it doesn't exist. (Though it should.)

I want to keep it out of this PR (as this is essentially a refactoring) but will add it to a followup: PR #862

Copy link
Contributor

Choose a reason for hiding this comment

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

SG.

Please wait for the M22 freeze to lift before merging. Alternatively, if enough of these are stacking up let's target a post-m22 branch.

EncodeBool(&stream, true);
} else {
FIREBASE_DEV_ASSERT(field_value == FieldValue::FalseValue());
EncodeBool(&stream, false);
}
break;

case FieldValue::Type::Integer:
Expand All @@ -167,7 +146,7 @@ void Serializer::EncodeTypedValue(const TypedValue& value,
// TODO(rsgowman): figure out error handling
abort();
}
EncodeInteger(&stream, value.value.integer_value);
EncodeInteger(&stream, field_value.integer_value());
break;

default:
Expand All @@ -178,23 +157,7 @@ void Serializer::EncodeTypedValue(const TypedValue& value,
out_bytes->insert(out_bytes->end(), buf, buf + stream.bytes_written);
}

FieldValue Serializer::DecodeFieldValue(
const Serializer::TypedValue& value_proto) {
switch (value_proto.type) {
case FieldValue::Type::Null:
return FieldValue::NullValue();
case FieldValue::Type::Boolean:
return FieldValue::BooleanValue(value_proto.value.boolean_value);
case FieldValue::Type::Integer:
return FieldValue::IntegerValue(value_proto.value.integer_value);
default:
// TODO(rsgowman): implement the other types
abort();
}
}

Serializer::TypedValue Serializer::DecodeTypedValue(const uint8_t* bytes,
size_t length) {
FieldValue Serializer::DecodeFieldValue(const uint8_t* bytes, size_t length) {
pb_istream_t stream = pb_istream_from_buffer(bytes, length);
pb_wire_type_t wire_type;
uint32_t tag;
Expand All @@ -205,51 +168,19 @@ Serializer::TypedValue Serializer::DecodeTypedValue(const uint8_t* bytes,
abort();
}

Serializer::TypedValue result{FieldValue::Type::Null,
google_firestore_v1beta1_Value_init_default};
switch (tag) {
case google_firestore_v1beta1_Value_null_value_tag:
result.type = FieldValue::Type::Null;
DecodeNull(&stream);
break;
return FieldValue::NullValue();
case google_firestore_v1beta1_Value_boolean_value_tag:
result.type = FieldValue::Type::Boolean;
result.value.boolean_value = DecodeBool(&stream);
break;
return FieldValue::BooleanValue(DecodeBool(&stream));
case google_firestore_v1beta1_Value_integer_value_tag:
result.type = FieldValue::Type::Integer;
result.value.integer_value = DecodeInteger(&stream);
break;
return FieldValue::IntegerValue(DecodeInteger(&stream));

default:
// TODO(rsgowman): figure out error handling
abort();
}

return result;
}

bool operator==(const Serializer::TypedValue& lhs,
const Serializer::TypedValue& rhs) {
if (lhs.type != rhs.type) {
return false;
}

switch (lhs.type) {
case FieldValue::Type::Null:
FIREBASE_DEV_ASSERT(lhs.value.null_value ==
google_protobuf_NullValue_NULL_VALUE);
FIREBASE_DEV_ASSERT(rhs.value.null_value ==
google_protobuf_NullValue_NULL_VALUE);
return true;
case FieldValue::Type::Boolean:
return lhs.value.boolean_value == rhs.value.boolean_value;
case FieldValue::Type::Integer:
return lhs.value.integer_value == rhs.value.integer_value;
default:
// TODO(rsgowman): implement the other types
abort();
}
}

} // namespace remote
Expand Down
60 changes: 18 additions & 42 deletions Firestore/core/src/firebase/firestore/remote/serializer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,25 +33,17 @@ namespace remote {
* @brief Converts internal model objects to their equivalent protocol buffer
* form, and protocol buffer objects to their equivalent bytes.
*
* Methods starting with "Encode" either convert from a model object to a
* protocol buffer or from a protocol buffer to bytes, and methods starting with
* "Decode" either convert from a protocol buffer to a model object or from
* bytes to a protocol buffer.
*
* Methods starting with "Encode" convert from a model object to a protocol
* buffer (or directly to bytes in cases where the proto uses a 'oneof', due to
* limitations in nanopb), and methods starting with "Decode" convert from a
* protocol buffer to a model object (or from bytes directly to a model
* objects.)
*/
// TODO(rsgowman): Original docs also has this: "Throws an exception if a
// protocol buffer is missing a critical field or has a value we can't
// interpret." Adjust for C++.
class Serializer {
public:
/**
* @brief Wraps (nanopb) google_firestore_v1beta1_Value with type information.
*/
struct TypedValue {
firebase::firestore::model::FieldValue::Type type;
google_firestore_v1beta1_Value value;
};

Serializer() {
}
// TODO(rsgowman): We eventually need the DatabaseId, but can't add it just
Expand All @@ -68,62 +60,46 @@ class Serializer {
//}

/**
* Converts the FieldValue model passed into the Value proto equivalent.
* Converts the FieldValue model passed into bytes.
*
* @param field_value the model to convert.
* @return the proto representation of the model.
*/
static Serializer::TypedValue EncodeFieldValue(
const firebase::firestore::model::FieldValue& field_value);

/**
* @brief Converts the value proto passed into bytes.
*
* @param[out] out_bytes A buffer to place the output. The bytes will be
* appended to this vector.
*/
// TODO(rsgowman): error handling, incl return code.
static void EncodeTypedValue(const TypedValue& value,
std::vector<uint8_t>* out_bytes);
static void EncodeFieldValue(
const firebase::firestore::model::FieldValue& field_value,
std::vector<uint8_t>* out_bytes);

/**
* Converts from the proto Value format to the model FieldValue format
*
* @return The model equivalent of the proto data.
*/
static firebase::firestore::model::FieldValue DecodeFieldValue(
const Serializer::TypedValue& value_proto);

/**
* @brief Converts from bytes to the nanopb proto.
* @brief Converts from bytes to the model FieldValue format.
*
* @param bytes The bytes to convert. It's assumed that exactly all of the
* bytes will be used by this conversion.
* @return The (nanopb) proto equivalent of the bytes.
* @return The model equivalent of the bytes.
*/
// TODO(rsgowman): error handling.
static TypedValue DecodeTypedValue(const uint8_t* bytes, size_t length);
static firebase::firestore::model::FieldValue DecodeFieldValue(
const uint8_t* bytes, size_t length);

/**
* @brief Converts from bytes to the nanopb proto.
* @brief Converts from bytes to the model FieldValue format.
*
* @param bytes The bytes to convert. It's assumed that exactly all of the
* bytes will be used by this conversion.
* @return The (nanopb) proto equivalent of the bytes.
* @return The model equivalent of the bytes.
*/
// TODO(rsgowman): error handling.
static TypedValue DecodeTypedValue(const std::vector<uint8_t>& bytes) {
return DecodeTypedValue(bytes.data(), bytes.size());
static firebase::firestore::model::FieldValue DecodeFieldValue(
const std::vector<uint8_t>& bytes) {
return DecodeFieldValue(bytes.data(), bytes.size());
}

private:
// TODO(rsgowman): We don't need the database_id_ yet (but will eventually).
// const firebase::firestore::model::DatabaseId& database_id_;
};

bool operator==(const Serializer::TypedValue& lhs,
const Serializer::TypedValue& rhs);

} // namespace remote
} // namespace firestore
} // namespace firebase
Expand Down
80 changes: 13 additions & 67 deletions Firestore/core/test/firebase/firestore/remote/serializer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,60 +63,27 @@ class SerializerTest : public ::testing::Test {
Serializer serializer;

void ExpectRoundTrip(const FieldValue& model,
const Serializer::TypedValue& proto,
const std::vector<uint8_t>& bytes,
FieldValue::Type type) {
EXPECT_EQ(type, model.type());
EXPECT_EQ(type, proto.type);
Serializer::TypedValue actual_proto = serializer.EncodeFieldValue(model);
EXPECT_EQ(type, actual_proto.type);
EXPECT_EQ(proto, actual_proto);
EXPECT_EQ(model, serializer.DecodeFieldValue(proto));
}

void ExpectRoundTrip(const Serializer::TypedValue& proto,
std::vector<uint8_t> bytes,
FieldValue::Type type) {
EXPECT_EQ(type, proto.type);
std::vector<uint8_t> actual_bytes;
Serializer::EncodeTypedValue(proto, &actual_bytes);
serializer.EncodeFieldValue(model, &actual_bytes);
EXPECT_EQ(bytes, actual_bytes);
Serializer::TypedValue actual_proto = Serializer::DecodeTypedValue(bytes);
EXPECT_EQ(type, actual_proto.type);
EXPECT_EQ(proto, actual_proto);
FieldValue actual_model = serializer.DecodeFieldValue(bytes);
EXPECT_EQ(type, actual_model.type());
EXPECT_EQ(model, actual_model);
}
};

TEST_F(SerializerTest, EncodesNullModelToProto) {
TEST_F(SerializerTest, EncodesNullModelToBytes) {
FieldValue model = FieldValue::NullValue();
Serializer::TypedValue proto{FieldValue::Type::Null,
google_firestore_v1beta1_Value_init_default};
// sanity check (the _init_default above should set this to _NULL_VALUE)
EXPECT_EQ(google_protobuf_NullValue_NULL_VALUE, proto.value.null_value);
ExpectRoundTrip(model, proto, FieldValue::Type::Null);
}

TEST_F(SerializerTest, EncodesNullProtoToBytes) {
Serializer::TypedValue proto{FieldValue::Type::Null,
google_firestore_v1beta1_Value_init_default};
// sanity check (the _init_default above should set this to _NULL_VALUE)
EXPECT_EQ(google_protobuf_NullValue_NULL_VALUE, proto.value.null_value);

// TEXT_FORMAT_PROTO: 'null_value: NULL_VALUE'
std::vector<uint8_t> bytes{0x58, 0x00};
ExpectRoundTrip(proto, bytes, FieldValue::Type::Null);
ExpectRoundTrip(model, bytes, FieldValue::Type::Null);
}

TEST_F(SerializerTest, EncodesBoolModelToProto) {
for (bool test : {true, false}) {
FieldValue model = FieldValue::BooleanValue(test);
Serializer::TypedValue proto{FieldValue::Type::Boolean,
google_firestore_v1beta1_Value_init_default};
proto.value.boolean_value = test;
ExpectRoundTrip(model, proto, FieldValue::Type::Boolean);
}
}

TEST_F(SerializerTest, EncodesBoolProtoToBytes) {
TEST_F(SerializerTest, EncodesBoolModelToBytes) {
struct TestCase {
bool value;
std::vector<uint8_t> bytes;
Expand All @@ -128,31 +95,12 @@ TEST_F(SerializerTest, EncodesBoolProtoToBytes) {
{false, {0x08, 0x00}}};

for (const TestCase& test : cases) {
Serializer::TypedValue proto{FieldValue::Type::Boolean,
google_firestore_v1beta1_Value_init_default};
proto.value.boolean_value = test.value;
ExpectRoundTrip(proto, test.bytes, FieldValue::Type::Boolean);
}
}

TEST_F(SerializerTest, EncodesIntegersModelToProto) {
std::vector<int64_t> testCases{0,
1,
-1,
100,
-100,
std::numeric_limits<int64_t>::min(),
std::numeric_limits<int64_t>::max()};
for (int64_t test : testCases) {
FieldValue model = FieldValue::IntegerValue(test);
Serializer::TypedValue proto{FieldValue::Type::Integer,
google_firestore_v1beta1_Value_init_default};
proto.value.integer_value = test;
ExpectRoundTrip(model, proto, FieldValue::Type::Integer);
FieldValue model = FieldValue::BooleanValue(test.value);
ExpectRoundTrip(model, test.bytes, FieldValue::Type::Boolean);
}
}

TEST_F(SerializerTest, EncodesIntegersProtoToBytes) {
TEST_F(SerializerTest, EncodesIntegersModelToBytes) {
// NB: because we're calculating the bytes via protoc (instead of
// libprotobuf) we have to look at values from numeric_limits<T> ahead of
// time. So these test cases make the following assumptions about
Expand Down Expand Up @@ -198,10 +146,8 @@ TEST_F(SerializerTest, EncodesIntegersProtoToBytes) {
{0x10, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f}}};

for (const TestCase& test : cases) {
Serializer::TypedValue proto{FieldValue::Type::Integer,
google_firestore_v1beta1_Value_init_default};
proto.value.integer_value = test.value;
ExpectRoundTrip(proto, test.bytes, FieldValue::Type::Integer);
FieldValue model = FieldValue::IntegerValue(test.value);
ExpectRoundTrip(model, test.bytes, FieldValue::Type::Integer);
}
}

Expand Down