-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Changes from 3 commits
099d8b3
6dbfd5b
99204f4
74892fc
37d7c6c
5e390dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,65 @@ namespace firebase { | |
namespace firestore { | ||
namespace remote { | ||
|
||
namespace { | ||
|
||
void 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(); | ||
} | ||
} | ||
|
||
void EncodeNull(pb_ostream_t* stream) { | ||
return EncodeVarint(stream, google_firestore_v1beta1_Value_null_value_tag, | ||
google_protobuf_NullValue_NULL_VALUE); | ||
} | ||
|
||
void EncodeBool(pb_ostream_t* stream, bool bool_value) { | ||
return EncodeVarint(stream, google_firestore_v1beta1_Value_boolean_value_tag, | ||
bool_value); | ||
} | ||
|
||
uint64_t 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; | ||
} | ||
|
||
void 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 DecodeBool(pb_istream_t* stream) { | ||
uint64_t varint = DecodeVarint(stream); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nanopb offers both signed and unsigned varints that use different encodings. While the unsigned variant appears to be the one used to encode There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So maybe DecodeUnsignedVarint() DecodeSignedVarint() ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
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( | ||
|
@@ -33,6 +92,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(); | ||
|
@@ -42,42 +109,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(); | ||
|
@@ -96,14 +156,45 @@ Serializer::TypedValue Serializer::DecodeTypedValue(const uint8_t* bytes, | |
abort(); | ||
} | ||
|
||
Serializer::TypedValue retval{FieldValue::Type::Null, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
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; | ||
case firebase::firestore::model::FieldValue::Type::Boolean: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You already use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Turns out, I'm already |
||
return lhs.value.boolean_value == rhs.value.boolean_value; | ||
default: | ||
// TODO(rsgowman): implement the other types | ||
abort(); | ||
} | ||
} | ||
|
||
} // namespace remote | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,26 @@ | |
* limitations under the License. | ||
*/ | ||
|
||
/* NB: proto bytes were created via: | ||
echo 'TEXT_FORMAT_PROTO' \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
i.e. driving the test from the script? So something like this:
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:
(Either way, if we decide to do 2 or 3, I'll issue a followup PR rather than including in this one.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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/ \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: you're mixing trailing slashes and non-trailing slashes in the same command. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
--encode=google.firestore.v1beta1.Value \ | ||
google/firestore/v1beta1/document.proto \ | ||
> output.bin | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest the final step here would be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done ( |
||
* where TEXT_FORMAT_PROTO is the text format of the protobuf. (go/textformat). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While not strictly prohibited, go links in open source are frowned upon because they don't mean anything outside Google's network. Try to find an equivalent in public documentation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did. There isn't any. :( Best I was able to find was this: https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.text_format Oh, and also this SO question asking for details: https://stackoverflow.com/questions/18873924/what-does-the-protobuf-text-format-look-like |
||
* | ||
* 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> | ||
|
@@ -80,18 +100,40 @@ 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}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Continuing the discussion from the previous PR: why does There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Why would this lead to wrapping other nanopb structs? Is it for symmetry? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed offline. Summary:
Decision: Defer for now. (But others are welcome to chime in.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
}; | ||
|
||
// TEXT_FORMAT_PROTO: 'boolean_value: true' | ||
std::vector<uint8_t> true_bytes{0x08, 0x01}; | ||
// TEXT_FORMAT_PROTO: 'boolean_value: false' | ||
std::vector<uint8_t> false_bytes{0x08, 0x00}; | ||
|
||
for (const TestCase& test : | ||
{TestCase{true, true_bytes}, TestCase{false, false_bytes}}) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) {
// ...
} There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file can quickly become a mess because it's doing a lot. I propose that you organize things at the lowest level as Encode/Decode pairs so that it's easy to see that each lowest-level operation is balanced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm; done.