From e318923d318151e503de03be53aef1a0cffc58e3 Mon Sep 17 00:00:00 2001 From: rsgowman Date: Mon, 28 May 2018 16:42:23 -0400 Subject: "Handle" BatchGetDocumentsResponse.transaction (by ignoring it) (#1318) --- .../core/test/firebase/firestore/remote/serializer_test.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) (limited to 'Firestore/core/test/firebase/firestore/remote') diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc index ba9ea47..db13228 100644 --- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc +++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc @@ -230,20 +230,21 @@ class SerializerTest : public ::testing::Test { /** * Creates entries in the proto that we don't care about. * - * We ignore create time in our serializer. We never set it, and never read it - * (other than to throw it away). But the server could (and probably does) set - * it, so we need to be able to discard it properly. The ExpectRoundTrip deals - * with this asymmetry. + * We ignore certain fields in our serializer. We never set them, and never + * read them (other than to throw them away). But the server could (and + * probably does) set them, so we need to be able to discard them properly. + * The ExpectRoundTrip deals with this asymmetry. * * This method adds these ignored fields to the proto. */ void TouchIgnoredBatchGetDocumentsResponseFields( v1beta1::BatchGetDocumentsResponse* proto) { + proto->set_transaction("random bytes"); + // TODO(rsgowman): This method currently assumes that this is a 'found' // document. We (probably) will need to adjust this to work with NoDocuments // too. v1beta1::Document* doc_proto = proto->mutable_found(); - google::protobuf::Timestamp* create_time_proto = doc_proto->mutable_create_time(); create_time_proto->set_seconds(8765); -- cgit v1.2.3 From 3fd7b456d5b58481fada7a31305bb21bb64dbd06 Mon Sep 17 00:00:00 2001 From: rsgowman Date: Tue, 29 May 2018 10:33:10 -0400 Subject: Handle deserializing BatchGetDocumentsResponse error case... (#1344) ... where neither 'found' nor 'missing' fields set. --- .../src/firebase/firestore/remote/serializer.cc | 7 ++-- .../firebase/firestore/remote/serializer_test.cc | 47 +++++++++++++++------- 2 files changed, 37 insertions(+), 17 deletions(-) (limited to 'Firestore/core/test/firebase/firestore/remote') diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index e06dcb4..ccc5ac3 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -583,9 +583,10 @@ std::unique_ptr Serializer::DecodeBatchGetDocumentsResponse( } else if (!missing.empty()) { return absl::make_unique(DecodeKey(missing), read_time); } else { - // Neither 'found' nor 'missing' fields were set. - // TODO(rsgowman): Handle the error case. - abort(); + reader->set_status(Status(FirestoreErrorCode::DataLoss, + "Invalid BatchGetDocumentsReponse message: " + "Neither 'found' nor 'missing' fields set.")); + return nullptr; } } diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc index db13228..999acb8 100644 --- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc +++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc @@ -146,13 +146,21 @@ class SerializerTest : public ::testing::Test { * * @param status the expected (failed) status. Only the code() is verified. */ - void ExpectFailedStatusDuringDecode(Status status, - const std::vector& bytes) { + void ExpectFailedStatusDuringFieldValueDecode( + Status status, const std::vector& bytes) { StatusOr bad_status = serializer.DecodeFieldValue(bytes); ASSERT_NOT_OK(bad_status); EXPECT_EQ(status.code(), bad_status.status().code()); } + void ExpectFailedStatusDuringMaybeDocumentDecode( + Status status, const std::vector& bytes) { + StatusOr> bad_status = + serializer.DecodeMaybeDocument(bytes); + ASSERT_NOT_OK(bad_status); + EXPECT_EQ(status.code(), bad_status.status().code()); + } + v1beta1::Value ValueProto(nullptr_t) { std::vector bytes = EncodeFieldValue(&serializer, FieldValue::NullValue()); @@ -473,7 +481,7 @@ TEST_F(SerializerTest, BadNullValue) { // Alter the null value from 0 to 1. Mutate(&bytes[1], /*expected_initial_value=*/0, /*new_value=*/1); - ExpectFailedStatusDuringDecode( + ExpectFailedStatusDuringFieldValueDecode( Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); } @@ -484,7 +492,7 @@ TEST_F(SerializerTest, BadBoolValue) { // Alter the bool value from 1 to 2. (Value values are 0,1) Mutate(&bytes[1], /*expected_initial_value=*/1, /*new_value=*/2); - ExpectFailedStatusDuringDecode( + ExpectFailedStatusDuringFieldValueDecode( Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); } @@ -503,7 +511,7 @@ TEST_F(SerializerTest, BadIntegerValue) { bytes.resize(12); bytes[11] = 0x7f; - ExpectFailedStatusDuringDecode( + ExpectFailedStatusDuringFieldValueDecode( Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); } @@ -515,7 +523,7 @@ TEST_F(SerializerTest, BadStringValue) { // used by the encoded tag.) Mutate(&bytes[2], /*expected_initial_value=*/1, /*new_value=*/5); - ExpectFailedStatusDuringDecode( + ExpectFailedStatusDuringFieldValueDecode( Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); } @@ -526,7 +534,7 @@ TEST_F(SerializerTest, BadTimestampValue_TooLarge) { // Add some time, which should push us above the maximum allowed timestamp. Mutate(&bytes[4], 0x82, 0x83); - ExpectFailedStatusDuringDecode( + ExpectFailedStatusDuringFieldValueDecode( Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); } @@ -537,7 +545,7 @@ TEST_F(SerializerTest, BadTimestampValue_TooSmall) { // Remove some time, which should push us below the minimum allowed timestamp. Mutate(&bytes[4], 0x92, 0x91); - ExpectFailedStatusDuringDecode( + ExpectFailedStatusDuringFieldValueDecode( Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); } @@ -556,9 +564,9 @@ TEST_F(SerializerTest, BadTag) { // status. Remove this EXPECT_ANY_THROW statement (and reenable the // following commented out statement) once the corresponding assert has been // removed from serializer.cc. - EXPECT_ANY_THROW(ExpectFailedStatusDuringDecode( + EXPECT_ANY_THROW(ExpectFailedStatusDuringFieldValueDecode( Status(FirestoreErrorCode::DataLoss, "ignored"), bytes)); - // ExpectFailedStatusDuringDecode( + // ExpectFailedStatusDuringFieldValueDecode( // Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); } @@ -571,7 +579,7 @@ TEST_F(SerializerTest, TagVarintWiretypeStringMismatch) { // would do.) Mutate(&bytes[0], /*expected_initial_value=*/0x08, /*new_value=*/0x0a); - ExpectFailedStatusDuringDecode( + ExpectFailedStatusDuringFieldValueDecode( Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); } @@ -582,7 +590,7 @@ TEST_F(SerializerTest, TagStringWiretypeVarintMismatch) { // 0x88 represents a string value encoded as a varint. Mutate(&bytes[0], /*expected_initial_value=*/0x8a, /*new_value=*/0x88); - ExpectFailedStatusDuringDecode( + ExpectFailedStatusDuringFieldValueDecode( Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); } @@ -595,13 +603,13 @@ TEST_F(SerializerTest, IncompleteFieldValue) { ASSERT_EQ(0x00, bytes[1]); bytes.pop_back(); - ExpectFailedStatusDuringDecode( + ExpectFailedStatusDuringFieldValueDecode( Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); } TEST_F(SerializerTest, IncompleteTag) { std::vector bytes; - ExpectFailedStatusDuringDecode( + ExpectFailedStatusDuringFieldValueDecode( Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); } @@ -714,5 +722,16 @@ TEST_F(SerializerTest, DecodesNoDocument) { ExpectNoDocumentDeserializationRoundTrip(key, read_time, proto); } +TEST_F(SerializerTest, DecodeMaybeDocWithoutFoundOrMissingSetShouldFail) { + v1beta1::BatchGetDocumentsResponse proto; + + std::vector bytes(proto.ByteSizeLong()); + bool status = proto.SerializeToArray(bytes.data(), bytes.size()); + EXPECT_TRUE(status); + + ExpectFailedStatusDuringMaybeDocumentDecode( + Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); +} + // TODO(rsgowman): Test [en|de]coding multiple protos into the same output // vector. -- cgit v1.2.3 From 2a24b363a9351e40d80fbe7b9b92836203a147cb Mon Sep 17 00:00:00 2001 From: rsgowman Date: Wed, 30 May 2018 10:05:40 -0400 Subject: 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".) --- .../src/firebase/firestore/remote/serializer.cc | 147 ++++++++++++--------- .../firebase/firestore/remote/serializer_test.cc | 56 ++++++++ 2 files changed, 139 insertions(+), 64 deletions(-) (limited to 'Firestore/core/test/firebase/firestore/remote') 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(DecodeTimestamp)); - case google_firestore_v1beta1_Value_map_value_tag: - return FieldValue::ObjectValueFromMap( - reader->ReadNestedMessage(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(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(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 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 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 bytes = EncodeFieldValue(&serializer, FieldValue::NullValue()); -- cgit v1.2.3 From 26b8ac9ee43bb67b08e4bc62af98ac6bda2f121c Mon Sep 17 00:00:00 2001 From: rsgowman Date: Mon, 4 Jun 2018 15:31:10 -0400 Subject: Skip unknown fields while decoding FieldValue proto objects. (#1354) --- .../core/src/firebase/firestore/nanopb/reader.cc | 8 +++ .../core/src/firebase/firestore/nanopb/reader.h | 9 +++ .../src/firebase/firestore/remote/serializer.cc | 40 +++++++------ .../firebase/firestore/remote/serializer_test.cc | 65 ++++++++++++++++++---- 4 files changed, 95 insertions(+), 27 deletions(-) (limited to 'Firestore/core/test/firebase/firestore/remote') diff --git a/Firestore/core/src/firebase/firestore/nanopb/reader.cc b/Firestore/core/src/firebase/firestore/nanopb/reader.cc index ec4282d..69e3d83 100644 --- a/Firestore/core/src/firebase/firestore/nanopb/reader.cc +++ b/Firestore/core/src/firebase/firestore/nanopb/reader.cc @@ -136,6 +136,14 @@ std::string Reader::ReadString() { return result; } +void Reader::SkipField(const Tag& tag) { + if (!status_.ok()) return; + + if (!pb_skip_field(&stream_, tag.wire_type)) { + status_ = Status(FirestoreErrorCode::DataLoss, PB_GET_ERROR(&stream_)); + } +} + } // namespace nanopb } // namespace firestore } // namespace firebase diff --git a/Firestore/core/src/firebase/firestore/nanopb/reader.h b/Firestore/core/src/firebase/firestore/nanopb/reader.h index 930211a..2c16ec4 100644 --- a/Firestore/core/src/firebase/firestore/nanopb/reader.h +++ b/Firestore/core/src/firebase/firestore/nanopb/reader.h @@ -89,6 +89,15 @@ class Reader { template T ReadNestedMessage(const std::function& read_message_fn); + /** + * Discards the bytes associated with the given tag. + * + * @param tag The tag associated with the field that is otherwise about to be + * read. This method uses the tag to determine how many bytes should be + * discarded. + */ + void SkipField(const Tag& tag); + size_t bytes_left() const { return stream_.bytes_left; } diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index 0839efc..eee56f8 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -204,20 +204,20 @@ FieldValue DecodeFieldValueImpl(Reader* reader) { } break; - 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. + case google_firestore_v1beta1_Value_double_value_tag: + case google_firestore_v1beta1_Value_bytes_value_tag: + case google_firestore_v1beta1_Value_reference_value_tag: + case google_firestore_v1beta1_Value_geo_point_value_tag: + case google_firestore_v1beta1_Value_array_value_tag: + // TODO(b/74243929): Implement remaining types. 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: + // Unknown tag. According to the proto spec, we need to ignore these. No + // action required here, though we'll need to skip the relevant bytes + // below. + break; } if (!reader->status().ok()) return FieldValue::NullValue(); @@ -247,12 +247,18 @@ FieldValue DecodeFieldValueImpl(Reader* reader) { reader->ReadNestedMessage(DecodeMapValue)); break; + case google_firestore_v1beta1_Value_double_value_tag: + case google_firestore_v1beta1_Value_bytes_value_tag: + case google_firestore_v1beta1_Value_reference_value_tag: + case google_firestore_v1beta1_Value_geo_point_value_tag: + case google_firestore_v1beta1_Value_array_value_tag: + // TODO(b/74243929): Implement remaining types. + HARD_FAIL("Unhandled message field number (tag): %i.", + tag.field_number); + 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."); + // Unknown tag. According to the proto spec, we need to ignore these. + reader->SkipField(tag); } } diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc index 96ffa9e..7dcdf6e 100644 --- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc +++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc @@ -605,7 +605,12 @@ TEST_F(SerializerTest, BadTimestampValue_TooSmall) { Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); } -TEST_F(SerializerTest, BadTag) { +TEST_F(SerializerTest, BadFieldValueTagAndNoOtherTagPresent) { + // A bad tag should be ignored. But if there are *no* valid tags, then we + // don't know the type of the FieldValue. Although it might be reasonable to + // assume some sort of default type in this situation, we've decided to fail + // the deserialization process in this case instead. + std::vector bytes = EncodeFieldValue(&serializer, FieldValue::NullValue()); @@ -615,15 +620,55 @@ TEST_F(SerializerTest, BadTag) { // Specifically 31. 0xf8 represents field number 31 encoded as a varint. Mutate(&bytes[0], /*expected_initial_value=*/0x58, /*new_value=*/0xf8); - // TODO(rsgowman): The behaviour is *temporarily* slightly different during - // development; this will cause a failed assertion rather than a failed - // status. Remove this EXPECT_ANY_THROW statement (and reenable the - // following commented out statement) once the corresponding assert has been - // removed from serializer.cc. - EXPECT_ANY_THROW(ExpectFailedStatusDuringFieldValueDecode( - Status(FirestoreErrorCode::DataLoss, "ignored"), bytes)); - // ExpectFailedStatusDuringFieldValueDecode( - // Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); + ExpectFailedStatusDuringFieldValueDecode( + Status(FirestoreErrorCode::DataLoss, "ignored"), bytes); +} + +TEST_F(SerializerTest, BadFieldValueTagWithOtherValidTagsPresent) { + // A bad tag should be ignored, in which case, we should successfully + // deserialize the rest of the bytes as if it wasn't there. To craft these + // bytes, we'll use the same technique as + // EncodesFieldValuesWithRepeatedEntries (so go read the comments there + // first). + + // 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. Also modified such that integer_value + // now has an invalid tag (instead of 2). + const int invalid_tag = 31; + 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(invalid_tag, 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, normally implying integer_value should "win". Except that + // integer_value isn't a valid tag, so it should be ignored here. + google_firestore_v1beta1_Value_Fake crafty_value{false, int64_t{42}}; + std::vector 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 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::BooleanValue(false); + EXPECT_EQ(FieldValue::Type::Boolean, actual_model.type()); + EXPECT_EQ(expected_model, actual_model); } TEST_F(SerializerTest, TagVarintWiretypeStringMismatch) { -- cgit v1.2.3 From 9307f4893008f7d6cf9473e906d4c896546c5c8c Mon Sep 17 00:00:00 2001 From: rsgowman Date: Tue, 5 Jun 2018 12:24:14 -0400 Subject: Skip unknown fields while decoding BatchGetDocumentsResponse proto objects (#1377) --- .../src/firebase/firestore/remote/serializer.cc | 14 +++-- .../firebase/firestore/remote/serializer_test.cc | 62 ++++++++++++++++++++++ 2 files changed, 68 insertions(+), 8 deletions(-) (limited to 'Firestore/core/test/firebase/firestore/remote') diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index eee56f8..19a068b 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -551,9 +551,10 @@ std::unique_ptr Serializer::DecodeBatchGetDocumentsResponse( break; default: - reader->set_status(Status( - FirestoreErrorCode::DataLoss, - "Input proto bytes cannot be parsed (invalid field number (tag))")); + // Unknown tag. According to the proto spec, we need to ignore these. No + // action required here, though we'll need to skip the relevant bytes + // below. + break; } if (!reader->status().ok()) return nullptr; @@ -595,11 +596,8 @@ std::unique_ptr Serializer::DecodeBatchGetDocumentsResponse( 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."); + // Unknown tag. According to the proto spec, we need to ignore these. + reader->SkipField(tag); } } diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc index 7dcdf6e..a902b5d 100644 --- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc +++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc @@ -804,6 +804,68 @@ TEST_F(SerializerTest, EncodesNonEmptyDocument) { ExpectRoundTrip(key, fields, update_time, proto); } +TEST_F(SerializerTest, + BadBatchGetDocumentsResponseTagWithOtherValidTagsPresent) { + // A bad tag should be ignored, in which case, we should successfully + // deserialize the rest of the bytes as if it wasn't there. To craft these + // bytes, we'll use the same technique as + // EncodesFieldValuesWithRepeatedEntries (so go read the comments there + // first). + + // Copy of the real one (from the nanopb generated firestore.pb.h), but with + // only "missing" (a field from the original proto) and an extra_field. + struct google_firestore_v1beta1_BatchGetDocumentsResponse_Fake { + pb_callback_t missing; + int64_t extra_field; + }; + + // Copy of the real one (from the nanopb generated firestore.pb.c), but with + // only missing and an extra_field. Also modified such that extra_field + // now has a tag of 31. + const int invalid_tag = 31; + const pb_field_t + google_firestore_v1beta1_BatchGetDocumentsResponse_fields_Fake[2] = { + PB_FIELD(2, STRING, SINGULAR, CALLBACK, FIRST, + google_firestore_v1beta1_BatchGetDocumentsResponse_Fake, + missing, missing, 0), + PB_FIELD(invalid_tag, INT64, SINGULAR, STATIC, OTHER, + google_firestore_v1beta1_BatchGetDocumentsResponse_Fake, + extra_field, missing, 0), + }; + + const char* missing_value = "projects/p/databases/d/documents/one/two"; + google_firestore_v1beta1_BatchGetDocumentsResponse_Fake crafty_value; + crafty_value.missing.funcs.encode = + [](pb_ostream_t* stream, const pb_field_t* field, void* const* arg) { + const char* missing_value = static_cast(*arg); + if (!pb_encode_tag_for_field(stream, field)) return false; + return pb_encode_string(stream, + reinterpret_cast(missing_value), + strlen(missing_value)); + }; + crafty_value.missing.arg = const_cast(missing_value); + crafty_value.extra_field = 42; + + std::vector bytes(128); + pb_ostream_t stream = pb_ostream_from_buffer(bytes.data(), bytes.size()); + pb_encode(&stream, + google_firestore_v1beta1_BatchGetDocumentsResponse_fields_Fake, + &crafty_value); + bytes.resize(stream.bytes_written); + + // Decode the bytes into the model + StatusOr> actual_model_status = + serializer.DecodeMaybeDocument(bytes); + EXPECT_OK(actual_model_status); + std::unique_ptr actual_model = + std::move(actual_model_status).ValueOrDie(); + + // Ensure the decoded model is as expected. + NoDocument expected_model = + NoDocument(Key("one/two"), SnapshotVersion::None()); + EXPECT_EQ(expected_model, *actual_model.get()); +} + TEST_F(SerializerTest, DecodesNoDocument) { // We can't actually *encode* a NoDocument; the method exposed by the // serializer requires both the document key and contents (as an ObjectValue, -- cgit v1.2.3