Addressing comments #1

Author: wu-hui

Firestore/Protos/nanopb/google/firestore/v1/document.nanopb.cc

@@ -33,7 +33,7 @@ const pb_field_t google_firestore_v1_Document_fields[5] = {
     PB_FIELD(  1, BYTES   , SINGULAR, POINTER , FIRST, google_firestore_v1_Document, name, name, 0),
     PB_FIELD(  2, MESSAGE , REPEATED, POINTER , OTHER, google_firestore_v1_Document, fields, name, &google_firestore_v1_Document_FieldsEntry_fields),
     PB_FIELD(  3, MESSAGE , SINGULAR, STATIC  , OTHER, google_firestore_v1_Document, create_time, fields, &google_protobuf_Timestamp_fields),
-    PB_FIELD(  4, MESSAGE , SINGULAR, STATIC  , OTHER, google_firestore_v1_Document, update_time, create_time, &google_protobuf_Timestamp_fields),
+    PB_FIELD(  4, MESSAGE , OPTIONAL, STATIC  , OTHER, google_firestore_v1_Document, update_time, create_time, &google_protobuf_Timestamp_fields),
     PB_LAST_FIELD
 };
 

Firestore/Protos/nanopb/google/firestore/v1/document.nanopb.h

@@ -56,6 +56,7 @@ typedef struct _google_firestore_v1_Document {
     pb_size_t fields_count;
     struct _google_firestore_v1_Document_FieldsEntry *fields;
     google_protobuf_Timestamp create_time;
+    bool has_update_time;
     google_protobuf_Timestamp update_time;
 /* @@protoc_insertion_point(struct:google_firestore_v1_Document) */
 } google_firestore_v1_Document;
