aboutsummaryrefslogtreecommitdiffhomepage
path: root/Firestore/core/src
diff options
context:
space:
mode:
authorGravatar rsgowman <rgowman@google.com>2018-06-25 10:06:35 -0400
committerGravatar GitHub <noreply@github.com>2018-06-25 10:06:35 -0400
commit47ab39aee6330f59263b0a9b2fc36536544651fd (patch)
treeefff1d390bc56c7a7d00628da6d81dca377684be /Firestore/core/src
parent137ce37363d72ac1d643ea9f93bc6e6665367e57 (diff)
Refactor nanopb decoding methods (#1438)
Rather than decoding the type, and then the contents, decode them both at once.
Diffstat (limited to 'Firestore/core/src')
-rw-r--r--Firestore/core/src/firebase/firestore/local/local_serializer.cc24
-rw-r--r--Firestore/core/src/firebase/firestore/nanopb/reader.cc11
-rw-r--r--Firestore/core/src/firebase/firestore/nanopb/reader.h10
-rw-r--r--Firestore/core/src/firebase/firestore/remote/serializer.cc95
4 files changed, 50 insertions, 90 deletions
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<model::MaybeDocument> 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<model::MaybeDocument> LocalSerializer::DecodeMaybeDocument(
[&](Reader* reader) -> std::unique_ptr<model::Document> {
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<model::MaybeDocument> 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
@@ -58,6 +58,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.
*
* This essentially wraps calls to nanopb's pb_decode() method. If we didn't
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<Timestamp>(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<MaybeDocument> 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<MaybeDocument> 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<MaybeDocument> 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<Timestamp>(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);