Skip to content
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

Serialize (and deserialize) bool values #791

Merged
merged 6 commits into from
Feb 16, 2018
Merged

Serialize (and deserialize) bool values #791

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
099d8b3
Serialize (and deserialize) bool values
rsgowman Feb 13, 2018
6dbfd5b
refactoring
rsgowman Feb 13, 2018
99204f4
Review feedback
rsgowman Feb 14, 2018
74892fc
review feedback 2
rsgowman Feb 14, 2018
37d7c6c
review feedback 2
rsgowman Feb 15, 2018
5e390dd
review feedback 3
rsgowman Feb 15, 2018
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
102 changes: 85 additions & 17 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 @@ -33,6 +33,14 @@ Serializer::TypedValue Serializer::EncodeFieldValue(
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;
default:
// TODO(rsgowman): implement the other types
abort();
Expand All @@ -42,42 +50,61 @@ Serializer::TypedValue Serializer::EncodeFieldValue(

void Serializer::EncodeTypedValue(const TypedValue& value,
std::vector<uint8_t>* out_bytes) {
bool status;
// 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
// going to need.
uint8_t buf[1024];
pb_ostream_t stream = pb_ostream_from_buffer(buf, sizeof(buf));
switch (value.type) {
case FieldValue::Type::Null:
status = pb_encode_tag(&stream, PB_WT_VARINT,
google_firestore_v1beta1_Value_null_value_tag);
if (!status) {
// TODO(rsgowman): figure out error handling
abort();
}

status = pb_encode_varint(&stream, value.value.null_value);
if (!status) {
// TODO(rsgowman): figure out error handling
abort();
}

out_bytes->insert(out_bytes->end(), buf, buf + stream.bytes_written);
EncodeNull(&stream);
break;

case FieldValue::Type::Boolean:
EncodeBool(&stream, value.value.boolean_value);
break;

default:
// TODO(rsgowman): implement the other types
abort();
}

out_bytes->insert(out_bytes->end(), buf, buf + stream.bytes_written);
}

void Serializer::EncodeNull(pb_ostream_t* stream) {
return EncodeVarint(stream, google_firestore_v1beta1_Value_null_value_tag,
google_protobuf_NullValue_NULL_VALUE);
}

void Serializer::EncodeBool(pb_ostream_t* stream, bool bool_value) {
return EncodeVarint(stream, google_firestore_v1beta1_Value_boolean_value_tag,
bool_value);
}

void Serializer::EncodeVarint(pb_ostream_t* stream,
uint32_t field_number,
uint64_t value) {
bool status = pb_encode_tag(stream, PB_WT_VARINT, field_number);
if (!status) {
// TODO(rsgowman): figure out error handling
abort();
}

status = pb_encode_varint(stream, value);
if (!status) {
// TODO(rsgowman): figure out error handling
abort();
}
}

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);
default:
// TODO(rsgowman): implement the other types
abort();
Expand All @@ -96,16 +123,57 @@ Serializer::TypedValue Serializer::DecodeTypedValue(const uint8_t* bytes,
abort();
}

Serializer::TypedValue retval{FieldValue::Type::Null,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer more something more natural reading/sounding than "retval". How about "result"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. (retval is an old habit of mine; I can guarantee you'll see it again. Feel free to keep calling it out.)

google_firestore_v1beta1_Value_init_default};
switch (tag) {
case google_firestore_v1beta1_Value_null_value_tag:
return Serializer::TypedValue{
FieldValue::Type::Null, google_firestore_v1beta1_Value_init_default};
retval.type = FieldValue::Type::Null;
DecodeNull(&stream);
break;
case google_firestore_v1beta1_Value_boolean_value_tag:
retval.type = FieldValue::Type::Boolean;
retval.value.boolean_value = DecodeBool(&stream);
break;

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

return retval;
}

void Serializer::DecodeNull(pb_istream_t* stream) {
uint64_t varint = DecodeVarint(stream);
if (varint != google_protobuf_NullValue_NULL_VALUE) {
// TODO(rsgowman): figure out error handling
abort();
}
}

bool Serializer::DecodeBool(pb_istream_t* stream) {
uint64_t varint = DecodeVarint(stream);
switch (varint) {
case 0:
return false;
case 1:
return true;
default:
// TODO(rsgowman): figure out error handling
abort();
}
}

uint64_t Serializer::DecodeVarint(pb_istream_t* stream) {
uint64_t varint_value;
bool status = pb_decode_varint(stream, &varint_value);
if (!status) {
// TODO(rsgowman): figure out error handling
abort();
}
return varint_value;
}

} // namespace remote
} // namespace firestore
} // namespace firebase
12 changes: 12 additions & 0 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 @@ -117,6 +117,16 @@ class Serializer {
}

private:
static void EncodeNull(pb_ostream_t* stream);
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: these can be free functions in .cc file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

static void EncodeBool(pb_ostream_t* stream, bool bool_value);
static void EncodeVarint(pb_ostream_t* stream,
uint32_t field_number,
uint64_t value);