@@ -93,13 +94,13 @@ typedef struct _google_firestore_v1_MapValue_FieldsEntry {
 /* Default values for struct fields */
 
 /* Initializer values for message structs */
-#define google_firestore_v1_Document_init_default {NULL, 0, NULL, google_protobuf_Timestamp_init_default, google_protobuf_Timestamp_init_default}
+#define google_firestore_v1_Document_init_default {NULL, 0, NULL, google_protobuf_Timestamp_init_default, false, google_protobuf_Timestamp_init_default}
 #define google_firestore_v1_Document_FieldsEntry_init_default {NULL, google_firestore_v1_Value_init_default}
 #define google_firestore_v1_Value_init_default   {0, {0}}
 #define google_firestore_v1_ArrayValue_init_default {0, NULL}
 #define google_firestore_v1_MapValue_init_default {0, NULL}
 #define google_firestore_v1_MapValue_FieldsEntry_init_default {NULL, google_firestore_v1_Value_init_default}
-#define google_firestore_v1_Document_init_zero   {NULL, 0, NULL, google_protobuf_Timestamp_init_zero, google_protobuf_Timestamp_init_zero}
+#define google_firestore_v1_Document_init_zero   {NULL, 0, NULL, google_protobuf_Timestamp_init_zero, false, google_protobuf_Timestamp_init_zero}
 #define google_firestore_v1_Document_FieldsEntry_init_zero {NULL, google_firestore_v1_Value_init_zero}
 #define google_firestore_v1_Value_init_zero      {0, {0}}
 #define google_firestore_v1_ArrayValue_init_zero {0, NULL}

Firestore/Protos/protos/google/firestore/v1/document.options

@@ -0,0 +1,22 @@
+# Copyright 2019 Google
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+# In proto3 mode, Nanopb doesn't allow distinguishing between unset fields and
+# fields having default values, even for non-primitive types. Using the
+# workaround suggested in
+# https://github.com/nanopb/nanopb/issues/255#issuecomment-291842903
+
+# Serializer needs to verify update_time is set.
+google.firestore.v1.Document.update_time proto3:false

Firestore/Protos/protos/google/firestore/v1/firestore.options

@@ -18,6 +18,6 @@
 # workaround suggested in
 # https://github.com/nanopb/nanopb/issues/255#issuecomment-291842903
 
-# cause is not set if everything is OK, serializer need to be able to tell
+# cause is not set if everything is OK, serializer needs to be able to tell
 # that is the case.
 google.firestore.v1.TargetChange.cause proto3:false

Firestore/core/src/firebase/firestore/local/local_serializer.cc

@@ -142,7 +142,7 @@ google_firestore_v1_Document LocalSerializer::EncodeDocument(
   }
 
   result.update_time = rpc_serializer_.EncodeVersion(doc.version());
-
+  result.has_update_time = true;
   // Ignore Document.create_time. (We don't use this in our on-disk protos.)
 
   return result;

Firestore/core/src/firebase/firestore/remote/serializer.cc

@@ -1496,6 +1496,7 @@ MutationResult Serializer::DecodeMutationResult(
       write_result.has_update_time
           ? DecodeSnapshotVersion(reader, write_result.update_time)
           : commit_version;
+
   absl::optional<std::vector<FieldValue>> transform_results;
   if (write_result.transform_results_count > 0) {
     transform_results = std::vector<FieldValue>{};
@@ -1504,17 +1505,19 @@ MutationResult Serializer::DecodeMutationResult(
           DecodeFieldValue(reader, write_result.transform_results[i]));
     }
   }
-  return MutationResult(std::move(version), std::move(transform_results));
+
+  return MutationResult(version, std::move(transform_results));
 }
 
 std::unordered_map<std::string, std::string>
 Serializer::EncodeListenRequestLabels(const QueryData& query_data) const {
   auto value = EncodeLabel(query_data.purpose());
-
-  std::unordered_map<std::string, std::string> result(1);
-  if (!value.empty()) {
-    result["goog-listen-tags"] = value;
+  if (value.empty()) {
+    return {};
   }
+
+  std::unordered_map<std::string, std::string> result;
+  result["goog-listen-tags"] = std::move(value);
   return result;
 }
 
@@ -1526,9 +1529,8 @@ std::string Serializer::EncodeLabel(QueryPurpose purpose) const {
       return "existence-filter-mismatch";
     case QueryPurpose::LimboResolution:
       return "limbo-document";
-    default:
-      HARD_FAIL("Unrecognized query purpose: %s", purpose);
   }
+  UNREACHABLE();
 }
 
 std::unique_ptr<WatchChange> Serializer::DecodeWatchChange(
@@ -1549,11 +1551,8 @@ std::unique_ptr<WatchChange> Serializer::DecodeWatchChange(
 
     case google_firestore_v1_ListenResponse_filter_tag:
       return DecodeExistenceFilterWatchChange(reader, watch_change.filter);
-
-    default:
-      HARD_FAIL("Unknown WatchChange.changeType %s",
-                watch_change.which_response_type);
   }
+  UNREACHABLE();
 }
 
 SnapshotVersion Serializer::DecodeVersion(
@@ -1569,28 +1568,18 @@ SnapshotVersion Serializer::DecodeVersion(
   if (listen_response.target_change.target_ids_count != 0) {
     return SnapshotVersion::None();
   }
-  return DecodeSnapshotVersion(reader, listen_response.target_change.read_time);
-}
-
-std::vector<TargetId> Serializer::DecodeTargetIdArray(nanopb::Reader* reader,
-                                                      int32_t* array,
-                                                      pb_size_t size) const {
-  std::vector<TargetId> target_ids;
 
-  for (pb_size_t i = 0; i < size; i++) {
-    target_ids.push_back(array[i]);
-  }
-  return target_ids;
+  return DecodeSnapshotVersion(reader, listen_response.target_change.read_time);
 }
 
 std::unique_ptr<WatchChange> Serializer::DecodeTargetChange(
     nanopb::Reader* reader,
     const google_firestore_v1_TargetChange& change) const {
   WatchTargetChangeState state =
       DecodeTargetChangeState(reader, change.target_change_type);
-  auto target_ids =
-      DecodeTargetIdArray(reader, change.target_ids, change.target_ids_count);
-  ByteString resume_token = ByteString(change.resume_token);
+  std::vector<TargetId> target_ids(change.target_ids,
+                                   change.target_ids + change.target_ids_count);
+  ByteString resume_token(change.resume_token);
 
   util::Status cause;
   if (change.has_cause) {
@@ -1616,9 +1605,8 @@ WatchTargetChangeState Serializer::DecodeTargetChangeState(
       return WatchTargetChangeState::Current;
     case google_firestore_v1_TargetChange_TargetChangeType_RESET:
       return WatchTargetChangeState::Reset;
-    default:
-      HARD_FAIL("Unexpected TargetChange.state: %s", state);
   }
+  UNREACHABLE();
 }
 
 std::unique_ptr<WatchChange> Serializer::DecodeDocumentChange(
@@ -1627,19 +1615,23 @@ std::unique_ptr<WatchChange> Serializer::DecodeDocumentChange(
   ObjectValue value = ObjectValue::FromMap(DecodeFields(
       reader, change.document.fields_count, change.document.fields));
   DocumentKey key = DecodeKey(reader, change.document.name);
+
+  HARD_ASSERT(change.document.has_update_time,
+              "Got a document change with no snapshot version");
   SnapshotVersion version =
       DecodeSnapshotVersion(reader, change.document.update_time);
-  HARD_ASSERT(version != SnapshotVersion::None(),
-              "Got a document change with no snapshot version");
-  // The document may soon be re-serialized back to protos in order to store it
-  // in local persistence. Memoize the encoded form to avoid encoding it again.
-  Document document(std::move(value), key, version, DocumentState::kSynced,
-                    change.document);
 
-  auto updated_target_ids =
-      DecodeTargetIdArray(reader, change.target_ids, change.target_ids_count);
-  auto removed_target_ids = DecodeTargetIdArray(
-      reader, change.removed_target_ids, change.removed_target_ids_count);
+  // TODO(wuandy): Originally `document` is constructed with `change.document`
+  // as last argument, such that it does not have to encode the proto again
+  // when saving it. It's dangerous because last argument is an `any` type.
+  // Revisit this when porting local serializer to see if we can do it safely.
+  Document document(std::move(value), key, version, DocumentState::kSynced);
+
+  std::vector<TargetId> updated_target_ids(
+      change.target_ids, change.target_ids + change.target_ids_count);
+  std::vector<TargetId> removed_target_ids(
+      change.removed_target_ids,
+      change.removed_target_ids + change.removed_target_ids_count);
 
   return absl::make_unique<DocumentWatchChange>(
       std::move(updated_target_ids), std::move(removed_target_ids),
@@ -1651,14 +1643,15 @@ std::unique_ptr<WatchChange> Serializer::DecodeDocumentDelete(
     const google_firestore_v1_DocumentDelete& change) const {
   DocumentKey key = DecodeKey(reader, change.document);
   // Note that version might be unset in which case we use
-  // SnapshotVersion::None()
+  // SnapshotVersion::None().
   SnapshotVersion version =
       change.has_read_time ? DecodeSnapshotVersion(reader, change.read_time)
                            : SnapshotVersion::None();
   NoDocument document(key, version, /* has_committed_mutations= */ false);
 
-  std::vector<TargetId> removed_target_ids = DecodeTargetIdArray(
-      reader, change.removed_target_ids, change.removed_target_ids_count);
+  std::vector<TargetId> removed_target_ids(
+      change.removed_target_ids,
+      change.removed_target_ids + change.removed_target_ids_count);
 
   return absl::make_unique<DocumentWatchChange>(
       std::vector<TargetId>{}, std::move(removed_target_ids), std::move(key),
@@ -1669,8 +1662,9 @@ std::unique_ptr<WatchChange> Serializer::DecodeDocumentRemove(
     nanopb::Reader* reader,
     const google_firestore_v1_DocumentRemove& change) const {
   DocumentKey key = DecodeKey(reader, change.document);
-  std::vector<TargetId> removed_target_ids = DecodeTargetIdArray(
-      reader, change.removed_target_ids, change.removed_target_ids_count);
+  std::vector<TargetId> removed_target_ids(
+      change.removed_target_ids,
+      change.removed_target_ids + change.removed_target_ids_count);
 
   return absl::make_unique<DocumentWatchChange>(std::vector<TargetId>{},
                                                 std::move(removed_target_ids),

Firestore/core/src/firebase/firestore/remote/serializer.h

@@ -349,9 +349,6 @@ class Serializer {
   std::unique_ptr<remote::WatchChange> DecodeDocumentChange(
       nanopb::Reader* reader,
       const google_firestore_v1_DocumentChange& change) const;
-  std::vector<model::TargetId> DecodeTargetIdArray(nanopb::Reader* reader,
-                                                   int32_t* array,
-                                                   pb_size_t size) const;
   std::unique_ptr<remote::WatchChange> DecodeDocumentDelete(
       nanopb::Reader* reader,
       const google_firestore_v1_DocumentDelete& change) const;

Firestore/core/test/firebase/firestore/remote/serializer_test.cc

@@ -120,23 +120,26 @@ QueryData CreateQueryData(absl::string_view str) {
   return CreateQueryData(Query(str));
 }
 
+template <typename T>
+bool Equals(const WatchChange& lhs, const WatchChange& rhs) {
+  return static_cast<const T&>(lhs) == static_cast<const T&>(rhs);
+}
+
+// Compares two `WatchChange`s taking into account their actual derived type.
 bool operator==(const WatchChange& lhs, const WatchChange& rhs) {
   if (lhs.type() != rhs.type()) {
     return false;
   }
+
   switch (lhs.type()) {
     case WatchChange::Type::Document:
-      return static_cast<const DocumentWatchChange&>(lhs) ==
-             static_cast<const DocumentWatchChange&>(rhs);
-    case WatchChange::Type::TargetChange:
-      return static_cast<const WatchTargetChange&>(lhs) ==
-             static_cast<const WatchTargetChange&>(rhs);
+      return Equals<DocumentWatchChange>(lhs, rhs);
     case WatchChange::Type::ExistenceFilter:
-      return static_cast<const ExistenceFilterWatchChange&>(lhs) ==
-             static_cast<const ExistenceFilterWatchChange&>(rhs);
-    default:
-      HARD_FAIL("Unknown WatchChange.type() %s", lhs.type());
+      return Equals<ExistenceFilterWatchChange>(lhs, rhs);
+    case WatchChange::Type::TargetChange:
+      return Equals<WatchTargetChange>(lhs, rhs);
   }
+  UNREACHABLE();
 }
 
 }  // namespace
@@ -498,7 +501,7 @@ class SerializerTest : public ::testing::Test {
         google_firestore_v1_ListenResponse_fields,
         std::mem_fn(&Serializer::DecodeWatchChange), proto);
 
-    ASSERT_TRUE(model == *actual_model);
+    EXPECT_TRUE(model == *actual_model);
   }
 
   template <typename T>
@@ -1468,18 +1471,17 @@ TEST_F(SerializerTest, EncodesResumeTokens) {
 TEST_F(SerializerTest, EncodesListenRequestLabels) {
   core::Query q = Query("docs");
 
-  std::vector<
-      std::pair<QueryPurpose, std::unordered_map<std::string, std::string>>>
-      purposeToLabel = {
+  std::map<QueryPurpose, std::unordered_map<std::string, std::string>>
+      purpose_to_label = {
           {QueryPurpose::Listen, {}},
           {QueryPurpose::LimboResolution,
            {{"goog-listen-tags", "limbo-document"}}},
           {QueryPurpose::ExistenceFilterMismatch,
            {{"goog-listen-tags", "existence-filter-mismatch"}}},
       };
 
-  for (const auto& p : purposeToLabel) {
-    QueryData model(q, 1, 0, p.first, SnapshotVersion::None(), Bytes(1, 2, 3));
+  for (const auto& p : purpose_to_label) {
+    QueryData model(q, 1, 0, p.first);
 
     auto result = serializer.EncodeListenRequestLabels(model);
     EXPECT_EQ(result, p.second);