Add FieldValue helpers #7642

schmidt-sebastian · 2021-03-02T22:44:30Z

This PR adds equal, compareTo and canonicalId helpers that work directly on Protobuf field values.

This is a port of

https://github.com/firebase/firebase-android-sdk/blob/master/firebase-firestore/src/main/java/com/google/firebase/firestore/model/Values.java
https://github.com/firebase/firebase-android-sdk/blob/master/firebase-firestore/src/test/java/com/google/firebase/firestore/model/ValuesTest.java

and of some of the code that we need from https://github.com/firebase/firebase-android-sdk/blob/master/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ServerTimestamps.java

google-oss-bot · 2021-03-02T22:45:21Z

	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-04T18:44:11Z

Coverage Report

Affected SDKs

`FirebaseFirestore-iOS-FirebaseFirestore.framework`

SDK overall coverage changed from 88.65% (5f6feba) to 88.64% (164abb8) by -0.00%.

Filename	Base (`5f6feba`)	Head (`164abb8`)	Diff
grpc_nanopb.cc	100.00%	95.45%	-4.55%
leveldb_key.cc	97.60%	98.08%	+0.48%
ordered_code.cc	93.24%	92.68%	-0.56%
server_timestamp_util.cc	?	84.85%	?
task.cc	95.21%	96.41%	+1.20%
value_util.cc	?	89.16%	?
watch_stream.cc	96.15%	92.31%	-3.85%

Test Logs

Base (5f6febab): https://github.com/firebase/firebase-ios-sdk/actions/runs/615408483
Head (164abb8d): https://github.com/firebase/firebase-ios-sdk/actions/runs/647054048

wu-hui

Mostly around how strings are handled.

Firestore/core/src/model/server_timestamps.h

Firestore/core/src/model/server_timestamps.cc

Firestore/core/src/model/values.h

Firestore/core/src/model/values.cc

wu-hui · 2021-03-08T20:26:17Z

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

+
+const uint64_t kNanBits = 0x7fff000000000000ULL;
+
+}  // namespace


This entire test should be in the unnamed namespace, Costa would give you a link explaining why, I am just forwarding his review comments from my PR..something to do with avoid potential conflict.

Looking forward to the link :) Fixed

If you're interested, here's the recommendation. The idea is to prevent potential name clashes. If you were to accidentally write, say, two different functions the same name and signature in two different test files, they will clash -- the linker would presume they are the same function. That is because by default functions have external linkage and are visible (to the linker) outside of their translation unit (every .cc file corresponds to a translation unit, and the entire program is then built from these TUs). However, functions (and variables, etc.) placed in the unnamed namespace have internal linkage and thus cannot clash (even if they have the same name, the linker will correctly interpret functions with internal linkage as two different functions).

I am always interested in learning :) I hope I will remember this time.

wu-hui · 2021-03-08T22:21:54Z

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

+  // latin small letter e + combining acute accent
+  Add(equals_group, Wrap("e\u0301b"));
+  // latin small letter e with acute accent
+  Add(equals_group, Wrap("\u00e9a"));


Other possible case: string with '\0' in the middle of it.

There should already be done in the line below (added in that review cycle). I added additional ones for comparison + canoncial ID now.

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

schmidt-sebastian

Thanks for the review! I assigning it to Costa as well for some final thoughts.

Firestore/core/src/model/server_timestamps.cc

Firestore/core/src/model/server_timestamps.h

Firestore/core/src/model/values.cc

Firestore/core/src/model/values.h

Firestore/core/src/util/comparison.h

schmidt-sebastian · 2021-03-09T00:05:52Z

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

+
+const uint64_t kNanBits = 0x7fff000000000000ULL;
+
+}  // namespace


Looking forward to the link :) Fixed

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

…dk into mrschmidt/values

var-const · 2021-03-09T21:07:28Z

Firestore/core/src/model/values.h

+const int32_t kTypeOrderArray = 9;
+const int32_t kTypeOrderMap = 10;
+
+class Values {


General observation: this class is very Java-style. Since there are no free functions in Java, creating classes that only have static methods makes sense. In C++, it's more natural to just use free functions (note, there's nothing wrong with using free functions when they are appropriate). The private static functions can just be moved into the implementation file (the .cc file), with the additional benefit of a smaller header file.

Similarly, the pattern of naming an utility class that deals with Foos Foos is specific to Java. Consider a file name like value_ordering, field_value_ordering, etc.

I made all methods free-standing and renamed the file to "value.h/cc". Should we create a separate namespace the functions?

Thanks! Looks much better now.

