Protobuf-backed FieldValues #1156

schmidt-sebastian · 2020-01-23T01:06:08Z

This adds a new top-level 'firestore.model.protovalue' package that adds a new ObjectValue and PrimitiveValue. Both are backed by Protobuf Value types. The reason I am adding a new package is to avoid breaking existing code. Before merging into master, I will move the files into the existing package.

The new ObjectValue should be mostly API compatible with the existing ObjectValue. I went through great length to figure out how to efficiently manipulate nested Protobuf Maps. I hope I succeeded.

codecov · 2020-01-23T01:14:25Z

Codecov Report

Merging #1156 into mrschmidt/rewritefieldvalue will increase coverage by 21.05%.
The diff coverage is 97.26%.

Flag	Coverage Δ	Complexity Δ
#FirebaseFirestore	`62.23% <97.26%> (?)`	`2349 <17> (?)`
#FirebaseFirestore_Ktx	`41.17% <ø> (ø)`	`0 <ø> (ø)`	⬇️

Impacted Files	Coverage Δ	Complexity Δ
...ase/firestore/model/protovalue/PrimitiveValue.java	`93.61% <ø> (ø)`	`28 <0> (?)`
...le/firebase/firestore/model/value/ProtoValues.java	`90.51% <100%> (ø)`	`62 <10> (?)`
...rebase/firestore/model/protovalue/ObjectValue.java	`95.55% <96.36%> (ø)`	`16 <7> (?)`
...oogle/firebase/firestore/core/FirestoreClient.java	`0% <0%> (ø)`	`0% <0%> (?)`
...loud/datastore/core/number/IndexNumberDecoder.java	`0% <0%> (ø)`	`0% <0%> (?)`
...firebase/firestore/remote/ConnectivityMonitor.java	`0% <0%> (ø)`	`0% <0%> (?)`
...le/firebase/firestore/model/value/StringValue.java	`100% <0%> (ø)`	`10% <0%> (?)`
...firebase/firestore/model/value/ReferenceValue.java	`71.42% <0%> (ø)`	`9% <0%> (?)`
...va/com/google/firebase/firestore/core/OrderBy.java	`86.2% <0%> (ø)`	`10% <0%> (?)`
...se/firestore/local/MemoryLruReferenceDelegate.java	`89.33% <0%> (ø)`	`29% <0%> (?)`
... and 167 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 618f903...035970b. Read the comment docs.

This will be used (and tested) in the follow-up CL that adds the FieldValue tpes.

This exposes a similar interface as wrap() and allows me to change the existing FieldValue test to use Value protos

wilhuff · 2020-01-27T17:47:16Z

...e-firestore/src/main/java/com/google/firebase/firestore/model/protovalue/PrimitiveValue.java

