Add ObjectValue #7712

schmidt-sebastian · 2021-03-13T15:59:38Z

This PR adds a mutable, Protobuf-backed ObjectValue similar to https://github.com/firebase/firebase-android-sdk/blob/master/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java

Since nanopb Protos can be directly manipulated, I did not port the Overlay approach from Android. Rather, the protos are modified directly. To ensure O(log n) Map lookups, Maps are kept sorted as well (all maps start out sorted when we receive them from the backend, but on Web and Android they are not necessarily sorted after we apply local modifications). The guaranteed sorted order also allows me to simplify some of the code in value_utils as well.

google-oss-bot · 2021-03-13T16:00:47Z

	1 Warning
⚠️	Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

google-oss-bot · 2021-03-13T16:33:41Z

Coverage Report

Affected SDKs

`FirebaseFirestore-iOS-FirebaseFirestore.framework`

SDK overall coverage changed from 88.65% (5f6feba) to 88.44% (618fe52) by -0.21%.

Filename	Base (`5f6feba`)	Head (`618fe52`)	Diff
leveldb_key.cc	97.60%	98.08%	+0.48%
mutable_document.cc	?	0.00%	?
object_value.cc	?	98.02%	?
ordered_code.cc	93.24%	92.68%	-0.56%
server_timestamp_util.cc	?	84.85%	?
task.cc	95.21%	97.60%	+2.40%
value_util.cc	?	88.50%	?

Test Logs

Base (5f6febab): https://github.com/firebase/firebase-ios-sdk/actions/runs/615408483
Head (618fe520): https://github.com/firebase/firebase-ios-sdk/actions/runs/665784704

schmidt-sebastian · 2021-03-13T21:31:25Z

Firestore/core/test/unit/model/object_value_test.cc

