Fix small issues in C++ serializer and port remaining tests (#4012)

These were discovered by running integration tests using the new `Serializer`.

Also, port all the missing tests from `FSTSerializerBetaTests.mm` (that I could find).

Author: var-const

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

@@ -33,8 +33,8 @@ const pb_field_t google_firestore_v1_Write_fields[6] = {
     PB_ANONYMOUS_ONEOF_FIELD(operation,   1, MESSAGE , ONEOF, STATIC  , FIRST, google_firestore_v1_Write, update, update, &google_firestore_v1_Document_fields),
     PB_ANONYMOUS_ONEOF_FIELD(operation,   2, BYTES   , ONEOF, POINTER , UNION, google_firestore_v1_Write, delete_, delete_, 0),
     PB_ANONYMOUS_ONEOF_FIELD(operation,   6, MESSAGE , ONEOF, STATIC  , UNION, google_firestore_v1_Write, transform, transform, &google_firestore_v1_DocumentTransform_fields),
-    PB_FIELD(  3, MESSAGE , SINGULAR, STATIC  , OTHER, google_firestore_v1_Write, update_mask, transform, &google_firestore_v1_DocumentMask_fields),
-    PB_FIELD(  4, MESSAGE , SINGULAR, STATIC  , OTHER, google_firestore_v1_Write, current_document, update_mask, &google_firestore_v1_Precondition_fields),
+    PB_FIELD(  3, MESSAGE , OPTIONAL, STATIC  , OTHER, google_firestore_v1_Write, update_mask, transform, &google_firestore_v1_DocumentMask_fields),
+    PB_FIELD(  4, MESSAGE , OPTIONAL, STATIC  , OTHER, google_firestore_v1_Write, current_document, update_mask, &google_firestore_v1_Precondition_fields),
     PB_LAST_FIELD
 };
 

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

@@ -108,7 +108,9 @@ typedef struct _google_firestore_v1_Write {
         pb_bytes_array_t *delete_;
         google_firestore_v1_DocumentTransform transform;
     };
+    bool has_update_mask;
     google_firestore_v1_DocumentMask update_mask;
+    bool has_current_document;
     google_firestore_v1_Precondition current_document;
 /* @@protoc_insertion_point(struct:google_firestore_v1_Write) */
 } google_firestore_v1_Write;