Re. the file name -- do you plan to add more functions (that aren't concerned with comparisons) to this file in the future?

Should we create a separate namespace the functions?

We use namespaces for system layers, so to speak (e.g. model or remote), but not for finer-grained combinations. This is fine to just be in the model namespace.

This file contains equals, compareTo and canonical ID support. That's all we have on Web and Android, but in general we treated this file as a "proto values utility" class on the other platforms.

Nit: a file named value seems to indicate that it defines a Value class. I don't have a better suggestion than the uninspired value_helpers, but personally, I feel it would convey the intention better (same for the server timestamp one). If you can think of a better name, that's great.

Renamed to value_util.h to match nanopb_util.h

Firestore/core/src/model/values.h

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

var-const · 2021-03-10T01:01:43Z

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

+
+  template <typename... Args>
+  google_firestore_v1_Value WrapObject(Args... key_value_pairs) {
+    FieldValue fv = testutil::WrapObject((key_value_pairs)...);


Nits:

the extra parentheses around key_value_pairs are unnecessary;

it's not very important because it's a test file, but ideally you would make sure to forward the arguments to WrapObject to avoid extra copies (though WrapObject's implementation would then proceed to create those copies regardless -- ideally it should be fixed, but that's out of scope for this PR):

template <typename... Args> // Make `Args` a forwarding reference google_firestore_v1_Value WrapObject(Args&&... key_value_pairs) { FieldValue fv = testutil::WrapObject(std::forward<Args>(key_value_pairs)...);

Feel free to disregard the point re. forwarding.

Fixed both. Thanks for the explanation.

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

var-const

Thanks for implementing the feedback!

Firestore/core/src/model/server_timestamps.h

Firestore/core/src/model/value.h

var-const · 2021-03-10T23:36:41Z

Firestore/core/src/model/values.h

+const int32_t kTypeOrderArray = 9;
+const int32_t kTypeOrderMap = 10;
+
+class Values {


Thanks! Looks much better now.

Re. the file name -- do you plan to add more functions (that aren't concerned with comparisons) to this file in the future?

Should we create a separate namespace the functions?

We use namespaces for system layers, so to speak (e.g. model or remote), but not for finer-grained combinations. This is fine to just be in the model namespace.

Firestore/core/src/model/values.cc

var-const · 2021-03-10T23:52:04Z

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

+
+const uint64_t kNanBits = 0x7fff000000000000ULL;
+
+}  // namespace


If you're interested, here's the recommendation. The idea is to prevent potential name clashes. If you were to accidentally write, say, two different functions the same name and signature in two different test files, they will clash -- the linker would presume they are the same function. That is because by default functions have external linkage and are visible (to the linker) outside of their translation unit (every .cc file corresponds to a translation unit, and the entire program is then built from these TUs). However, functions (and variables, etc.) placed in the unnamed namespace have internal linkage and thus cannot clash (even if they have the same name, the linker will correctly interpret functions with internal linkage as two different functions).

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

var-const · 2021-03-11T00:03:05Z

Firestore/core/src/model/values.cc

+  }
+
+  for (size_t i = 0; i < right_map.fields_count; ++i) {
+    const std::string& key = nanopb::MakeString(right_map.fields[i].key);


Are you trying to use flat_hash_map (judging by container_internal::FlatHashMapPolicy)? Can you try adding absl::flat_hash_map to the dependencies?

var-const · 2021-03-11T21:17:31Z

Firestore/core/src/model/value.h

-  kTypeOrderArray = 9,
-  kTypeOrderMap = 10
+enum class TypeOrder {
+  Null = 0,


Nit: enum members should be named like constants, i.e. prefixed with a k (e.g. kNull).

I should get my inspirations from the style guide, and not from other places in the code :)

Firestore/core/src/model/value.h

var-const · 2021-03-11T21:22:43Z

Firestore/core/src/model/values.cc

+  }
+
+  for (size_t i = 0; i < right_map.fields_count; ++i) {
+    const std::string& key = nanopb::MakeString(right_map.fields[i].key);


Note: as far as I understand, the underscored targets are supposed to be internal, and the public Abseil targets have semicolons in them (e.g. absl::flat_hash_map is public) (I'm basing this on this readme). If you're interested, you can try to add absl::flat_hash_map to the list of dependencies. If that doesn't help, it's probably not worth spending any extra time on.

var-const · 2021-03-12T00:23:02Z

Firestore/core/src/model/values.h

+const int32_t kTypeOrderArray = 9;
+const int32_t kTypeOrderMap = 10;
+
+class Values {


Nit: a file named value seems to indicate that it defines a Value class. I don't have a better suggestion than the uninspired value_helpers, but personally, I feel it would convey the intention better (same for the server timestamp one). If you can think of a better name, that's great.

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

google-oss-bot added the api: firestore label Mar 3, 2021

schmidt-sebastian force-pushed the mrschmidt/values branch from e1e8d0f to 1b6d50e Compare March 4, 2021 04:26

Add FieldValue helpers

3098e4a

schmidt-sebastian force-pushed the mrschmidt/values branch from 136e2c8 to 3098e4a Compare March 4, 2021 17:39

schmidt-sebastian changed the base branch from master to mrschmidt/mutabledocuments March 4, 2021 17:40

schmidt-sebastian changed the title ~~WIP~~ Add FieldValue helpers Mar 4, 2021

schmidt-sebastian marked this pull request as ready for review March 4, 2021 17:40

schmidt-sebastian requested a review from wu-hui March 4, 2021 17:46

schmidt-sebastian assigned wu-hui Mar 4, 2021

schmidt-sebastian added 2 commits March 4, 2021 10:51

Fix warnings

3a125e5

Update serializer.h

9358c64

Includes

30c78ad

wu-hui requested changes Mar 8, 2021

View reviewed changes

wu-hui assigned schmidt-sebastian and unassigned wu-hui Mar 8, 2021

Feedback

e57316b

schmidt-sebastian commented Mar 9, 2021

View reviewed changes

schmidt-sebastian assigned wu-hui and var-const and unassigned schmidt-sebastian Mar 9, 2021

schmidt-sebastian added 7 commits March 8, 2021 18:13

Error messages

6a1fff0

Cast

fbd2195

Format

5ad67d7

Use StringView

41a7114

Update server_timestamps.h

5300722

Comment

493f2f2

Merge branch 'mrschmidt/values' of github.com:firebase/firebase-ios-s…

b2e7e03

…dk into mrschmidt/values

schmidt-sebastian assigned var-const Mar 9, 2021

schmidt-sebastian added 2 commits March 9, 2021 11:14

Test changes

a3a3f40

Add length to std::string

32804b8

wu-hui approved these changes Mar 9, 2021

View reviewed changes

wu-hui requested a review from var-const March 9, 2021 21:15

wu-hui removed their assignment Mar 9, 2021

var-const suggested changes Mar 10, 2021

View reviewed changes

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

Review

d497b66

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

var-const suggested changes Mar 11, 2021

View reviewed changes

Review

7817339

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

Lint

9d965b7

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

var-const approved these changes Mar 12, 2021

View reviewed changes

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

schmidt-sebastian added 5 commits March 11, 2021 19:21

Renames

6612267

Flat hash map

6784f9b

Lint

2b286a6

Cmake

9fc219b

Update Podspec

164abb8

schmidt-sebastian merged commit 4f537e7 into mrschmidt/mutabledocuments Mar 12, 2021

schmidt-sebastian deleted the mrschmidt/values branch March 12, 2021 19:42

firebase locked and limited conversation to collaborators Apr 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FieldValue helpers #7642

Add FieldValue helpers #7642

schmidt-sebastian commented Mar 2, 2021 •

edited

Loading

google-oss-bot commented Mar 2, 2021

google-oss-bot commented Mar 4, 2021 •

edited

Loading

`FirebaseFirestore-iOS-FirebaseFirestore.framework`

wu-hui left a comment

wu-hui Mar 8, 2021

schmidt-sebastian Mar 9, 2021

var-const Mar 10, 2021

schmidt-sebastian Mar 11, 2021

wu-hui Mar 8, 2021

wu-hui Mar 9, 2021

schmidt-sebastian Mar 9, 2021

schmidt-sebastian left a comment

schmidt-sebastian Mar 9, 2021

var-const Mar 9, 2021

schmidt-sebastian Mar 10, 2021

var-const Mar 10, 2021 •

edited

Loading

schmidt-sebastian Mar 11, 2021

var-const Mar 12, 2021

schmidt-sebastian Mar 12, 2021

var-const Mar 10, 2021

schmidt-sebastian Mar 10, 2021

var-const left a comment

var-const Mar 10, 2021 •

edited

Loading

var-const Mar 10, 2021

var-const Mar 11, 2021

var-const Mar 11, 2021

schmidt-sebastian Mar 12, 2021

var-const Mar 11, 2021

var-const Mar 12, 2021


		const uint64_t kNanBits = 0x7fff000000000000ULL;

		} // namespace

Add FieldValue helpers #7642

Add FieldValue helpers #7642

Conversation

schmidt-sebastian commented Mar 2, 2021 • edited Loading

google-oss-bot commented Mar 2, 2021

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

Coverage Report

Affected SDKs

FirebaseFirestore-iOS-FirebaseFirestore.framework

Test Logs

wu-hui 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

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 Mar 10, 2021 • edited Loading

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

var-const Mar 10, 2021 • edited Loading

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

Choose a reason for hiding this comment

schmidt-sebastian commented Mar 2, 2021 •

edited

Loading

google-oss-bot commented Mar 4, 2021 •

edited

Loading

`FirebaseFirestore-iOS-FirebaseFirestore.framework`

var-const Mar 10, 2021 •

edited

Loading

var-const Mar 10, 2021 •

edited

Loading