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

Jump to bottom

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 all 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
128 changes: 111 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 @@ -23,6 +23,68 @@ namespace firebase {
namespace firestore {
namespace remote {

namespace {

void EncodeUnsignedVarint(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();
}
}

uint64_t DecodeUnsignedVarint(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;
}

void EncodeNull(pb_ostream_t* stream) {
return EncodeUnsignedVarint(stream,
google_firestore_v1beta1_Value_null_value_tag,
google_protobuf_NullValue_NULL_VALUE);
}

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

void EncodeBool(pb_ostream_t* stream, bool bool_value) {
return EncodeUnsignedVarint(
stream, google_firestore_v1beta1_Value_boolean_value_tag, bool_value);
}

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

} // namespace

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

Serializer::TypedValue Serializer::EncodeFieldValue(
Expand All @@ -33,6 +95,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 +112,35 @@ 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);
}

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,14 +159,45 @@ 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:
return Serializer::TypedValue{
FieldValue::Type::Null, google_firestore_v1beta1_Value_init_default};
result.type = FieldValue::Type::Null;
DecodeNull(&stream);
break;
case google_firestore_v1beta1_Value_boolean_value_tag:
result.type = FieldValue::Type::Boolean;
result.value.boolean_value = DecodeBool(&stream);
break;

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;
default:
// TODO(rsgowman): implement the other types
abort();
}
}

} // namespace remote
Expand Down
20 changes: 2 additions & 18 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 @@ -121,24 +121,8 @@ class Serializer {
// const firebase::firestore::model::DatabaseId& database_id_;
};

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

switch (lhs.type) {
case firebase::firestore::model::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;
default:
// TODO(rsgowman): implement the other types
abort();
}
}
bool operator==(const Serializer::TypedValue& lhs,
const Serializer::TypedValue& rhs);

} // namespace remote
} // namespace firestore
Expand Down
61 changes: 52 additions & 9 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 @@ -14,6 +14,26 @@
* limitations under the License.
*/

/* NB: proto bytes were created via:
echo 'TEXT_FORMAT_PROTO' \
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could wrap this up in a little script we just check in. Constructing this command-line is tedious (though perhaps that's just because of all the plus signs in the diff).

Perhaps even that script should run at the higher level and just run through the cases we care about.

Feel free to push back.

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 wonder if we could wrap this up in a little script we just check in. Constructing this command-line is tedious (though perhaps that's just because of all the plus signs in the diff).

In practice, constructing the command line is easy. (Just sweep your mouse over it... unless you're looking at a diff.) Also note the lack of '*' at the beginning of these lines to enable that.

Checking in a script would be reasonable. Alternatively, we could link the test suite (not the main sdk) against libprotobuf and use the normal protobuf api, or even the libprotobuf text format api to construct these protos.

Perhaps even that script should run at the higher level and just run through the cases we care about.

i.e. driving the test from the script? So something like this:

#/bin/sh
set -e
for test in [ 'field:value' 'field2:value2' ] ; do
  echo 'field: value' | protoc --params | ./testEncodeDecode --maybeSomeParamsHere
done

I like that it wouldn't require us to record the raw bytes in the .cc file... but I'm less wild about driving the test from outside the normal mechanism. If we're going to build the bytes on the fly, I like linking libprotobuf better.

So I think our options are:

  1. noop. Just keep building the bytes with protoc and adding them to the test suite.
  2. drive this test with a script that looks something like the above (still hooked into cmake such that make test runs it)
  3. link libprotobuf to test suite and use protobuf api OR text format proto api.

(Either way, if we decide to do 2 or 3, I'll issue a followup PR rather than including in this one.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't mean to change the way tests are run; I think that's a bridge too far. GoogleTest is our test runner and changing that is too painful. I can't imagine making this play nice with Xcode.

Option 3 (using the protobuf API) has some serious appeal but is probably the most work. I worry that encoding bytes directly works at this low a level but but is basically infeasible once you get up to the oneofs in e.g. StructuredQuery and friends.

We can proceed for now and switch to 3 once this approach proves impractical

Copy link
Member Author

Choose a reason for hiding this comment

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

sg

| ./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 \
| hexdump -C
* where TEXT_FORMAT_PROTO is the text format of the protobuf. (go/textformat).
*
* Examples:
* - For null, TEXT_FORMAT_PROTO would be 'null_value: NULL_VALUE' and would
* yield the bytes: { 0x58, 0x00 }.
* - For true, TEXT_FORMAT_PROTO would be 'boolean_value: true' and would yield
* the bytes { 0x08, 0x01 }.
*
* All uses are documented below, so search for TEXT_FORMAT_PROTO to find more
* examples.
*/

#include "Firestore/core/src/firebase/firestore/remote/serializer.h"

#include <pb.h>
Expand Down Expand Up @@ -80,18 +100,41 @@ TEST_F(SerializerTest, EncodesNullProtoToBytes) {
// sanity check (the _init_default above should set this to _NULL_VALUE)
EXPECT_EQ(google_protobuf_NullValue_NULL_VALUE, proto.value.null_value);

/* NB: proto bytes were created via:
echo 'null_value: NULL_VALUE' \
| ./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
*/
// TEXT_FORMAT_PROTO: 'null_value: NULL_VALUE'
std::vector<uint8_t> bytes{0x58, 0x00};
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;
};

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) {
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.

// TODO(rsgowman): Death test for decoding invalid bytes.