Skip to content

Commit

Permalink
Eliminate TypedValue and serialize direct from FieldValue to bytes. (#…
Browse files Browse the repository at this point in the history 
…860)

This will likely only apply for proto messages that use 'oneof's. (Because we need to serialize these manually.)

The problem was that as we were adding additional types, TypeValue was evolving into a parallel implementation of the FieldValue union.

When serializing/deserializing oneofs we need to supply a custom value to serialize and a custom function to do the work. There's no value in translating from FieldValue to TypeValue just to store as this custom object. We might as well just store the FieldValue model directly and write the custom serializer/deserializer to use the model directly.
  • Loading branch information
@rsgowman
rsgowman authored Feb 27, 2018
1 parent f058881 commit 13aeb61
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 191 deletions.
95 changes: 13 additions & 82 deletions Firestore/core/src/firebase/firestore/remote/serializer.cc
View file
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()) {
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
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
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

0 comments on commit 13aeb61

Please sign in to comment.