From 0846729cc97cdab40986f6917913e84949b2b7b5 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Mon, 26 Feb 2018 18:15:03 -0500 Subject: [PATCH 1/3] Serialize (and deserialize) string values --- .../firebase/firestore/model/field_value.h | 5 ++ .../firebase/firestore/remote/serializer.cc | 77 ++++++++++++++++++- .../firestore/remote/serializer_test.cc | 41 ++++++++++ 3 files changed, 122 insertions(+), 1 deletion(-) diff --git a/Firestore/core/src/firebase/firestore/model/field_value.h b/Firestore/core/src/firebase/firestore/model/field_value.h index 15945b9075f..2d0388c04a5 100644 --- a/Firestore/core/src/firebase/firestore/model/field_value.h +++ b/Firestore/core/src/firebase/firestore/model/field_value.h @@ -101,6 +101,11 @@ class FieldValue { return integer_value_; } + const std::string& string_value() const { + FIREBASE_ASSERT(tag_ == Type::String); + return string_value_; + } + /** factory methods. */ static const FieldValue& NullValue(); static const FieldValue& TrueValue(); diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index 02013fa6b4b..52f9e65bcda 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -18,6 +18,7 @@ #include #include +#include namespace firebase { namespace firestore { @@ -98,6 +99,47 @@ int64_t DecodeInteger(pb_istream_t* stream) { return DecodeVarint(stream); } +void EncodeString(pb_ostream_t* stream, const std::string& string_value) { + bool status = pb_encode_string( + stream, reinterpret_cast(string_value.c_str()), + string_value.length()); + if (!status) { + // TODO(rsgowman): figure out error handling + abort(); + } +} + +std::string DecodeString(pb_istream_t* stream) { + pb_istream_t substream; + bool status = pb_make_string_substream(stream, &substream); + if (!status) { + // TODO(rsgowman): figure out error handling + abort(); + } + + std::string result(substream.bytes_left, '\0'); + status = pb_read(&substream, reinterpret_cast(&result[0]), + substream.bytes_left); + if (!status) { + // TODO(rsgowman): figure out error handling + abort(); + } + + // NB: future versions of nanopb read the remaining characters out of the + // substream (and return false if that fails) as an additional safety + // check within pb_clost_string_substream. Unfortunately, that's not present + // in the current version (0.38). We'll make a stronger assertion and check + // to make sure there *are* no remaining characters in the substream. + if (substream.bytes_left != 0) { + // TODO(rsgowman): figure out error handling + abort(); + } + + pb_close_string_substream(stream, &substream); + + return result; +} + } // namespace using firebase::firestore::model::FieldValue; @@ -149,6 +191,16 @@ void Serializer::EncodeFieldValue(const FieldValue& field_value, EncodeInteger(&stream, field_value.integer_value()); break; + case FieldValue::Type::String: + status = pb_encode_tag(&stream, PB_WT_STRING, + google_firestore_v1beta1_Value_string_value_tag); + if (!status) { + // TODO(rsgowman): figure out error handling + abort(); + } + EncodeString(&stream, field_value.string_value()); + break; + default: // TODO(rsgowman): implement the other types abort(); @@ -163,11 +215,32 @@ FieldValue Serializer::DecodeFieldValue(const uint8_t* bytes, size_t length) { uint32_t tag; bool eof; bool status = pb_decode_tag(&stream, &wire_type, &tag, &eof); - if (!status || wire_type != PB_WT_VARINT) { + if (!status) { // TODO(rsgowman): figure out error handling abort(); } + // Ensure the tag matches the wire type + // TODO(rsgowman): figure out error handling + switch (tag) { + case google_firestore_v1beta1_Value_null_value_tag: + case google_firestore_v1beta1_Value_boolean_value_tag: + case google_firestore_v1beta1_Value_integer_value_tag: + if (wire_type != PB_WT_VARINT) { + abort(); + } + break; + + case google_firestore_v1beta1_Value_string_value_tag: + if (wire_type != PB_WT_STRING) { + abort(); + } + break; + + default: + abort(); + } + switch (tag) { case google_firestore_v1beta1_Value_null_value_tag: DecodeNull(&stream); @@ -176,6 +249,8 @@ FieldValue Serializer::DecodeFieldValue(const uint8_t* bytes, size_t length) { return FieldValue::BooleanValue(DecodeBool(&stream)); case google_firestore_v1beta1_Value_integer_value_tag: return FieldValue::IntegerValue(DecodeInteger(&stream)); + case google_firestore_v1beta1_Value_string_value_tag: + return FieldValue::StringValue(DecodeString(&stream)); default: // TODO(rsgowman): figure out error handling diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc index 9ce60f572ca..1409325552e 100644 --- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc +++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc @@ -151,6 +151,47 @@ TEST_F(SerializerTest, EncodesIntegersModelToBytes) { } } +TEST_F(SerializerTest, EncodesStringModelToBytes) { + struct TestCase { + std::string value; + std::vector bytes; + }; + + std::vector cases{ + // TEXT_FORMAT_PROTO: 'string_value: ""' + {"", {0x8a, 0x01, 0x00}}, + // TEXT_FORMAT_PROTO: 'string_value: "a"' + {"a", {0x8a, 0x01, 0x01, 0x61}}, + // TEXT_FORMAT_PROTO: 'string_value: "abc def"' + {"abc def", {0x8a, 0x01, 0x07, 0x61, 0x62, 0x63, 0x20, 0x64, 0x65, 0x66}}, + // TEXT_FORMAT_PROTO: 'string_value: "æ"' + {"æ", {0x8a, 0x01, 0x02, 0xc3, 0xa6}}, + // TEXT_FORMAT_PROTO: 'string_value: "\0\ud7ff\ue000\uffff"' + // Some magic: Due to the unicode escapes (\u) and + // the lack of u"" prefix, this gets narrowed to a + // multibyte string. This is the same as + // "\0\xed\x9f\xbf\xee\x80\x80\xef\xbf\xbf". The + // size of 10 must be added, or else std::string + // will see the \0 at the start and assume that's + // the end of the string. + {{"\0\ud7ff\ue000\uffff", 10}, + {0x8a, 0x01, 0x0a, 0x00, 0xed, 0x9f, 0xbf, 0xee, 0x80, 0x80, 0xef, 0xbf, + 0xbf}}, + {{"\0\xed\x9f\xbf\xee\x80\x80\xef\xbf\xbf", 10}, + {0x8a, 0x01, 0x0a, 0x00, 0xed, 0x9f, 0xbf, 0xee, 0x80, 0x80, 0xef, 0xbf, + 0xbf}}, + // TEXT_FORMAT_PROTO: 'string_value: "(╯°□°）╯︵ ┻━┻"' + {"(╯°□°）╯︵ ┻━┻", + {0x8a, 0x01, 0x1e, 0x28, 0xe2, 0x95, 0xaf, 0xc2, 0xb0, 0xe2, 0x96, + 0xa1, 0xc2, 0xb0, 0xef, 0xbc, 0x89, 0xe2, 0x95, 0xaf, 0xef, 0xb8, + 0xb5, 0x20, 0xe2, 0x94, 0xbb, 0xe2, 0x94, 0x81, 0xe2, 0x94, 0xbb}}}; + + for (const TestCase& test : cases) { + FieldValue model = FieldValue::StringValue(test.value); + ExpectRoundTrip(model, test.bytes, FieldValue::Type::String); + } +} + // TODO(rsgowman): Test [en|de]coding multiple protos into the same output // vector. From cc7799475d1c4e4cd2e194945ad1c65fb8173519 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Tue, 27 Feb 2018 16:48:04 -0500 Subject: [PATCH 2/3] review feedback --- .../src/firebase/firestore/remote/serializer.cc | 2 +- .../firebase/firestore/remote/serializer_test.cc | 13 ++++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index 52f9e65bcda..11c4bb607be 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -127,7 +127,7 @@ std::string DecodeString(pb_istream_t* stream) { // NB: future versions of nanopb read the remaining characters out of the // substream (and return false if that fails) as an additional safety - // check within pb_clost_string_substream. Unfortunately, that's not present + // check within pb_close_string_substream. Unfortunately, that's not present // in the current version (0.38). We'll make a stronger assertion and check // to make sure there *are* no remaining characters in the substream. if (substream.bytes_left != 0) { diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc index 1409325552e..8f890640fc6 100644 --- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc +++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc @@ -167,13 +167,12 @@ TEST_F(SerializerTest, EncodesStringModelToBytes) { // TEXT_FORMAT_PROTO: 'string_value: "æ"' {"æ", {0x8a, 0x01, 0x02, 0xc3, 0xa6}}, // TEXT_FORMAT_PROTO: 'string_value: "\0\ud7ff\ue000\uffff"' - // Some magic: Due to the unicode escapes (\u) and - // the lack of u"" prefix, this gets narrowed to a - // multibyte string. This is the same as - // "\0\xed\x9f\xbf\xee\x80\x80\xef\xbf\xbf". The - // size of 10 must be added, or else std::string - // will see the \0 at the start and assume that's - // the end of the string. + // Note: Each one of the three embedded universal character names + // (\u-escaped) maps to three chars, so the total length of the string + // literal is 10 (ignoring the terminating null), and the resulting string + // literal is the same as '\0\xed\x9f\xbf\xee\x80\x80\xef\xbf\xbf'". The + // size of 10 must be added, or else std::string will see the \0 at the + // start and assume that's the end of the string. {{"\0\ud7ff\ue000\uffff", 10}, {0x8a, 0x01, 0x0a, 0x00, 0xed, 0x9f, 0xbf, 0xee, 0x80, 0x80, 0xef, 0xbf, 0xbf}}, From 9199b4882b2d7ac12b2b78aa8e1d769b3874f965 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Wed, 28 Feb 2018 11:34:27 -0500 Subject: [PATCH 3/3] review feedback 2 --- Firestore/core/src/firebase/firestore/remote/serializer.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index 11c4bb607be..6b41016f8db 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -18,6 +18,7 @@ #include #include + #include namespace firebase {