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".)

2 files changed, 139 insertions, 64 deletions

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