Adding Proto-based equality and comparison #2678

schmidt-sebastian · 2020-02-25T00:46:44Z

Port of firebase/firebase-android-sdk#1157

Sending this to @wilhuff since it includes some gnarly type normalization that does not exist on Android (e.g. converts string numbers to numbers).

wilhuff · 2020-02-25T17:25:43Z

packages/firestore/src/model/proto_values.ts

+  }
+}
+
+function compareBooleans(b1: boolean, b2: boolean): number {


This is unnecessary. Javascript booleans compare naturally using < so you can just use primitiveComparator.

wilhuff · 2020-02-25T17:28:10Z

packages/firestore/src/remote/serializer.ts

@@ -512,7 +445,7 @@ export class JsonProtoSerializer {
      return new fieldValue.GeoPointValue(new GeoPoint(latitude, longitude));
    } else if ('bytesValue' in obj) {
      assertPresent(obj.bytesValue, 'bytesValue');
-      const blob = this.fromBlob(obj.bytesValue);
+      const blob = normalizeByteString(obj.bytesValue);


This is now a ByteString, so better to name this bytes.

wilhuff · 2020-02-25T20:13:14Z

packages/firestore/src/model/proto_values.ts

@@ -0,0 +1,385 @@
+/**
+ * @license
+ * Copyright 202 Google LLC


I thought the year was meant to represent when the file was originally created...
Updated.

wilhuff · 2020-02-25T20:15:05Z

packages/firestore/src/model/proto_values.ts

+ * limitations under the License.
+ */
+
+import * as api from '../protos/firestore_proto_api';


Since we already have an "api" package this is potentially confusing. Consider importing this as protos or something that more concretely points toward this being the messages.

We use this exact import in a lot of places in the SDK. My naming was inspired by these imports.

Kept as is.

wilhuff · 2020-02-25T20:27:46Z

packages/firestore/src/model/proto_values.ts

+
+/** Extracts the backend's type order for the provided value. */
+export function typeOrder(value: api.Value): TypeOrder {
+  if ('nullValue' in value) {


It seems like we could take the first key in value and then look this up in a map. Something like:

const typeOrderMap = { 'nullValue': TypeOrder.NullValue, // ... } export function typeOrder(value: api.Value): TypeOrder { for (const key in value) { const result = typeOrderMap[key]; if (result !== undefined) { return result; } } return fail('Invalid value type: ' + JSON.stringify(value)); }

This seems like it would result in more code. I am not opposed, but the current implementation closely follows

firebase-js-sdk/packages/firestore/src/remote/serializer.ts

Line 408 in 14931c2

if ('nullValue' in obj) {

Kept as is.

wilhuff · 2020-02-25T20:32:03Z

packages/firestore/src/model/proto_values.ts

+export function numberEquals(left: api.Value, right: api.Value): boolean {
+  if ('integerValue' in left && 'integerValue' in right) {
+    return (
+      normalizeNumber(left.integerValue) === normalizeNumber(right.integerValue)


Should the serializer just perform these normalizations once so we don't have to do it on every comparison?

That's something we should potentially discuss offline. It breaks one of the goals of this field value rewrite (as it would require us to do a full pass of incoming objects), but it would simplify this code a bit and could be something that we could just do for Node.

If we do end up pre-normalizing our data, we will likely also end up with two different versions of the Value type, and we should have a strategy on how to cleanly separate the "incoming" Value type from the internal Value type.

wilhuff · 2020-02-25T20:35:40Z

packages/firestore/src/model/proto_values.ts

+}
+
+function compareTimestamps(
+  left: string | { seconds?: string; nanos?: number },


This type is a mouthful. Maybe make an alias for it?

wilhuff · 2020-02-25T20:40:19Z

packages/firestore/src/model/proto_values.ts

+  const rightMap = right.fields || {};
+  const rightKeys = keys(leftMap);
+
+  leftKeys.sort();


Are these sorts required? The protos we get from the backend are guaranteed to come with keys in order, and JS guarantees that traversing keys happens in the order they're added to the object.

I added a comment here (that I stole from the canonical ID implementation on Android). The maps can get out of order when documents are added locally. Furthermore, I don't think iteration order is guaranteed here (on Android, iteration by insertion order is enforced by the backing LinkedHashMap).

wilhuff · 2020-02-25T20:40:54Z

packages/firestore/src/model/proto_values.ts

+export function normalizeTimestamp(
+  date: string | { seconds?: string; nanos?: number }
+): { seconds: number; nanos: number } {
+  assert(!!date, 'Cannot deserialize null or undefined timestamp.');


s/deserialize/normalize/ ?

wilhuff · 2020-02-25T20:43:11Z

packages/firestore/src/model/proto_values.ts

+    return value;
+  } else if (typeof value === 'string') {
+    // Proto 3 uses the string values 'NaN' and 'Infinity'.
+    if (value === 'NaN') {


Number() parses these values just fine. Do we really need to special case them?

Good call. This is actually from the serializer, but we don't need to this code here (or there).

wilhuff

LGTM

schmidt-sebastian changed the base branch from mrschmidt/apiblob to mrschmidt/rewritefieldvalue February 25, 2020 04:07

schmidt-sebastian force-pushed the mrschmidt/rewritefieldvalue-2 branch 6 times, most recently from 4079cff to 7e97b72 Compare February 25, 2020 05:32

Adding Proto-based equality and comparison

14931c2

schmidt-sebastian force-pushed the mrschmidt/rewritefieldvalue-2 branch from 7e97b72 to 14931c2 Compare February 25, 2020 05:33

schmidt-sebastian requested a review from wilhuff February 25, 2020 17:04

schmidt-sebastian assigned wilhuff Feb 25, 2020

schmidt-sebastian added the api: firestore label Feb 25, 2020

wilhuff reviewed Feb 25, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Feb 25, 2020

Address feedback

114c0de

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Feb 25, 2020

wilhuff approved these changes Feb 25, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Feb 25, 2020

schmidt-sebastian merged commit 799bc96 into mrschmidt/rewritefieldvalue Feb 25, 2020

schmidt-sebastian deleted the mrschmidt/rewritefieldvalue-2 branch February 28, 2020 23:03

firebase locked and limited conversation to collaborators Mar 27, 2020

Adding Proto-based equality and comparison #2678

Adding Proto-based equality and comparison #2678

Uh oh!

Conversation

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

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!

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!

Uh oh!

schmidt-sebastian commented Feb 25, 2020 •

edited

Loading