From 1ae13d1f57306e479d5db5ceda9309856fd32c5d Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 23 Apr 2020 10:54:21 -0700 Subject: [PATCH] Untangle ObjectValue and ObjectValueBuilder (#2970) --- .../firestore/src/api/user_data_reader.ts | 6 +- packages/firestore/src/local/local_store.ts | 3 +- packages/firestore/src/model/field_value.ts | 75 ++++++++----------- packages/firestore/src/model/mutation.ts | 8 +- .../test/unit/model/field_value.test.ts | 36 ++++----- .../test/unit/model/mutation.test.ts | 12 +-- .../unit/model/object_value_builder.test.ts | 67 ++++++++++------- 7 files changed, 101 insertions(+), 106 deletions(-) diff --git a/packages/firestore/src/api/user_data_reader.ts b/packages/firestore/src/api/user_data_reader.ts index b4f197f8b1d..51f75fd8533 100644 --- a/packages/firestore/src/api/user_data_reader.ts +++ b/packages/firestore/src/api/user_data_reader.ts @@ -36,7 +36,7 @@ import { debugAssert, fail } from '../util/assert'; import { Code, FirestoreError } from '../util/error'; import { isPlainObject, valueDescription } from '../util/input_validation'; import { Dict, forEach, isEmpty } from '../util/obj'; -import { ObjectValue } from '../model/field_value'; +import { ObjectValue, ObjectValueBuilder } from '../model/field_value'; import { ArrayRemoveTransformOperation, ArrayUnionTransformOperation, @@ -390,7 +390,7 @@ export class UserDataReader { validatePlainObject('Data must be an object, but it was:', context, input); let fieldMaskPaths = new SortedSet(FieldPath.comparator); - const updateData = ObjectValue.newBuilder(); + const updateData = new ObjectValueBuilder(); forEach(input as Dict, (key, value) => { const path = fieldPathFromDotSeparatedString(methodName, key); @@ -450,7 +450,7 @@ export class UserDataReader { } let fieldMaskPaths = new SortedSet(FieldPath.comparator); - const updateData = ObjectValue.newBuilder(); + const updateData = new ObjectValueBuilder(); for (let i = 0; i < keys.length; ++i) { const path = keys[i]; diff --git a/packages/firestore/src/local/local_store.ts b/packages/firestore/src/local/local_store.ts index 1fcc424d874..1beceb929e8 100644 --- a/packages/firestore/src/local/local_store.ts +++ b/packages/firestore/src/local/local_store.ts @@ -66,6 +66,7 @@ import { IndexedDbPersistence } from './indexeddb_persistence'; import { IndexedDbMutationQueue } from './indexeddb_mutation_queue'; import { IndexedDbRemoteDocumentCache } from './indexeddb_remote_document_cache'; import { IndexedDbTargetCache } from './indexeddb_target_cache'; +import { extractFieldMask } from '../model/field_value'; const LOG_TAG = 'LocalStore'; @@ -339,7 +340,7 @@ export class LocalStore { new PatchMutation( mutation.key, baseValue, - baseValue.fieldMask(), + extractFieldMask(baseValue.proto.mapValue!), Precondition.exists(true) ) ); diff --git a/packages/firestore/src/model/field_value.ts b/packages/firestore/src/model/field_value.ts index 5d82d97e353..ed91379a7c3 100644 --- a/packages/firestore/src/model/field_value.ts +++ b/packages/firestore/src/model/field_value.ts @@ -59,9 +59,8 @@ export class ObjectValue { ); } - /** Returns a new Builder instance that is based on an empty object. */ - static newBuilder(): ObjectValueBuilder { - return ObjectValue.EMPTY.toBuilder(); + static empty(): ObjectValue { + return new ObjectValue({ mapValue: {} }); } /** @@ -90,48 +89,9 @@ export class ObjectValue { } } - /** - * Returns a FieldMask built from all FieldPaths starting from this - * ObjectValue, including paths from nested objects. - */ - fieldMask(): FieldMask { - return this.extractFieldMask(this.proto.mapValue!); - } - - private extractFieldMask(value: api.MapValue): FieldMask { - let fields = new SortedSet(FieldPath.comparator); - forEach(value.fields || {}, (key, value) => { - const currentPath = new FieldPath([key]); - if (typeOrder(value) === TypeOrder.ObjectValue) { - const nestedMask = this.extractFieldMask(value.mapValue!); - const nestedFields = nestedMask.fields; - if (nestedFields.isEmpty()) { - // Preserve the empty map by adding it to the FieldMask. - fields = fields.add(currentPath); - } else { - // For nested and non-empty ObjectValues, add the FieldPath of the - // leaf nodes. - nestedFields.forEach(nestedPath => { - fields = fields.add(currentPath.child(nestedPath)); - }); - } - } else { - // For nested and non-empty ObjectValues, add the FieldPath of the leaf - // nodes. - fields = fields.add(currentPath); - } - }); - return FieldMask.fromSet(fields); - } - isEqual(other: ObjectValue): boolean { return valueEquals(this.proto, other.proto); } - - /** Creates a ObjectValueBuilder instance that is based on the current value. */ - toBuilder(): ObjectValueBuilder { - return new ObjectValueBuilder(this); - } } /** @@ -152,7 +112,7 @@ export class ObjectValueBuilder { /** * @param baseObject The object to mutate. */ - constructor(private readonly baseObject: ObjectValue) {} + constructor(private readonly baseObject: ObjectValue = ObjectValue.empty()) {} /** * Sets the field to the provided value. @@ -278,3 +238,32 @@ export class ObjectValueBuilder { return modified ? { mapValue: { fields: resultAtPath } } : null; } } + +/** + * Returns a FieldMask built from all fields in a MapValue. + */ +export function extractFieldMask(value: api.MapValue): FieldMask { + let fields = new SortedSet(FieldPath.comparator); + forEach(value!.fields || {}, (key, value) => { + const currentPath = new FieldPath([key]); + if (isMapValue(value)) { + const nestedMask = extractFieldMask(value.mapValue!); + const nestedFields = nestedMask.fields; + if (nestedFields.isEmpty()) { + // Preserve the empty map by adding it to the FieldMask. + fields = fields.add(currentPath); + } else { + // For nested and non-empty ObjectValues, add the FieldPath of the + // leaf nodes. + nestedFields.forEach(nestedPath => { + fields = fields.add(currentPath.child(nestedPath)); + }); + } + } else { + // For nested and non-empty ObjectValues, add the FieldPath of the leaf + // nodes. + fields = fields.add(currentPath); + } + }); + return FieldMask.fromSet(fields); +} diff --git a/packages/firestore/src/model/mutation.ts b/packages/firestore/src/model/mutation.ts index b65980439ba..c783e52e138 100644 --- a/packages/firestore/src/model/mutation.ts +++ b/packages/firestore/src/model/mutation.ts @@ -488,13 +488,13 @@ export class PatchMutation extends Mutation { if (maybeDoc instanceof Document) { data = maybeDoc.data(); } else { - data = ObjectValue.EMPTY; + data = ObjectValue.empty(); } return this.patchObject(data); } private patchObject(data: ObjectValue): ObjectValue { - const builder = data.toBuilder(); + const builder = new ObjectValueBuilder(data); this.fieldMask.fields.forEach(fieldPath => { if (!fieldPath.isEmpty()) { const newValue = this.data.field(fieldPath); @@ -601,7 +601,7 @@ export class TransformMutation extends Mutation { if (coercedValue != null) { if (baseObject == null) { - baseObject = ObjectValue.newBuilder().set( + baseObject = new ObjectValueBuilder().set( fieldTransform.field, coercedValue ); @@ -729,7 +729,7 @@ export class TransformMutation extends Mutation { 'TransformResults length mismatch.' ); - const builder = data.toBuilder(); + const builder = new ObjectValueBuilder(data); for (let i = 0; i < this.fieldTransforms.length; i++) { const fieldTransform = this.fieldTransforms[i]; const fieldPath = fieldTransform.field; diff --git a/packages/firestore/test/unit/model/field_value.test.ts b/packages/firestore/test/unit/model/field_value.test.ts index c5833bda15a..29abad0ff96 100644 --- a/packages/firestore/test/unit/model/field_value.test.ts +++ b/packages/firestore/test/unit/model/field_value.test.ts @@ -18,7 +18,12 @@ import * as api from '../../../src/protos/firestore_proto_api'; import { expect } from 'chai'; -import { ObjectValue, TypeOrder } from '../../../src/model/field_value'; +import { + extractFieldMask, + ObjectValue, + ObjectValueBuilder, + TypeOrder +} from '../../../src/model/field_value'; import { typeOrder } from '../../../src/model/values'; import { wrap, wrapObject, field, mask } from '../../util/helpers'; @@ -79,13 +84,11 @@ describe('FieldValue', () => { }); it('can add multiple new fields', () => { - let objValue = ObjectValue.EMPTY; - objValue = objValue - .toBuilder() + let objValue = ObjectValue.empty(); + objValue = new ObjectValueBuilder(objValue) .set(field('a'), wrap('a')) .build(); - objValue = objValue - .toBuilder() + objValue = new ObjectValueBuilder(objValue) .set(field('b'), wrap('b')) .set(field('c'), wrap('c')) .build(); @@ -154,8 +157,7 @@ describe('FieldValue', () => { it('can delete added keys', () => { let objValue = wrapObject({}); - objValue = objValue - .toBuilder() + objValue = new ObjectValueBuilder(objValue) .set(field('a'), wrap('a')) .delete(field('a')) .build(); @@ -189,12 +191,8 @@ describe('FieldValue', () => { it('can delete multiple fields', () => { let objValue = wrapObject({ a: 'a', b: 'a', c: 'c' }); - objValue = objValue - .toBuilder() - .delete(field('a')) - .build(); - objValue = objValue - .toBuilder() + objValue = new ObjectValueBuilder(objValue).delete(field('a')).build(); + objValue = new ObjectValueBuilder(objValue) .delete(field('b')) .delete(field('c')) .build(); @@ -216,7 +214,7 @@ describe('FieldValue', () => { 'map.nested.d', 'emptymap' ); - const actualMask = objValue.fieldMask(); + const actualMask = extractFieldMask(objValue.proto.mapValue!); expect(actualMask.isEqual(expectedMask)).to.be.true; }); @@ -225,8 +223,7 @@ describe('FieldValue', () => { fieldPath: string, value: api.Value ): ObjectValue { - return objectValue - .toBuilder() + return new ObjectValueBuilder(objectValue) .set(field(fieldPath), value) .build(); } @@ -235,10 +232,7 @@ describe('FieldValue', () => { objectValue: ObjectValue, fieldPath: string ): ObjectValue { - return objectValue - .toBuilder() - .delete(field(fieldPath)) - .build(); + return new ObjectValueBuilder(objectValue).delete(field(fieldPath)).build(); } function assertObjectEquals( diff --git a/packages/firestore/test/unit/model/mutation.test.ts b/packages/firestore/test/unit/model/mutation.test.ts index d4e68cc27f1..378cebe89fe 100644 --- a/packages/firestore/test/unit/model/mutation.test.ts +++ b/packages/firestore/test/unit/model/mutation.test.ts @@ -47,6 +47,7 @@ import { wrap, wrapObject } from '../../util/helpers'; +import { ObjectValueBuilder } from '../../../src/model/field_value'; describe('Mutation', () => { addEqualityMatcher(); @@ -191,11 +192,12 @@ describe('Mutation', () => { ); // Server timestamps aren't parsed, so we manually insert it. - const data = wrapObject({ - foo: { bar: '' }, - baz: 'baz-value' - }) - .toBuilder() + const data = new ObjectValueBuilder( + wrapObject({ + foo: { bar: '' }, + baz: 'baz-value' + }) + ) .set(field('foo.bar'), serverTimestamp(timestamp, null)) .build(); const expectedDoc = new Document(key('collection/key'), version(0), data, { diff --git a/packages/firestore/test/unit/model/object_value_builder.test.ts b/packages/firestore/test/unit/model/object_value_builder.test.ts index 7b38ed873ad..090502a1e08 100644 --- a/packages/firestore/test/unit/model/object_value_builder.test.ts +++ b/packages/firestore/test/unit/model/object_value_builder.test.ts @@ -18,31 +18,34 @@ import { expect } from 'chai'; import { field, wrap, wrapObject } from '../../util/helpers'; -import { ObjectValue } from '../../../src/model/field_value'; +import { + ObjectValue, + ObjectValueBuilder +} from '../../../src/model/field_value'; describe('ObjectValueBuilder', () => { it('supports empty builders', () => { - const builder = ObjectValue.newBuilder(); + const builder = new ObjectValueBuilder(); const object = builder.build(); expect(object.isEqual(ObjectValue.EMPTY)).to.be.true; }); it('sets single field', () => { - const builder = ObjectValue.newBuilder(); + const builder = new ObjectValueBuilder(); builder.set(field('foo'), wrap('foo')); const object = builder.build(); expect(object.isEqual(wrapObject({ 'foo': 'foo' }))).to.be.true; }); it('sets empty object', () => { - const builder = ObjectValue.newBuilder(); + const builder = new ObjectValueBuilder(); builder.set(field('foo'), wrap({})); const object = builder.build(); expect(object.isEqual(wrapObject({ 'foo': {} }))).to.be.true; }); it('sets multiple fields', () => { - const builder = ObjectValue.newBuilder(); + const builder = new ObjectValueBuilder(); builder.set(field('foo'), wrap('foo')); builder.set(field('bar'), wrap('bar')); const object = builder.build(); @@ -51,7 +54,7 @@ describe('ObjectValueBuilder', () => { }); it('sets nested fields', () => { - const builder = ObjectValue.newBuilder(); + const builder = new ObjectValueBuilder(); builder.set(field('a.b'), wrap('foo')); builder.set(field('c.d.e'), wrap('bar')); const object = builder.build(); @@ -63,7 +66,7 @@ describe('ObjectValueBuilder', () => { }); it('sets two fields in nested object', () => { - const builder = ObjectValue.newBuilder(); + const builder = new ObjectValueBuilder(); builder.set(field('a.b'), wrap('foo')); builder.set(field('a.c'), wrap('bar')); const object = builder.build(); @@ -72,7 +75,7 @@ describe('ObjectValueBuilder', () => { }); it('sets field in nested object', () => { - const builder = ObjectValue.newBuilder(); + const builder = new ObjectValueBuilder(); builder.set(field('a'), wrap({ b: 'foo' })); builder.set(field('a.c'), wrap('bar')); const object = builder.build(); @@ -81,7 +84,7 @@ describe('ObjectValueBuilder', () => { }); it('sets deeply nested field in nested object', () => { - const builder = ObjectValue.newBuilder(); + const builder = new ObjectValueBuilder(); builder.set(field('a.b.c.d.e.f'), wrap('foo')); const object = builder.build(); expect( @@ -94,7 +97,7 @@ describe('ObjectValueBuilder', () => { }); it('sets nested field multiple times', () => { - const builder = ObjectValue.newBuilder(); + const builder = new ObjectValueBuilder(); builder.set(field('a.c'), wrap('foo')); builder.set(field('a'), wrap({ b: 'foo' })); const object = builder.build(); @@ -102,7 +105,7 @@ describe('ObjectValueBuilder', () => { }); it('sets and deletes field', () => { - const builder = ObjectValue.newBuilder(); + const builder = new ObjectValueBuilder(); builder.set(field('foo'), wrap('foo')); builder.delete(field('foo')); const object = builder.build(); @@ -110,7 +113,7 @@ describe('ObjectValueBuilder', () => { }); it('sets and deletes nested field', () => { - const builder = ObjectValue.newBuilder(); + const builder = new ObjectValueBuilder(); builder.set(field('a.b.c'), wrap('foo')); builder.set(field('a.b.d'), wrap('foo')); builder.set(field('f.g'), wrap('foo')); @@ -126,23 +129,25 @@ describe('ObjectValueBuilder', () => { }); it('sets single field in existing object', () => { - const builder = wrapObject({ a: 'foo' }).toBuilder(); + const builder = new ObjectValueBuilder(wrapObject({ a: 'foo' })); builder.set(field('b'), wrap('foo')); const object = builder.build(); expect(object.isEqual(wrapObject({ a: 'foo', b: 'foo' }))).to.be.true; }); it('overwrites field', () => { - const builder = wrapObject({ a: 'foo' }).toBuilder(); + const builder = new ObjectValueBuilder(wrapObject({ a: 'foo' })); builder.set(field('a'), wrap('bar')); const object = builder.build(); expect(object.isEqual(wrapObject({ a: 'bar' }))).to.be.true; }); it('overwrites nested field', () => { - const builder = wrapObject({ - a: { b: 'foo', c: { 'd': 'foo' } } - }).toBuilder(); + const builder = new ObjectValueBuilder( + wrapObject({ + a: { b: 'foo', c: { 'd': 'foo' } } + }) + ); builder.set(field('a.b'), wrap('bar')); builder.set(field('a.c.d'), wrap('bar')); const object = builder.build(); @@ -151,14 +156,14 @@ describe('ObjectValueBuilder', () => { }); it('overwrites deeply nested field', () => { - const builder = wrapObject({ a: { b: 'foo' } }).toBuilder(); + const builder = new ObjectValueBuilder(wrapObject({ a: { b: 'foo' } })); builder.set(field('a.b.c'), wrap('bar')); const object = builder.build(); expect(object.isEqual(wrapObject({ a: { b: { c: 'bar' } } }))).to.be.true; }); it('merges existing object', () => { - const builder = wrapObject({ a: { b: 'foo' } }).toBuilder(); + const builder = new ObjectValueBuilder(wrapObject({ a: { b: 'foo' } })); builder.set(field('a.c'), wrap('foo')); const object = builder.build(); expect(object.isEqual(wrapObject({ a: { b: 'foo', c: 'foo' } }))).to.be @@ -166,9 +171,11 @@ describe('ObjectValueBuilder', () => { }); it('overwrites nested object', () => { - const builder = wrapObject({ - a: { b: { c: 'foo', d: 'foo' } } - }).toBuilder(); + const builder = new ObjectValueBuilder( + wrapObject({ + a: { b: { c: 'foo', d: 'foo' } } + }) + ); builder.set(field('a.b'), wrap('bar')); const object = builder.build(); expect(object.isEqual(wrapObject({ a: { b: 'bar' } }))).to.be.true; @@ -176,37 +183,39 @@ describe('ObjectValueBuilder', () => { it('replaces nested object', () => { const singleValueObject = wrap({ c: 'bar' }); - const builder = wrapObject({ a: { b: 'foo' } }).toBuilder(); + const builder = new ObjectValueBuilder(wrapObject({ a: { b: 'foo' } })); builder.set(field('a'), singleValueObject); const object = builder.build(); expect(object.isEqual(wrapObject({ a: { c: 'bar' } }))).to.be.true; }); it('deletes single field', () => { - const builder = wrapObject({ a: 'foo', b: 'foo' }).toBuilder(); + const builder = new ObjectValueBuilder(wrapObject({ a: 'foo', b: 'foo' })); builder.delete(field('a')); const object = builder.build(); expect(object.isEqual(wrapObject({ b: 'foo' }))).to.be.true; }); it('deletes nested object', () => { - const builder = wrapObject({ - a: { b: { c: 'foo', d: 'foo' }, f: 'foo' } - }).toBuilder(); + const builder = new ObjectValueBuilder( + wrapObject({ + a: { b: { c: 'foo', d: 'foo' }, f: 'foo' } + }) + ); builder.delete(field('a.b')); const object = builder.build(); expect(object.isEqual(wrapObject({ a: { f: 'foo' } }))).to.be.true; }); it('deletes non-existing field', () => { - const builder = wrapObject({ a: 'foo' }).toBuilder(); + const builder = new ObjectValueBuilder(wrapObject({ a: 'foo' })); builder.delete(field('b')); const object = builder.build(); expect(object.isEqual(wrapObject({ a: 'foo' }))).to.be.true; }); it('deletes non-existing nested field', () => { - const builder = wrapObject({ a: { b: 'foo' } }).toBuilder(); + const builder = new ObjectValueBuilder(wrapObject({ a: { b: 'foo' } })); builder.delete(field('a.b.c')); const object = builder.build(); expect(object.isEqual(wrapObject({ a: { b: 'foo' } }))).to.be.true;