From 47ab39aee6330f59263b0a9b2fc36536544651fd Mon Sep 17 00:00:00 2001 From: rsgowman Date: Mon, 25 Jun 2018 10:06:35 -0400 Subject: Refactor nanopb decoding methods (#1438) Rather than decoding the type, and then the contents, decode them both at once. --- .../firebase/firestore/local/local_serializer.cc | 24 ++---- .../core/src/firebase/firestore/nanopb/reader.cc | 11 +++ .../core/src/firebase/firestore/nanopb/reader.h | 10 +++ .../src/firebase/firestore/remote/serializer.cc | 95 ++++++---------------- 4 files changed, 50 insertions(+), 90 deletions(-) (limited to 'Firestore/core/src') diff --git a/Firestore/core/src/firebase/firestore/local/local_serializer.cc b/Firestore/core/src/firebase/firestore/local/local_serializer.cc index fea99fb..d8e039b 100644 --- a/Firestore/core/src/firebase/firestore/local/local_serializer.cc +++ b/Firestore/core/src/firebase/firestore/local/local_serializer.cc @@ -84,25 +84,8 @@ std::unique_ptr LocalSerializer::DecodeMaybeDocument( // Ensure the tag matches the wire type switch (tag.field_number) { case firestore_client_MaybeDocument_document_tag: - case firestore_client_MaybeDocument_no_document_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))")); - return nullptr; - } - break; - - 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->RequireWireType(PB_WT_STRING, tag)) return nullptr; - switch (tag.field_number) { - case firestore_client_MaybeDocument_document_tag: // 'no_document' and 'document' are part of a oneof. The proto docs // claim that if both are set on the wire, the last one wins. no_document = nullptr; @@ -113,9 +96,12 @@ std::unique_ptr LocalSerializer::DecodeMaybeDocument( [&](Reader* reader) -> std::unique_ptr { return rpc_serializer_.DecodeDocument(reader); }); + break; case firestore_client_MaybeDocument_no_document_tag: + if (!reader->RequireWireType(PB_WT_STRING, tag)) return nullptr; + // 'no_document' and 'document' are part of a oneof. The proto docs // claim that if both are set on the wire, the last one wins. document = nullptr; @@ -123,6 +109,8 @@ std::unique_ptr LocalSerializer::DecodeMaybeDocument( // TODO(rsgowman): Parse the no_document field. abort(); + break; + default: // Unknown tag. According to the proto spec, we need to ignore these. reader->SkipField(tag); diff --git a/Firestore/core/src/firebase/firestore/nanopb/reader.cc b/Firestore/core/src/firebase/firestore/nanopb/reader.cc index 3b102f0..3e6d92e 100644 --- a/Firestore/core/src/firebase/firestore/nanopb/reader.cc +++ b/Firestore/core/src/firebase/firestore/nanopb/reader.cc @@ -46,6 +46,17 @@ Tag Reader::ReadTag() { return tag; } +bool Reader::RequireWireType(pb_wire_type_t wire_type, Tag tag) { + if (!status_.ok()) return false; + if (wire_type != tag.wire_type) { + set_status(Status(FirestoreErrorCode::DataLoss, + "Input proto bytes cannot be parsed (mismatch between " + "the wiretype and the field number (tag))")); + return false; + } + return true; +} + void Reader::ReadNanopbMessage(const pb_field_t fields[], void* dest_struct) { if (!status_.ok()) return; diff --git a/Firestore/core/src/firebase/firestore/nanopb/reader.h b/Firestore/core/src/firebase/firestore/nanopb/reader.h index 7dd7432..76dc3b6 100644 --- a/Firestore/core/src/firebase/firestore/nanopb/reader.h +++ b/Firestore/core/src/firebase/firestore/nanopb/reader.h @@ -57,6 +57,16 @@ class Reader { */ Tag ReadTag(); + /** + * Ensures the specified tag is of the specified type. If not, then + * Reader::status() will return a non-ok value (with the code set to + * FirestoreErrorCode::DataLoss). + * + * @return Convenience indicator for success. (If false, then status() will + * return a non-ok value.) + */ + bool RequireWireType(pb_wire_type_t wire_type, Tag tag); + /** * Reads a nanopb message from the input stream. * diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc index 044b3db..ce133e6 100644 --- a/Firestore/core/src/firebase/firestore/remote/serializer.cc +++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc @@ -122,64 +122,40 @@ FieldValue DecodeFieldValueImpl(Reader* reader) { // 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; - - 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; - - 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: - // 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(); - - switch (tag.field_number) { - case google_firestore_v1beta1_Value_null_value_tag: + if (!reader->RequireWireType(PB_WT_VARINT, tag)) + return FieldValue::NullValue(); reader->ReadNull(); result = FieldValue::NullValue(); break; + case google_firestore_v1beta1_Value_boolean_value_tag: + if (!reader->RequireWireType(PB_WT_VARINT, tag)) + return FieldValue::NullValue(); result = FieldValue::BooleanValue(reader->ReadBool()); break; + case google_firestore_v1beta1_Value_integer_value_tag: + if (!reader->RequireWireType(PB_WT_VARINT, tag)) + return FieldValue::NullValue(); result = FieldValue::IntegerValue(reader->ReadInteger()); break; + case google_firestore_v1beta1_Value_string_value_tag: + if (!reader->RequireWireType(PB_WT_STRING, tag)) + return FieldValue::NullValue(); result = FieldValue::StringValue(reader->ReadString()); break; + case google_firestore_v1beta1_Value_timestamp_value_tag: + if (!reader->RequireWireType(PB_WT_STRING, tag)) + return FieldValue::NullValue(); result = FieldValue::TimestampValue( reader->ReadNestedMessage(DecodeTimestamp)); break; + case google_firestore_v1beta1_Value_map_value_tag: + if (!reader->RequireWireType(PB_WT_STRING, tag)) + return FieldValue::NullValue(); // TODO(rsgowman): We should merge the existing map (if any) with the // newly parsed map. result = FieldValue::ObjectValueFromMap( @@ -474,28 +450,7 @@ std::unique_ptr Serializer::DecodeBatchGetDocumentsResponse( // Ensure the tag matches the wire type switch (tag.field_number) { case google_firestore_v1beta1_BatchGetDocumentsResponse_found_tag: - case google_firestore_v1beta1_BatchGetDocumentsResponse_missing_tag: - case google_firestore_v1beta1_BatchGetDocumentsResponse_transaction_tag: - case google_firestore_v1beta1_BatchGetDocumentsResponse_read_time_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; - - 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 nullptr; - - switch (tag.field_number) { - case google_firestore_v1beta1_BatchGetDocumentsResponse_found_tag: + if (!reader->RequireWireType(PB_WT_STRING, tag)) return nullptr; // 'found' and 'missing' are part of a oneof. The proto docs claim that // if both are set on the wire, the last one wins. missing = ""; @@ -509,6 +464,7 @@ std::unique_ptr Serializer::DecodeBatchGetDocumentsResponse( break; case google_firestore_v1beta1_BatchGetDocumentsResponse_missing_tag: + if (!reader->RequireWireType(PB_WT_STRING, tag)) return nullptr; // 'found' and 'missing' are part of a oneof. The proto docs claim that // if both are set on the wire, the last one wins. found = nullptr; @@ -516,20 +472,15 @@ std::unique_ptr Serializer::DecodeBatchGetDocumentsResponse( missing = reader->ReadString(); break; - case google_firestore_v1beta1_BatchGetDocumentsResponse_transaction_tag: - // This field is ignored by the client sdk, but we still need to extract - // it. - // TODO(rsgowman) switch this to reader->SkipField() (or whatever we end - // up calling it) once that exists. Possibly group this with other - // ignored and/or unknown fields - reader->ReadString(); - break; - case google_firestore_v1beta1_BatchGetDocumentsResponse_read_time_tag: + if (!reader->RequireWireType(PB_WT_STRING, tag)) return nullptr; read_time = SnapshotVersion{ reader->ReadNestedMessage(DecodeTimestamp)}; break; + case google_firestore_v1beta1_BatchGetDocumentsResponse_transaction_tag: + // This field is ignored by the client sdk, but we still need to extract + // it. default: // Unknown tag. According to the proto spec, we need to ignore these. reader->SkipField(tag); -- cgit v1.2.3