+import java.util.List;
+import java.util.Map;
+
+public class PrimitiveValue extends FieldValue {


Added. FWIW, I am debating merging this with FieldValue before the final merge.

Merging SGTM. There seems to be little reason to keep this separate.

...base-firestore/src/main/java/com/google/firebase/firestore/model/protovalue/ObjectValue.java

wilhuff · 2020-01-28T17:06:55Z

...base-firestore/src/main/java/com/google/firebase/firestore/model/protovalue/ObjectValue.java

+    /** Create the first field path that is not part of the subtree created by `currentPath`. */
+    private FieldPath createSuccessor(FieldPath currentPath) {
+      hardAssert(!currentPath.isEmpty(), "Can't create a successor for an empty path");
+      return currentPath.popLast().append(currentPath.getLastSegment() + '0');


Shouldn't this be a null byte ('\0')?

This is also no longer needed (yay for simpler code).

I chose '0' because it is the first ASCII code that can be part of a valid field path. '\0' may trigger validation errors in our field value handling.

wilhuff · 2020-01-28T17:12:54Z

...base-firestore/src/main/java/com/google/firebase/firestore/model/protovalue/ObjectValue.java

+     * @return Whether any modifications were applied (in any part of the subtree under
+     *     currentPath).
+     */
+    private boolean applyOverlay(FieldPath currentPath, MapValue.Builder resultAtPath) {


This method is quite complex and pretty hard to follow. You could extract a few methods here that would make this clearer. For example, you could extract a applyOverlayLeaf(String, @Nullable Value) which would pull lines 219 through 228 out.

You might also consider pulling some part of 230 through 240 out into a existingMapValueBuilder(FieldPath) or similar.

I believe this was close to impossible to follow, but it was the cleanest design I could come up with given the data structure I chose. I have now chosen a different data structure for the overlap map, which allowed me to drastically simplify the code.

wilhuff · 2020-01-28T17:15:09Z

...base-firestore/src/main/java/com/google/firebase/firestore/model/protovalue/ObjectValue.java

+        // Shrink the subtree to contain only values after the current field path. Note that we are
+        // still bound by the subtree created at the initial method invocation. The current loop
+        // exits when the subtree becomes empty.
+        currentSlice = currentSlice.tailMap(createSuccessor(fieldPath));


Why not just remove fieldPath?

This is no longer part of the PR, but a remove here would make this method non-idempotent and might incur additional tree re-balancing.

wilhuff · 2020-01-28T17:56:15Z

...base-firestore/src/test/java/com/google/firebase/firestore/model/ObjectValueBuilderTest.java

+  public void emptyBuilder() {
+    ObjectValue.Builder builder = ObjectValue.emptyObject().toBuilder();
+    ObjectValue object = builder.build();
+    assertEquals(wrapObject(), object);


Is this test verifying that wrapObject with no parameters results in an empty object?

It seems like this should really be assertEquals(ObjectValue.emptyObject(), object).

Good catch. It is meant to verify that a build on an empty builder works.

wilhuff · 2020-01-28T17:57:53Z

...base-firestore/src/test/java/com/google/firebase/firestore/model/ObjectValueBuilderTest.java

+  }
+
+  @Test
+  public void setSingleField() {


Consider naming these methods as phrases with an implicit leading "it" (in this case setsASingleField). This makes the names directly portable to our typescript tests.

wilhuff · 2020-01-28T17:59:30Z

...base-firestore/src/test/java/com/google/firebase/firestore/model/ObjectValueBuilderTest.java

+
+  @Test
+  public void setSingleField() {
+    ObjectValue.Builder builder = ObjectValue.emptyObject().toBuilder();


I could have sworn we added an ObjectValue.newBuilder or some short form like that. Could we use that to cut down some of the verbosity here?

This only exists in the old value.ObjectValue. I also added it to protovalue.ObjectValue

wilhuff · 2020-01-28T18:03:40Z

...base-firestore/src/test/java/com/google/firebase/firestore/model/ObjectValueBuilderTest.java

+  public void setSuccessorField() {
+    ObjectValue.Builder builder = ObjectValue.emptyObject().toBuilder();
+    builder.set(field("a"), fooValue);
+    builder.set(field("a0"), fooValue);


Your current successor implementation would break if you also had a value with a character with an ordinal less than 0 (U+0030), e.g. space (U+0020) or bang (U+0021).

wilhuff · 2020-01-28T18:33:37Z

...base-firestore/src/test/java/com/google/firebase/firestore/model/ObjectValueBuilderTest.java

+  public void setFieldInNestedObject() {
+    ObjectValue.Builder builder = ObjectValue.emptyObject().toBuilder();
+    builder.set(field("a"), map("b", fooValue));
+    builder.set(field("a.c"), fooValue);


Also consider a test where you set a nested object at a more-than-one-level difference of depth.

Sure thing. Added setsDeeplyNestedFieldInNestedObject()

schmidt-sebastian

Thank you for your review. I drastically simplified the implementation based on your feedback.

...base-firestore/src/main/java/com/google/firebase/firestore/model/protovalue/ObjectValue.java

schmidt-sebastian · 2020-01-29T00:14:01Z

...base-firestore/src/main/java/com/google/firebase/firestore/model/protovalue/ObjectValue.java

+     * @return Whether any modifications were applied (in any part of the subtree under
+     *     currentPath).
+     */
+    private boolean applyOverlay(FieldPath currentPath, MapValue.Builder resultAtPath) {


I believe this was close to impossible to follow, but it was the cleanest design I could come up with given the data structure I chose. I have now chosen a different data structure for the overlap map, which allowed me to drastically simplify the code.

schmidt-sebastian · 2020-01-29T00:18:38Z

...base-firestore/src/main/java/com/google/firebase/firestore/model/protovalue/ObjectValue.java

+
+      boolean modified = false;
+
+      while (!currentSlice.isEmpty()) {


I mainly used the TreeMap representation because it allowed me to store a <FieldPath, Value> map, which is more typesafe than any nested data structure I could come up with. If I trade type safety for readable code, the end result looks pretty much like you suggested. I am now storing the overlays in a <String, Object> map, where the key represents a single field segment and the value represents either:

A Value proto for a leaf node (this includes MapValues that were set as part of a single user operation)

A Map<String, Value> value that indicates additional nesting

null to indicate field removals

This drastically simplifies the code and reduce the runtime of applyOverlay() to O(n * m) (where n is the number of entries and m the nesting level). I believe before it was O(n log n * m).

schmidt-sebastian · 2020-01-29T00:20:48Z

...base-firestore/src/main/java/com/google/firebase/firestore/model/protovalue/ObjectValue.java

+        // Shrink the subtree to contain only values after the current field path. Note that we are
+        // still bound by the subtree created at the initial method invocation. The current loop
+        // exits when the subtree becomes empty.
+        currentSlice = currentSlice.tailMap(createSuccessor(fieldPath));


This is no longer part of the PR, but a remove here would make this method non-idempotent and might incur additional tree re-balancing.

schmidt-sebastian · 2020-01-29T00:22:00Z

...base-firestore/src/main/java/com/google/firebase/firestore/model/protovalue/ObjectValue.java

+    /** Create the first field path that is not part of the subtree created by `currentPath`. */
+    private FieldPath createSuccessor(FieldPath currentPath) {
+      hardAssert(!currentPath.isEmpty(), "Can't create a successor for an empty path");
+      return currentPath.popLast().append(currentPath.getLastSegment() + '0');


This is also no longer needed (yay for simpler code).

I chose '0' because it is the first ASCII code that can be part of a valid field path. '\0' may trigger validation errors in our field value handling.

schmidt-sebastian · 2020-01-29T00:29:32Z

firebase-firestore/src/test/java/com/google/firebase/firestore/model/FieldValueTest.java

+    return (ObjectValue) object;
+  }
+
+  private PrimitiveValue wrap(Object map) {


Changed to value.

schmidt-sebastian · 2020-01-29T00:30:33Z

...base-firestore/src/test/java/com/google/firebase/firestore/model/ObjectValueBuilderTest.java

+  public void emptyBuilder() {
+    ObjectValue.Builder builder = ObjectValue.emptyObject().toBuilder();
+    ObjectValue object = builder.build();
+    assertEquals(wrapObject(), object);


Good catch. It is meant to verify that a build on an empty builder works.

schmidt-sebastian · 2020-01-29T00:36:02Z

...base-firestore/src/test/java/com/google/firebase/firestore/model/ObjectValueBuilderTest.java

+  }
+
+  @Test
+  public void setSingleField() {


schmidt-sebastian · 2020-01-29T00:38:40Z

...base-firestore/src/test/java/com/google/firebase/firestore/model/ObjectValueBuilderTest.java

+
+  @Test
+  public void setSingleField() {
+    ObjectValue.Builder builder = ObjectValue.emptyObject().toBuilder();


This only exists in the old value.ObjectValue. I also added it to protovalue.ObjectValue

schmidt-sebastian · 2020-01-29T00:40:50Z

...base-firestore/src/test/java/com/google/firebase/firestore/model/ObjectValueBuilderTest.java

+  public void setFieldInNestedObject() {
+    ObjectValue.Builder builder = ObjectValue.emptyObject().toBuilder();
+    builder.set(field("a"), map("b", fooValue));
+    builder.set(field("a.c"), fooValue);


Sure thing. Added setsDeeplyNestedFieldInNestedObject()

wilhuff

LGTM

wilhuff · 2020-01-29T18:15:02Z

...base-firestore/src/main/java/com/google/firebase/firestore/model/protovalue/ObjectValue.java

+     */
+    public Builder delete(FieldPath path) {
+      hardAssert(!path.isEmpty(), "Cannot delete field for empty path on ObjectValue");
+      setOverlay(path, null);


null is pretty overloaded. I wonder if it would be helpful to store a sentinel in here instead.

Something like this up top:

private static final Object DELETED = new Object();

And then this would read as

setOverlay(path, DELETED);

I don't think this changes any of the handling below, so probably not worth it, but it has some visual appeal.

Feel free to ignore.

I am leaving this as is for now. The reason is the DELETE sentinel would need to of type Value so I can pass it to setOverlay. That makes my instanceof checks more complicated, as I now have to differentiate between user-provided values and the sentinel.

wilhuff · 2020-01-29T18:17:44Z

...e-firestore/src/main/java/com/google/firebase/firestore/model/protovalue/PrimitiveValue.java

+import java.util.List;
+import java.util.Map;
+
+public class PrimitiveValue extends FieldValue {


Merging SGTM. There seems to be little reason to keep this separate.

Port of firebase/firebase-android-sdk#1156

google-oss-bot added the do-not-merge/work-in-progress label Jan 23, 2020

googlebot added the cla: yes Override cla label Jan 23, 2020

schmidt-sebastian added api: firestore and removed cla: yes Override cla labels Jan 23, 2020

googlebot added cla: yes Override cla labels Jan 23, 2020

google-oss-bot added the size/XXL label Jan 23, 2020

schmidt-sebastian added 2 commits January 23, 2020 08:50

Adding Proto-based equality and comparison

5d3f44d

This will be used (and tested) in the follow-up CL that adds the FieldValue tpes.

Adding test utilities to create Value types

5d006f0

This exposes a similar interface as wrap() and allows me to change the existing FieldValue test to use Value protos

This was referenced Jan 23, 2020

Adding Proto-based equality and comparison #1157

Merged

Adding test utilities to create Value types #1158

Merged

schmidt-sebastian added 5 commits January 23, 2020 10:01

Merge branch 'mrschmidt/comparisons' into mrschmidt/testhelpers

8440ced

Address feeback

c315cdc

Address feeback

a6f880a

Merge branch 'mrschmidt/comparisons' into mrschmidt/testhelpers

529881b

Review feedback

c8d1ce8

schmidt-sebastian force-pushed the mrschmidt/valueproto branch from 0c0064d to 7bd837f Compare January 23, 2020 22:08

schmidt-sebastian removed the do-not-merge/work-in-progress label Jan 23, 2020

schmidt-sebastian changed the title ~~WIP: Protobuf-backed FieldValues~~ Protobuf-backed FieldValues Jan 23, 2020

schmidt-sebastian changed the base branch from mrschmidt/rewritefieldvalue to mrschmidt/testhelpers January 23, 2020 22:11

google-oss-bot added size/XL and removed size/XXL labels Jan 23, 2020

Add Protobuf-backed FieldValue

074f88a

schmidt-sebastian force-pushed the mrschmidt/valueproto branch from 7bd837f to 074f88a Compare January 23, 2020 22:27

schmidt-sebastian assigned wilhuff Jan 23, 2020

schmidt-sebastian requested a review from wilhuff January 23, 2020 22:28

schmidt-sebastian added 3 commits January 23, 2020 16:27

Fix compile

85e9fff

More compile fixes

74f8895

Format

618f903

Merge

2750928

schmidt-sebastian changed the base branch from mrschmidt/testhelpers to mrschmidt/rewritefieldvalue January 24, 2020 05:37

google-oss-bot added size/XXL and removed size/XL labels Jan 24, 2020

Remove duplicate file

c599106

google-oss-bot added size/XL and removed size/XXL labels Jan 24, 2020

schmidt-sebastian added 3 commits January 24, 2020 09:02

Remove exceess newline

9354a50

Imports

2baaba1

Add package-info.java

7bf5e40

wilhuff suggested changes Jan 28, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Jan 28, 2020

Review comments

035970b

schmidt-sebastian force-pushed the mrschmidt/valueproto branch from 47c659c to 035970b Compare January 29, 2020 00:51

schmidt-sebastian commented Jan 29, 2020

View reviewed changes

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Jan 29, 2020

wilhuff approved these changes Jan 29, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Jan 29, 2020

schmidt-sebastian merged commit 9354294 into mrschmidt/rewritefieldvalue Jan 29, 2020

schmidt-sebastian deleted the mrschmidt/valueproto branch February 7, 2020 00:22

schmidt-sebastian added a commit to firebase/firebase-js-sdk that referenced this pull request Feb 26, 2020

Protobuf-backed FieldValues

4eab635

Port of firebase/firebase-android-sdk#1156

schmidt-sebastian mentioned this pull request Feb 26, 2020

Protobuf-backed FieldValues firebase/firebase-js-sdk#2682

Merged

firebase locked and limited conversation to collaborators Feb 29, 2020

Protobuf-backed FieldValues #1156

Protobuf-backed FieldValues #1156

Uh oh!

Conversation

schmidt-sebastian commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

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!

wilhuff 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!

Uh oh!

schmidt-sebastian commented Jan 23, 2020 •

edited

Loading

codecov bot commented Jan 23, 2020 •

edited

Loading