Replace usage of transforms with update transforms #7276

thebrianchen · 2021-01-08T16:20:06Z

Porting from web and Android.

First commit contains initial port changes. Second commit changes contains code removal for localTransformResults, which was done in Android and backported to web.

Port from iOS firebase/firebase-ios-sdk#7276

schmidt-sebastian

I am surprised by how pretty this looks. Thanks for doing this!

schmidt-sebastian · 2021-01-19T17:07:13Z

Firestore/core/src/local/local_serializer.cc

+  for (size_t i = proto.writes_count; i > 0; --i) {
+    // Calculate appropriate index from `i`. We do this because decrementing
+    // `i` when i == 0 results in wraparound.
+    size_t index = i - 1;


Can you use a more specific name? "i" and "index" both mean the same thing. Also maybe "underflow" instead of "wraparound?"

N/A now since we're iterating in the normal direction.

schmidt-sebastian · 2021-01-19T17:10:33Z

Firestore/core/src/local/local_serializer.cc

+  // 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());


We could probably get rid of the underflow issue and the reversing if we flipped the order of iterations and looked ahead in line 344 (index +1 < proto.writes_count && which_operation ...).

Big brain suggestion!! Done, and removed the test with only a transformMutation as well.

schmidt-sebastian · 2021-01-19T17:11:49Z

Firestore/core/src/local/local_serializer.cc

+      for (size_t j = 0; j < mutation.transform.field_transforms_count; ++j) {
+        new_mutation.update_transforms[j] =
+            mutation.transform.field_transforms[j];
+      }


I don't know the answer here - but could we just assign the old array to the new location?

schmidt-sebastian · 2021-01-19T17:13:52Z

Firestore/core/src/model/mutation.cc

+  for (const FieldTransform& transform : field_transforms_) {
+    absl::optional<FieldValue> existing_value;
+    if (maybe_doc && maybe_doc->is_document()) {
+      existing_value = Document(*maybe_doc).field(transform.path());


This creates a new Document for every iteration. We should probably do this outside of th eloop.

schmidt-sebastian · 2021-01-19T17:33:04Z

Firestore/core/src/model/mutation.h

+     * @return The transform results array.
+     */
+    virtual std::vector<FieldValue> ServerTransformResults(
+        const absl::optional<MaybeDocument>& base_doc,


Now that we removed base_doc from LocalTransformResults, should this be maybe_doc?

Good catch, done.

schmidt-sebastian · 2021-01-19T17:40:16Z

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

+    EXPECT_EQ(google_firestore_v1_Write_update_tag, encoded.which_operation);
+    EXPECT_EQ(2, encoded.update.fields_count);
+    EXPECT_EQ("a", nanopb::MakeString(encoded.update.fields[0].key));
+    EXPECT_EQ("b",
+              nanopb::MakeString(encoded.update.fields[0].value.string_value));
+    EXPECT_EQ("num", nanopb::MakeString(encoded.update.fields[1].key));
+    EXPECT_EQ(1, encoded.update.fields[1].value.integer_value);
+    EXPECT_FALSE(encoded.has_update_mask);
+    EXPECT_FALSE(encoded.has_current_document);


thanks for your help in helping me debug this earlier!

schmidt-sebastian · 2021-01-19T17:43:58Z

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

+  //  WriteMutations({
+  //      testutil::PatchMutation("foo/bar", Map(), {}),
+  //      testutil::PatchMutation("foo/bar", Map(),
+  //                              {testutil::Increment("sum", Value(1))}),
+  //  });


Oh no! Such a great PR, ruined by this careless mistake. I guess I gotta press 'Request Changes' now :)

aw shucks I was so close

schmidt-sebastian · 2021-01-19T17:45:52Z

Firestore/core/test/unit/remote/serializer_test.cc


-  ExpectRoundTrip(model, proto);
+  PatchMutation patch_model =


This test could be split in two. Both verification chains are mostly independent and smaller tests are usually better. You might also be able to create some helper methods for some of the state manipulation that is duplicated in this test and the test below.

I kept them together in order to keep parity with Web/Android. There aren't that many helpers I could pull, and these two tests don't share much else in common with the other tests in this file, which would end up making the tests harder to read.

schmidt-sebastian · 2021-01-19T17:46:13Z

Firestore/core/test/unit/remote/serializer_test.cc


-  ExpectRoundTrip(model, proto);
+  PatchMutation patch_model = testutil::PatchMutation(


Same comment as above.

Firestore/core/test/unit/testutil/testutil.cc

Port from iOS firebase/firebase-ios-sdk#7276

thebrianchen

thanks!

thebrianchen · 2021-01-20T00:33:14Z

Firestore/core/src/model/mutation.h

+     * @return The transform results array.
+     */
+    virtual std::vector<FieldValue> ServerTransformResults(
+        const absl::optional<MaybeDocument>& base_doc,


Good catch, done.

thebrianchen · 2021-01-20T00:34:32Z

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

+  //  WriteMutations({
+  //      testutil::PatchMutation("foo/bar", Map(), {}),
+  //      testutil::PatchMutation("foo/bar", Map(),
+  //                              {testutil::Increment("sum", Value(1))}),
+  //  });


aw shucks I was so close

thebrianchen · 2021-01-20T00:38:13Z

Firestore/core/src/model/mutation.cc

+  for (const FieldTransform& transform : field_transforms_) {
+    absl::optional<FieldValue> existing_value;
+    if (maybe_doc && maybe_doc->is_document()) {
+      existing_value = Document(*maybe_doc).field(transform.path());


thebrianchen · 2021-01-20T00:38:34Z

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

+    EXPECT_EQ(google_firestore_v1_Write_update_tag, encoded.which_operation);
+    EXPECT_EQ(2, encoded.update.fields_count);
+    EXPECT_EQ("a", nanopb::MakeString(encoded.update.fields[0].key));
+    EXPECT_EQ("b",
+              nanopb::MakeString(encoded.update.fields[0].value.string_value));
+    EXPECT_EQ("num", nanopb::MakeString(encoded.update.fields[1].key));
+    EXPECT_EQ(1, encoded.update.fields[1].value.integer_value);
+    EXPECT_FALSE(encoded.has_update_mask);
+    EXPECT_FALSE(encoded.has_current_document);


thanks for your help in helping me debug this earlier!

thebrianchen · 2021-01-20T00:52:24Z

Firestore/core/src/local/local_serializer.cc

+  // 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());


Big brain suggestion!! Done, and removed the test with only a transformMutation as well.

thebrianchen · 2021-01-20T00:53:13Z

Firestore/core/src/local/local_serializer.cc

+      for (size_t j = 0; j < mutation.transform.field_transforms_count; ++j) {
+        new_mutation.update_transforms[j] =
+            mutation.transform.field_transforms[j];
+      }


thebrianchen · 2021-01-20T00:53:42Z

Firestore/core/src/local/local_serializer.cc

+  for (size_t i = proto.writes_count; i > 0; --i) {
+    // Calculate appropriate index from `i`. We do this because decrementing
+    // `i` when i == 0 results in wraparound.
+    size_t index = i - 1;


N/A now since we're iterating in the normal direction.

thebrianchen · 2021-01-20T16:04:34Z

Firestore/core/test/unit/remote/serializer_test.cc


-  ExpectRoundTrip(model, proto);
+  PatchMutation patch_model =


I kept them together in order to keep parity with Web/Android. There aren't that many helpers I could pull, and these two tests don't share much else in common with the other tests in this file, which would end up making the tests harder to read.

schmidt-sebastian · 2021-01-21T03:54:38Z

Firestore/core/src/local/local_serializer.cc

+      new_mutation.update_transforms =
+          MakeArray<_google_firestore_v1_DocumentTransform_FieldTransform>(
+              transform_mutation.transform.field_transforms_count);


Do we still need this call?

Port from iOS firebase/firebase-ios-sdk#7276

google-cla bot added the cla: yes label Jan 8, 2021

google-oss-bot added the api: firestore label Jan 8, 2021

thebrianchen force-pushed the bc/update-transforms branch 3 times, most recently from 11e07ae to 05f33ce Compare January 13, 2021 23:58

Initial port of Update Transforms

a0e7d50

thebrianchen force-pushed the bc/update-transforms branch from 05f33ce to a0e7d50 Compare January 14, 2021 00:18

Merge branch 'master' into bc/update-transforms

b4f6400

thebrianchen force-pushed the bc/update-transforms branch from 6b64d5e to 78a79e5 Compare January 14, 2021 00:33

thebrianchen self-assigned this Jan 14, 2021

thebrianchen marked this pull request as ready for review January 14, 2021 00:36

thebrianchen force-pushed the bc/update-transforms branch from 78a79e5 to edc3f47 Compare January 14, 2021 03:42

Remove baseDoc check in localTransformResults()

7257be4

thebrianchen force-pushed the bc/update-transforms branch from edc3f47 to 7257be4 Compare January 14, 2021 03:48

Fix increment logic to pass cmake

41f2137

thebrianchen removed their assignment Jan 14, 2021

thebrianchen requested a review from schmidt-sebastian January 14, 2021 17:32

thebrianchen assigned schmidt-sebastian Jan 14, 2021

thebrianchen changed the title ~~WIP: failing merge tests~~ Replace usage of transforms with update transforms Jan 14, 2021

schmidt-sebastian added a commit to firebase/firebase-android-sdk that referenced this pull request Jan 19, 2021

Remove unused baseDoc argument

ec42a19

Port from iOS firebase/firebase-ios-sdk#7276

schmidt-sebastian mentioned this pull request Jan 19, 2021

Remove unused baseDoc argument firebase/firebase-android-sdk#2340

Merged

schmidt-sebastian suggested changes Jan 19, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Jan 19, 2021

schmidt-sebastian added a commit to firebase/firebase-android-sdk that referenced this pull request Jan 20, 2021

Remove unused baseDoc argument (#2340)

4370f77

Port from iOS firebase/firebase-ios-sdk#7276

Brian Chen added 2 commits January 20, 2021 10:24

resolve comments

fefe12f

rename maybe_doc

0dab4d4

thebrianchen commented Jan 20, 2021

View reviewed changes

thebrianchen requested a review from schmidt-sebastian January 20, 2021 17:31

thebrianchen removed their assignment Jan 20, 2021

thebrianchen assigned schmidt-sebastian Jan 20, 2021

schmidt-sebastian approved these changes Jan 21, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Jan 21, 2021

remove MakeArray line

97eefd8

thebrianchen merged commit 0088d09 into master Jan 21, 2021

thebrianchen deleted the bc/update-transforms branch January 21, 2021 18:03

andreaowu pushed a commit to firebase/firebase-android-sdk that referenced this pull request Jan 29, 2021

Remove unused baseDoc argument (#2340)

564908a

Port from iOS firebase/firebase-ios-sdk#7276

firebase locked and limited conversation to collaborators Feb 21, 2021


		ExpectRoundTrip(model, proto);
		PatchMutation patch_model = testutil::PatchMutation(

Replace usage of transforms with update transforms #7276

Replace usage of transforms with update transforms #7276

Uh oh!

Conversation

thebrianchen commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thebrianchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thebrianchen commented Jan 8, 2021 •

edited

Loading