diff options
author | rsgowman <rgowman@google.com> | 2018-05-30 10:05:40 -0400 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-05-30 10:05:40 -0400 |
commit | 2a24b363a9351e40d80fbe7b9b92836203a147cb (patch) | |
tree | 6b719c59b9920bf3e79c10580e7fce9273b7b432 /Firestore | |
parent | bb30adaa211e7d2f7a78871046a1c142012c0ea2 (diff) |
Allow repeated entries in Value proto. (#1346)
Normally, this would be unexpected, as only a single entry in the Value
proto *should* be present. However, the proto docs state that parsers
should be able to handle repeated fields. (In the case of repeated
fields, the last one "wins".)
Diffstat (limited to 'Firestore')
-rw-r--r-- | Firestore/core/src/firebase/firestore/remote/serializer.cc | 147 | ||||
-rw-r--r-- | Firestore/core/test/firebase/firestore/remote/serializer_test.cc | 56 |
2 files changed, 139 insertions, 64 deletions
diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index ccc5ac3..03fbb1a 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -167,77 +167,96 @@ void EncodeFieldValueImpl(Writer* writer, const FieldValue& field_value) { FieldValue DecodeFieldValueImpl(Reader* reader) { if (!reader->status().ok()) return FieldValue::NullValue(); - Tag tag = reader->ReadTag(); - if (!reader->status().ok()) return FieldValue::NullValue(); + // There needs to be at least one entry in the FieldValue. + if (reader->bytes_left() == 0) { + reader->set_status(Status(FirestoreErrorCode::DataLoss, + "Input Value proto missing contents")); + return FieldValue::NullValue(); + } - // Ensure the tag matches the wire type - switch (tag.field_number) { - 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 (tag.wire_type != PB_WT_VARINT) { - reader->set_status( - Status(FirestoreErrorCode::DataLoss, - "Input proto bytes cannot be parsed (mismatch between " - "the wiretype and the field number (tag))")); - } - break; + FieldValue result = FieldValue::NullValue(); - case google_firestore_v1beta1_Value_string_value_tag: - case google_firestore_v1beta1_Value_timestamp_value_tag: - case google_firestore_v1beta1_Value_map_value_tag: - if (tag.wire_type != PB_WT_STRING) { - reader->set_status( - Status(FirestoreErrorCode::DataLoss, - "Input proto bytes cannot be parsed (mismatch between " - "the wiretype and the field number (tag))")); - } - break; + while (reader->bytes_left()) { + Tag tag = reader->ReadTag(); + if (!reader->status().ok()) return FieldValue::NullValue(); - default: - // We could get here for one of two reasons; either because the input - // bytes are corrupt, or because we're attempting to parse a tag that we - // haven't implemented yet. Long term, the latter reason should become - // less likely (especially in production), so we'll assume former. - - // TODO(rsgowman): While still in development, we'll contradict the above - // and assume the latter. Remove the following assertion when we're - // confident that we're handling all the tags in the protos. - HARD_FAIL( - "Unhandled message field number (tag): %s. (Or possibly " - "corrupt input bytes)", - tag.field_number); - reader->set_status(Status( - FirestoreErrorCode::DataLoss, - "Input proto bytes cannot be parsed (invalid field number (tag))")); - } + // Ensure the tag matches the wire type + switch (tag.field_number) { + 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 (tag.wire_type != PB_WT_VARINT) { + reader->set_status( + Status(FirestoreErrorCode::DataLoss, + "Input proto bytes cannot be parsed (mismatch between " + "the wiretype and the field number (tag))")); + } + break; - if (!reader->status().ok()) return FieldValue::NullValue(); + case google_firestore_v1beta1_Value_string_value_tag: + case google_firestore_v1beta1_Value_timestamp_value_tag: + case google_firestore_v1beta1_Value_map_value_tag: + if (tag.wire_type != PB_WT_STRING) { + reader->set_status( + Status(FirestoreErrorCode::DataLoss, + "Input proto bytes cannot be parsed (mismatch between " + "the wiretype and the field number (tag))")); + } + break; - switch (tag.field_number) { - case google_firestore_v1beta1_Value_null_value_tag: - reader->ReadNull(); - return FieldValue::NullValue(); - case google_firestore_v1beta1_Value_boolean_value_tag: - return FieldValue::BooleanValue(reader->ReadBool()); - case google_firestore_v1beta1_Value_integer_value_tag: - return FieldValue::IntegerValue(reader->ReadInteger()); - case google_firestore_v1beta1_Value_string_value_tag: - return FieldValue::StringValue(reader->ReadString()); - case google_firestore_v1beta1_Value_timestamp_value_tag: - return FieldValue::TimestampValue( - reader->ReadNestedMessage<Timestamp>(DecodeTimestamp)); - case google_firestore_v1beta1_Value_map_value_tag: - return FieldValue::ObjectValueFromMap( - reader->ReadNestedMessage<ObjectValue::Map>(DecodeMapValue)); + default: + // We could get here for one of two reasons; either because the input + // bytes are corrupt, or because we're attempting to parse a tag that we + // haven't implemented yet. Long term, the latter reason should become + // less likely (especially in production), so we'll assume former. + + // TODO(rsgowman): While still in development, we'll contradict the + // above and assume the latter. Remove the following assertion when + // we're confident that we're handling all the tags in the protos. + HARD_FAIL("Unhandled message field number (tag): %i.", + tag.field_number); + reader->set_status(Status( + FirestoreErrorCode::DataLoss, + "Input proto bytes cannot be parsed (invalid field number (tag))")); + } - default: - // This indicates an internal error as we've already ensured that this is - // a valid field_number. - HARD_FAIL( - "Somehow got an unexpected field number (tag) after verifying that " - "the field number was expected."); + if (!reader->status().ok()) return FieldValue::NullValue(); + + switch (tag.field_number) { + case google_firestore_v1beta1_Value_null_value_tag: + reader->ReadNull(); + result = FieldValue::NullValue(); + break; + case google_firestore_v1beta1_Value_boolean_value_tag: + result = FieldValue::BooleanValue(reader->ReadBool()); + break; + case google_firestore_v1beta1_Value_integer_value_tag: + result = FieldValue::IntegerValue(reader->ReadInteger()); + break; + case google_firestore_v1beta1_Value_string_value_tag: + result = FieldValue::StringValue(reader->ReadString()); + break; + case google_firestore_v1beta1_Value_timestamp_value_tag: + result = FieldValue::TimestampValue( + reader->ReadNestedMessage<Timestamp>(DecodeTimestamp)); + break; + case google_firestore_v1beta1_Value_map_value_tag: + // TODO(rsgowman): We should merge the existing map (if any) with the + // newly parsed map. + result = FieldValue::ObjectValueFromMap( + reader->ReadNestedMessage<ObjectValue::Map>(DecodeMapValue)); + break; + + default: + // This indicates an internal error as we've already ensured that this + // is a valid field_number. + HARD_FAIL( + "Somehow got an unexpected field number (tag) after verifying that " + "the field number was expected."); + } } + + return result; } /** diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc index 999acb8..96ffa9e 100644 --- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc +++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc @@ -474,6 +474,62 @@ TEST_F(SerializerTest, EncodesNestedObjects) { ExpectRoundTrip(model, proto, FieldValue::Type::Object); } +TEST_F(SerializerTest, EncodesFieldValuesWithRepeatedEntries) { + // Technically, serialized Value protos can contain multiple values. (The last + // one "wins".) However, well-behaved proto emitters (such as libprotobuf) + // won't generate that, so to test, we either need to use hand-crafted, raw + // bytes or use a proto message that's *almost* the same as the real one, such + // that when it's encoded, you can generate these repeated fields. (This is + // how libprotobuf tests itself.) + // + // Using libprotobuf for this purpose is mildly inconvenient for us, since we + // don't run protoc as part of the build process, so we'd need to either add + // these fake messages to our protos tree (Protos/testprotos?) and then check + // in the results (which isn't great when writing new tests). Fortunately, we + // have another alternative: nanopb. + // + // So we'll create a nanopb struct that *looks* like + // google_firestore_v1beta1_Value, and then populate and serialize it using + // the normal nanopb mechanisms. This should give us a wire-compatible Value + // message, but with multiple values set. + + // Copy of the real one (from the nanopb generated document.pb.h), but with + // only boolean_value and integer_value. + struct google_firestore_v1beta1_Value_Fake { + bool boolean_value; + int64_t integer_value; + }; + + // Copy of the real one (from the nanopb generated document.pb.c), but with + // only boolean_value and integer_value. + const pb_field_t google_firestore_v1beta1_Value_fields_Fake[2] = { + PB_FIELD(1, BOOL, SINGULAR, STATIC, FIRST, + google_firestore_v1beta1_Value_Fake, boolean_value, + boolean_value, 0), + PB_FIELD(2, INT64, SINGULAR, STATIC, OTHER, + google_firestore_v1beta1_Value_Fake, integer_value, + boolean_value, 0), + }; + + // Craft the bytes. boolean_value has a smaller tag, so it'll get encoded + // first. Implying integer_value should "win". + google_firestore_v1beta1_Value_Fake crafty_value{false, int64_t{42}}; + std::vector<uint8_t> bytes(128); + pb_ostream_t stream = pb_ostream_from_buffer(bytes.data(), bytes.size()); + pb_encode(&stream, google_firestore_v1beta1_Value_fields_Fake, &crafty_value); + bytes.resize(stream.bytes_written); + + // Decode the bytes into the model + StatusOr<FieldValue> actual_model_status = serializer.DecodeFieldValue(bytes); + EXPECT_OK(actual_model_status); + FieldValue actual_model = actual_model_status.ValueOrDie(); + + // Ensure the decoded model is as expected. + FieldValue expected_model = FieldValue::IntegerValue(42); + EXPECT_EQ(FieldValue::Type::Integer, actual_model.type()); + EXPECT_EQ(expected_model, actual_model); +} + TEST_F(SerializerTest, BadNullValue) { std::vector<uint8_t> bytes = EncodeFieldValue(&serializer, FieldValue::NullValue()); |