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') 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') 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') 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 bb546e19885ae084823e0315e93564a44c0a8257 Mon Sep 17 00:00:00 2001 From: Gil Date: Fri, 1 Jun 2018 13:42:47 -0700 Subject: Fix Firestore compilation under Xcode < 9.2 (#1367) * Don't rely on specialization failure to determine when std::hash is unavailable. Instead manually declare the conditions under which std::hash should be defined. * Fix detection of Objective-C classes in Xcode < 9.2 std::is_base_of{} is false there so the overloads defined for Objective-C types weren't getting enabled. * Add explicit tests for StringFormat using Objective-C objects * Add explicit tests for HasStdHash --- .../Example/Firestore.xcodeproj/project.pbxproj | 8 ++ Firestore/Source/Remote/FSTSerializerBeta.mm | 2 +- .../src/firebase/firestore/util/CMakeLists.txt | 1 + .../core/src/firebase/firestore/util/hashing.h | 39 +++++++++- .../src/firebase/firestore/util/string_format.h | 28 +++---- .../core/src/firebase/firestore/util/type_traits.h | 90 ++++++++++++++++++++++ .../test/firebase/firestore/util/CMakeLists.txt | 9 +++ .../test/firebase/firestore/util/hashing_test.cc | 18 +++++ .../firestore/util/string_format_apple_test.mm | 60 +++++++++++++++ .../firestore/util/type_traits_apple_test.mm | 50 ++++++++++++ 10 files changed, 283 insertions(+), 22 deletions(-) create mode 100644 Firestore/core/src/firebase/firestore/util/type_traits.h create mode 100644 Firestore/core/test/firebase/firestore/util/string_format_apple_test.mm create mode 100644 Firestore/core/test/firebase/firestore/util/type_traits_apple_test.mm (limited to 'Firestore/core/test') diff --git a/Firestore/Example/Firestore.xcodeproj/project.pbxproj b/Firestore/Example/Firestore.xcodeproj/project.pbxproj index ca8b598..9207ad2 100644 --- a/Firestore/Example/Firestore.xcodeproj/project.pbxproj +++ b/Firestore/Example/Firestore.xcodeproj/project.pbxproj @@ -24,6 +24,7 @@ /* End PBXAggregateTarget section */ /* Begin PBXBuildFile section */ + 0535C1B65DADAE1CE47FA3CA /* string_format_apple_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = 9CFD366B783AE27B9E79EE7A /* string_format_apple_test.mm */; }; 132E3E53179DE287D875F3F2 /* FSTLevelDBTransactionTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 132E36BB104830BD806351AC /* FSTLevelDBTransactionTests.mm */; }; 3B843E4C1F3A182900548890 /* remote_store_spec_test.json in Resources */ = {isa = PBXBuildFile; fileRef = 3B843E4A1F3930A400548890 /* remote_store_spec_test.json */; }; 54131E9720ADE679001DF3FF /* string_format_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54131E9620ADE678001DF3FF /* string_format_test.cc */; }; @@ -180,6 +181,7 @@ B6FB4690208F9BB300554BA2 /* executor_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B6FB4688208F9B9100554BA2 /* executor_test.cc */; }; BF219E98F1C5A1DAEB5EEC86 /* Pods_Firestore_Example_iOS_SwiftBuildTest.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 379B34A1536045869826D82A /* Pods_Firestore_Example_iOS_SwiftBuildTest.framework */; }; C1AA536F90A0A576CA2816EB /* Pods_Firestore_Example_iOS_Firestore_SwiftTests_iOS.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = BB92EB03E3F92485023F64ED /* Pods_Firestore_Example_iOS_Firestore_SwiftTests_iOS.framework */; }; + C80B10E79CDD7EF7843C321E /* type_traits_apple_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = 2A0CF41BA5AED6049B0BEB2C /* type_traits_apple_test.mm */; }; C8D3CE2343E53223E6487F2C /* Pods_Firestore_Example_iOS.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 5918805E993304321A05E82B /* Pods_Firestore_Example_iOS.framework */; }; DE03B2D41F2149D600A30B9C /* XCTest.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 6003F5AF195388D20070C39A /* XCTest.framework */; }; DE03B2D51F2149D600A30B9C /* UIKit.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 6003F591195388D20070C39A /* UIKit.framework */; }; @@ -251,6 +253,7 @@ 1277F98C20D2DF0867496976 /* Pods-Firestore_IntegrationTests_iOS.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Firestore_IntegrationTests_iOS.debug.xcconfig"; path = "Pods/Target Support Files/Pods-Firestore_IntegrationTests_iOS/Pods-Firestore_IntegrationTests_iOS.debug.xcconfig"; sourceTree = ""; }; 12F4357299652983A615F886 /* LICENSE */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text; name = LICENSE; path = ../LICENSE; sourceTree = ""; }; 132E36BB104830BD806351AC /* FSTLevelDBTransactionTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = FSTLevelDBTransactionTests.mm; sourceTree = ""; }; + 2A0CF41BA5AED6049B0BEB2C /* type_traits_apple_test.mm */ = {isa = PBXFileReference; includeInIndex = 1; path = type_traits_apple_test.mm; sourceTree = ""; }; 2B50B3A0DF77100EEE887891 /* Pods_Firestore_Tests_iOS.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_Firestore_Tests_iOS.framework; sourceTree = BUILT_PRODUCTS_DIR; }; 379B34A1536045869826D82A /* Pods_Firestore_Example_iOS_SwiftBuildTest.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_Firestore_Example_iOS_SwiftBuildTest.framework; sourceTree = BUILT_PRODUCTS_DIR; }; 3B843E4A1F3930A400548890 /* remote_store_spec_test.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = remote_store_spec_test.json; sourceTree = ""; }; @@ -399,6 +402,7 @@ 74ACEC3603BE58B57A7E8D4C /* Pods-Firestore_Example_iOS-SwiftBuildTest.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Firestore_Example_iOS-SwiftBuildTest.release.xcconfig"; path = "Pods/Target Support Files/Pods-Firestore_Example_iOS-SwiftBuildTest/Pods-Firestore_Example_iOS-SwiftBuildTest.release.xcconfig"; sourceTree = ""; }; 873B8AEA1B1F5CCA007FD442 /* Main.storyboard */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = file.storyboard; name = Main.storyboard; path = Base.lproj/Main.storyboard; sourceTree = ""; }; 8E002F4AD5D9B6197C940847 /* Firestore.podspec */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text; name = Firestore.podspec; path = ../Firestore.podspec; sourceTree = ""; }; + 9CFD366B783AE27B9E79EE7A /* string_format_apple_test.mm */ = {isa = PBXFileReference; includeInIndex = 1; path = string_format_apple_test.mm; sourceTree = ""; }; AB356EF6200EA5EB0089B766 /* field_value_test.cc */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = field_value_test.cc; sourceTree = ""; }; AB380CF82019382300D97691 /* target_id_generator_test.cc */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = target_id_generator_test.cc; sourceTree = ""; }; AB380CFC201A2EE200D97691 /* string_util_test.cc */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = string_util_test.cc; sourceTree = ""; }; @@ -558,8 +562,10 @@ 54A0352C20A3B3D7003E0143 /* status_test.cc */, 54A0352B20A3B3D7003E0143 /* status_test_util.h */, 54A0352D20A3B3D7003E0143 /* statusor_test.cc */, + 9CFD366B783AE27B9E79EE7A /* string_format_apple_test.mm */, 54131E9620ADE678001DF3FF /* string_format_test.cc */, AB380CFC201A2EE200D97691 /* string_util_test.cc */, + 2A0CF41BA5AED6049B0BEB2C /* type_traits_apple_test.mm */, ); path = util; sourceTree = ""; @@ -1557,6 +1563,7 @@ 549CCA5020A36DBC00BCEB75 /* sorted_set_test.cc in Sources */, 54A0352F20A3B3D8003E0143 /* status_test.cc in Sources */, 54A0353020A3B3D8003E0143 /* statusor_test.cc in Sources */, + 0535C1B65DADAE1CE47FA3CA /* string_format_apple_test.mm in Sources */, 54131E9720ADE679001DF3FF /* string_format_test.cc in Sources */, AB380CFE201A2F4500D97691 /* string_util_test.cc in Sources */, AB380CFB2019388600D97691 /* target_id_generator_test.cc in Sources */, @@ -1565,6 +1572,7 @@ ABC1D7E12023A40C00BA84F0 /* token_test.cc in Sources */, 54A0352720A3AED0003E0143 /* transform_operations_test.mm in Sources */, 549CCA5120A36DBC00BCEB75 /* tree_sorted_map_test.cc in Sources */, + C80B10E79CDD7EF7843C321E /* type_traits_apple_test.mm in Sources */, ABC1D7DE2023A05300BA84F0 /* user_test.cc in Sources */, ); runOnlyForDeploymentPostprocessing = 0; diff --git a/Firestore/Source/Remote/FSTSerializerBeta.mm b/Firestore/Source/Remote/FSTSerializerBeta.mm index ab40dd6..263fe6d 100644 --- a/Firestore/Source/Remote/FSTSerializerBeta.mm +++ b/Firestore/Source/Remote/FSTSerializerBeta.mm @@ -921,7 +921,7 @@ NS_ASSUME_NONNULL_BEGIN } else if ([filter isKindOfClass:[FSTNullFilter class]]) { proto.unaryFilter.op = GCFSStructuredQuery_UnaryFilter_Operator_IsNull; } else { - HARD_FAIL("Unrecognized filter: %s", static_cast(filter)); + HARD_FAIL("Unrecognized filter: %s", filter); } return proto; } diff --git a/Firestore/core/src/firebase/firestore/util/CMakeLists.txt b/Firestore/core/src/firebase/firestore/util/CMakeLists.txt index 043713f..ed3a301 100644 --- a/Firestore/core/src/firebase/firestore/util/CMakeLists.txt +++ b/Firestore/core/src/firebase/firestore/util/CMakeLists.txt @@ -200,6 +200,7 @@ cc_library( statusor_internals.h string_util.cc string_util.h + type_traits.h DEPENDS absl_base firebase_firestore_util_base diff --git a/Firestore/core/src/firebase/firestore/util/hashing.h b/Firestore/core/src/firebase/firestore/util/hashing.h index d8058c8..21c0bd6 100644 --- a/Firestore/core/src/firebase/firestore/util/hashing.h +++ b/Firestore/core/src/firebase/firestore/util/hashing.h @@ -18,6 +18,7 @@ #define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_HASHING_H_ #include +#include #include namespace firebase { @@ -48,6 +49,41 @@ namespace util { namespace impl { +/** + * A type trait that identifies whether or not std::hash is available for a + * given type. + * + * This type should not be necessary since specialization failure on an + * expression like `decltype(std::hash{}(value)` should be enough to disable + * overloads that require `std::hash` to be defined but unfortunately some + * standard libraries ship with std::hash defined for all types that only + * fail later (e.g. via static_assert). One such implementation is the libc++ + * that ships with Xcode 8.3.3, which is a supported platform. + */ +template +struct has_std_hash { + // There may be other types for which std::hash is defined but they don't + // matter for our purposes. + enum { + value = std::is_arithmetic{} || std::is_pointer{} || + std::is_same{} + }; + + constexpr operator bool() const { + return value; + } +}; + +/** + * A type that's equivalent to size_t if std::hash is defined or a compile + * error otherwise. + * + * This is effectively just a safe implementation of + * `decltype(std::hash{}(std::declval()))`. + */ +template +using std_hash_type = typename std::enable_if{}, size_t>::type; + /** * Combines a hash_value with whatever accumulated state there is so far. */ @@ -100,8 +136,7 @@ auto RankedInvokeHash(const K& value, HashChoice<0>) -> decltype(value.Hash()) { * @return The result of `std::hash{}(value)` */ template -auto RankedInvokeHash(const K& value, HashChoice<1>) - -> decltype(std::hash{}(value)) { +std_hash_type RankedInvokeHash(const K& value, HashChoice<1>) { return std::hash{}(value); } diff --git a/Firestore/core/src/firebase/firestore/util/string_format.h b/Firestore/core/src/firebase/firestore/util/string_format.h index d691984..01776a9 100644 --- a/Firestore/core/src/firebase/firestore/util/string_format.h +++ b/Firestore/core/src/firebase/firestore/util/string_format.h @@ -22,6 +22,7 @@ #include #include "Firestore/core/src/firebase/firestore/util/string_apple.h" +#include "Firestore/core/src/firebase/firestore/util/type_traits.h" #include "absl/base/attributes.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" @@ -43,7 +44,7 @@ template struct FormatChoice : FormatChoice {}; template <> -struct FormatChoice<4> {}; +struct FormatChoice<5> {}; } // namespace internal @@ -87,8 +88,8 @@ class FormatArg : public absl::AlphaNum { */ template < typename T, - typename = typename std::enable_if{}>::type> - FormatArg(T* object, internal::FormatChoice<0>) + typename = typename std::enable_if{}>::type> + FormatArg(T object, internal::FormatChoice<1>) : AlphaNum{MakeStringView([object description])} { } @@ -96,20 +97,9 @@ class FormatArg : public absl::AlphaNum { * Creates a FormatArg from any Objective-C Class type. Objective-C Class * types are a special struct that aren't of a type derived from NSObject. */ - FormatArg(Class object, internal::FormatChoice<0>) + FormatArg(Class object, internal::FormatChoice<1>) : AlphaNum{MakeStringView(NSStringFromClass(object))} { } - - /** - * Creates a FormatArg from any id pointer. Note that instances of `id` - * (which means "pointer conforming to the protocol Foo") do not match this - * without first casting to type `id`. There's no way to express a template of - * `id` since `id` isn't actually a C++ template and `id` isn't a - * parameterized C++ class. - */ - FormatArg(id object, internal::FormatChoice<0>) - : AlphaNum{MakeStringView([object description])} { - } #endif /** @@ -117,7 +107,7 @@ class FormatArg : public absl::AlphaNum { * handled specially to avoid ambiguity with generic pointers, which are * handled differently. */ - FormatArg(std::nullptr_t, internal::FormatChoice<1>) : AlphaNum{"null"} { + FormatArg(std::nullptr_t, internal::FormatChoice<2>) : AlphaNum{"null"} { } /** @@ -125,7 +115,7 @@ class FormatArg : public absl::AlphaNum { * handled specially to avoid ambiguity with generic pointers, which are * handled differently. */ - FormatArg(const char* string_value, internal::FormatChoice<2>) + FormatArg(const char* string_value, internal::FormatChoice<3>) : AlphaNum{string_value == nullptr ? "null" : string_value} { } @@ -134,7 +124,7 @@ class FormatArg : public absl::AlphaNum { * hexidecimal integer literal. */ template - FormatArg(T* pointer_value, internal::FormatChoice<3>) + FormatArg(T* pointer_value, internal::FormatChoice<4>) : AlphaNum{absl::Hex{reinterpret_cast(pointer_value)}} { } @@ -143,7 +133,7 @@ class FormatArg : public absl::AlphaNum { * absl::AlphaNum accepts. */ template - FormatArg(T&& value, internal::FormatChoice<4>) + FormatArg(T&& value, internal::FormatChoice<5>) : AlphaNum{std::forward(value)} { } }; diff --git a/Firestore/core/src/firebase/firestore/util/type_traits.h b/Firestore/core/src/firebase/firestore/util/type_traits.h new file mode 100644 index 0000000..52feb6b --- /dev/null +++ b/Firestore/core/src/firebase/firestore/util/type_traits.h @@ -0,0 +1,90 @@ +/* + * Copyright 2018 Google + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_TYPE_TRAITS_H_ +#define FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_TYPE_TRAITS_H_ + +#if __OBJC__ +#import // for id +#endif + +#include + +namespace firebase { +namespace firestore { +namespace util { + +#if __OBJC__ + +/** + * A type trait that identifies whether or not the given pointer points to an + * Objective-C object. + * + * is_objective_c_pointer::value == true + * is_objective_c_pointer*>::value == true + * + * // id is a dynamically typed pointer to an Objective-C object. + * is_objective_c_pointer::value == true + * + * // pointers to C++ classes are not Objective-C pointers. + * is_objective_c_pointer::value == false + * is_objective_c_pointer::value == false + * is_objective_c_pointer>::value == false + */ +template +struct is_objective_c_pointer { + private: + using yes_type = char (&)[10]; + using no_type = char (&)[1]; + + /** + * A non-existent function declared to produce a pointer to type T (which is + * consistent with the way Objective-C objects are referenced). + * + * Note that there is no definition for this function but that's okay because + * we only need it to reason about the function's type at compile type. + */ + static T Pointer(); + + static yes_type Choose(id value); + static no_type Choose(...); + + public: + using value_type = bool; + + enum { value = sizeof(Choose(Pointer())) == sizeof(yes_type) }; + + constexpr operator bool() const { + return value; + } + + constexpr bool operator()() const { + return value; + } +}; + +// Hard-code the answer for `void` because you can't pass arguments of type +// `void` to another function. +template <> +struct is_objective_c_pointer : public std::false_type {}; + +#endif // __OBJC__ + +} // namespace util +} // namespace firestore +} // namespace firebase + +#endif // FIRESTORE_CORE_SRC_FIREBASE_FIRESTORE_UTIL_TYPE_TRAITS_H_ diff --git a/Firestore/core/test/firebase/firestore/util/CMakeLists.txt b/Firestore/core/test/firebase/firestore/util/CMakeLists.txt index bcb1c84..c07a4ea 100644 --- a/Firestore/core/test/firebase/firestore/util/CMakeLists.txt +++ b/Firestore/core/test/firebase/firestore/util/CMakeLists.txt @@ -137,3 +137,12 @@ cc_test( firebase_firestore_util gmock ) + +if(APPLE) + target_sources( + firebase_firestore_util_test + PUBLIC + string_format_apple_test.mm + type_traits_apple_test.mm + ) +endif() diff --git a/Firestore/core/test/firebase/firestore/util/hashing_test.cc b/Firestore/core/test/firebase/firestore/util/hashing_test.cc index e5d9ff8..2c5c2f7 100644 --- a/Firestore/core/test/firebase/firestore/util/hashing_test.cc +++ b/Firestore/core/test/firebase/firestore/util/hashing_test.cc @@ -16,6 +16,9 @@ #include "Firestore/core/src/firebase/firestore/util/hashing.h" +#include +#include + #include "absl/strings/string_view.h" #include "gtest/gtest.h" @@ -29,6 +32,21 @@ struct HasHashMember { } }; +TEST(HashingTest, HasStdHash) { + EXPECT_TRUE(impl::has_std_hash::value); + EXPECT_TRUE(impl::has_std_hash::value); + EXPECT_TRUE(impl::has_std_hash::value); + EXPECT_TRUE(impl::has_std_hash::value); + EXPECT_TRUE(impl::has_std_hash::value); + EXPECT_TRUE(impl::has_std_hash::value); + EXPECT_TRUE(impl::has_std_hash::value); + + struct Foo {}; + EXPECT_FALSE(impl::has_std_hash::value); + EXPECT_FALSE(impl::has_std_hash::value); + EXPECT_FALSE((impl::has_std_hash>::value)); +} + TEST(HashingTest, Int) { ASSERT_EQ(std::hash{}(0), Hash(0)); } diff --git a/Firestore/core/test/firebase/firestore/util/string_format_apple_test.mm b/Firestore/core/test/firebase/firestore/util/string_format_apple_test.mm new file mode 100644 index 0000000..f0bcd35 --- /dev/null +++ b/Firestore/core/test/firebase/firestore/util/string_format_apple_test.mm @@ -0,0 +1,60 @@ +/* + * Copyright 2018 Google + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "Firestore/core/src/firebase/firestore/util/string_format.h" + +#import + +#include "gtest/gtest.h" + +@interface FSTDescribable : NSObject +@end + +@implementation FSTDescribable + +- (NSString*)description { + return @"description"; +} + +@end + +namespace firebase { +namespace firestore { +namespace util { + +TEST(StringFormatTest, NSString) { + EXPECT_EQ("Hello World", StringFormat("Hello %s", @"World")); + + NSString* hello = [NSString stringWithUTF8String:"Hello"]; + EXPECT_EQ("Hello World", StringFormat("%s World", hello)); + + // NOLINTNEXTLINE false positive on "string" + NSMutableString* world = [NSMutableString string]; + [world appendString:@"World"]; + EXPECT_EQ("Hello World", StringFormat("Hello %s", world)); +} + +TEST(StringFormatTest, FSTDescribable) { + FSTDescribable* desc = [[FSTDescribable alloc] init]; + EXPECT_EQ("Hello description", StringFormat("Hello %s", desc)); + + id desc_id = desc; + EXPECT_EQ("Hello description", StringFormat("Hello %s", desc_id)); +} + +} // namespace util +} // namespace firestore +} // namespace firebase diff --git a/Firestore/core/test/firebase/firestore/util/type_traits_apple_test.mm b/Firestore/core/test/firebase/firestore/util/type_traits_apple_test.mm new file mode 100644 index 0000000..dfb03bb --- /dev/null +++ b/Firestore/core/test/firebase/firestore/util/type_traits_apple_test.mm @@ -0,0 +1,50 @@ +/* + * Copyright 2018 Google + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "Firestore/core/src/firebase/firestore/util/type_traits.h" + +#import +#import +#import + +#include "gtest/gtest.h" + +namespace firebase { +namespace firestore { +namespace util { + +TEST(TypeTraitsTest, IsObjectiveCPointer) { + static_assert(is_objective_c_pointer{}, "NSObject"); + static_assert(is_objective_c_pointer{}, "NSString"); + static_assert(is_objective_c_pointer*>{}, + "NSArray"); + + static_assert(is_objective_c_pointer{}, "id"); + static_assert(is_objective_c_pointer>{}, "id"); + + static_assert(!is_objective_c_pointer{}, "int*"); + static_assert(!is_objective_c_pointer{}, "void*"); + static_assert(!is_objective_c_pointer{}, "int"); + static_assert(!is_objective_c_pointer{}, "void"); + + struct Foo {}; + static_assert(!is_objective_c_pointer{}, "Foo"); + static_assert(!is_objective_c_pointer{}, "Foo"); +} + +} // namespace util +} // namespace firestore +} // namespace firebase -- 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') 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 dc4f87527f61f0d20a5fc175ca19ad3f367133c1 Mon Sep 17 00:00:00 2001 From: Gil Date: Mon, 4 Jun 2018 13:06:43 -0700 Subject: Build cleanup (#1375) * Remove extraneous firebase_firestore_util_async_queue target * Remove unimplemented declaration in string_util.h --- Firestore/core/src/firebase/firestore/util/CMakeLists.txt | 15 ++++----------- Firestore/core/src/firebase/firestore/util/string_util.h | 5 ----- .../core/test/firebase/firestore/util/CMakeLists.txt | 4 ++-- 3 files changed, 6 insertions(+), 18 deletions(-) (limited to 'Firestore/core/test') diff --git a/Firestore/core/src/firebase/firestore/util/CMakeLists.txt b/Firestore/core/src/firebase/firestore/util/CMakeLists.txt index ed3a301..30589a0 100644 --- a/Firestore/core/src/firebase/firestore/util/CMakeLists.txt +++ b/Firestore/core/src/firebase/firestore/util/CMakeLists.txt @@ -29,6 +29,7 @@ cc_library( absl_strings ) + ## assert and log cc_library( @@ -158,16 +159,6 @@ else() endif() -cc_library( - firebase_firestore_util_async_queue - SOURCES - async_queue.cc - async_queue.h - DEPENDS - ${FIREBASE_FIRESTORE_UTIL_EXECUTOR} - ${FIREBASE_FIRESTORE_UTIL_LOG} - EXCLUDE_FROM_ALL -) ## main library @@ -179,6 +170,8 @@ configure_file( cc_library( firebase_firestore_util SOURCES + async_queue.cc + async_queue.h autoid.cc autoid.h bits.cc @@ -204,7 +197,7 @@ cc_library( DEPENDS absl_base firebase_firestore_util_base - firebase_firestore_util_async_queue + ${FIREBASE_FIRESTORE_UTIL_EXECUTOR} ${FIREBASE_FIRESTORE_UTIL_LOG} ${FIREBASE_FIRESTORE_UTIL_RANDOM} ) diff --git a/Firestore/core/src/firebase/firestore/util/string_util.h b/Firestore/core/src/firebase/firestore/util/string_util.h index 96ba0b0..86acc56 100644 --- a/Firestore/core/src/firebase/firestore/util/string_util.h +++ b/Firestore/core/src/firebase/firestore/util/string_util.h @@ -65,11 +65,6 @@ std::string PrefixSuccessor(absl::string_view prefix); */ std::string ImmediateSuccessor(absl::string_view s); -/** - * Returns true if the given value starts with the given prefix. - */ -bool StartsWith(const std::string &value, const std::string &prefix); - } // namespace util } // namespace firestore } // namespace firebase diff --git a/Firestore/core/test/firebase/firestore/util/CMakeLists.txt b/Firestore/core/test/firebase/firestore/util/CMakeLists.txt index c07a4ea..44a3b61 100644 --- a/Firestore/core/test/firebase/firestore/util/CMakeLists.txt +++ b/Firestore/core/test/firebase/firestore/util/CMakeLists.txt @@ -98,7 +98,7 @@ cc_test( async_tests_util.h DEPENDS firebase_firestore_util_executor_std - firebase_firestore_util_async_queue + firebase_firestore_util ) if(HAVE_LIBDISPATCH) @@ -111,7 +111,7 @@ if(HAVE_LIBDISPATCH) async_tests_util.h DEPENDS firebase_firestore_util_executor_libdispatch - firebase_firestore_util_async_queue + firebase_firestore_util ) endif() -- 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') 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