From 6a94dc3b8981aa0e88185c5855314e79259d29e8 Mon Sep 17 00:00:00 2001
From: Rich Gowman <rgowman@google.com>
Date: Tue, 27 Feb 2018 11:43:46 -0500
Subject: [PATCH 01/10] Serialize empty maps within FieldValue

---
 .../firebase/firestore/model/field_value.h    |  5 ++
 .../firebase/firestore/remote/serializer.cc   | 48 ++++++++++++++++++-
 .../firestore/remote/serializer_test.cc       |  7 +++
 3 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/Firestore/core/src/firebase/firestore/model/field_value.h b/Firestore/core/src/firebase/firestore/model/field_value.h
index cb219f55c1d..98e0ec6cc75 100644
--- a/Firestore/core/src/firebase/firestore/model/field_value.h
+++ b/Firestore/core/src/firebase/firestore/model/field_value.h
@@ -111,6 +111,11 @@ class FieldValue {
     return string_value_;
   }
 
+  const std::map<const std::string, const FieldValue> object_value() const {
+    FIREBASE_ASSERT(tag_ == Type::Object);
+    return object_value_;
+  }
+
   /** factory methods. */
   static const FieldValue& NullValue();
   static const FieldValue& TrueValue();
diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc
index b7ab891c1af..7210b000209 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.cc
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc
@@ -19,12 +19,15 @@
 #include <pb_decode.h>
 #include <pb_encode.h>
 