+using testutil::Map;
+using testutil::Value;
+
+class ObjectValueTest : public ::testing::Test {


Note - can be compared to end result of firebase/firebase-android-sdk#2494

schmidt-sebastian · 2021-03-14T19:58:13Z

I think this leaks memory - we need to free the underlying Protobuf structures when MutableObjectValue is destructed. I will work on a fix.

schmidt-sebastian · 2021-03-15T14:56:02Z

I think this leaks memory - we need to free the underlying Protobuf structures when MutableObjectValue is destructed. I will work on a fix.

Done

var-const

Did a partial review.

FirebaseFirestore.podspec

Firestore/core/CMakeLists.txt

Firestore/core/src/nanopb/message.h

Firestore/core/src/model/object_value.h

Firestore/core/src/model/object_value.cc

FirebaseFirestore.podspec

Firestore/core/src/model/object_value.cc

Firestore/core/test/unit/model/value_util_test.cc

var-const · 2021-03-17T00:13:54Z

Firestore/core/test/unit/model/value_util_test.cc

+    // it to verify that all values own their own memory.
+    nanopb::Message<google_firestore_v1_Value> clone1{DeepClone(value)};
+    nanopb::Message<google_firestore_v1_Value> clone2{DeepClone(*clone1)};
+    EXPECT_TRUE(value == *clone1);


Question: when these tests fail, what do the failure messages look like? Is it clear which invocation of VerifyDeepClone failed?

No, it requires debugging. I don't know how to solve this though (other than by passing in the index of the callsite). I wish there was a proper stracktrace.

I added a better error message though, which should help a bit.

Can you check again after replacing EXPECT_TRUE with EXPECT_EQ? One possible approach is to add SCOPED_TRACE before every invocation of this helper, but please check with EXPECT_EQ first to see if it's necessary. Another approach, should EXPECT_EQ not work out, is to log the stringified form of both values being compared upon failure.

I added a log for both message (similar to what I do above). I am not sure why EXPECT_EQ does not compile.

schmidt-sebastian

Next round. Thanks for the feedback.

FirebaseFirestore.podspec

Firestore/core/src/model/object_value.cc

Firestore/core/src/model/object_value.h

Firestore/core/test/unit/model/value_util_test.cc

schmidt-sebastian · 2021-03-17T01:36:46Z

Firestore/core/test/unit/model/value_util_test.cc

+    // it to verify that all values own their own memory.
+    nanopb::Message<google_firestore_v1_Value> clone1{DeepClone(value)};
+    nanopb::Message<google_firestore_v1_Value> clone2{DeepClone(*clone1)};
+    EXPECT_TRUE(value == *clone1);


No, it requires debugging. I don't know how to solve this though (other than by passing in the index of the callsite). I wish there was a proper stracktrace.

I added a better error message though, which should help a bit.

schmidt-sebastian · 2021-03-17T15:30:20Z

Firestore/core/src/model/object_value.cc

+  for (pb_size_t source_index = 0, target_index = 0;
+       target_index < target_count;) {
+    if (source_index < source_count) {
+      std::string key = MakeString(source_fields[source_index].key);


Note: Using a string_view here causes an ASAN failure as the key may get freed using FreeNanopbMessage in the loop.

Hmm, but key doesn't seem to be used after FreeNanopbMessage? AFAICS, the memory can only be freed after a comparison and it causes the loop to continue.

var-const

Close to LGTM. Please see a couple of comments I bumped from the last time.

var-const · 2021-03-17T19:11:20Z

Firestore/core/test/unit/model/value_util_test.cc

+
+    [&] {
+      nanopb::Message<google_firestore_v1_Value> clone2{DeepClone(value)};
+      EXPECT_TRUE(value == *clone2)


You can use EXPECT_EQ. Also, a failure message should contain the line where it happened, so the before/after message is probably not necessary.

I wasn't able to get this to work:

/Users/mrschmidt/GitHub/firebase/firebase-ios-sdk/cmake-build-debug/external/src/googletest/googletest/include/gtest/gtest.h:1527:11: error: invalid operands to binary expression ('const firebase::firestore::_google_firestore_v1_Value' and 'const firebase::firestore::_google_firestore_v1_Value') if (lhs == rhs) { ~~~ ^ ~~~ /Users/mrschmidt/GitHub/firebase/firebase-ios-sdk/cmake-build-debug/external/src/googletest/googletest/include/gtest/gtest.h:1554:12: note: in instantiation of function template specialization 'testing::internal::CmpHelperEQ<firebase::firestore::_google_firestore_v1_Value, firebase::firestore::_google_firestore_v1_Value>' requested here return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs); ^ /Users/mrschmidt/GitHub/firebase/firebase-ios-sdk/Firestore/core/test/unit/model/value_util_test.cc:141:7: note: in instantiation of function template specialization 'testing::internal::EqHelper::Compare<firebase::firestore::_google_firestore_v1_Value, firebase::firestore::_google_firestore_v1_Value, nullptr>' requested here EXPECT_EQ(value, *clone2) ^ /Users/mrschmidt/GitHub/firebase/firebase-ios-sdk/cmake-build-debug/external/src/googletest/googletest/include/gtest/gtest.h:2028:54: note: expanded from macro 'EXPECT_EQ' EXPECT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2) ^ /Users/mrschmidt/GitHub/firebase/firebase-ios-sdk/Firestore/core/include/firebase/firestore/geo_point.h:113:13: note: candidate function not viable: no known conversion from 'const firebase::firestore::_google_firestore_v1_Value' to 'const firebase::firestore::GeoPoint' for 1st argument inline bool operator==(const GeoPoint& lhs, const GeoPoint& rhs) { ^ /Users/mrschmidt/GitHub/firebase/firebase-ios-sdk/cmake-build-debug/external/src/googletest/googletest/include/gtest/gtest.h:1518:13: note: candidate function not viable: no known conversion from 'const firebase::firestore::_google_firestore_v1_Value' to 'testing::internal::faketype' for 1st argument inline bool operator==(faketype, faketype) { return true; }

I tried to use EXPECT_EQ in my earlier attempts at writing these tests and for some reason the == operator is not recognized.

Ok, what happens is that due to the way EXPECT_EQ is implemented, the operator== has to be in the same namespace as at least one of the classes being compared. In this case, google_firestore_v1_Value is in the firebase::firestore namespace, but the comparison operator is in firebase::firestore::model. It's probably a good idea to move operator== one namespace level up.

Firestore/core/src/model/object_value.cc

FirebaseFirestore.podspec

Firestore/core/src/model/object_value.cc

var-const · 2021-03-17T19:27:15Z

Firestore/core/test/unit/model/value_util_test.cc

+    // it to verify that all values own their own memory.
+    nanopb::Message<google_firestore_v1_Value> clone1{DeepClone(value)};
+    nanopb::Message<google_firestore_v1_Value> clone2{DeepClone(*clone1)};
+    EXPECT_TRUE(value == *clone1);


Can you check again after replacing EXPECT_TRUE with EXPECT_EQ? One possible approach is to add SCOPED_TRACE before every invocation of this helper, but please check with EXPECT_EQ first to see if it's necessary. Another approach, should EXPECT_EQ not work out, is to log the stringified form of both values being compared upon failure.

Firestore/core/src/model/object_value.cc

var-const · 2021-03-17T19:31:42Z

Firestore/core/src/model/object_value.cc

+  for (pb_size_t source_index = 0, target_index = 0;
+       target_index < target_count;) {
+    if (source_index < source_count) {
+      std::string key = MakeString(source_fields[source_index].key);


Hmm, but key doesn't seem to be used after FreeNanopbMessage? AFAICS, the memory can only be freed after a comparison and it causes the loop to continue.

var-const

Thanks for addressing all the feedback! LGTM (with one small comment about includes).

var-const · 2021-03-18T19:51:59Z

Firestore/core/src/nanopb/fields_array.h

@@ -23,6 +23,7 @@
 #include "Firestore/Protos/nanopb/google/firestore/v1/document.nanopb.h"
 #include "Firestore/Protos/nanopb/google/firestore/v1/firestore.nanopb.h"
 #include "Firestore/Protos/nanopb/google/type/latlng.nanopb.h"
+#include "Firestore/core/src/nanopb/message.h"


This include should no longer be necessary.

Add ObjectValue

fea617f

google-cla bot added the cla: yes label Mar 13, 2021

schmidt-sebastian requested a review from var-const March 13, 2021 15:59

schmidt-sebastian assigned var-const Mar 13, 2021

schmidt-sebastian added the api: firestore label Mar 13, 2021

Update docs

c73263f

schmidt-sebastian commented Mar 13, 2021

View reviewed changes

schmidt-sebastian added 3 commits March 14, 2021 16:56

Use Message to back Value

ae4785e

DeepClone test

b20b399

Empty commit

d03b840

var-const suggested changes Mar 16, 2021

View reviewed changes

var-const assigned schmidt-sebastian and unassigned var-const Mar 16, 2021

Review

680cf4a

schmidt-sebastian assigned var-const and unassigned schmidt-sebastian Mar 16, 2021

var-const suggested changes Mar 17, 2021

View reviewed changes

Feedback

a8db28d

schmidt-sebastian commented Mar 17, 2021

View reviewed changes

schmidt-sebastian added 4 commits March 16, 2021 19:46

Use sorted map

65f16f1

Attempt to re-gen key

bbd50fa

Use std::string

bf058d6

Undo CMakeList change

f51d4b0

schmidt-sebastian commented Mar 17, 2021

View reviewed changes

var-const suggested changes Mar 17, 2021

View reviewed changes

var-const removed their assignment Mar 17, 2021

var-const assigned schmidt-sebastian Mar 17, 2021

schmidt-sebastian added 6 commits March 17, 2021 16:45

Review #2

ee377d6

Fix build

e311459

Fix EXPECT_EQ and FreeNanoPb message

900c491

Format

adcfce1

Includes

eeec993

Format

13e8011

schmidt-sebastian assigned var-const and unassigned schmidt-sebastian Mar 18, 2021

var-const approved these changes Mar 18, 2021

View reviewed changes

var-const assigned schmidt-sebastian and unassigned var-const Mar 18, 2021

Update fields_array.h

618fe52

schmidt-sebastian merged commit a12b78e into mrschmidt/mutabledocuments Mar 18, 2021

schmidt-sebastian deleted the mrschmidt/sortedvalue branch March 18, 2021 21:43

firebase locked and limited conversation to collaborators Apr 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ObjectValue #7712

Add ObjectValue #7712

schmidt-sebastian commented Mar 13, 2021

google-oss-bot commented Mar 13, 2021

google-oss-bot commented Mar 13, 2021 •

edited

Loading

`FirebaseFirestore-iOS-FirebaseFirestore.framework`

schmidt-sebastian Mar 13, 2021

schmidt-sebastian commented Mar 14, 2021

schmidt-sebastian commented Mar 15, 2021

var-const left a comment

var-const Mar 17, 2021

schmidt-sebastian Mar 17, 2021

var-const Mar 17, 2021

schmidt-sebastian Mar 17, 2021

schmidt-sebastian left a comment

schmidt-sebastian Mar 17, 2021

schmidt-sebastian Mar 17, 2021

var-const Mar 17, 2021

var-const left a comment

var-const Mar 17, 2021

schmidt-sebastian Mar 17, 2021

var-const Mar 17, 2021

var-const Mar 17, 2021

var-const Mar 17, 2021

var-const left a comment

var-const Mar 18, 2021

Add ObjectValue #7712

Add ObjectValue #7712

Conversation

schmidt-sebastian commented Mar 13, 2021

google-oss-bot commented Mar 13, 2021

google-oss-bot commented Mar 13, 2021 • edited Loading

Coverage Report

Affected SDKs

FirebaseFirestore-iOS-FirebaseFirestore.framework

Test Logs

Choose a reason for hiding this comment

schmidt-sebastian commented Mar 14, 2021

schmidt-sebastian commented Mar 15, 2021

var-const left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

var-const left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

var-const left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-bot commented Mar 13, 2021 •

edited

Loading

`FirebaseFirestore-iOS-FirebaseFirestore.framework`