LocalSerializer feature complete (#4046)

* LocalSerializer feature complete

* Addressing comments 1

* Walk around nanopb boolean issue

* Rename to SafeReadBoolean

Author: wu-hui

Firestore/Example/App/Testing

@@ -0,0 +1 @@
+Subproject commit 0688b41b5c9ac80b6f8e17394680070926a65d63

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

@@ -42,15 +42,19 @@ namespace {
 
 using core::Query;
 using model::Document;
+using model::DocumentState;
+using model::FieldValue;
 using model::MaybeDocument;
 using model::Mutation;
 using model::MutationBatch;
 using model::NoDocument;
+using model::ObjectValue;
 using model::SnapshotVersion;
 using model::UnknownDocument;
 using nanopb::ByteString;
 using nanopb::CheckedSize;
 using nanopb::Reader;
+using nanopb::SafeReadBoolean;
 using nanopb::Writer;
 using remote::InvalidQuery;
 using remote::MakeArray;
@@ -64,34 +68,34 @@ firestore_client_MaybeDocument LocalSerializer::EncodeMaybeDocument(
   firestore_client_MaybeDocument result{};
 
   switch (maybe_doc.type()) {
-    case MaybeDocument::Type::Document:
+    case MaybeDocument::Type::Document: {
       result.which_document_type = firestore_client_MaybeDocument_document_tag;
-      result.document = EncodeDocument(static_cast<const Document&>(maybe_doc));
-      // TODO(rsgowman): heldwriteacks:
-      // result.has_committed_mutations = existing_doc.HasCommittedMutations();
+      Document doc(maybe_doc);
+      // TODO(wuandy): Check if `doc` already has a proto and use that if yes.
+      result.document = EncodeDocument(doc);
+      result.has_committed_mutations = doc.has_committed_mutations();
       return result;
+    }
 
-    case MaybeDocument::Type::NoDocument:
+    case MaybeDocument::Type::NoDocument: {
       result.which_document_type =
           firestore_client_MaybeDocument_no_document_tag;
-      result.no_document =
-          EncodeNoDocument(static_cast<const NoDocument&>(maybe_doc));
-      // TODO(rsgowman): heldwriteacks:
-      // result.has_committed_mutations = no_doc.HasCommittedMutations();
+      NoDocument no_doc(maybe_doc);
+      result.no_document = EncodeNoDocument(no_doc);
+      result.has_committed_mutations = no_doc.has_committed_mutations();
       return result;
+    }
 
     case MaybeDocument::Type::UnknownDocument:
       result.which_document_type =
           firestore_client_MaybeDocument_unknown_document_tag;
       result.unknown_document =
-          EncodeUnknownDocument(static_cast<const UnknownDocument&>(maybe_doc));
-      // TODO(rsgowman): heldwriteacks:
-      // result.has_committed_mutations = true;
+          EncodeUnknownDocument(UnknownDocument(maybe_doc));
+      result.has_committed_mutations = true;
       return result;
 
     case MaybeDocument::Type::Invalid:
-      // TODO(rsgowman): Error handling
-      abort();
+      HARD_FAIL("Unknown document type %s", maybe_doc.type());
   }
 
   UNREACHABLE();
@@ -103,10 +107,12 @@ MaybeDocument LocalSerializer::DecodeMaybeDocument(
 
   switch (proto.which_document_type) {
     case firestore_client_MaybeDocument_document_tag:
-      return rpc_serializer_.DecodeDocument(reader, proto.document);
+      return DecodeDocument(reader, proto.document,
+                            SafeReadBoolean(proto.has_committed_mutations));
 
     case firestore_client_MaybeDocument_no_document_tag:
-      return DecodeNoDocument(reader, proto.no_document);
+      return DecodeNoDocument(reader, proto.no_document,
+                              SafeReadBoolean(proto.has_committed_mutations));
 
     case firestore_client_MaybeDocument_unknown_document_tag:
       return DecodeUnknownDocument(reader, proto.unknown_document);
@@ -148,6 +154,23 @@ google_firestore_v1_Document LocalSerializer::EncodeDocument(
   return result;
 }
 
+Document LocalSerializer::DecodeDocument(
+    Reader* reader,
+    const google_firestore_v1_Document& proto,
+    bool has_committed_mutations) const {
+  ObjectValue fields =
+      rpc_serializer_.DecodeFields(reader, proto.fields_count, proto.fields);
+  SnapshotVersion version =
+      rpc_serializer_.DecodeVersion(reader, proto.update_time);
+
+  DocumentState state = has_committed_mutations
+                            ? DocumentState::kCommittedMutations
+                            : DocumentState::kSynced;
+  return Document(std::move(fields),
+                  rpc_serializer_.DecodeKey(reader, proto.name), version,
+                  state);
+}
+
 firestore_client_NoDocument LocalSerializer::EncodeNoDocument(
     const NoDocument& no_doc) const {
   firestore_client_NoDocument result{};
@@ -159,15 +182,14 @@ firestore_client_NoDocument LocalSerializer::EncodeNoDocument(
 }
 
 NoDocument LocalSerializer::DecodeNoDocument(
-    Reader* reader, const firestore_client_NoDocument& proto) const {
+    Reader* reader,
+    const firestore_client_NoDocument& proto,
+    bool has_committed_mutations) const {
   SnapshotVersion version =
       rpc_serializer_.DecodeVersion(reader, proto.read_time);
 
-  // TODO(rsgowman): Fix hardcoding of has_committed_mutations.
-  // Instead, we should grab this from the proto (see other ports). However,
-  // we'll defer until the nanopb-master gets merged to master.
   return NoDocument(rpc_serializer_.DecodeKey(reader, proto.name), version,
-                    /*has_committed_mutations=*/false);
+                    has_committed_mutations);
 }
 
 firestore_client_UnknownDocument LocalSerializer::EncodeUnknownDocument(
@@ -203,13 +225,8 @@ firestore_client_Target LocalSerializer::EncodeQueryData(
 
   const Query& query = query_data.query();
   if (query.IsDocumentQuery()) {
-    // TODO(rsgowman): Implement. Probably like this (once EncodeDocumentsTarget
-    // exists):
-    /*
-    result.which_target_type = firestore_client_Target_document_tag;
+    result.which_target_type = firestore_client_Target_documents_tag;
     result.documents = rpc_serializer_.EncodeDocumentsTarget(query);
-    */
-    abort();
   } else {
     result.which_target_type = firestore_client_Target_query_tag;
     result.query = rpc_serializer_.EncodeQueryTarget(query);
@@ -223,7 +240,6 @@ QueryData LocalSerializer::DecodeQueryData(
   if (!reader->status().ok()) return QueryData::Invalid();
 
   model::TargetId target_id = proto.target_id;
-  // TODO(rgowman): How to handle truncation of integer types?
   model::ListenSequenceNumber sequence_number =
       static_cast<model::ListenSequenceNumber>(
           proto.last_listen_sequence_number);
@@ -238,8 +254,8 @@ QueryData LocalSerializer::DecodeQueryData(
       break;
 
     case firestore_client_Target_documents_tag:
-      // TODO(rsgowman): Implement.
-      abort();
+      query = rpc_serializer_.DecodeDocumentsTarget(reader, proto.documents);
+      break;
 
     default:
       reader->Fail(
@@ -303,6 +319,16 @@ MutationBatch LocalSerializer::DecodeMutationBatch(
                        std::move(mutations));
 }
 
+google_protobuf_Timestamp LocalSerializer::EncodeVersion(
+    const model::SnapshotVersion& version) const {
+  return rpc_serializer_.EncodeVersion(version);
+}
+
+model::SnapshotVersion LocalSerializer::DecodeVersion(
+    nanopb::Reader* reader, const google_protobuf_Timestamp& proto) const {
+  return rpc_serializer_.DecodeVersion(reader, proto);
+}
+
 }  // namespace local
 }  // namespace firestore
 }  // namespace firebase

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

@@ -113,6 +113,12 @@ class LocalSerializer {
   model::MutationBatch DecodeMutationBatch(
       nanopb::Reader* reader, const firestore_client_WriteBatch& proto) const;
 
+  google_protobuf_Timestamp EncodeVersion(
+      const model::SnapshotVersion& version) const;
+
+  model::SnapshotVersion DecodeVersion(
+      nanopb::Reader* reader, const google_protobuf_Timestamp& proto) const;
+
  private:
   /**
    * Encodes a Document for local storage. This differs from the v1 RPC
@@ -121,11 +127,16 @@ class LocalSerializer {
    */
   google_firestore_v1_Document EncodeDocument(const model::Document& doc) const;
 
+  model::Document DecodeDocument(nanopb::Reader* reader,
+                                 const google_firestore_v1_Document& proto,
+                                 bool has_committed_mutations) const;
+
   firestore_client_NoDocument EncodeNoDocument(
       const model::NoDocument& no_doc) const;
 
-  model::NoDocument DecodeNoDocument(
-      nanopb::Reader* reader, const firestore_client_NoDocument& proto) const;
+  model::NoDocument DecodeNoDocument(nanopb::Reader* reader,
+                                     const firestore_client_NoDocument& proto,
+                                     bool has_committed_mutations) const;
 
   firestore_client_UnknownDocument EncodeUnknownDocument(
       const model::UnknownDocument& unknown_doc) const;

Firestore/core/src/firebase/firestore/nanopb/nanopb_util.h

@@ -27,6 +27,8 @@
 #include "Firestore/core/src/firebase/firestore/nanopb/byte_string.h"
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
 #include "Firestore/core/src/firebase/firestore/util/nullability.h"
+#include "absl/base/casts.h"
+#include "absl/memory/memory.h"
 
 namespace firebase {
 namespace firestore {
@@ -88,6 +90,20 @@ inline std::vector<uint8_t> MakeVector(const ByteString& str) {
   return {str.begin(), str.end()};
 }
 
+/**
+ * Due to the nanopb implementation, nanopb_boolean could be an integer
+ * other than 0 or 1, (such as 2). This leads to undefined behaviour when
+ * it's read as a boolean. eg. on at least gcc, the value is treated as
+ * both true *and* false. So we'll instead memcpy to an integer (via
+ * absl::bit_cast) and compare with 0.
+ *
+ * Note that it is necessary to pass-by-reference here to get the original
+ * value of `nanopb_boolean`.
+ */
+inline bool SafeReadBoolean(const bool& nanopb_boolean) {
+  return absl::bit_cast<int8_t>(nanopb_boolean) != 0;
+}
+
 #if __OBJC__
 inline ByteString MakeByteString(NSData* _Nullable value) {
   if (value == nil) return ByteString();

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

@@ -49,8 +49,6 @@
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
 #include "Firestore/core/src/firebase/firestore/util/string_format.h"
 #include "absl/algorithm/container.h"
-#include "absl/base/casts.h"
-#include "absl/memory/memory.h"
 
 namespace firebase {
 namespace firestore {
@@ -96,6 +94,7 @@ using nanopb::ByteString;
 using nanopb::CheckedSize;
 using nanopb::MakeStringView;
 using nanopb::Reader;
+using nanopb::SafeReadBoolean;
 using nanopb::Writer;
 using remote::WatchChange;
 using util::Status;
@@ -355,7 +354,7 @@ FieldValue::Map::value_type Serializer::DecodeFieldsEntry(
   return FieldValue::Map::value_type{std::move(key), std::move(value)};
 }
 
-FieldValue::Map Serializer::DecodeFields(
+ObjectValue Serializer::DecodeFields(
     Reader* reader,
     size_t count,
     const google_firestore_v1_Document_FieldsEntry* fields) const {
@@ -365,7 +364,7 @@ FieldValue::Map Serializer::DecodeFields(
     result = result.insert(std::move(kv.first), std::move(kv.second));
   }
 
-  return result;
+  return ObjectValue::FromMap(result);
 }
 
 FieldValue::Map Serializer::DecodeMapValue(
@@ -392,13 +391,7 @@ FieldValue Serializer::DecodeFieldValue(
       return FieldValue::Null();
 
     case google_firestore_v1_Value_boolean_value_tag: {
-      // Due to the nanopb implementation, msg.boolean_value could be an integer
-      // other than 0 or 1, (such as 2). This leads to undefined behaviour when
-      // it's read as a boolean. eg. on at least gcc, the value is treated as
-      // both true *and* false. So we'll instead memcpy to an integer (via
-      // absl::bit_cast) and compare with 0.
-      int bool_as_int = absl::bit_cast<int8_t>(msg.boolean_value);
-      return FieldValue::FromBoolean(bool_as_int != 0);
+      return FieldValue::FromBoolean(SafeReadBoolean(msg.boolean_value));
     }
 
     case google_firestore_v1_Value_integer_value_tag:
@@ -551,16 +544,16 @@ Document Serializer::DecodeFoundDocument(
               "Tried to deserialize a found document from a missing document.");
 
   DocumentKey key = DecodeKey(reader, response.found.name);
-  FieldValue::Map value =
+  ObjectValue value =
       DecodeFields(reader, response.found.fields_count, response.found.fields);
   SnapshotVersion version = DecodeVersion(reader, response.found.update_time);
 
   if (version == SnapshotVersion::None()) {
     reader->Fail("Got a document response with no snapshot version");
   }
 
-  return Document(ObjectValue::FromMap(std::move(value)), std::move(key),
-                  version, DocumentState::kSynced);
+  return Document(std::move(value), std::move(key), version,
+                  DocumentState::kSynced);
 }
 
 NoDocument Serializer::DecodeMissingDocument(
@@ -582,17 +575,6 @@ NoDocument Serializer::DecodeMissingDocument(
                     /*has_committed_mutations=*/false);
 }
 
-Document Serializer::DecodeDocument(
-    Reader* reader, const google_firestore_v1_Document& proto) const {
-  FieldValue::Map fields_internal =
-      DecodeFields(reader, proto.fields_count, proto.fields);
-  SnapshotVersion version = DecodeVersion(reader, proto.update_time);
-
-  return Document(ObjectValue::FromMap(std::move(fields_internal)),
-                  DecodeKey(reader, proto.name), version,
-                  DocumentState::kSynced);
-}
-
 google_firestore_v1_Write Serializer::EncodeMutation(
     const Mutation& mutation) const {
   HARD_ASSERT(mutation.is_valid(), "Invalid mutation encountered.");
@@ -672,8 +654,8 @@ Mutation Serializer::DecodeMutation(
   switch (mutation.which_operation) {
     case google_firestore_v1_Write_update_tag: {
       DocumentKey key = DecodeKey(reader, mutation.update.name);
-      ObjectValue value = ObjectValue::FromMap(DecodeFields(
-          reader, mutation.update.fields_count, mutation.update.fields));
+      ObjectValue value = DecodeFields(reader, mutation.update.fields_count,
+                                       mutation.update.fields);
       FieldMask mask = DecodeFieldMask(mutation.update_mask);
       if (mask.size() > 0) {
         return PatchMutation(std::move(key), std::move(value), std::move(mask),
@@ -1634,8 +1616,8 @@ WatchTargetChangeState Serializer::DecodeTargetChangeState(
 std::unique_ptr<WatchChange> Serializer::DecodeDocumentChange(
     nanopb::Reader* reader,
     const google_firestore_v1_DocumentChange& change) const {
-  ObjectValue value = ObjectValue::FromMap(DecodeFields(
-      reader, change.document.fields_count, change.document.fields));
+  ObjectValue value = DecodeFields(reader, change.document.fields_count,
+                                   change.document.fields);
   DocumentKey key = DecodeKey(reader, change.document.name);
 
   HARD_ASSERT(change.document.has_update_time,

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

@@ -212,9 +212,6 @@ class Serializer {
   static pb_bytes_array_t* EncodeFieldPath(const model::FieldPath& field_path);
   static model::FieldPath DecodeFieldPath(const pb_bytes_array_t* field_path);
 
-  model::Document DecodeDocument(
-      nanopb::Reader* reader, const google_firestore_v1_Document& proto) const;
-
   static google_protobuf_Timestamp EncodeVersion(
       const model::SnapshotVersion& version);
 
@@ -260,6 +257,11 @@ class Serializer {
       nanopb::Reader* reader,
       const google_firestore_v1_ListenResponse& listen_response) const;
 
+  model::ObjectValue DecodeFields(
+      nanopb::Reader* reader,
+      size_t count,
+      const google_firestore_v1_Document_FieldsEntry* fields) const;
+
   // Public for the sake of tests.
   google_firestore_v1_StructuredQuery_Filter EncodeFilters(
       const core::FilterList& filters) const;
@@ -296,10 +298,6 @@ class Serializer {
   model::FieldValue::Map::value_type DecodeFieldsEntry(
       nanopb::Reader* reader,
       const google_firestore_v1_Document_FieldsEntry& fields) const;
-  model::FieldValue::Map DecodeFields(
-      nanopb::Reader* reader,
-      size_t count,
-      const google_firestore_v1_Document_FieldsEntry* fields) const;
 
   model::FieldValue::Map DecodeMapValue(
       nanopb::Reader* reader,