+#include <map>
 #include <string>
 
 namespace firebase {
 namespace firestore {
 namespace remote {
 
+using firebase::firestore::model::FieldValue;
+
 namespace {
 
 /**
@@ -141,9 +144,36 @@ std::string DecodeString(pb_istream_t* stream) {
   return result;
 }
 
-}  // namespace
+void EncodeObject(
+    pb_ostream_t* stream,
+    const std::map<const std::string, const FieldValue>& object_value
+    __attribute__((unused))) {
+  google_firestore_v1beta1_MapValue mapValue =
+      google_firestore_v1beta1_MapValue_init_zero;
+  bool status = pb_encode_delimited(
+      stream, google_firestore_v1beta1_MapValue_fields, &mapValue);
+  if (!status) {
+    // TODO(rsgowman): figure out error handling
+    abort();
+  }
+}
 
-using firebase::firestore::model::FieldValue;
+std::map<const std::string, const FieldValue> DecodeObject(
+    pb_istream_t* stream) {
+  google_firestore_v1beta1_MapValue mapValue =
+      google_firestore_v1beta1_MapValue_init_zero;
+  bool status = pb_decode_delimited(
+      stream, google_firestore_v1beta1_MapValue_fields, &mapValue);
+  if (!status) {
+    // TODO(rsgowman): figure out error handling
+    abort();
+  }
+
+  std::map<const std::string, const FieldValue> result;
+  return result;
+}
+
+}  // namespace
 
 void Serializer::EncodeFieldValue(const FieldValue& field_value,
                                   std::vector<uint8_t>* out_bytes) {
@@ -197,6 +227,17 @@ void Serializer::EncodeFieldValue(const FieldValue& field_value,
       EncodeString(&stream, field_value.string_value());
       break;
 
+    case FieldValue::Type::Object:
+      // NB: submessages use a wiretype of PB_WT_STRING
+      status = pb_encode_tag(&stream, PB_WT_STRING,
+                             google_firestore_v1beta1_Value_map_value_tag);
+      if (!status) {
+        // TODO(rsgowman): figure out error handling
+        abort();
+      }
+      EncodeObject(&stream, field_value.object_value());
+      break;
+
     default:
       // TODO(rsgowman): implement the other types
       abort();
@@ -228,6 +269,7 @@ FieldValue Serializer::DecodeFieldValue(const uint8_t* bytes, size_t length) {
       break;
 
     case google_firestore_v1beta1_Value_string_value_tag:
+    case google_firestore_v1beta1_Value_map_value_tag:
       if (wire_type != PB_WT_STRING) {
         abort();
       }
@@ -247,6 +289,8 @@ FieldValue Serializer::DecodeFieldValue(const uint8_t* bytes, size_t length) {
       return FieldValue::IntegerValue(DecodeInteger(&stream));
     case google_firestore_v1beta1_Value_string_value_tag:
       return FieldValue::StringValue(DecodeString(&stream));
+    case google_firestore_v1beta1_Value_map_value_tag:
+      return FieldValue::ObjectValue(DecodeObject(&stream));
 
     default:
       // TODO(rsgowman): figure out error handling
diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
index 8f890640fc6..ca5d2652a12 100644
--- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
+++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
@@ -191,6 +191,13 @@ TEST_F(SerializerTest, EncodesStringModelToBytes) {
   }
 }
 
+TEST_F(SerializerTest, EncodesEmptyMapToBytes) {
+  FieldValue model = FieldValue::ObjectValue({});
+  // TEXT_FORMAT_PROTO: 'map_value: {}'
+  std::vector<uint8_t> bytes{0x32, 0x00};
+  ExpectRoundTrip(model, bytes, FieldValue::Type::Object);
+}
+
 // TODO(rsgowman): Test [en|de]coding multiple protos into the same output
 // vector.
 

From 152c9e468b4fed9ae37bc649f49eba4232b41ca1 Mon Sep 17 00:00:00 2001
From: Rich Gowman <rgowman@google.com>
Date: Tue, 6 Mar 2018 10:27:54 -0500
Subject: [PATCH 02/10] Split [De|En]codeFieldValue into a stream and vector
 version

No code changes; refactoring only. This commit is present only so that I
don't move and change code in the same commit. (i.e. see next commit for
context.)
---
 .../firebase/firestore/remote/serializer.cc   | 124 ++++++++++--------
 1 file changed, 69 insertions(+), 55 deletions(-)

diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc
index 7210b000209..da8cd34f225 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.cc
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc
@@ -146,112 +146,78 @@ std::string DecodeString(pb_istream_t* stream) {
 
 void EncodeObject(
     pb_ostream_t* stream,
-    const std::map<const std::string, const FieldValue>& object_value
-    __attribute__((unused))) {
-  google_firestore_v1beta1_MapValue mapValue =
-      google_firestore_v1beta1_MapValue_init_zero;
-  bool status = pb_encode_delimited(
-      stream, google_firestore_v1beta1_MapValue_fields, &mapValue);
-  if (!status) {
-    // TODO(rsgowman): figure out error handling
-    abort();
-  }
-}
-
+    const std::map<const std::string, const FieldValue>& object_value);
 std::map<const std::string, const FieldValue> DecodeObject(
-    pb_istream_t* stream) {
-  google_firestore_v1beta1_MapValue mapValue =
-      google_firestore_v1beta1_MapValue_init_zero;
-  bool status = pb_decode_delimited(
-      stream, google_firestore_v1beta1_MapValue_fields, &mapValue);
-  if (!status) {
-    // TODO(rsgowman): figure out error handling
-    abort();
-  }
-
-  std::map<const std::string, const FieldValue> result;
-  return result;
-}
-
-}  // namespace
-
-void Serializer::EncodeFieldValue(const FieldValue& field_value,
-                                  std::vector<uint8_t>* out_bytes) {
-  // TODO(rsgowman): how large should the output buffer be? Do some
-  // investigation to see if we can get nanopb to tell us how much space it's
-  // going to need.
-  uint8_t buf[1024];
-  pb_ostream_t stream = pb_ostream_from_buffer(buf, sizeof(buf));
+    pb_istream_t* stream);
 
+// Named '..Impl' so as to not conflict with Serializer::EncodeFieldValue.
+void EncodeFieldValueImpl(pb_ostream_t* stream, const FieldValue& field_value) {
   // TODO(rsgowman): some refactoring is in order... but will wait until after a
   // non-varint, non-fixed-size (i.e. string) type is present before doing so.
   bool status = false;
   switch (field_value.type()) {
     case FieldValue::Type::Null:
-      status = pb_encode_tag(&stream, PB_WT_VARINT,
+      status = pb_encode_tag(stream, PB_WT_VARINT,
                              google_firestore_v1beta1_Value_null_value_tag);
       if (!status) {
         // TODO(rsgowman): figure out error handling
         abort();
       }
-      EncodeNull(&stream);
+      EncodeNull(stream);
       break;
 
     case FieldValue::Type::Boolean:
-      status = pb_encode_tag(&stream, PB_WT_VARINT,
+      status = pb_encode_tag(stream, PB_WT_VARINT,
                              google_firestore_v1beta1_Value_boolean_value_tag);
       if (!status) {
         // TODO(rsgowman): figure out error handling
         abort();
       }
-      EncodeBool(&stream, field_value.boolean_value());
+      EncodeBool(stream, field_value.boolean_value());
       break;
 
     case FieldValue::Type::Integer:
-      status = pb_encode_tag(&stream, PB_WT_VARINT,
+      status = pb_encode_tag(stream, PB_WT_VARINT,
                              google_firestore_v1beta1_Value_integer_value_tag);
       if (!status) {
         // TODO(rsgowman): figure out error handling
         abort();
       }
-      EncodeInteger(&stream, field_value.integer_value());
+      EncodeInteger(stream, field_value.integer_value());
       break;
 
     case FieldValue::Type::String:
-      status = pb_encode_tag(&stream, PB_WT_STRING,
+      status = pb_encode_tag(stream, PB_WT_STRING,
                              google_firestore_v1beta1_Value_string_value_tag);
       if (!status) {
         // TODO(rsgowman): figure out error handling
         abort();
       }
-      EncodeString(&stream, field_value.string_value());
+      EncodeString(stream, field_value.string_value());
       break;
 
     case FieldValue::Type::Object:
       // NB: submessages use a wiretype of PB_WT_STRING
-      status = pb_encode_tag(&stream, PB_WT_STRING,
+      status = pb_encode_tag(stream, PB_WT_STRING,
                              google_firestore_v1beta1_Value_map_value_tag);
       if (!status) {
         // TODO(rsgowman): figure out error handling
         abort();
       }
-      EncodeObject(&stream, field_value.object_value());
+      EncodeObject(stream, field_value.object_value());
       break;
 
     default:
       // TODO(rsgowman): implement the other types
       abort();
   }
-
-  out_bytes->insert(out_bytes->end(), buf, buf + stream.bytes_written);
 }
 
-FieldValue Serializer::DecodeFieldValue(const uint8_t* bytes, size_t length) {
-  pb_istream_t stream = pb_istream_from_buffer(bytes, length);
+FieldValue DecodeFieldValueImpl(pb_istream_t* stream) {
   pb_wire_type_t wire_type;
   uint32_t tag;
   bool eof;
-  bool status = pb_decode_tag(&stream, &wire_type, &tag, &eof);
+  bool status = pb_decode_tag(stream, &wire_type, &tag, &eof);
   if (!status) {
     // TODO(rsgowman): figure out error handling
     abort();
@@ -281,16 +247,16 @@ FieldValue Serializer::DecodeFieldValue(const uint8_t* bytes, size_t length) {
 
   switch (tag) {
     case google_firestore_v1beta1_Value_null_value_tag:
-      DecodeNull(&stream);
+      DecodeNull(stream);
       return FieldValue::NullValue();
     case google_firestore_v1beta1_Value_boolean_value_tag:
-      return FieldValue::BooleanValue(DecodeBool(&stream));
+      return FieldValue::BooleanValue(DecodeBool(stream));
     case google_firestore_v1beta1_Value_integer_value_tag:
-      return FieldValue::IntegerValue(DecodeInteger(&stream));
+      return FieldValue::IntegerValue(DecodeInteger(stream));
     case google_firestore_v1beta1_Value_string_value_tag:
-      return FieldValue::StringValue(DecodeString(&stream));
+      return FieldValue::StringValue(DecodeString(stream));
     case google_firestore_v1beta1_Value_map_value_tag:
-      return FieldValue::ObjectValue(DecodeObject(&stream));
+      return FieldValue::ObjectValue(DecodeObject(stream));
 
     default:
       // TODO(rsgowman): figure out error handling
@@ -298,6 +264,54 @@ FieldValue Serializer::DecodeFieldValue(const uint8_t* bytes, size_t length) {
   }
 }
 
+void EncodeObject(
+    pb_ostream_t* stream,
+    const std::map<const std::string, const FieldValue>& object_value
+    __attribute__((unused))) {
+  google_firestore_v1beta1_MapValue mapValue =
+      google_firestore_v1beta1_MapValue_init_zero;
+  bool status = pb_encode_delimited(
+      stream, google_firestore_v1beta1_MapValue_fields, &mapValue);
+  if (!status) {
+    // TODO(rsgowman): figure out error handling
+    abort();
+  }
+}
+
+std::map<const std::string, const FieldValue> DecodeObject(
+    pb_istream_t* stream) {
+  google_firestore_v1beta1_MapValue map_value =
+      google_firestore_v1beta1_MapValue_init_zero;
+  bool status = pb_decode_delimited(
+      stream, google_firestore_v1beta1_MapValue_fields, &map_value);
+  if (!status) {
+    // TODO(rsgowman): figure out error handling
+    abort();
+  }
+
+  std::map<const std::string, const FieldValue> result;
+  return result;
+}
+
+}  // namespace
+
+void Serializer::EncodeFieldValue(const FieldValue& field_value,
+                                  std::vector<uint8_t>* out_bytes) {
+  // TODO(rsgowman): how large should the output buffer be? Do some
+  // investigation to see if we can get nanopb to tell us how much space it's
+  // going to need. (Hint: use a sizing stream, i.e. PB_OSTREAM_SIZING)
+  uint8_t buf[1024];
+  memset(buf, 0x42, sizeof(buf));
+  pb_ostream_t stream = pb_ostream_from_buffer(buf, sizeof(buf));
+  EncodeFieldValueImpl(&stream, field_value);
+  out_bytes->insert(out_bytes->end(), buf, buf + stream.bytes_written);
+}
+
+FieldValue Serializer::DecodeFieldValue(const uint8_t* bytes, size_t length) {
+  pb_istream_t stream = pb_istream_from_buffer(bytes, length);
+  return DecodeFieldValueImpl(&stream);
+}
+
 }  // namespace remote
 }  // namespace firestore
 }  // namespace firebase

From df70fd52a654115b97f6f92494df02b4100db654 Mon Sep 17 00:00:00 2001
From: Rich Gowman <rgowman@google.com>
Date: Tue, 6 Mar 2018 10:37:42 -0500
Subject: [PATCH 03/10] [De]Serialize FieldValue map_values ("Objects")

These can (recursively) contain other FieldValues.
---
 .../firebase/firestore/remote/serializer.cc   | 196 +++++++++++++++++-
 .../firestore/remote/serializer_test.cc       |  70 +++++++
 2 files changed, 263 insertions(+), 3 deletions(-)

diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc
index da8cd34f225..ed2a944b3a9 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.cc
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc
@@ -21,6 +21,9 @@
 
 #include <map>
 #include <string>
+#include <utility>
+
+#include "Firestore/core/src/firebase/firestore/util/firebase_assert.h"
 
 namespace firebase {
 namespace firestore {
@@ -264,12 +267,175 @@ FieldValue DecodeFieldValueImpl(pb_istream_t* stream) {
   }
 }
 
+void EncodeSubFieldValue(pb_ostream_t* stream, const FieldValue& field_value) {
+  // Implementation note: This is roughly modeled on pb_encode_delimited (which
+  // is actually pb_encode_submessage), adjusted to account for the oneof in
+  // FieldValue.
+
+  // First calculate the message size using a non-writing substream.
+  pb_ostream_t substream = PB_OSTREAM_SIZING;
+  EncodeFieldValueImpl(&substream, field_value);
+  size_t size = substream.bytes_written;
+
+  // Write out the size to the output stream.
+  EncodeVarint(stream, size);
+
+  // If stream is itself a sizing stream, then we don't need to actually parse
+  // field_value a second time; just update the bytes_written via a call to
+  // pb_write. (If we try to write the contents into a sizing stream, it'll
+  // fail since sizing streams don't actually have any buffer space.)
+  if (stream->callback == NULL) {
+    bool status = pb_write(stream, NULL, size);
+    if (!status) {
+      // TODO(rsgowman): figure out error handling
+      abort();
+    }
+    return;
+  }
+
+  // Ensure the output stream has enough space
+  if (stream->bytes_written + size > stream->max_size) {
+    // TODO(rsgowman): figure out error handling
+    abort();
+  }
+
+  // Use a substream to verify that a callback doesn't write more than what it
+  // did the first time. (Use an initializer rather than setting fields
+  // individually like nanopb does. This gives us a *chance* of noticing if
+  // nanopb adds new fields.)
+  substream = {stream->callback, stream->state, /*max_size=*/size,
+               /*bytes_written=*/0, /*errmsg=*/NULL};
+
+  EncodeFieldValueImpl(&substream, field_value);
+  stream->bytes_written += substream.bytes_written;
+  stream->state = substream.state;
+  stream->errmsg = substream.errmsg;
+
+  if (substream.bytes_written != size) {
+    // submsg size changed
+    // TODO(rsgowman): figure out error handling
+    abort();
+  }
+}
+
+FieldValue DecodeSubFieldValue(pb_istream_t* stream) {
+  // Implementation note: This is roughly modeled on pb_decode_delimited,
+  // adjusted to account for the oneof in FieldValue.
+  pb_istream_t substream;
+  bool status = pb_make_string_substream(stream, &substream);
+  if (!status) {
+    // TODO(rsgowman): figure out error handling
+    abort();
+  }
+
+  FieldValue fv = DecodeFieldValueImpl(&substream);
+
+  // NB: future versions of nanopb read the remaining characters out of the
+  // substream (and return false if that fails) as an additional safety
+  // check within pb_clost_string_substream. Unfortunately, that's not present
+  // in the current version (0.38).  We'll make a stronger assertion and check
+  // to make sure there *are* no remaining characters in the substream.
+  if (substream.bytes_left != 0) {
+    // TODO(rsgowman): figure out error handling
+    abort();
+  }
+  pb_close_string_substream(stream, &substream);
+
+  return fv;
+}
+
+void EncodeFieldEntry(
+    pb_ostream_t* stream,
+    const std::pair<const std::string, const FieldValue>& kv) {
+  // Calculate the size of this FieldEntry. This is the size of the key and
+  // value, plus an additional 2 for the two tags.
+  pb_ostream_t sizing_stream = PB_OSTREAM_SIZING;
+  EncodeString(&sizing_stream, kv.first);
+  EncodeSubFieldValue(&sizing_stream, kv.second);
+
+  // additional 2 for the two tags
+  EncodeVarint(stream, sizing_stream.bytes_written + 2);
+
+  // Encode the key (string)
+  bool status =
+      pb_encode_tag(stream, PB_WT_STRING,
+                    google_firestore_v1beta1_MapValue_FieldsEntry_key_tag);
+  if (!status) {
+    // TODO(rsgowman): figure out error handling
+    abort();
+  }
+  EncodeString(stream, kv.first);
+
+  // Encode the value (FieldValue)
+  status =
+      pb_encode_tag(stream, PB_WT_STRING,
+                    google_firestore_v1beta1_MapValue_FieldsEntry_value_tag);
+  if (!status) {
+    // TODO(rsgowman): figure out error handling
+    abort();
+  }
+  EncodeSubFieldValue(stream, kv.second);
+}
+
+std::pair<const std::string, const FieldValue> DecodeFieldEntry(
+    pb_istream_t* stream) {
+  pb_wire_type_t wire_type;
+  uint32_t tag;
+  bool eof;
+  bool status = pb_decode_tag(stream, &wire_type, &tag, &eof);
+  // TODO(rsgowman): figure out error handling: We can do better than a failed
+  // assertion.
+  FIREBASE_ASSERT(tag == google_firestore_v1beta1_MapValue_FieldsEntry_key_tag);
+  FIREBASE_ASSERT(wire_type == PB_WT_STRING);
+  FIREBASE_ASSERT(!eof);
+  FIREBASE_ASSERT(status);
+  std::string key = DecodeString(stream);
+
+  status = pb_decode_tag(stream, &wire_type, &tag, &eof);
+  FIREBASE_ASSERT(tag ==
+                  google_firestore_v1beta1_MapValue_FieldsEntry_value_tag);
+  // NB: PB_WT_STRING is used for submessages too.
+  FIREBASE_ASSERT(wire_type == PB_WT_STRING);
+  FIREBASE_ASSERT(!eof);
+  FIREBASE_ASSERT(status);
+
+  FieldValue value = DecodeSubFieldValue(stream);
+
+  return std::make_pair(key, value);
+}
+
 void EncodeObject(
     pb_ostream_t* stream,
-    const std::map<const std::string, const FieldValue>& object_value
-    __attribute__((unused))) {
+    const std::map<const std::string, const FieldValue>& object_value) {
   google_firestore_v1beta1_MapValue mapValue =
       google_firestore_v1beta1_MapValue_init_zero;
+  // NB: c-style callbacks can't use *capturing* lambdas, so we'll pass in the
+  // object_value via the arg field (and therefore need to do a bunch of
+  // casting).
+  mapValue.fields.funcs.encode = [](pb_ostream_t* stream, const pb_field_t*,
+                                    void* const* arg) -> bool {
+    auto* object_value =
+        reinterpret_cast<const std::map<const std::string, const FieldValue>*>(
+            *arg);
+
+    // Encode each FieldEntry (i.e. key value pair.)
+    for (const auto& kv : *object_value) {
+      bool status =
+          pb_encode_tag(stream, PB_WT_STRING,
+                        google_firestore_v1beta1_MapValue_FieldsEntry_key_tag);
+      if (!status) {
+        // TODO(rsgowman): figure out error handling
+        abort();
+      }
+
+      EncodeFieldEntry(stream, kv);
+    }
+
+    return true;
+  };
+  mapValue.fields.arg =
+      const_cast<void*>(reinterpret_cast<const void*>(&object_value));
+
   bool status = pb_encode_delimited(
       stream, google_firestore_v1beta1_MapValue_fields, &mapValue);
   if (!status) {
@@ -282,6 +448,31 @@ std::map<const std::string, const FieldValue> DecodeObject(
     pb_istream_t* stream) {
   google_firestore_v1beta1_MapValue map_value =
       google_firestore_v1beta1_MapValue_init_zero;
+  std::map<const std::string, const FieldValue> result;
+  // NB: c-style callbacks can't use *capturing* lambdas, so we'll pass in the
+  // object_value via the arg field (and therefore need to do a bunch of
+  // casting).
+  map_value.fields.funcs.decode =
+      [](pb_istream_t* stream, const pb_field_t* field __attribute((unused)),
+         void** arg) -> bool {
+    auto* result =
+        reinterpret_cast<std::map<const std::string, const FieldValue>*>(*arg);
+
+    std::pair<const std::string, const FieldValue> fv =
+        DecodeFieldEntry(stream);
+
+    // Sanity check: ensure that this key doesn't already exist in the map.
+    // TODO(rsgowman): figure out error handling: We can do better than a failed
+    // assertion.
+    FIREBASE_ASSERT(result->find(fv.first) == result->end());
+
+    // Add this key,fieldvalue to the results map.
+    result->emplace(fv);
+
+    return true;
+  };
+  map_value.fields.arg = &result;
+
   bool status = pb_decode_delimited(
       stream, google_firestore_v1beta1_MapValue_fields, &map_value);
   if (!status) {
@@ -289,7 +480,6 @@ std::map<const std::string, const FieldValue> DecodeObject(
     abort();
   }
 
-  std::map<const std::string, const FieldValue> result;
   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 ca5d2652a12..6ea42749dd5 100644
--- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
+++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
@@ -198,6 +198,76 @@ TEST_F(SerializerTest, EncodesEmptyMapToBytes) {
   ExpectRoundTrip(model, bytes, FieldValue::Type::Object);
 }
 
+TEST_F(SerializerTest, EncodesNestedObjectsToBytes) {
+  // As above, verify max int64_t value.
+  EXPECT_EQ(9223372036854775807, std::numeric_limits<int64_t>::max());
+
+  FieldValue model = FieldValue::ObjectValue(
+      {{"b", FieldValue::TrueValue()},
+       // TODO(rsgowman): add doubles (once they're supported)
+       //{"d", FieldValue::DoubleValue(std::numeric_limits<double>::max())},
+       {"i", FieldValue::IntegerValue(1)},
+       {"n", FieldValue::NullValue()},
+       {"s", FieldValue::StringValue("foo")},
+       // TODO(rsgowman): add arrays (once they're supported)
+       //{"a", [2, "bar", {"b", false}]},
+       {"o", FieldValue::ObjectValue(
+                 {{"d", FieldValue::IntegerValue(100)},
+                  {"nested",
+                   FieldValue::ObjectValue(
+                       {{"e", FieldValue::IntegerValue(
+                                  std::numeric_limits<int64_t>::max())}})}})}});
+
+  /* WARNING: "Wire format ordering and map iteration ordering of map values is
+   * undefined, so you cannot rely on your map items being in a particular
+   * order."
+   * - https://developers.google.com/protocol-buffers/docs/proto#maps-features
+   *
+   * In reality, the map items are serialized in whatever order you provide them
+   * in. Since FieldValue::ObjectValue is currently backed by a std::map (and
+   * not an unordered_map) this implies ~alpha ordering. So we need to provide
+   * the text format input in alpha ordering for things to match up.
+   *
+   * This is... not ideal. Nothing stops libprotobuf from changing this
+   * behaviour (since it's not guaranteed) nor does anything stop us from
+   * switching map->unordered_map in FieldValue. (Arguably, that would be
+   * better.) But the alternative is to not test the serializing to bytes, and
+   * instead just assume we got that right. A *better* solution is to serialize
+   * to bytes, and then deserialize with libprotobuf (rather than nanopb) and
+   * then do a second test of serializing via libprotobuf and deserializing via
+   * nanopb. In both cases, we would ignore the bytes themselves (since the
+   * ordering is not defined) and instead compare the input objects with the
+   * output objects.
+   *
+   * TODO(rsgowman): ^
+   *
+   * TEXT_FORMAT_PROTO (with multi-line formatting to preserve sanity):
+   'map_value: {
+     fields: {key:"b", value:{boolean_value: true}}
+     fields: {key:"i", value:{integer_value: 1}}
+     fields: {key:"n", value:{null_value: NULL_VALUE}}
+     fields: {key:"o", value:{map_value: {
+       fields: {key:"d", value:{integer_value: 100}}
+       fields: {key:"nested", value{map_value: {
+         fields: {key:"e", value:{integer_value: 9223372036854775807}}
+       }}}
+     }}}
+     fields: {key:"s", value:{string_value: "foo"}}
+   }'
+   */
+  std::vector<uint8_t> bytes{
+      0x32, 0x59, 0x0a, 0x07, 0x0a, 0x01, 0x62, 0x12, 0x02, 0x08, 0x01, 0x0a,
+      0x07, 0x0a, 0x01, 0x69, 0x12, 0x02, 0x10, 0x01, 0x0a, 0x07, 0x0a, 0x01,
+      0x6e, 0x12, 0x02, 0x58, 0x00, 0x0a, 0x2f, 0x0a, 0x01, 0x6f, 0x12, 0x2a,
+      0x32, 0x28, 0x0a, 0x07, 0x0a, 0x01, 0x64, 0x12, 0x02, 0x10, 0x64, 0x0a,
+      0x1d, 0x0a, 0x06, 0x6e, 0x65, 0x73, 0x74, 0x65, 0x64, 0x12, 0x13, 0x32,
+      0x11, 0x0a, 0x0f, 0x0a, 0x01, 0x65, 0x12, 0x0a, 0x10, 0xff, 0xff, 0xff,
+      0xff, 0xff, 0xff, 0xff, 0xff, 0x7f, 0x0a, 0x0b, 0x0a, 0x01, 0x73, 0x12,
+      0x06, 0x8a, 0x01, 0x03, 0x66, 0x6f, 0x6f};
+
+  ExpectRoundTrip(model, bytes, FieldValue::Type::Object);
+}
+
 // TODO(rsgowman): Test [en|de]coding multiple protos into the same output
 // vector.
 

From 362838924db7a002f6c4c89f93461a42b8d87f62 Mon Sep 17 00:00:00 2001
From: Rich Gowman <rgowman@google.com>
Date: Tue, 6 Mar 2018 12:32:54 -0500
Subject: [PATCH 04/10] style.sh + lint.sh

---
 Firestore/core/src/firebase/firestore/remote/serializer.cc   | 5 ++---
 .../core/test/firebase/firestore/remote/serializer_test.cc   | 4 ++--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc
index ed2a944b3a9..29d769eea87 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.cc
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc
@@ -452,9 +452,8 @@ std::map<const std::string, const FieldValue> DecodeObject(
   // NB: c-style callbacks can't use *capturing* lambdas, so we'll pass in the
   // object_value via the arg field (and therefore need to do a bunch of
   // casting).
-  map_value.fields.funcs.decode =
-      [](pb_istream_t* stream, const pb_field_t* field __attribute((unused)),
-         void** arg) -> bool {
+  map_value.fields.funcs.decode = [](pb_istream_t* stream, const pb_field_t*,
+                                     void** arg) -> bool {
     auto* result =
         reinterpret_cast<std::map<const std::string, const FieldValue>*>(*arg);
 
diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
index 6ea42749dd5..30afa30b12b 100644
--- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
+++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
@@ -205,12 +205,12 @@ TEST_F(SerializerTest, EncodesNestedObjectsToBytes) {
   FieldValue model = FieldValue::ObjectValue(
       {{"b", FieldValue::TrueValue()},
        // TODO(rsgowman): add doubles (once they're supported)
-       //{"d", FieldValue::DoubleValue(std::numeric_limits<double>::max())},
+       // {"d", FieldValue::DoubleValue(std::numeric_limits<double>::max())},
        {"i", FieldValue::IntegerValue(1)},
        {"n", FieldValue::NullValue()},
        {"s", FieldValue::StringValue("foo")},
        // TODO(rsgowman): add arrays (once they're supported)
-       //{"a", [2, "bar", {"b", false}]},
+       // {"a", [2, "bar", {"b", false}]},
        {"o", FieldValue::ObjectValue(
                  {{"d", FieldValue::IntegerValue(100)},
                   {"nested",

From d7f05949c86238d698b7a0746bbf5681bde9f5f2 Mon Sep 17 00:00:00 2001
From: Rich Gowman <rgowman@google.com>
Date: Tue, 6 Mar 2018 13:27:12 -0500
Subject: [PATCH 05/10] Review feedback:

- spelling: [En|De]codeFieldEntry -> [En|De]codeFieldsEntry
- docstring added to EncodeFieldsEntry
---
 .../firebase/firestore/remote/serializer.cc   | 32 +++++++++++++++----
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc
index 29d769eea87..b61f1d3502d 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.cc
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc
@@ -344,10 +344,30 @@ FieldValue DecodeSubFieldValue(pb_istream_t* stream) {
   return fv;
 }
 
-void EncodeFieldEntry(
+/**
+ * Encodes a 'FieldsEntry' object, within a FieldValue's map_value type.
+ *
+ * In protobuf, maps are implemented as a repeated set of key/values. For
+ * instance, this:
+ *   message Foo {
+ *     map<string, Value> fields = 1;
+ *   }
+ * would be encoded (in proto text format) as:
+ *   {
+ *     fields: {key:"key string 1", value:{<Value message here>}}
+ *     fields: {key:"key string 2", value:{<Value message here>}}
+ *     ...
+ *   }
+ *
+ * This method encodes an individual entry from that list. It is expected that
+ * this method will be called once for each entry in the map.
+ *
+ * @param kv The individual key/value pair to encode.
+ */
+void EncodeFieldsEntry(
     pb_ostream_t* stream,
     const std::pair<const std::string, const FieldValue>& kv) {
-  // Calculate the size of this FieldEntry. This is the size of the key and
+  // Calculate the size of this FieldsEntry. This is the size of the key and
   // value, plus an additional 2 for the two tags.
   pb_ostream_t sizing_stream = PB_OSTREAM_SIZING;
   EncodeString(&sizing_stream, kv.first);
@@ -377,7 +397,7 @@ void EncodeFieldEntry(
   EncodeSubFieldValue(stream, kv.second);
 }
 
-std::pair<const std::string, const FieldValue> DecodeFieldEntry(
+std::pair<const std::string, const FieldValue> DecodeFieldsEntry(
     pb_istream_t* stream) {
   pb_wire_type_t wire_type;
   uint32_t tag;
@@ -418,7 +438,7 @@ void EncodeObject(
         reinterpret_cast<const std::map<const std::string, const FieldValue>*>(
             *arg);
 
-    // Encode each FieldEntry (i.e. key value pair.)
+    // Encode each FieldsEntry (i.e. key value pair.)
     for (const auto& kv : *object_value) {
       bool status =
           pb_encode_tag(stream, PB_WT_STRING,
@@ -428,7 +448,7 @@ void EncodeObject(
         abort();
       }
 
-      EncodeFieldEntry(stream, kv);
+      EncodeFieldsEntry(stream, kv);
     }
 
     return true;
@@ -458,7 +478,7 @@ std::map<const std::string, const FieldValue> DecodeObject(
         reinterpret_cast<std::map<const std::string, const FieldValue>*>(*arg);
 
     std::pair<const std::string, const FieldValue> fv =
-        DecodeFieldEntry(stream);
+        DecodeFieldsEntry(stream);
 
     // Sanity check: ensure that this key doesn't already exist in the map.
     // TODO(rsgowman): figure out error handling: We can do better than a failed

From a57cb68397084e80f21c550155053a985cffe03d Mon Sep 17 00:00:00 2001
From: Rich Gowman <rgowman@google.com>
Date: Tue, 6 Mar 2018 13:52:02 -0500
Subject: [PATCH 06/10] Remove 'const' from FieldValue::object_value's key.

std::map already declares the key as const, so this is redundant.
---
 .../firebase/firestore/model/field_value.cc   | 10 +++++-----
 .../firebase/firestore/model/field_value.h    |  8 ++++----
 .../firebase/firestore/remote/serializer.cc   | 14 ++++++-------
 .../firestore/model/field_value_test.cc       | 20 +++++++++----------
 4 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/Firestore/core/src/firebase/firestore/model/field_value.cc b/Firestore/core/src/firebase/firestore/model/field_value.cc
index 03cf1d4a8a2..875c22d185b 100644
--- a/Firestore/core/src/firebase/firestore/model/field_value.cc
+++ b/Firestore/core/src/firebase/firestore/model/field_value.cc
@@ -113,7 +113,7 @@ FieldValue& FieldValue::operator=(const FieldValue& value) {
     }
     case Type::Object: {
       // copy-and-swap
-      std::map<const std::string, const FieldValue> tmp = value.object_value_;
+      std::map<std::string, const FieldValue> tmp = value.object_value_;
       std::swap(object_value_, tmp);
       break;
     }
@@ -281,13 +281,13 @@ FieldValue FieldValue::ArrayValue(std::vector<FieldValue>&& value) {
 }
 
 FieldValue FieldValue::ObjectValue(
-    const std::map<const std::string, const FieldValue>& value) {
-  std::map<const std::string, const FieldValue> copy(value);
+    const std::map<std::string, const FieldValue>& value) {
+  std::map<std::string, const FieldValue> copy(value);
   return ObjectValue(std::move(copy));
 }
 
 FieldValue FieldValue::ObjectValue(
-    std::map<const std::string, const FieldValue>&& value) {
+    std::map<std::string, const FieldValue>&& value) {
   FieldValue result;
   result.SwitchTo(Type::Object);
   std::swap(result.object_value_, value);
@@ -418,7 +418,7 @@ void FieldValue::SwitchTo(const Type type) {
       new (&array_value_) std::vector<FieldValue>();
       break;
     case Type::Object:
-      new (&object_value_) std::map<const std::string, const FieldValue>();
+      new (&object_value_) std::map<std::string, const FieldValue>();
       break;
     default: {}  // The other types where there is nothing to worry about.
   }
diff --git a/Firestore/core/src/firebase/firestore/model/field_value.h b/Firestore/core/src/firebase/firestore/model/field_value.h
index 98e0ec6cc75..94a04d27a78 100644
--- a/Firestore/core/src/firebase/firestore/model/field_value.h
+++ b/Firestore/core/src/firebase/firestore/model/field_value.h
@@ -111,7 +111,7 @@ class FieldValue {
     return string_value_;
   }
 
-  const std::map<const std::string, const FieldValue> object_value() const {
+  const std::map<std::string, const FieldValue>& object_value() const {
     FIREBASE_ASSERT(tag_ == Type::Object);
     return object_value_;
   }
@@ -140,9 +140,9 @@ class FieldValue {
   static FieldValue ArrayValue(const std::vector<FieldValue>& value);
   static FieldValue ArrayValue(std::vector<FieldValue>&& value);
   static FieldValue ObjectValue(
-      const std::map<const std::string, const FieldValue>& value);
+      const std::map<std::string, const FieldValue>& value);
   static FieldValue ObjectValue(
-      std::map<const std::string, const FieldValue>&& value);
+      std::map<std::string, const FieldValue>&& value);
 
   friend bool operator<(const FieldValue& lhs, const FieldValue& rhs);
 
@@ -169,7 +169,7 @@ class FieldValue {
     firebase::firestore::model::ReferenceValue reference_value_;
     GeoPoint geo_point_value_;
     std::vector<FieldValue> array_value_;
-    std::map<const std::string, const FieldValue> object_value_;
+    std::map<std::string, const FieldValue> object_value_;
   };
 };
 
diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc
index b61f1d3502d..c7e15cacffc 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.cc
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc
@@ -149,8 +149,8 @@ std::string DecodeString(pb_istream_t* stream) {
 
 void EncodeObject(
     pb_ostream_t* stream,
-    const std::map<const std::string, const FieldValue>& object_value);
-std::map<const std::string, const FieldValue> DecodeObject(
+    const std::map<std::string, const FieldValue>& object_value);
+std::map<std::string, const FieldValue> DecodeObject(
     pb_istream_t* stream);
 
 // Named '..Impl' so as to not conflict with Serializer::EncodeFieldValue.
@@ -426,7 +426,7 @@ std::pair<const std::string, const FieldValue> DecodeFieldsEntry(
 
 void EncodeObject(
     pb_ostream_t* stream,
-    const std::map<const std::string, const FieldValue>& object_value) {
+    const std::map<std::string, const FieldValue>& object_value) {
   google_firestore_v1beta1_MapValue mapValue =
       google_firestore_v1beta1_MapValue_init_zero;
   // NB: c-style callbacks can't use *capturing* lambdas, so we'll pass in the
@@ -435,7 +435,7 @@ void EncodeObject(
   mapValue.fields.funcs.encode = [](pb_ostream_t* stream, const pb_field_t*,
                                     void* const* arg) -> bool {
     auto* object_value =
-        reinterpret_cast<const std::map<const std::string, const FieldValue>*>(
+        reinterpret_cast<const std::map<std::string, const FieldValue>*>(
             *arg);
 
     // Encode each FieldsEntry (i.e. key value pair.)
@@ -464,18 +464,18 @@ void EncodeObject(
   }
 }
 
-std::map<const std::string, const FieldValue> DecodeObject(
+std::map<std::string, const FieldValue> DecodeObject(
     pb_istream_t* stream) {
   google_firestore_v1beta1_MapValue map_value =
       google_firestore_v1beta1_MapValue_init_zero;
-  std::map<const std::string, const FieldValue> result;
+  std::map<std::string, const FieldValue> result;
   // NB: c-style callbacks can't use *capturing* lambdas, so we'll pass in the
   // object_value via the arg field (and therefore need to do a bunch of
   // casting).
   map_value.fields.funcs.decode = [](pb_istream_t* stream, const pb_field_t*,
                                      void** arg) -> bool {
     auto* result =
-        reinterpret_cast<std::map<const std::string, const FieldValue>*>(*arg);
+        reinterpret_cast<std::map<std::string, const FieldValue>*>(*arg);
 
     std::pair<const std::string, const FieldValue> fv =
         DecodeFieldsEntry(stream);
diff --git a/Firestore/core/test/firebase/firestore/model/field_value_test.cc b/Firestore/core/test/firebase/firestore/model/field_value_test.cc
index 5a64d59406e..9fa2d845b00 100644
--- a/Firestore/core/test/firebase/firestore/model/field_value_test.cc
+++ b/Firestore/core/test/firebase/firestore/model/field_value_test.cc
@@ -190,14 +190,14 @@ TEST(FieldValue, ArrayType) {
 
 TEST(FieldValue, ObjectType) {
   const FieldValue empty =
-      FieldValue::ObjectValue(std::map<const std::string, const FieldValue>{});
-  std::map<const std::string, const FieldValue> object{
+      FieldValue::ObjectValue(std::map<std::string, const FieldValue>{});
+  std::map<std::string, const FieldValue> object{
       {"null", FieldValue::NullValue()},
       {"true", FieldValue::TrueValue()},
       {"false", FieldValue::FalseValue()}};
   // copy the map
   const FieldValue small = FieldValue::ObjectValue(object);
-  std::map<const std::string, const FieldValue> another_object{
+  std::map<std::string, const FieldValue> another_object{
       {"null", FieldValue::NullValue()}, {"true", FieldValue::FalseValue()}};
   // move the array
   const FieldValue large = FieldValue::ObjectValue(std::move(another_object));
@@ -337,23 +337,23 @@ TEST(FieldValue, Copy) {
   EXPECT_EQ(FieldValue::NullValue(), clone);
 
   const FieldValue object_value =
-      FieldValue::ObjectValue(std::map<const std::string, const FieldValue>{
+      FieldValue::ObjectValue(std::map<std::string, const FieldValue>{
           {"true", FieldValue::TrueValue()},
           {"false", FieldValue::FalseValue()}});
   clone = object_value;
   EXPECT_EQ(
-      FieldValue::ObjectValue(std::map<const std::string, const FieldValue>{
+      FieldValue::ObjectValue(std::map<std::string, const FieldValue>{
           {"true", FieldValue::TrueValue()},
           {"false", FieldValue::FalseValue()}}),
       clone);
   EXPECT_EQ(
-      FieldValue::ObjectValue(std::map<const std::string, const FieldValue>{
+      FieldValue::ObjectValue(std::map<std::string, const FieldValue>{
           {"true", FieldValue::TrueValue()},
           {"false", FieldValue::FalseValue()}}),
       object_value);
   clone = clone;
   EXPECT_EQ(
-      FieldValue::ObjectValue(std::map<const std::string, const FieldValue>{
+      FieldValue::ObjectValue(std::map<std::string, const FieldValue>{
           {"true", FieldValue::TrueValue()},
           {"false", FieldValue::FalseValue()}}),
       clone);
@@ -436,12 +436,12 @@ TEST(FieldValue, Move) {
   EXPECT_EQ(FieldValue::NullValue(), clone);
 
   FieldValue object_value =
-      FieldValue::ObjectValue(std::map<const std::string, const FieldValue>{
+      FieldValue::ObjectValue(std::map<std::string, const FieldValue>{
           {"true", FieldValue::TrueValue()},
           {"false", FieldValue::FalseValue()}});
   clone = std::move(object_value);
   EXPECT_EQ(
-      FieldValue::ObjectValue(std::map<const std::string, const FieldValue>{
+      FieldValue::ObjectValue(std::map<std::string, const FieldValue>{
           {"true", FieldValue::TrueValue()},
           {"false", FieldValue::FalseValue()}}),
       clone);
@@ -463,7 +463,7 @@ TEST(FieldValue, CompareMixedType) {
   const FieldValue array_value =
       FieldValue::ArrayValue(std::vector<FieldValue>());
   const FieldValue object_value =
-      FieldValue::ObjectValue(std::map<const std::string, const FieldValue>());
+      FieldValue::ObjectValue(std::map<std::string, const FieldValue>());
   EXPECT_TRUE(null_value < true_value);
   EXPECT_TRUE(true_value < number_value);
   EXPECT_TRUE(number_value < timestamp_value);

From e4b811bd3e5851c3317ecf65fc23289d1725dd55 Mon Sep 17 00:00:00 2001
From: Rich Gowman <rgowman@google.com>
Date: Tue, 6 Mar 2018 15:15:05 -0500
Subject: [PATCH 07/10] More review feedback

- Remove 'const' from FieldValue::object_value's value (since it's
  immutable.)
- mapValue -> map_value
- rm memset(42)
---
 .../firebase/firestore/model/field_value.cc   | 10 ++++----
 .../firebase/firestore/model/field_value.h    |  8 +++----
 .../firebase/firestore/remote/serializer.cc   | 23 +++++++++----------
 .../firestore/model/field_value_test.cc       | 20 ++++++++--------
 .../firestore/remote/serializer_test.cc       |  4 ++++
 5 files changed, 34 insertions(+), 31 deletions(-)

diff --git a/Firestore/core/src/firebase/firestore/model/field_value.cc b/Firestore/core/src/firebase/firestore/model/field_value.cc
index 875c22d185b..a9d0bf3a817 100644
--- a/Firestore/core/src/firebase/firestore/model/field_value.cc
+++ b/Firestore/core/src/firebase/firestore/model/field_value.cc
@@ -113,7 +113,7 @@ FieldValue& FieldValue::operator=(const FieldValue& value) {
     }
     case Type::Object: {
       // copy-and-swap
-      std::map<std::string, const FieldValue> tmp = value.object_value_;
+      std::map<std::string, FieldValue> tmp = value.object_value_;
       std::swap(object_value_, tmp);
       break;
     }
@@ -281,13 +281,13 @@ FieldValue FieldValue::ArrayValue(std::vector<FieldValue>&& value) {
 }
 
 FieldValue FieldValue::ObjectValue(
-    const std::map<std::string, const FieldValue>& value) {
-  std::map<std::string, const FieldValue> copy(value);
+    const std::map<std::string, FieldValue>& value) {
+  std::map<std::string, FieldValue> copy(value);
   return ObjectValue(std::move(copy));
 }
 
 FieldValue FieldValue::ObjectValue(
-    std::map<std::string, const FieldValue>&& value) {
+    std::map<std::string, FieldValue>&& value) {
   FieldValue result;
   result.SwitchTo(Type::Object);
   std::swap(result.object_value_, value);
@@ -418,7 +418,7 @@ void FieldValue::SwitchTo(const Type type) {
       new (&array_value_) std::vector<FieldValue>();
       break;
     case Type::Object:
-      new (&object_value_) std::map<std::string, const FieldValue>();
+      new (&object_value_) std::map<std::string, FieldValue>();
       break;
     default: {}  // The other types where there is nothing to worry about.
   }
diff --git a/Firestore/core/src/firebase/firestore/model/field_value.h b/Firestore/core/src/firebase/firestore/model/field_value.h
index 94a04d27a78..9d0184b9ac9 100644
--- a/Firestore/core/src/firebase/firestore/model/field_value.h
+++ b/Firestore/core/src/firebase/firestore/model/field_value.h
@@ -111,7 +111,7 @@ class FieldValue {
     return string_value_;
   }
 
-  const std::map<std::string, const FieldValue>& object_value() const {
+  const std::map<std::string, FieldValue>& object_value() const {
     FIREBASE_ASSERT(tag_ == Type::Object);
     return object_value_;
   }
@@ -140,9 +140,9 @@ class FieldValue {
   static FieldValue ArrayValue(const std::vector<FieldValue>& value);
   static FieldValue ArrayValue(std::vector<FieldValue>&& value);
   static FieldValue ObjectValue(
-      const std::map<std::string, const FieldValue>& value);
+      const std::map<std::string, FieldValue>& value);
   static FieldValue ObjectValue(
-      std::map<std::string, const FieldValue>&& value);
+      std::map<std::string, FieldValue>&& value);
 
   friend bool operator<(const FieldValue& lhs, const FieldValue& rhs);
 
@@ -169,7 +169,7 @@ class FieldValue {
     firebase::firestore::model::ReferenceValue reference_value_;
     GeoPoint geo_point_value_;
     std::vector<FieldValue> array_value_;
-    std::map<std::string, const FieldValue> object_value_;
+    std::map<std::string, FieldValue> object_value_;
   };
 };
 
diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc
index c7e15cacffc..1c96d4c6764 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.cc
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc
@@ -149,8 +149,8 @@ std::string DecodeString(pb_istream_t* stream) {
 
 void EncodeObject(
     pb_ostream_t* stream,
-    const std::map<std::string, const FieldValue>& object_value);
-std::map<std::string, const FieldValue> DecodeObject(
+    const std::map<std::string, FieldValue>& object_value);
+std::map<std::string, FieldValue> DecodeObject(
     pb_istream_t* stream);
 
 // Named '..Impl' so as to not conflict with Serializer::EncodeFieldValue.
@@ -426,16 +426,16 @@ std::pair<const std::string, const FieldValue> DecodeFieldsEntry(
 
 void EncodeObject(
     pb_ostream_t* stream,
-    const std::map<std::string, const FieldValue>& object_value) {
-  google_firestore_v1beta1_MapValue mapValue =
+    const std::map<std::string, FieldValue>& object_value) {
+  google_firestore_v1beta1_MapValue map_value =
       google_firestore_v1beta1_MapValue_init_zero;
   // NB: c-style callbacks can't use *capturing* lambdas, so we'll pass in the
   // object_value via the arg field (and therefore need to do a bunch of
   // casting).
-  mapValue.fields.funcs.encode = [](pb_ostream_t* stream, const pb_field_t*,
+  map_value.fields.funcs.encode = [](pb_ostream_t* stream, const pb_field_t*,
                                     void* const* arg) -> bool {
     auto* object_value =
-        reinterpret_cast<const std::map<std::string, const FieldValue>*>(
+        reinterpret_cast<const std::map<std::string, FieldValue>*>(
             *arg);
 
     // Encode each FieldsEntry (i.e. key value pair.)
@@ -453,29 +453,29 @@ void EncodeObject(
 
     return true;
   };
-  mapValue.fields.arg =
+  map_value.fields.arg =
       const_cast<void*>(reinterpret_cast<const void*>(&object_value));
 
   bool status = pb_encode_delimited(
-      stream, google_firestore_v1beta1_MapValue_fields, &mapValue);
+      stream, google_firestore_v1beta1_MapValue_fields, &map_value);
   if (!status) {
     // TODO(rsgowman): figure out error handling
     abort();
   }
 }
 
-std::map<std::string, const FieldValue> DecodeObject(
+std::map<std::string, FieldValue> DecodeObject(
     pb_istream_t* stream) {
   google_firestore_v1beta1_MapValue map_value =
       google_firestore_v1beta1_MapValue_init_zero;
-  std::map<std::string, const FieldValue> result;
+  std::map<std::string, FieldValue> result;
   // NB: c-style callbacks can't use *capturing* lambdas, so we'll pass in the
   // object_value via the arg field (and therefore need to do a bunch of
   // casting).
   map_value.fields.funcs.decode = [](pb_istream_t* stream, const pb_field_t*,
                                      void** arg) -> bool {
     auto* result =
-        reinterpret_cast<std::map<std::string, const FieldValue>*>(*arg);
+        reinterpret_cast<std::map<std::string, FieldValue>*>(*arg);
 
     std::pair<const std::string, const FieldValue> fv =
         DecodeFieldsEntry(stream);
@@ -510,7 +510,6 @@ void Serializer::EncodeFieldValue(const FieldValue& field_value,
   // investigation to see if we can get nanopb to tell us how much space it's
   // going to need. (Hint: use a sizing stream, i.e. PB_OSTREAM_SIZING)
   uint8_t buf[1024];
-  memset(buf, 0x42, sizeof(buf));
   pb_ostream_t stream = pb_ostream_from_buffer(buf, sizeof(buf));
   EncodeFieldValueImpl(&stream, field_value);
   out_bytes->insert(out_bytes->end(), buf, buf + stream.bytes_written);
diff --git a/Firestore/core/test/firebase/firestore/model/field_value_test.cc b/Firestore/core/test/firebase/firestore/model/field_value_test.cc
index 9fa2d845b00..cf42dd911ea 100644
--- a/Firestore/core/test/firebase/firestore/model/field_value_test.cc
+++ b/Firestore/core/test/firebase/firestore/model/field_value_test.cc
@@ -190,14 +190,14 @@ TEST(FieldValue, ArrayType) {
 
 TEST(FieldValue, ObjectType) {
   const FieldValue empty =
-      FieldValue::ObjectValue(std::map<std::string, const FieldValue>{});
-  std::map<std::string, const FieldValue> object{
+      FieldValue::ObjectValue(std::map<std::string, FieldValue>{});
+  std::map<std::string, FieldValue> object{
       {"null", FieldValue::NullValue()},
       {"true", FieldValue::TrueValue()},
       {"false", FieldValue::FalseValue()}};
   // copy the map
   const FieldValue small = FieldValue::ObjectValue(object);
-  std::map<std::string, const FieldValue> another_object{
+  std::map<std::string, FieldValue> another_object{
       {"null", FieldValue::NullValue()}, {"true", FieldValue::FalseValue()}};
   // move the array
   const FieldValue large = FieldValue::ObjectValue(std::move(another_object));
@@ -337,23 +337,23 @@ TEST(FieldValue, Copy) {
   EXPECT_EQ(FieldValue::NullValue(), clone);
 
   const FieldValue object_value =
-      FieldValue::ObjectValue(std::map<std::string, const FieldValue>{
+      FieldValue::ObjectValue(std::map<std::string, FieldValue>{
           {"true", FieldValue::TrueValue()},
           {"false", FieldValue::FalseValue()}});
   clone = object_value;
   EXPECT_EQ(
-      FieldValue::ObjectValue(std::map<std::string, const FieldValue>{
+      FieldValue::ObjectValue(std::map<std::string, FieldValue>{
           {"true", FieldValue::TrueValue()},
           {"false", FieldValue::FalseValue()}}),
       clone);
   EXPECT_EQ(
-      FieldValue::ObjectValue(std::map<std::string, const FieldValue>{
+      FieldValue::ObjectValue(std::map<std::string, FieldValue>{
           {"true", FieldValue::TrueValue()},
           {"false", FieldValue::FalseValue()}}),
       object_value);
   clone = clone;
   EXPECT_EQ(
-      FieldValue::ObjectValue(std::map<std::string, const FieldValue>{
+      FieldValue::ObjectValue(std::map<std::string, FieldValue>{
           {"true", FieldValue::TrueValue()},
           {"false", FieldValue::FalseValue()}}),
       clone);
@@ -436,12 +436,12 @@ TEST(FieldValue, Move) {
   EXPECT_EQ(FieldValue::NullValue(), clone);
 
   FieldValue object_value =
-      FieldValue::ObjectValue(std::map<std::string, const FieldValue>{
+      FieldValue::ObjectValue(std::map<std::string, FieldValue>{
           {"true", FieldValue::TrueValue()},
           {"false", FieldValue::FalseValue()}});
   clone = std::move(object_value);
   EXPECT_EQ(
-      FieldValue::ObjectValue(std::map<std::string, const FieldValue>{
+      FieldValue::ObjectValue(std::map<std::string, FieldValue>{
           {"true", FieldValue::TrueValue()},
           {"false", FieldValue::FalseValue()}}),
       clone);
@@ -463,7 +463,7 @@ TEST(FieldValue, CompareMixedType) {
   const FieldValue array_value =
       FieldValue::ArrayValue(std::vector<FieldValue>());
   const FieldValue object_value =
-      FieldValue::ObjectValue(std::map<std::string, const FieldValue>());
+      FieldValue::ObjectValue(std::map<std::string, FieldValue>());
   EXPECT_TRUE(null_value < true_value);
   EXPECT_TRUE(true_value < number_value);
   EXPECT_TRUE(number_value < timestamp_value);
diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
index 30afa30b12b..8298b670f81 100644
--- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
+++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
@@ -116,6 +116,8 @@ TEST_F(SerializerTest, EncodesIntegersModelToBytes) {
   // For now, lets at least verify these values:
   EXPECT_EQ(-9223372036854775807 - 1, std::numeric_limits<int64_t>::min());
   EXPECT_EQ(9223372036854775807, std::numeric_limits<int64_t>::max());
+  // TODO(rsgowman): link libprotobuf to the test suite and eliminate the
+  // above.
 
   struct TestCase {
     int64_t value;
@@ -201,6 +203,8 @@ TEST_F(SerializerTest, EncodesEmptyMapToBytes) {
 TEST_F(SerializerTest, EncodesNestedObjectsToBytes) {
   // As above, verify max int64_t value.
   EXPECT_EQ(9223372036854775807, std::numeric_limits<int64_t>::max());
+  // TODO(rsgowman): link libprotobuf to the test suite and eliminate the
+  // above.
 
   FieldValue model = FieldValue::ObjectValue(
       {{"b", FieldValue::TrueValue()},

From 7abad06679dcdf8f904f942b90831910e1bf3883 Mon Sep 17 00:00:00 2001
From: Rich Gowman <rgowman@google.com>
Date: Tue, 6 Mar 2018 18:02:17 -0500
Subject: [PATCH 08/10] more review feedback

---
 .../firebase/firestore/model/field_value.cc   |  3 +-
 .../firebase/firestore/model/field_value.h    |  6 +-
 .../firebase/firestore/remote/serializer.cc   | 89 ++++++++++---------
 .../firestore/model/field_value_test.cc       | 57 ++++++------
 .../firestore/remote/serializer_test.cc       |  9 +-
 5 files changed, 80 insertions(+), 84 deletions(-)

diff --git a/Firestore/core/src/firebase/firestore/model/field_value.cc b/Firestore/core/src/firebase/firestore/model/field_value.cc
index a9d0bf3a817..a38a676d17d 100644
--- a/Firestore/core/src/firebase/firestore/model/field_value.cc
+++ b/Firestore/core/src/firebase/firestore/model/field_value.cc
@@ -286,8 +286,7 @@ FieldValue FieldValue::ObjectValue(
   return ObjectValue(std::move(copy));
 }
 
-FieldValue FieldValue::ObjectValue(
-    std::map<std::string, FieldValue>&& value) {
+FieldValue FieldValue::ObjectValue(std::map<std::string, FieldValue>&& value) {
   FieldValue result;
   result.SwitchTo(Type::Object);
   std::swap(result.object_value_, value);
diff --git a/Firestore/core/src/firebase/firestore/model/field_value.h b/Firestore/core/src/firebase/firestore/model/field_value.h
index 9d0184b9ac9..fc8619d684d 100644
--- a/Firestore/core/src/firebase/firestore/model/field_value.h
+++ b/Firestore/core/src/firebase/firestore/model/field_value.h
@@ -139,10 +139,8 @@ class FieldValue {
   static FieldValue GeoPointValue(const GeoPoint& value);
   static FieldValue ArrayValue(const std::vector<FieldValue>& value);
   static FieldValue ArrayValue(std::vector<FieldValue>&& value);
-  static FieldValue ObjectValue(
-      const std::map<std::string, FieldValue>& value);
-  static FieldValue ObjectValue(
-      std::map<std::string, FieldValue>&& value);
+  static FieldValue ObjectValue(const std::map<std::string, FieldValue>& value);
+  static FieldValue ObjectValue(std::map<std::string, FieldValue>&& value);
 
   friend bool operator<(const FieldValue& lhs, const FieldValue& rhs);
 
diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc
index 1c96d4c6764..40f855d4133 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.cc
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc
@@ -33,6 +33,11 @@ using firebase::firestore::model::FieldValue;
 
 namespace {
 
+void EncodeObject(pb_ostream_t* stream,
+                  const std::map<std::string, FieldValue>& object_value);
+
+std::map<std::string, FieldValue> DecodeObject(pb_istream_t* stream);
+
 /**
  * Note that (despite the value parameter type) this works for bool, enum,
  * int32, int64, uint32 and uint64 proto field types.
@@ -147,13 +152,10 @@ std::string DecodeString(pb_istream_t* stream) {
   return result;
 }
 
-void EncodeObject(
-    pb_ostream_t* stream,
-    const std::map<std::string, FieldValue>& object_value);
-std::map<std::string, FieldValue> DecodeObject(
-    pb_istream_t* stream);
-
 // Named '..Impl' so as to not conflict with Serializer::EncodeFieldValue.
+// TODO(rsgowman): Refactor to use a helper class that wraps the stream struct.
+// This will help with error handling, and should eliminate the issue of two
+// 'EncodeFieldValue' methods.
 void EncodeFieldValueImpl(pb_ostream_t* stream, const FieldValue& field_value) {
   // TODO(rsgowman): some refactoring is in order... but will wait until after a
   // non-varint, non-fixed-size (i.e. string) type is present before doing so.
@@ -267,7 +269,19 @@ FieldValue DecodeFieldValueImpl(pb_istream_t* stream) {
   }
 }
 
-void EncodeSubFieldValue(pb_ostream_t* stream, const FieldValue& field_value) {
+/**
+ * Encodes a FieldValue *and* it's length.
+ *
+ * When encoding a top level message, protobuf doesn't include the length (since
+ * you can get that already from the length of the binary output.) But when
+ * encoding a sub/nested message, you must include the length in the
+ * serialization.
+ *
+ * Call this method when encoding a non top level FieldValue. Otherwise call
+ * EncodeFieldValue[Impl].
+ */
+void EncodeNestedFieldValue(pb_ostream_t* stream,
+                            const FieldValue& field_value) {
   // Implementation note: This is roughly modeled on pb_encode_delimited (which
   // is actually pb_encode_submessage), adjusted to account for the oneof in
   // FieldValue.
@@ -332,7 +346,7 @@ FieldValue DecodeSubFieldValue(pb_istream_t* stream) {
 
   // NB: future versions of nanopb read the remaining characters out of the
   // substream (and return false if that fails) as an additional safety
-  // check within pb_clost_string_substream. Unfortunately, that's not present
+  // check within pb_close_string_substream. Unfortunately, that's not present
   // in the current version (0.38).  We'll make a stronger assertion and check
   // to make sure there *are* no remaining characters in the substream.
   if (substream.bytes_left != 0) {
@@ -364,18 +378,8 @@ FieldValue DecodeSubFieldValue(pb_istream_t* stream) {
  *
  * @param kv The individual key/value pair to encode.
  */
-void EncodeFieldsEntry(
-    pb_ostream_t* stream,
-    const std::pair<const std::string, const FieldValue>& kv) {
-  // Calculate the size of this FieldsEntry. This is the size of the key and
-  // value, plus an additional 2 for the two tags.
-  pb_ostream_t sizing_stream = PB_OSTREAM_SIZING;
-  EncodeString(&sizing_stream, kv.first);
-  EncodeSubFieldValue(&sizing_stream, kv.second);
-
-  // additional 2 for the two tags
-  EncodeVarint(stream, sizing_stream.bytes_written + 2);
-
+void EncodeFieldsEntry(pb_ostream_t* stream,
+                       const std::pair<std::string, FieldValue>& kv) {
   // Encode the key (string)
   bool status =
       pb_encode_tag(stream, PB_WT_STRING,
@@ -394,11 +398,10 @@ void EncodeFieldsEntry(
     // TODO(rsgowman): figure out error handling
     abort();
   }
-  EncodeSubFieldValue(stream, kv.second);
+  EncodeNestedFieldValue(stream, kv.second);
 }
 
-std::pair<const std::string, const FieldValue> DecodeFieldsEntry(
-    pb_istream_t* stream) {
+std::pair<std::string, FieldValue> DecodeFieldsEntry(pb_istream_t* stream) {
   pb_wire_type_t wire_type;
   uint32_t tag;
   bool eof;
@@ -421,25 +424,23 @@ std::pair<const std::string, const FieldValue> DecodeFieldsEntry(
 
   FieldValue value = DecodeSubFieldValue(stream);
 
-  return std::make_pair(key, value);
+  return {key, value};
 }
 
-void EncodeObject(
-    pb_ostream_t* stream,
-    const std::map<std::string, FieldValue>& object_value) {
+void EncodeObject(pb_ostream_t* stream,
+                  const std::map<std::string, FieldValue>& object_value) {
   google_firestore_v1beta1_MapValue map_value =
       google_firestore_v1beta1_MapValue_init_zero;
   // NB: c-style callbacks can't use *capturing* lambdas, so we'll pass in the
   // object_value via the arg field (and therefore need to do a bunch of
   // casting).
   map_value.fields.funcs.encode = [](pb_ostream_t* stream, const pb_field_t*,
-                                    void* const* arg) -> bool {
-    auto* object_value =
-        reinterpret_cast<const std::map<std::string, FieldValue>*>(
-            *arg);
+                                     void* const* arg) -> bool {
+    auto& object_value =
+        *static_cast<const std::map<std::string, FieldValue>*>(*arg);
 
-    // Encode each FieldsEntry (i.e. key value pair.)
-    for (const auto& kv : *object_value) {
+    // Encode each FieldsEntry (i.e. key-value pair.)
+    for (const auto& kv : object_value) {
       bool status =
           pb_encode_tag(stream, PB_WT_STRING,
                         google_firestore_v1beta1_MapValue_FieldsEntry_key_tag);
@@ -448,13 +449,20 @@ void EncodeObject(
         abort();
       }
 
+      // Calculate the size of this FieldsEntry using a non-writing substream.
+      pb_ostream_t sizing_stream = PB_OSTREAM_SIZING;
+      EncodeFieldsEntry(&sizing_stream, kv);
+      size_t size = sizing_stream.bytes_written;
+      // Write out the size to the output stream.
+      EncodeVarint(stream, size);
+
       EncodeFieldsEntry(stream, kv);
     }
 
     return true;
   };
   map_value.fields.arg =
-      const_cast<void*>(reinterpret_cast<const void*>(&object_value));
+      const_cast<std::map<std::string, FieldValue>*>(&object_value);
 
   bool status = pb_encode_delimited(
       stream, google_firestore_v1beta1_MapValue_fields, &map_value);
@@ -464,8 +472,7 @@ void EncodeObject(
   }
 }
 
-std::map<std::string, FieldValue> DecodeObject(
-    pb_istream_t* stream) {
+std::map<std::string, FieldValue> DecodeObject(pb_istream_t* stream) {
   google_firestore_v1beta1_MapValue map_value =
       google_firestore_v1beta1_MapValue_init_zero;
   std::map<std::string, FieldValue> result;
@@ -474,19 +481,17 @@ std::map<std::string, FieldValue> DecodeObject(
   // casting).
   map_value.fields.funcs.decode = [](pb_istream_t* stream, const pb_field_t*,
                                      void** arg) -> bool {
-    auto* result =
-        reinterpret_cast<std::map<std::string, FieldValue>*>(*arg);
+    auto& result = *static_cast<std::map<std::string, FieldValue>*>(*arg);
 
-    std::pair<const std::string, const FieldValue> fv =
-        DecodeFieldsEntry(stream);
+    std::pair<std::string, FieldValue> fv = DecodeFieldsEntry(stream);
 
     // Sanity check: ensure that this key doesn't already exist in the map.
     // TODO(rsgowman): figure out error handling: We can do better than a failed
     // assertion.
-    FIREBASE_ASSERT(result->find(fv.first) == result->end());
+    FIREBASE_ASSERT(result.find(fv.first) == result.end());
 
     // Add this key,fieldvalue to the results map.
-    result->emplace(fv);
+    result.emplace(std::move(fv));
 
     return true;
   };
diff --git a/Firestore/core/test/firebase/firestore/model/field_value_test.cc b/Firestore/core/test/firebase/firestore/model/field_value_test.cc
index cf42dd911ea..93879f90980 100644
--- a/Firestore/core/test/firebase/firestore/model/field_value_test.cc
+++ b/Firestore/core/test/firebase/firestore/model/field_value_test.cc
@@ -191,10 +191,9 @@ TEST(FieldValue, ArrayType) {
 TEST(FieldValue, ObjectType) {
   const FieldValue empty =
       FieldValue::ObjectValue(std::map<std::string, FieldValue>{});
-  std::map<std::string, FieldValue> object{
-      {"null", FieldValue::NullValue()},
-      {"true", FieldValue::TrueValue()},
-      {"false", FieldValue::FalseValue()}};
+  std::map<std::string, FieldValue> object{{"null", FieldValue::NullValue()},
+                                           {"true", FieldValue::TrueValue()},
+                                           {"false", FieldValue::FalseValue()}};
   // copy the map
   const FieldValue small = FieldValue::ObjectValue(object);
   std::map<std::string, FieldValue> another_object{
@@ -336,27 +335,23 @@ TEST(FieldValue, Copy) {
   clone = null_value;
   EXPECT_EQ(FieldValue::NullValue(), clone);
 
-  const FieldValue object_value =
-      FieldValue::ObjectValue(std::map<std::string, FieldValue>{
-          {"true", FieldValue::TrueValue()},
-          {"false", FieldValue::FalseValue()}});
+  const FieldValue object_value = FieldValue::ObjectValue(
+      std::map<std::string, FieldValue>{{"true", FieldValue::TrueValue()},
+                                        {"false", FieldValue::FalseValue()}});
   clone = object_value;
-  EXPECT_EQ(
-      FieldValue::ObjectValue(std::map<std::string, FieldValue>{
-          {"true", FieldValue::TrueValue()},
-          {"false", FieldValue::FalseValue()}}),
-      clone);
-  EXPECT_EQ(
-      FieldValue::ObjectValue(std::map<std::string, FieldValue>{
-          {"true", FieldValue::TrueValue()},
-          {"false", FieldValue::FalseValue()}}),
-      object_value);
+  EXPECT_EQ(FieldValue::ObjectValue(std::map<std::string, FieldValue>{
+                {"true", FieldValue::TrueValue()},
+                {"false", FieldValue::FalseValue()}}),
+            clone);
+  EXPECT_EQ(FieldValue::ObjectValue(std::map<std::string, FieldValue>{
+                {"true", FieldValue::TrueValue()},
+                {"false", FieldValue::FalseValue()}}),
+            object_value);
   clone = clone;
-  EXPECT_EQ(
-      FieldValue::ObjectValue(std::map<std::string, FieldValue>{
-          {"true", FieldValue::TrueValue()},
-          {"false", FieldValue::FalseValue()}}),
-      clone);
+  EXPECT_EQ(FieldValue::ObjectValue(std::map<std::string, FieldValue>{
+                {"true", FieldValue::TrueValue()},
+                {"false", FieldValue::FalseValue()}}),
+            clone);
   clone = null_value;
   EXPECT_EQ(FieldValue::NullValue(), clone);
 }
@@ -435,16 +430,14 @@ TEST(FieldValue, Move) {
   clone = FieldValue::NullValue();
   EXPECT_EQ(FieldValue::NullValue(), clone);
 
-  FieldValue object_value =
-      FieldValue::ObjectValue(std::map<std::string, FieldValue>{
-          {"true", FieldValue::TrueValue()},
-          {"false", FieldValue::FalseValue()}});
+  FieldValue object_value = FieldValue::ObjectValue(
+      std::map<std::string, FieldValue>{{"true", FieldValue::TrueValue()},
+                                        {"false", FieldValue::FalseValue()}});
   clone = std::move(object_value);
-  EXPECT_EQ(
-      FieldValue::ObjectValue(std::map<std::string, FieldValue>{
-          {"true", FieldValue::TrueValue()},
-          {"false", FieldValue::FalseValue()}}),
-      clone);
+  EXPECT_EQ(FieldValue::ObjectValue(std::map<std::string, FieldValue>{
+                {"true", FieldValue::TrueValue()},
+                {"false", FieldValue::FalseValue()}}),
+            clone);
   clone = FieldValue::NullValue();
   EXPECT_EQ(FieldValue::NullValue(), clone);
 }
diff --git a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
index 8298b670f81..1e2f928d85a 100644
--- a/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
+++ b/Firestore/core/test/firebase/firestore/remote/serializer_test.cc
@@ -227,10 +227,11 @@ TEST_F(SerializerTest, EncodesNestedObjectsToBytes) {
    * order."
    * - https://developers.google.com/protocol-buffers/docs/proto#maps-features
    *
-   * In reality, the map items are serialized in whatever order you provide them
-   * in. Since FieldValue::ObjectValue is currently backed by a std::map (and
-   * not an unordered_map) this implies ~alpha ordering. So we need to provide
-   * the text format input in alpha ordering for things to match up.
+   * In reality, the map items are serialized by protoc in whatever order you
+   * provide them in. Since FieldValue::ObjectValue is currently backed by a
+   * std::map (and not an unordered_map) this implies ~alpha ordering. So we
+   * need to provide the text format input in alpha ordering for things to match
+   * up.
    *
    * This is... not ideal. Nothing stops libprotobuf from changing this
    * behaviour (since it's not guaranteed) nor does anything stop us from

From 61b22108472d888b7849dfcaa55a7cfc2d7c8095 Mon Sep 17 00:00:00 2001
From: Rich Gowman <rgowman@google.com>
Date: Wed, 7 Mar 2018 12:03:44 -0500
Subject: [PATCH 09/10] typo

---
 Firestore/core/src/firebase/firestore/remote/serializer.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc
index 40f855d4133..0d91163ed2e 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.cc
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc
@@ -270,7 +270,7 @@ FieldValue DecodeFieldValueImpl(pb_istream_t* stream) {
 }
 
 /**
- * Encodes a FieldValue *and* it's length.
+ * Encodes a FieldValue *and* its length.
  *
  * When encoding a top level message, protobuf doesn't include the length (since
  * you can get that already from the length of the binary output.) But when

From 05eb700621f82ce227734a090689d96fc79ed6b8 Mon Sep 17 00:00:00 2001
From: Rich Gowman <rgowman@google.com>
Date: Thu, 8 Mar 2018 09:57:46 -0500
Subject: [PATCH 10/10] more review feedback

---
 Firestore/core/src/firebase/firestore/remote/serializer.cc | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/Firestore/core/src/firebase/firestore/remote/serializer.cc b/Firestore/core/src/firebase/firestore/remote/serializer.cc
index 0d91163ed2e..21b499e2d8a 100644
--- a/Firestore/core/src/firebase/firestore/remote/serializer.cc
+++ b/Firestore/core/src/firebase/firestore/remote/serializer.cc
@@ -202,7 +202,6 @@ void EncodeFieldValueImpl(pb_ostream_t* stream, const FieldValue& field_value) {
       break;
 
     case FieldValue::Type::Object:
-      // NB: submessages use a wiretype of PB_WT_STRING
       status = pb_encode_tag(stream, PB_WT_STRING,
                              google_firestore_v1beta1_Value_map_value_tag);
       if (!status) {
@@ -332,7 +331,7 @@ void EncodeNestedFieldValue(pb_ostream_t* stream,
   }
 }
 
-FieldValue DecodeSubFieldValue(pb_istream_t* stream) {
+FieldValue DecodeNestedFieldValue(pb_istream_t* stream) {
   // Implementation note: This is roughly modeled on pb_decode_delimited,
   // adjusted to account for the oneof in FieldValue.
   pb_istream_t substream;
@@ -417,12 +416,11 @@ std::pair<std::string, FieldValue> DecodeFieldsEntry(pb_istream_t* stream) {
   status = pb_decode_tag(stream, &wire_type, &tag, &eof);
   FIREBASE_ASSERT(tag ==
                   google_firestore_v1beta1_MapValue_FieldsEntry_value_tag);
-  // NB: PB_WT_STRING is used for submessages too.
   FIREBASE_ASSERT(wire_type == PB_WT_STRING);
   FIREBASE_ASSERT(!eof);
   FIREBASE_ASSERT(status);
 
-  FieldValue value = DecodeSubFieldValue(stream);
+  FieldValue value = DecodeNestedFieldValue(stream);
 
   return {key, value};
 }