Simplify Serializer Decode*() methods to no longer deserialize manually.

Now, we use the re-worked nanopb deserializer (that mallocs). This also
had the advantage of moving the C++ serialization code closer to the
other platforms.

Still TODO:
- Reorder the methods within the files to resemble the other platforms.
- Rework error handling.
- Encode*() methods.

Author: rsgowman

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

@@ -29,6 +29,7 @@
 #include "Firestore/core/src/firebase/firestore/model/snapshot_version.h"
 #include "Firestore/core/src/firebase/firestore/nanopb/tag.h"
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
+#include "Firestore/core/src/firebase/firestore/util/string_format.h"
 
 namespace firebase {
 namespace firestore {
@@ -44,6 +45,7 @@ using nanopb::Reader;
 using nanopb::Tag;
 using nanopb::Writer;
 using util::Status;
+using util::StringFormat;
 
 void LocalSerializer::EncodeMaybeDocument(
     Writer* writer, const MaybeDocument& maybe_doc) const {
@@ -73,37 +75,24 @@ void LocalSerializer::EncodeMaybeDocument(
 }
 
 std::unique_ptr<MaybeDocument> LocalSerializer::DecodeMaybeDocument(
-    Reader* reader) const {
-  std::unique_ptr<MaybeDocument> result;
-
-  while (reader->good()) {
-    switch (reader->ReadTag()) {
-      case firestore_client_MaybeDocument_document_tag:
-        // TODO(rsgowman): If multiple 'document' values are found, we should
-        // merge them (rather than using the last one.)
-        result = reader->ReadNestedMessage<Document>(
-            rpc_serializer_, &remote::Serializer::DecodeDocument);
-        break;
-
-      case firestore_client_MaybeDocument_no_document_tag:
-        // TODO(rsgowman): If multiple 'no_document' values are found, we should
-        // merge them (rather than using the last one.)
-        result = reader->ReadNestedMessage<NoDocument>(
-            *this, &LocalSerializer::DecodeNoDocument);
-        break;
-
-      default:
-        reader->SkipUnknown();
-    }
-  }
+    Reader* reader, const firestore_client_MaybeDocument& proto) const {
+  if (!reader->status().ok()) return nullptr;
+
+  switch (proto.which_document_type) {
+    case firestore_client_MaybeDocument_document_tag:
+      return rpc_serializer_.DecodeDocument(reader, proto.document);
+
+    case firestore_client_MaybeDocument_no_document_tag:
+      return DecodeNoDocument(reader, proto.no_document);
 
-  if (!result) {
-    reader->Fail(
-        "Invalid MaybeDocument message: Neither 'no_document' nor 'document' "
-        "fields set.");
-    return nullptr;
+    default:
+      reader->Fail(
+          "Invalid MaybeDocument message: Neither 'no_document' nor 'document' "
+          "fields set.");
+      return nullptr;
   }
-  return result;
+
+  UNREACHABLE();
 }
 
 void LocalSerializer::EncodeDocument(Writer* writer,
@@ -146,30 +135,16 @@ void LocalSerializer::EncodeNoDocument(Writer* writer,
 }
 
 std::unique_ptr<NoDocument> LocalSerializer::DecodeNoDocument(
-    Reader* reader) const {
-  std::string name;
-  absl::optional<SnapshotVersion> version = SnapshotVersion::None();
-
-  while (reader->good()) {
-    switch (reader->ReadTag()) {
-      case firestore_client_NoDocument_name_tag:
-        name = reader->ReadString();
-        break;
-
-      case firestore_client_NoDocument_read_time_tag:
-        version = reader->ReadNestedMessage<SnapshotVersion>(
-            rpc_serializer_.DecodeSnapshotVersion);
-        break;
-
-      default:
-        reader->SkipUnknown();
-        break;
-    }
-  }
+    Reader* reader, const firestore_client_NoDocument& proto) const {
+  if (!reader->status().ok()) return nullptr;
+
+  absl::optional<SnapshotVersion> version =
+      rpc_serializer_.DecodeSnapshotVersion(reader, proto.read_time);
 
   if (!reader->status().ok()) return nullptr;
-  return absl::make_unique<NoDocument>(rpc_serializer_.DecodeKey(name),
-                                       *std::move(version));
+  return absl::make_unique<NoDocument>(
+      rpc_serializer_.DecodeKey(rpc_serializer_.DecodeString(proto.name)),
+      *std::move(version));
 }
 
 void LocalSerializer::EncodeQueryData(Writer* writer,
@@ -207,45 +182,28 @@ void LocalSerializer::EncodeQueryData(Writer* writer,
 }
 
 absl::optional<QueryData> LocalSerializer::DecodeQueryData(
-    Reader* reader) const {
-  model::TargetId target_id = 0;
-  absl::optional<SnapshotVersion> version = SnapshotVersion::None();
-  std::vector<uint8_t> resume_token;
+    Reader* reader, const firestore_client_Target& proto) const {
+  if (!reader->status().ok()) return absl::nullopt;
+
+  model::TargetId target_id = proto.target_id;
+  absl::optional<SnapshotVersion> version =
+      rpc_serializer_.DecodeSnapshotVersion(reader, proto.snapshot_version);
+  std::vector<uint8_t> resume_token =
+      rpc_serializer_.DecodeBytes(proto.resume_token);
   absl::optional<Query> query = Query::Invalid();
 
-  while (reader->good()) {
-    switch (reader->ReadTag()) {
-      case firestore_client_Target_target_id_tag:
-        // TODO(rsgowman): How to handle truncation of integer types?
-        target_id = static_cast<model::TargetId>(reader->ReadInteger());
-        break;
-
-      case firestore_client_Target_snapshot_version_tag:
-        version = reader->ReadNestedMessage<SnapshotVersion>(
-            rpc_serializer_.DecodeSnapshotVersion);
-        break;
-
-      case firestore_client_Target_resume_token_tag:
-        resume_token = reader->ReadBytes();
-        break;
-
-      case firestore_client_Target_query_tag:
-        // TODO(rsgowman): Clear 'documents' field (since query and documents
-        // are part of a 'oneof').
-        query =
-            reader->ReadNestedMessage<Query>(rpc_serializer_.DecodeQueryTarget);
-        break;
-
-      case firestore_client_Target_documents_tag:
-        // Clear 'query' field (since query and documents are part of a 'oneof')
-        query = Query::Invalid();
-        // TODO(rsgowman): Implement.
-        abort();
-
-      default:
-        reader->SkipUnknown();
-        break;
-    }
+  switch (proto.which_target_type) {
+    case firestore_client_Target_query_tag:
+      query = rpc_serializer_.DecodeQueryTarget(reader, proto.query);
+      break;
+
+    case firestore_client_Target_documents_tag:
+      // TODO(rsgowman): Implement.
+      abort();
+
+    default:
+      reader->Fail(
+          StringFormat("Unknown target_type: %s", proto.which_target_type));
   }
 
   if (!reader->status().ok()) return absl::nullopt;

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

@@ -20,6 +20,8 @@
 #include <memory>
 #include <vector>
 
+#include "Firestore/Protos/nanopb/firestore/local/maybe_document.nanopb.h"
+#include "Firestore/Protos/nanopb/firestore/local/target.nanopb.h"
 #include "Firestore/core/src/firebase/firestore/local/query_data.h"
 #include "Firestore/core/src/firebase/firestore/model/document.h"
 #include "Firestore/core/src/firebase/firestore/model/maybe_document.h"
@@ -62,19 +64,19 @@ class LocalSerializer {
                            const model::MaybeDocument& maybe_doc) const;
 
   /**
-   * @brief Decodes bytes representing a MaybeDocument proto to the equivalent
-   * model.
+   * @brief Decodes nanopb proto representing a MaybeDocument proto to the
+   * equivalent model.
    *
    * Check reader->status() to determine if an error occured while decoding.
    *
-   * @param reader The reader object containing the bytes to convert. It's
-   * assumed that exactly all of the bytes will be used by this conversion.
+   * @param reader The Reader object. Used only for error handling.
    * @return The model equivalent of the bytes or nullopt if an error occurred.
    * @post (reader->status().ok() && result) ||
    * (!reader->status().ok() && !result)
    */
   std::unique_ptr<model::MaybeDocument> DecodeMaybeDocument(
-      nanopb::Reader* reader) const;
+      nanopb::Reader* reader,
+      const firestore_client_MaybeDocument& proto) const;
 
   /**
    * @brief Encodes a QueryData to the equivalent bytes, representing a
@@ -88,19 +90,19 @@ class LocalSerializer {
                        const QueryData& query_data) const;
 
   /**
-   * @brief Decodes bytes representing a ::firestore::proto::Target proto to the
-   * equivalent QueryData.
+   * @brief Decodes nanopb proto representing a ::firestore::proto::Target proto
+   * to the equivalent QueryData.
    *
-   * Check writer->status() to determine if an error occured while decoding.
+   * Check reader->status() to determine if an error occured while decoding.
    *
-   * @param reader The reader object containing the bytes to convert. It's
-   * assumed that exactly all of the bytes will be used by this conversion.
+   * @param reader The Reader object. Used only for error handling.
    * @return The QueryData equivalent of the bytes or nullopt if an error
    * occurred.
    * @post (reader->status().ok() && result.has_value()) ||
    * (!reader->status().ok() && !result.has_value())
    */
-  absl::optional<QueryData> DecodeQueryData(nanopb::Reader* reader) const;
+  absl::optional<QueryData> DecodeQueryData(
+      nanopb::Reader* reader, const firestore_client_Target& proto) const;
 
  private:
   /**
@@ -114,7 +116,7 @@ class LocalSerializer {
                         const model::NoDocument& no_doc) const;
 
   std::unique_ptr<model::NoDocument> DecodeNoDocument(
-      nanopb::Reader* reader) const;
+      nanopb::Reader* reader, const firestore_client_NoDocument& proto) const;
 
   const remote::Serializer& rpc_serializer_;
 };

Firestore/core/src/firebase/firestore/nanopb/reader.cc

@@ -73,6 +73,10 @@ void Reader::ReadNanopbMessage(const pb_field_t fields[], void* dest_struct) {
   }
 }
 
+void Reader::FreeNanopbMessage(const pb_field_t fields[], void* dest_struct) {
+  pb_release(fields, dest_struct);
+}
+
 /**
  * Note that (despite the return type) this works for bool, enum, int32, int64,
  * uint32 and uint64 proto field types.

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

@@ -81,12 +81,27 @@ class Reader {
   /**
    * Reads a nanopb message from the input stream.
    *
-   * This essentially wraps calls to nanopb's pb_decode() method. If we didn't
-   * use `oneof`s in our protos, this would be the primary way of decoding
-   * messages.
+   * This essentially wraps calls to nanopb's pb_decode() method. This is the
+   * primary way of decoding messages.
+   *
+   * Note that this allocates memory. You must call FreeNanopbMessage() (which
+   * essentially wraps pb_release()) on the dest_struct in order to avoid memory
+   * leaks. (This also implies code that uses this is not exception safe.)
    */
+  // TODO(rsgowman): At the moment we rely on the caller to manually free
+  // dest_struct via FreeNanopbMessage(). We might instead see if we can
+  // register allocated messages, track them, and free them ourselves. This may
+  // be especially relevant if we start to use nanopb messages as the underlying
+  // data within the model objects.
   void ReadNanopbMessage(const pb_field_t fields[], void* dest_struct);
 
+  /**
+   * Release memory allocated by ReadNanopbMessage().
+   *
+   * This essentially wraps calls to nanopb's pb_release() method.
+   */
+  void FreeNanopbMessage(const pb_field_t fields[], void* dest_struct);
+
   void ReadNull();
   bool ReadBool();
   std::int64_t ReadInteger();