WIP: failing LocalSerializerTest

Author: Brian Chen

Firestore/core/src/local/local_serializer.cc

@@ -48,6 +48,7 @@ namespace {
 using core::Target;
 using model::Document;
 using model::DocumentState;
+using model::FieldTransform;
 using model::FieldValue;
 using model::MaybeDocument;
 using model::Mutation;
@@ -328,11 +329,47 @@ MutationBatch LocalSerializer::DecodeMutationBatch(
   }
 
   std::vector<Mutation> mutations;
-  for (size_t i = 0; i < proto.writes_count; i++) {
-    mutations.push_back(
-        rpc_serializer_.DecodeMutation(reader, proto.writes[i]));
+
+  // Squash old transform mutations into existing patch of set mutations. The
+  // replacement of representing `transforms` with `update_transforms` on the
+  // SDK means that old `transform` mutations stored in LevelDB need to be
+  // updated to `update_transforms`.
+  // TODO(b/174608374): Remove this code once we perform a schema migration.
+  for (size_t i = proto.writes_count - 1; i >= 0; --i) {
+    _google_firestore_v1_Write mutation = proto.writes[i];
+    if (mutation.which_operation == google_firestore_v1_Write_transform_tag) {
+      HARD_ASSERT(
+          i >= 1 && proto.writes[i - 1].which_operation ==
+                        google_firestore_v1_Write_update_tag,
+          "TransformMutation should be preceded by a patch or set mutation");
+      _google_firestore_v1_Write mutation_to_join = proto.writes[i - 1];
+      _google_firestore_v1_Write new_mutation{mutation_to_join};
+      new_mutation.update_transforms_count =
+          mutation.transform.field_transforms_count;
+      new_mutation.update_transforms =
+          MakeArray<_google_firestore_v1_DocumentTransform_FieldTransform>(
+              mutation.transform.field_transforms_count);
+      for (size_t j = 0; j < mutation.transform.field_transforms_count; ++j) {
+        new_mutation.update_transforms[j] =
+            mutation.transform.field_transforms[j];
+      }
+      mutations.push_back(rpc_serializer_.DecodeMutation(reader, new_mutation));
+      --i;
+    } else {
+      mutations.push_back(rpc_serializer_.DecodeMutation(reader, mutation));
+    }
+
+    // TODO: figure out how to exit the loop without size_t looping around
+    if (i == 0) {
+      break;
+    }
   }
 
+  // Reverse the mutations to preserve the original ordering since the above
+  // for-loop iterates in reverse order. We use reverse() instead of prepending
+  // the elements into the mutations array since prepending to a List is O(n).
+  std::reverse(mutations.begin(), mutations.end());
+
   return MutationBatch(batch_id, local_write_time, std::move(base_mutations),
                        std::move(mutations));
 }

Firestore/core/src/remote/serializer.cc

@@ -591,11 +591,9 @@ google_firestore_v1_Write Serializer::EncodeMutation(
   result.update_transforms =
       MakeArray<google_firestore_v1_DocumentTransform_FieldTransform>(count);
   int i = 0;
-  auto crazy = mutation.field_transforms();
-  auto crazy2 = mutation.key();
   for (const FieldTransform& field_transform : mutation.field_transforms()) {
     result.update_transforms[i] = EncodeFieldTransform(field_transform);
-    i++;
+    ++i;
   }
 
   switch (mutation.type()) {

Firestore/core/test/unit/local/local_serializer_test.cc

@@ -101,6 +101,8 @@ class LocalSerializerTest : public ::testing::Test {
   remote::Serializer remote_serializer;
   local::LocalSerializer serializer;
 
+  Timestamp write_time_ = Timestamp::Now();
+
   template <typename... Args>
   void ExpectRoundTrip(const Args&... args) {
     // First, serialize model with our (nanopb based) serializer, then
@@ -114,6 +116,75 @@ class LocalSerializerTest : public ::testing::Test {
     ExpectDeserializationRoundTrip(args...);
   }
 
+  static v1::Write SetProto() {
+    v1::Value b_value{};
+    *b_value.mutable_string_value() = "b";
+    v1::Value one_value{};
+    one_value.set_integer_value(1);
+
+    v1::Write set_proto{};
+    *set_proto.mutable_update()->mutable_name() =
+        "projects/p/databases/d/documents/docs/1";
+    (*set_proto.mutable_update()->mutable_fields())["a"] = b_value;
+    (*set_proto.mutable_update()->mutable_fields())["num"] = one_value;
+    return set_proto;
+  }
+
+  static v1::Write PatchProto() {
+    v1::Value b_value{};
+    *b_value.mutable_string_value() = "b";
+    v1::Value one_value{};
+    one_value.set_integer_value(1);
+
+    v1::Write patch_proto{};
+    *patch_proto.mutable_update()->mutable_name() =
+        "projects/p/databases/d/documents/docs/1";
+    (*patch_proto.mutable_update()->mutable_fields())["a"] = b_value;
+    (*patch_proto.mutable_update()->mutable_fields())["num"] = one_value;
+    patch_proto.mutable_update_mask()->add_field_paths("a");
+    patch_proto.mutable_current_document()->set_exists(true);
+    return patch_proto;
+  }
+
+  static v1::Write DeleteProto() {
+    v1::Write delete_proto{};
+    *delete_proto.mutable_delete_() = "projects/p/databases/d/documents/docs/1";
+    return delete_proto;
+  }
+
+  static v1::Write LegacyTransformProto() {
+    v1::Write transform_proto{};
+
+    v1::DocumentTransform::FieldTransform inc_proto1;
+    v1::Value inc1_value{};
+    inc1_value.set_integer_value(42);
+    inc_proto1.set_field_path("integer");
+    *inc_proto1.mutable_increment() = inc1_value;
+
+    v1::DocumentTransform::FieldTransform inc_proto2;
+    v1::Value inc2_value{};
+    inc2_value.set_double_value(13.37);
+    inc_proto2.set_field_path("double");
+    *inc_proto2.mutable_increment() = inc2_value;
+
+    *transform_proto.mutable_transform()->add_field_transforms() =
+        std::move(inc_proto1);
+    *transform_proto.mutable_transform()->add_field_transforms() =
+        std::move(inc_proto2);
+
+    transform_proto.mutable_current_document()->set_exists(true);
+    transform_proto.mutable_transform()->set_document(
+        "projects/p/databases/d/documents/docs/1");
+    return transform_proto;
+  }
+
+  ::google::protobuf::Timestamp WriteTimeProto() {
+    ::google::protobuf::Timestamp write_time_proto{};
+    write_time_proto.set_seconds(write_time_.seconds());
+    write_time_proto.set_nanos(write_time_.nanoseconds());
+    return write_time_proto;
+  }
+
  private:
   void ExpectSerializationRoundTrip(
       const MaybeDocument& model,
@@ -138,9 +209,9 @@ class LocalSerializerTest : public ::testing::Test {
     EXPECT_EQ(model, actual_model);
   }
 
-  ByteString EncodeMaybeDocument(local::LocalSerializer* serializer,
+  ByteString EncodeMaybeDocument(local::LocalSerializer* localSerializer,
                                  const MaybeDocument& maybe_doc) {
-    return MakeByteString(serializer->EncodeMaybeDocument(maybe_doc));
+    return MakeByteString(localSerializer->EncodeMaybeDocument(maybe_doc));
   }
 
   void ExpectSerializationRoundTrip(const TargetData& target_data,
@@ -163,10 +234,10 @@ class LocalSerializerTest : public ::testing::Test {
     EXPECT_EQ(target_data, actual_target_data);
   }
 
-  ByteString EncodeTargetData(local::LocalSerializer* serializer,
+  ByteString EncodeTargetData(local::LocalSerializer* localSerializer,
                               const TargetData& target_data) {
     EXPECT_EQ(target_data.purpose(), QueryPurpose::Listen);
-    return MakeByteString(serializer->EncodeTargetData(target_data));
+    return MakeByteString(localSerializer->EncodeTargetData(target_data));
   }
 
   void ExpectSerializationRoundTrip(
@@ -191,71 +262,129 @@ class LocalSerializerTest : public ::testing::Test {
     EXPECT_EQ(model, actual_mutation_batch);
   }
 
-  ByteString EncodeMutationBatch(local::LocalSerializer* serializer,
+  ByteString EncodeMutationBatch(local::LocalSerializer* localSerializer,
                                  const MutationBatch& mutation_batch) {
-    return MakeByteString(serializer->EncodeMutationBatch(mutation_batch));
+    return MakeByteString(localSerializer->EncodeMutationBatch(mutation_batch));
   }
 
   std::string message_differences;
   MessageDifferencer msg_diff;
 };
 
+// TODO(b/174608374): Remove these tests once we perform a schema migration.
+TEST_F(LocalSerializerTest, SetMutationAndTransformMutationAreSquashed) {
+  ::firestore::client::WriteBatch batch_proto{};
+  batch_proto.set_batch_id(42);
+  *batch_proto.add_writes() = SetProto();
+  *batch_proto.add_writes() = LegacyTransformProto();
+  *batch_proto.mutable_local_write_time() = WriteTimeProto();
+
+  ByteString bytes = ProtobufSerialize(batch_proto);
+  StringReader reader(bytes);
+  auto message = Message<firestore_client_WriteBatch>::TryParse(&reader);
+  MutationBatch decoded = serializer.DecodeMutationBatch(&reader, *message);
+  ASSERT_EQ(1, decoded.mutations().size());
+  ASSERT_EQ(Mutation::Type::Set, decoded.mutations()[0].type());
+  google_firestore_v1_Write encoded =
+      remote_serializer.EncodeMutation(decoded.mutations()[0]);
+
+  google_firestore_v1_Write expected{};
+  expected.which_operation = google_firestore_v1_Write_update_tag;
+  // TODO: How should I compare these two types?
+  EXPECT_EQ(expected, encoded);
+}
+
+// TODO(b/174608374): Remove these tests once we perform a schema migration.
+TEST_F(LocalSerializerTest, TransformAndTransformThrowError) {
+  ::firestore::client::WriteBatch batch_proto{};
+  batch_proto.set_batch_id(42);
+  *batch_proto.add_writes() = LegacyTransformProto();
+  *batch_proto.add_writes() = LegacyTransformProto();
+  *batch_proto.mutable_local_write_time() = WriteTimeProto();
+
+  ByteString bytes = ProtobufSerialize(batch_proto);
+  StringReader reader(bytes);
+  auto message = Message<firestore_client_WriteBatch>::TryParse(&reader);
+  EXPECT_ANY_THROW(serializer.DecodeMutationBatch(&reader, *message));
+}
+
+// TODO(b/174608374): Remove these tests once we perform a schema migration.
+TEST_F(LocalSerializerTest, OnlyTransformThrowsError) {
+  ::firestore::client::WriteBatch batch_proto{};
+  batch_proto.set_batch_id(42);
+  *batch_proto.add_writes() = LegacyTransformProto();
+  *batch_proto.mutable_local_write_time() = WriteTimeProto();
+
+  ByteString bytes = ProtobufSerialize(batch_proto);
+  StringReader reader(bytes);
+  auto message = Message<firestore_client_WriteBatch>::TryParse(&reader);
+  EXPECT_ANY_THROW(serializer.DecodeMutationBatch(&reader, *message));
+}
+
+// TODO(b/174608374): Remove these tests once we perform a schema migration.
+TEST_F(LocalSerializerTest, DeleteAndTransformThrowError) {
+  ::firestore::client::WriteBatch batch_proto{};
+  batch_proto.set_batch_id(42);
+  *batch_proto.add_writes() = DeleteProto();
+  *batch_proto.add_writes() = LegacyTransformProto();
+  *batch_proto.mutable_local_write_time() = WriteTimeProto();
+
+  ByteString bytes = ProtobufSerialize(batch_proto);
+  StringReader reader(bytes);
+  auto message = Message<firestore_client_WriteBatch>::TryParse(&reader);
+  EXPECT_ANY_THROW(serializer.DecodeMutationBatch(&reader, *message));
+}
+
+// TODO(b/174608374): Remove these tests once we perform a schema migration.
+TEST_F(LocalSerializerTest, MultipleMutationsAreSquashed) {
+  ::firestore::client::WriteBatch batch_proto{};
+  batch_proto.set_batch_id(42);
+  *batch_proto.add_writes() = SetProto();
+  *batch_proto.add_writes() = SetProto();
+  *batch_proto.add_writes() = LegacyTransformProto();
+  *batch_proto.add_writes() = DeleteProto();
+  *batch_proto.add_writes() = PatchProto();
+  *batch_proto.add_writes() = LegacyTransformProto();
+  *batch_proto.add_writes() = PatchProto();
+  *batch_proto.mutable_local_write_time() = WriteTimeProto();
+
+  ByteString bytes = ProtobufSerialize(batch_proto);
+  StringReader reader(bytes);
+  auto message = Message<firestore_client_WriteBatch>::TryParse(&reader);
+  MutationBatch decoded = serializer.DecodeMutationBatch(&reader, *message);
+  ASSERT_EQ(5, decoded.mutations().size());
+}
+
 TEST_F(LocalSerializerTest, EncodesMutationBatch) {
   Mutation base =
-      PatchMutation(Key("bar/baz"), WrapObject("a", "b"), FieldMask{Field("a")},
+      PatchMutation(Key("docs/1"), WrapObject("a", "b"), FieldMask{Field("a")},
                     Precondition::Exists(true));
 
-  Mutation set = testutil::SetMutation("foo/bar", Map("a", "b", "num", 1));
+  Mutation set = testutil::SetMutation("docs/1", Map("a", "b", "num", 1));
   Mutation patch =
-      PatchMutation(Key("bar/baz"), WrapObject("a", "b", "num", 1),
+      PatchMutation(Key("docs/1"), WrapObject("a", "b", "num", 1),
                     FieldMask{Field("a")}, Precondition::Exists(true));
-  Mutation del = testutil::DeleteMutation("baz/quux");
+  Mutation del = testutil::DeleteMutation("docs/1");
 
-  Timestamp write_time = Timestamp::Now();
-  MutationBatch model(42, write_time, {base}, {set, patch, del});
+  MutationBatch model(42, write_time_, {base}, {set, patch, del});
 
   v1::Value b_value{};
   *b_value.mutable_string_value() = "b";
-  v1::Value one_value{};
-  one_value.set_integer_value(1);
 
   v1::Write base_proto{};
   *base_proto.mutable_update()->mutable_name() =
-      "projects/p/databases/d/documents/bar/baz";
+      "projects/p/databases/d/documents/docs/1";
   (*base_proto.mutable_update()->mutable_fields())["a"] = b_value;
   base_proto.mutable_update_mask()->add_field_paths("a");
   base_proto.mutable_current_document()->set_exists(true);
 
-  v1::Write set_proto{};
-  *set_proto.mutable_update()->mutable_name() =
-      "projects/p/databases/d/documents/foo/bar";
-  (*set_proto.mutable_update()->mutable_fields())["a"] = b_value;
-  (*set_proto.mutable_update()->mutable_fields())["num"] = one_value;
-
-  v1::Write patch_proto{};
-  *patch_proto.mutable_update()->mutable_name() =
-      "projects/p/databases/d/documents/bar/baz";
-  (*patch_proto.mutable_update()->mutable_fields())["a"] = b_value;
-  (*patch_proto.mutable_update()->mutable_fields())["num"] = one_value;
-  patch_proto.mutable_update_mask()->add_field_paths("a");
-  patch_proto.mutable_current_document()->set_exists(true);
-
-  v1::Write del_proto{};
-  *del_proto.mutable_delete_() = "projects/p/databases/d/documents/baz/quux";
-
-  ::google::protobuf::Timestamp write_time_proto{};
-  write_time_proto.set_seconds(write_time.seconds());
-  write_time_proto.set_nanos(write_time.nanoseconds());
-
   ::firestore::client::WriteBatch batch_proto{};
   batch_proto.set_batch_id(42);
   *batch_proto.add_base_writes() = base_proto;
-  *batch_proto.add_writes() = set_proto;
-  assert(batch_proto.writes(0).update().name() ==
-         "projects/p/databases/d/documents/foo/bar");
-  *batch_proto.add_writes() = patch_proto;
-  *batch_proto.add_writes() = del_proto;
-  *batch_proto.mutable_local_write_time() = write_time_proto;
+  *batch_proto.add_writes() = SetProto();
+  *batch_proto.add_writes() = PatchProto();
+  *batch_proto.add_writes() = DeleteProto();
+  *batch_proto.mutable_local_write_time() = WriteTimeProto();
 
   ExpectRoundTrip(model, batch_proto);
 }