@@ -124,15 +126,15 @@ typedef struct _google_firestore_v1_WriteResult {
 /* Default values for struct fields */
 
 /* Initializer values for message structs */
-#define google_firestore_v1_Write_init_default   {0, {google_firestore_v1_Document_init_default}, google_firestore_v1_DocumentMask_init_default, google_firestore_v1_Precondition_init_default}
+#define google_firestore_v1_Write_init_default   {0, {google_firestore_v1_Document_init_default}, false, google_firestore_v1_DocumentMask_init_default, false, google_firestore_v1_Precondition_init_default}
 #define google_firestore_v1_DocumentTransform_init_default {NULL, 0, NULL}
 #define google_firestore_v1_DocumentTransform_FieldTransform_init_default {NULL, 0, {_google_firestore_v1_DocumentTransform_FieldTransform_ServerValue_MIN}}
 #define google_firestore_v1_WriteResult_init_default {false, google_protobuf_Timestamp_init_default, 0, NULL}
 #define google_firestore_v1_DocumentChange_init_default {google_firestore_v1_Document_init_default, 0, NULL, 0, NULL}
 #define google_firestore_v1_DocumentDelete_init_default {NULL, false, google_protobuf_Timestamp_init_default, 0, NULL}
 #define google_firestore_v1_DocumentRemove_init_default {NULL, 0, NULL, google_protobuf_Timestamp_init_default}
 #define google_firestore_v1_ExistenceFilter_init_default {0, 0}
-#define google_firestore_v1_Write_init_zero      {0, {google_firestore_v1_Document_init_zero}, google_firestore_v1_DocumentMask_init_zero, google_firestore_v1_Precondition_init_zero}
+#define google_firestore_v1_Write_init_zero      {0, {google_firestore_v1_Document_init_zero}, false, google_firestore_v1_DocumentMask_init_zero, false, google_firestore_v1_Precondition_init_zero}
 #define google_firestore_v1_DocumentTransform_init_zero {NULL, 0, NULL}
 #define google_firestore_v1_DocumentTransform_FieldTransform_init_zero {NULL, 0, {_google_firestore_v1_DocumentTransform_FieldTransform_ServerValue_MIN}}
 #define google_firestore_v1_WriteResult_init_zero {false, google_protobuf_Timestamp_init_zero, 0, NULL}

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

@@ -17,7 +17,15 @@
 # workaround suggested in
 # https://github.com/nanopb/nanopb/issues/255#issuecomment-291842903
 
-# update_time should not be set for deletes.
-google.firestore.v1.WriteResult.update_time proto3:false
 # read_time might not be set for deletes.
 google.firestore.v1.DocumentDelete.read_time proto3:false
+
+# `current_document` is an optional precondition.
+google.firestore.v1.Write.current_document proto3:false
+
+# Whether `update_mask` field is set is crucial for the backend to distinguish
+# between "set" and "patch" mutations.
+google.firestore.v1.Write.update_mask proto3:false
+
+# update_time should not be set for deletes.
+google.firestore.v1.WriteResult.update_time proto3:false

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

@@ -115,7 +115,7 @@ namespace {
  * Creates the prefix for a fully qualified resource path, without a local path
  * on the end.
  */
-ResourcePath EncodeDatabaseId(const DatabaseId& database_id) {
+ResourcePath DatabaseName(const DatabaseId& database_id) {
   return ResourcePath{"projects", database_id.project_id(), "databases",
                       database_id.database_id()};
 }
@@ -126,7 +126,7 @@ ResourcePath EncodeDatabaseId(const DatabaseId& database_id) {
  */
 pb_bytes_array_t* EncodeResourceName(const DatabaseId& database_id,
                                      const ResourcePath& path) {
-  return Serializer::EncodeString(EncodeDatabaseId(database_id)
+  return Serializer::EncodeString(DatabaseName(database_id)
                                       .Append("documents")
                                       .Append(path)
                                       .CanonicalString());
@@ -197,15 +197,18 @@ Query InvalidQuery() {
 }
 
 Serializer::Serializer(DatabaseId database_id)
-    : database_id_(std::move(database_id)),
-      database_name_(EncodeDatabaseId(database_id_).CanonicalString()) {
+    : database_id_(std::move(database_id)) {
 }
 
 void Serializer::FreeNanopbMessage(const pb_field_t fields[],
                                    void* dest_struct) {
   pb_release(fields, dest_struct);
 }
 
+pb_bytes_array_t* Serializer::EncodeDatabaseName() const {
+  return EncodeString(DatabaseName(database_id_).CanonicalString());
+}
+
 google_firestore_v1_Value Serializer::EncodeFieldValue(
     const FieldValue& field_value) const {
   switch (field_value.type()) {
@@ -596,6 +599,7 @@ google_firestore_v1_Write Serializer::EncodeMutation(
   google_firestore_v1_Write result{};
 
   if (!mutation.precondition().is_none()) {
+    result.has_current_document = true;
     result.current_document = EncodePrecondition(mutation.precondition());
   }
 
@@ -611,7 +615,13 @@ google_firestore_v1_Write Serializer::EncodeMutation(
       result.which_operation = google_firestore_v1_Write_update_tag;
       auto patch_mutation = static_cast<const PatchMutation&>(mutation);
       result.update = EncodeDocument(mutation.key(), patch_mutation.value());
-      result.update_mask = EncodeFieldMask(patch_mutation.mask());
+      // Note: the fact that this field is set (even if the mask is empty) is
+      // what makes the backend treat this as a patch mutation, not a set
+      // mutation.
+      result.has_update_mask = true;
+      if (patch_mutation.mask().size() != 0) {
+        result.update_mask = EncodeFieldMask(patch_mutation.mask());
+      }
       return result;
     }
 
@@ -632,6 +642,13 @@ google_firestore_v1_Write Serializer::EncodeMutation(
             EncodeFieldTransform(field_transform);
         i++;
       }
+
+      // NOTE: We set a precondition of exists: true as a safety-check, since we
+      // always combine TransformMutations with a SetMutation or PatchMutation
+      // which (if successful) should end up with an existing document.
+      result.has_current_document = true;
+      result.current_document = EncodePrecondition(Precondition::Exists(true));
+
       return result;
     }
 
@@ -647,8 +664,10 @@ google_firestore_v1_Write Serializer::EncodeMutation(
 
 Mutation Serializer::DecodeMutation(
     nanopb::Reader* reader, const google_firestore_v1_Write& mutation) const {
-  Precondition precondition =
-      DecodePrecondition(reader, mutation.current_document);
+  auto precondition = Precondition::None();
+  if (mutation.has_current_document) {
+    precondition = DecodePrecondition(reader, mutation.current_document);
+  }
 
   switch (mutation.which_operation) {
     case google_firestore_v1_Write_update_tag: {
@@ -805,6 +824,8 @@ Serializer::EncodeFieldTransform(const FieldTransform& field_transform) const {
       return proto;
 
     case Type::Increment: {
+      proto.which_transform_type =
+          google_firestore_v1_DocumentTransform_FieldTransform_increment_tag;
       const auto& increment = static_cast<const NumericIncrementTransform&>(
           field_transform.transformation());
       proto.increment = EncodeFieldValue(increment.operand());

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

@@ -124,6 +124,12 @@ class Serializer {
    */
   static pb_bytes_array_t* EncodeBytes(const std::vector<uint8_t>& bytes);
 
+  /**
+   * Returns the database ID, such as
+   * `projects/{project_id}/databases/{database_id}`.
+   */
+  pb_bytes_array_t* EncodeDatabaseName() const;
+
   /**
    * Release memory allocated by the Encode* methods that return protos.
    *
@@ -254,6 +260,13 @@ class Serializer {
       nanopb::Reader* reader,
       const google_firestore_v1_ListenResponse& listen_response) const;
 
+  // Public for the sake of tests.
+  google_firestore_v1_StructuredQuery_Filter EncodeFilters(
+      const core::FilterList& filters) const;
+  core::FilterList DecodeFilters(
+      nanopb::Reader* reader,
+      const google_firestore_v1_StructuredQuery_Filter& proto) const;
+
  private:
   google_firestore_v1_Value EncodeNull() const;
   google_firestore_v1_Value EncodeBoolean(bool value) const;
@@ -299,12 +312,6 @@ class Serializer {
 
   std::string EncodeLabel(local::QueryPurpose purpose) const;
 
-  google_firestore_v1_StructuredQuery_Filter EncodeFilters(
-      const core::FilterList& filters) const;
-  core::FilterList DecodeFilters(
-      nanopb::Reader* reader,
-      const google_firestore_v1_StructuredQuery_Filter& proto) const;
-
   google_firestore_v1_StructuredQuery_Filter EncodeSingularFilter(
       const core::FieldFilter& filter) const;
   core::Filter DecodeFieldFilter(
@@ -360,7 +367,8 @@ class Serializer {
       const google_firestore_v1_ExistenceFilter& filter) const;
 
   model::DatabaseId database_id_;
-  const std::string database_name_;
+  // TODO(varconst): Android caches the result of calling `EncodeDatabaseName`
+  // as well, consider implementing that.
 };
 
 }  // namespace remote