static void DecodeNull(pb_istream_t* stream);
static bool DecodeBool(pb_istream_t* stream);
static uint64_t DecodeVarint(pb_istream_t* stream);

// TODO(rsgowman): We don't need the database_id_ yet (but will eventually).
// const firebase::firestore::model::DatabaseId& database_id_;
};
Expand All @@ -134,6 +144,8 @@ inline bool operator==(const Serializer::TypedValue& lhs,
FIREBASE_DEV_ASSERT(rhs.value.null_value ==
google_protobuf_NullValue_NULL_VALUE);
return true;
case firebase::firestore::model::FieldValue::Type::Boolean:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to move this to .cc file, it's getting long quickly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's not going to get any shorter in the future either. Done.

return lhs.value.boolean_value == rhs.value.boolean_value;
default:
// TODO(rsgowman): implement the other types
abort();
Expand Down
37 changes: 37 additions & 0 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 @@ -93,5 +93,42 @@ TEST_F(SerializerTest, EncodesNullProtoToBytes) {
ExpectRoundTrip(proto, 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};
Copy link
Contributor

Choose a reason for hiding this comment

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

Continuing the discussion from the previous PR: why does TypedValue need to be POD?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't. It's done this way for symmetry with the underlying nanopb struct. We could add some ctors/etc, but we do still need to work with the underlying nanopb POD, so I don't think that's going to buy us very much.

Additionally, there (probably) won't be wrappers for the other nanopb protos. (The only reason this one exists is because of the oneof within the proto.) So code that uses this has to deal with nanopb POD structs anyways.

(All that said, I don't object to converting it. Though that path may lead us to wrapping all of nanopbs structs.)

Copy link
Contributor

@var-const var-const Feb 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In general, I think it's preferable to have higher-level code, unless there is a specific reason not to. So in this case, hiding the ..._init_default thing in a constructor, or perhaps a helper function, would avoid having to specify that non-intuitive initializer each time a TypedValue is created - it's repetitive and error-prone.

Why would this lead to wrapping other nanopb structs? Is it for symmetry?

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed offline. Summary:

  • all nanopbs are init'd in this manner.
  • _init_default is from the generated code.
  • existing (POD) wrapper is similar in spirit to existing nanopb structs
  • wrapping might still make sense (safer) even if it does end up being applied to all nanopb objects

Decision: Defer for now. (But others are welcome to chime in.)

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM to defer; I'd make the argument that we can deviate from convention if it keeps us from repeating code too much. We can also just wrap these in local functions if we find ourselves repeating this incantation too often in tests.

proto.value.boolean_value = test;
ExpectRoundTrip(model, proto, FieldValue::Type::Boolean);
}
}

TEST_F(SerializerTest, EncodesBoolProtoToBytes) {
struct TestCase {
bool value;
std::vector<uint8_t> bytes;
};

/* NB: proto true_bytes were created via (and similarly for false_bytes):
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is great, but a little repetitive with the one for nulls. Perhaps move the command to a file-level comment (along the lines of byte values used in tests were obtained by running echo 'APPROPRIATE_VALUE_HERE' | <the rest of the command>), and then only specify what was echoed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

echo 'boolean_value: true' \
| ./build/external/protobuf/src/protobuf-build/src/protoc \
-I./Firestore/Protos/protos \
-I./build/external/protobuf/src/protobuf/src/ \
--encode=google.firestore.v1beta1.Value \
google/firestore/v1beta1/document.proto \
> output.bin
*/
std::vector<uint8_t> true_bytes{0x08, 0x01};
std::vector<uint8_t> false_bytes{0x08, 0x00};

for (const TestCase& test :
{TestCase{true, true_bytes}, TestCase{false, false_bytes}}) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Iterating through two test cases is a little odd. I've done this for symmetry with java (as well as with the future test cases) but don't object to unrolling the loop either.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little clumsy specified inline in the loop this way. Also it needlessly separates the boolean from the bytes in the input. Maybe initialize the TestCases above?

// TEXT_FORMAT_PROTO: 'boolean_value: true'
TestCase true_case{true, {0x08, 0x01}};

// TEXT_FORMAT_PROTO: 'boolean_value: false'
TestCase false_case{false, {0x08, 0x00}};

for (const TestCase& test : {true_case, false_case}) {
  // ...
}

Or even:

std::vector<TestCase> cases{
  // TEXT_FORMAT_PROTO: 'boolean_value: true'
  {true, {0x08, 0x01}},

  // TEXT_FORMAT_PROTO: 'boolean_value: false'
  {false, {0x08, 0x00}
};

for (const TestCase& test : cases) {
  // ...
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I really like the latter option; done. (The style formatter really does a number on it though.)

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);
}
}

// TODO(rsgowman): Test [en|de]coding multiple protos into the same output
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: death-test for decode invalid bytes? say, if there is data-corruption, I guess those should be caught by decoder.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we'll definitely need that. Error handling's not there yet though, so I've just added a TODO for now.

// vector.