Skip to content

Commit 13aeb61

Browse files
rsgowmanrsgowman
authored
Eliminate TypedValue and serialize direct from FieldValue to bytes. (firebase#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.
1 parent f058881 commit 13aeb61

File tree

3 files changed

+44
-191
lines changed

3 files changed

+44
-191
lines changed

‎Firestore/core/src/firebase/firestore/remote/serializer.cc

Lines changed: 13 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -102,33 +102,7 @@ int64_t DecodeInteger(pb_istream_t* stream) {
102102

103103
using firebase::firestore::model::FieldValue;
104104

105-
Serializer::TypedValue Serializer::EncodeFieldValue(
106-
const FieldValue& field_value) {
107-
Serializer::TypedValue proto_value{
108-
field_value.type(), google_firestore_v1beta1_Value_init_default};
109-
switch (field_value.type()) {
110-
case FieldValue::Type::Null:
111-
proto_value.value.null_value = google_protobuf_NullValue_NULL_VALUE;
112-
break;
113-
case FieldValue::Type::Boolean:
114-
if (field_value == FieldValue::TrueValue()) {
115-
proto_value.value.boolean_value = true;
116-
} else {
117-
FIREBASE_DEV_ASSERT(field_value == FieldValue::FalseValue());
118-
proto_value.value.boolean_value = false;
119-
}
120-
break;
121-
case FieldValue::Type::Integer:
122-
proto_value.value.integer_value = field_value.integer_value();
123-
break;
124-
default:
125-
// TODO(rsgowman): implement the other types
126-
abort();
127-
}
128-
return proto_value;
129-
}
130-
131-
void Serializer::EncodeTypedValue(const TypedValue& value,
105+
void Serializer::EncodeFieldValue(const FieldValue& field_value,
132106
std::vector<uint8_t>* out_bytes) {
133107
// TODO(rsgowman): how large should the output buffer be? Do some
134108
// investigation to see if we can get nanopb to tell us how much space it's
@@ -139,7 +113,7 @@ void Serializer::EncodeTypedValue(const TypedValue& value,
139113
// TODO(rsgowman): some refactoring is in order... but will wait until after a
140114
// non-varint, non-fixed-size (i.e. string) type is present before doing so.
141115
bool status = false;
142-
switch (value.type) {
116+
switch (field_value.type()) {
143117
case FieldValue::Type::Null:
144118
status = pb_encode_tag(&stream, PB_WT_VARINT,
145119
google_firestore_v1beta1_Value_null_value_tag);
@@ -157,7 +131,12 @@ void Serializer::EncodeTypedValue(const TypedValue& value,
157131
// TODO(rsgowman): figure out error handling
158132
abort();
159133
}
160-
EncodeBool(&stream, value.value.boolean_value);
134+
if (field_value == FieldValue::TrueValue()) {
135+
EncodeBool(&stream, true);
136+
} else {
137+
FIREBASE_DEV_ASSERT(field_value == FieldValue::FalseValue());
138+
EncodeBool(&stream, false);
139+
}
161140
break;
162141

163142
case FieldValue::Type::Integer:
@@ -167,7 +146,7 @@ void Serializer::EncodeTypedValue(const TypedValue& value,
167146
// TODO(rsgowman): figure out error handling
168147
abort();
169148
}
170-
EncodeInteger(&stream, value.value.integer_value);
149+
EncodeInteger(&stream, field_value.integer_value());
171150
break;
172151

173152
default:
@@ -178,23 +157,7 @@ void Serializer::EncodeTypedValue(const TypedValue& value,
178157
out_bytes->insert(out_bytes->end(), buf, buf + stream.bytes_written);
179158
}
180159

181-
FieldValue Serializer::DecodeFieldValue(
182-
const Serializer::TypedValue& value_proto) {
183-
switch (value_proto.type) {
184-
case FieldValue::Type::Null:
185-
return FieldValue::NullValue();
186-
case FieldValue::Type::Boolean:
187-
return FieldValue::BooleanValue(value_proto.value.boolean_value);
188-
case FieldValue::Type::Integer:
189-
return FieldValue::IntegerValue(value_proto.value.integer_value);
190-
default:
191-
// TODO(rsgowman): implement the other types
192-
abort();
193-
}
194-
}
195-
196-
Serializer::TypedValue Serializer::DecodeTypedValue(const uint8_t* bytes,
197-
size_t length) {
160+
FieldValue Serializer::DecodeFieldValue(const uint8_t* bytes, size_t length) {
198161
pb_istream_t stream = pb_istream_from_buffer(bytes, length);
199162
pb_wire_type_t wire_type;
200163
uint32_t tag;
@@ -205,51 +168,19 @@ Serializer::TypedValue Serializer::DecodeTypedValue(const uint8_t* bytes,
205168
abort();
206169
}
207170

208-
Serializer::TypedValue result{FieldValue::Type::Null,
209-
google_firestore_v1beta1_Value_init_default};
210171
switch (tag) {
211172
case google_firestore_v1beta1_Value_null_value_tag:
212-
result.type = FieldValue::Type::Null;
213173
DecodeNull(&stream);
214-
break;
174+
return FieldValue::NullValue();
215175
case google_firestore_v1beta1_Value_boolean_value_tag:
216-
result.type = FieldValue::Type::Boolean;
217-
result.value.boolean_value = DecodeBool(&stream);
218-
break;
176+
return FieldValue::BooleanValue(DecodeBool(&stream));
219177
case google_firestore_v1beta1_Value_integer_value_tag:
220-
result.type = FieldValue::Type::Integer;
221-
result.value.integer_value = DecodeInteger(&stream);
222-
break;
178+
return FieldValue::IntegerValue(DecodeInteger(&stream));
223179

224180
default:
225181
// TODO(rsgowman): figure out error handling
226182
abort();
227183
}
228-
229-
return result;
230-
}
231-
232-
bool operator==(const Serializer::TypedValue& lhs,
233-
const Serializer::TypedValue& rhs) {
234-
if (lhs.type != rhs.type) {
235-
return false;
236-
}
237-
238-
switch (lhs.type) {
239-
case FieldValue::Type::Null:
240-
FIREBASE_DEV_ASSERT(lhs.value.null_value ==
241-
google_protobuf_NullValue_NULL_VALUE);
242-
FIREBASE_DEV_ASSERT(rhs.value.null_value ==
243-
google_protobuf_NullValue_NULL_VALUE);
244-
return true;
245-
case FieldValue::Type::Boolean:
246-
return lhs.value.boolean_value == rhs.value.boolean_value;
247-
case FieldValue::Type::Integer:
248-
return lhs.value.integer_value == rhs.value.integer_value;
249-
default:
250-
// TODO(rsgowman): implement the other types
251-
abort();
252-
}
253184
}
254185

255186
} // namespace remote

‎Firestore/core/src/firebase/firestore/remote/serializer.h

Lines changed: 18 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -33,25 +33,17 @@ namespace remote {
3333
* @brief Converts internal model objects to their equivalent protocol buffer
3434
* form, and protocol buffer objects to their equivalent bytes.
3535
*
36-
* Methods starting with "Encode" either convert from a model object to a
37-
* protocol buffer or from a protocol buffer to bytes, and methods starting with
38-
* "Decode" either convert from a protocol buffer to a model object or from
39-
* bytes to a protocol buffer.
40-
*
36+
* Methods starting with "Encode" convert from a model object to a protocol
37+
* buffer (or directly to bytes in cases where the proto uses a 'oneof', due to
38+
* limitations in nanopb), and methods starting with "Decode" convert from a
39+
* protocol buffer to a model object (or from bytes directly to a model
40+
* objects.)
4141
*/
4242
// TODO(rsgowman): Original docs also has this: "Throws an exception if a
4343
// protocol buffer is missing a critical field or has a value we can't
4444
// interpret." Adjust for C++.
4545
class Serializer {
4646
public:
47-
/**
48-
* @brief Wraps (nanopb) google_firestore_v1beta1_Value with type information.
49-
*/
50-
struct TypedValue {
51-
firebase::firestore::model::FieldValue::Type type;
52-
google_firestore_v1beta1_Value value;
53-
};
54-
5547
Serializer() {
5648
}
5749
// TODO(rsgowman): We eventually need the DatabaseId, but can't add it just
@@ -68,62 +60,46 @@ class Serializer {
6860
//}
6961

7062
/**
71-
* Converts the FieldValue model passed into the Value proto equivalent.
63+
* Converts the FieldValue model passed into bytes.
7264
*
7365
* @param field_value the model to convert.
74-
* @return the proto representation of the model.
75-
*/
76-
static Serializer::TypedValue EncodeFieldValue(
77-
const firebase::firestore::model::FieldValue& field_value);
78-
79-
/**
80-
* @brief Converts the value proto passed into bytes.
81-
*
8266
* @param[out] out_bytes A buffer to place the output. The bytes will be
8367
* appended to this vector.
8468
*/
8569
// TODO(rsgowman): error handling, incl return code.
86-
static void EncodeTypedValue(const TypedValue& value,
87-
std::vector<uint8_t>* out_bytes);
70+
static void EncodeFieldValue(
71+
const firebase::firestore::model::FieldValue& field_value,
72+
std::vector<uint8_t>* out_bytes);
8873

8974
/**
90-
* Converts from the proto Value format to the model FieldValue format
91-
*
92-
* @return The model equivalent of the proto data.
93-
*/
94-
static firebase::firestore::model::FieldValue DecodeFieldValue(
95-
const Serializer::TypedValue& value_proto);
96-
97-
/**
98-
* @brief Converts from bytes to the nanopb proto.
75+
* @brief Converts from bytes to the model FieldValue format.
9976
*
10077
* @param bytes The bytes to convert. It's assumed that exactly all of the
10178
* bytes will be used by this conversion.
102-
* @return The (nanopb) proto equivalent of the bytes.
79+
* @return The model equivalent of the bytes.
10380
*/
10481
// TODO(rsgowman): error handling.
105-
static TypedValue DecodeTypedValue(const uint8_t* bytes, size_t length);
82+
static firebase::firestore::model::FieldValue DecodeFieldValue(
83+
const uint8_t* bytes, size_t length);
10684

10785
/**
108-
* @brief Converts from bytes to the nanopb proto.
86+
* @brief Converts from bytes to the model FieldValue format.
10987
*
11088
* @param bytes The bytes to convert. It's assumed that exactly all of the
11189
* bytes will be used by this conversion.
112-
* @return The (nanopb) proto equivalent of the bytes.
90+
* @return The model equivalent of the bytes.
11391
*/
11492
// TODO(rsgowman): error handling.
115-
static TypedValue DecodeTypedValue(const std::vector<uint8_t>& bytes) {
116-
return DecodeTypedValue(bytes.data(), bytes.size());
93+
static firebase::firestore::model::FieldValue DecodeFieldValue(
94+
const std::vector<uint8_t>& bytes) {
95+
return DecodeFieldValue(bytes.data(), bytes.size());
11796
}
11897

11998
private:
12099
// TODO(rsgowman): We don't need the database_id_ yet (but will eventually).
121100
// const firebase::firestore::model::DatabaseId& database_id_;
122101
};
123102

124-
bool operator==(const Serializer::TypedValue& lhs,
125-
const Serializer::TypedValue& rhs);
126-
127103
} // namespace remote
128104
} // namespace firestore
129105
} // namespace firebase

‎Firestore/core/test/firebase/firestore/remote/serializer_test.cc

Lines changed: 13 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -63,60 +63,27 @@ class SerializerTest : public ::testing::Test {
6363
Serializer serializer;
6464

6565
void ExpectRoundTrip(const FieldValue& model,
66-
const Serializer::TypedValue& proto,
66+
const std::vector<uint8_t>& bytes,
6767
FieldValue::Type type) {
6868
EXPECT_EQ(type, model.type());
69-
EXPECT_EQ(type, proto.type);
70-
Serializer::TypedValue actual_proto = serializer.EncodeFieldValue(model);
71-
EXPECT_EQ(type, actual_proto.type);
72-
EXPECT_EQ(proto, actual_proto);
73-
EXPECT_EQ(model, serializer.DecodeFieldValue(proto));
74-
}
75-
76-
void ExpectRoundTrip(const Serializer::TypedValue& proto,
77-
std::vector<uint8_t> bytes,
78-
FieldValue::Type type) {
79-
EXPECT_EQ(type, proto.type);
8069
std::vector<uint8_t> actual_bytes;
81-
Serializer::EncodeTypedValue(proto, &actual_bytes);
70+
serializer.EncodeFieldValue(model, &actual_bytes);
8271
EXPECT_EQ(bytes, actual_bytes);
83-
Serializer::TypedValue actual_proto = Serializer::DecodeTypedValue(bytes);
84-
EXPECT_EQ(type, actual_proto.type);
85-
EXPECT_EQ(proto, actual_proto);
72+
FieldValue actual_model = serializer.DecodeFieldValue(bytes);
73+
EXPECT_EQ(type, actual_model.type());
74+
EXPECT_EQ(model, actual_model);
8675
}
8776
};
8877

89-
TEST_F(SerializerTest, EncodesNullModelToProto) {
78+
TEST_F(SerializerTest, EncodesNullModelToBytes) {
9079
FieldValue model = FieldValue::NullValue();
91-
Serializer::TypedValue proto{FieldValue::Type::Null,
92-
google_firestore_v1beta1_Value_init_default};
93-
// sanity check (the _init_default above should set this to _NULL_VALUE)
94-
EXPECT_EQ(google_protobuf_NullValue_NULL_VALUE, proto.value.null_value);
95-
ExpectRoundTrip(model, proto, FieldValue::Type::Null);
96-
}
97-
98-
TEST_F(SerializerTest, EncodesNullProtoToBytes) {
99-
Serializer::TypedValue proto{FieldValue::Type::Null,
100-
google_firestore_v1beta1_Value_init_default};
101-
// sanity check (the _init_default above should set this to _NULL_VALUE)
102-
EXPECT_EQ(google_protobuf_NullValue_NULL_VALUE, proto.value.null_value);
10380

10481
// TEXT_FORMAT_PROTO: 'null_value: NULL_VALUE'
10582
std::vector<uint8_t> bytes{0x58, 0x00};
106-
ExpectRoundTrip(proto, bytes, FieldValue::Type::Null);
83+
ExpectRoundTrip(model, bytes, FieldValue::Type::Null);
10784
}
10885

109-
TEST_F(SerializerTest, EncodesBoolModelToProto) {
110-
for (bool test : {true, false}) {
111-
FieldValue model = FieldValue::BooleanValue(test);
112-
Serializer::TypedValue proto{FieldValue::Type::Boolean,
113-
google_firestore_v1beta1_Value_init_default};
114-
proto.value.boolean_value = test;
115-
ExpectRoundTrip(model, proto, FieldValue::Type::Boolean);
116-
}
117-
}
118-
119-
TEST_F(SerializerTest, EncodesBoolProtoToBytes) {
86+
TEST_F(SerializerTest, EncodesBoolModelToBytes) {
12087
struct TestCase {
12188
bool value;
12289
std::vector<uint8_t> bytes;
@@ -128,31 +95,12 @@ TEST_F(SerializerTest, EncodesBoolProtoToBytes) {
12895
{false, {0x08, 0x00}}};
12996

13097
for (const TestCase& test : cases) {
131-
Serializer::TypedValue proto{FieldValue::Type::Boolean,
132-
google_firestore_v1beta1_Value_init_default};
133-
proto.value.boolean_value = test.value;
134-
ExpectRoundTrip(proto, test.bytes, FieldValue::Type::Boolean);
135-
}
136-
}
137-
138-
TEST_F(SerializerTest, EncodesIntegersModelToProto) {
139-
std::vector<int64_t> testCases{0,
140-
1,
141-
-1,
142-
100,
143-
-100,
144-
std::numeric_limits<int64_t>::min(),
145-
std::numeric_limits<int64_t>::max()};
146-
for (int64_t test : testCases) {
147-
FieldValue model = FieldValue::IntegerValue(test);
148-
Serializer::TypedValue proto{FieldValue::Type::Integer,
149-
google_firestore_v1beta1_Value_init_default};
150-
proto.value.integer_value = test;
151-
ExpectRoundTrip(model, proto, FieldValue::Type::Integer);
98+
FieldValue model = FieldValue::BooleanValue(test.value);
99+
ExpectRoundTrip(model, test.bytes, FieldValue::Type::Boolean);
152100
}
153101
}
154102

155-
TEST_F(SerializerTest, EncodesIntegersProtoToBytes) {
103+
TEST_F(SerializerTest, EncodesIntegersModelToBytes) {
156104
// NB: because we're calculating the bytes via protoc (instead of
157105
// libprotobuf) we have to look at values from numeric_limits<T> ahead of
158106
// time. So these test cases make the following assumptions about
@@ -198,10 +146,8 @@ TEST_F(SerializerTest, EncodesIntegersProtoToBytes) {
198146
{0x10, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f}}};
199147

200148
for (const TestCase& test : cases) {
201-
Serializer::TypedValue proto{FieldValue::Type::Integer,
202-
google_firestore_v1beta1_Value_init_default};
203-
proto.value.integer_value = test.value;
204-
ExpectRoundTrip(proto, test.bytes, FieldValue::Type::Integer);
149+
FieldValue model = FieldValue::IntegerValue(test.value);
150+
ExpectRoundTrip(model, test.bytes, FieldValue::Type::Integer);
205151
}
206152
}
207153

0 commit comments

Comments
 (0)