From 3216624e42c4e2af8d5e0141994def00f9ab10a6 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 21 May 2021 20:14:34 -0600 Subject: [PATCH 01/11] Simplify ObjectValue --- packages/firestore/src/model/object_value.ts | 225 +++++++------------ packages/firestore/src/model/values.ts | 24 ++ 2 files changed, 111 insertions(+), 138 deletions(-) diff --git a/packages/firestore/src/model/object_value.ts b/packages/firestore/src/model/object_value.ts index faa879044da..7a27d450a34 100644 --- a/packages/firestore/src/model/object_value.ts +++ b/packages/firestore/src/model/object_value.ts @@ -26,44 +26,24 @@ import { FieldMask } from './field_mask'; import { FieldPath } from './path'; import { isServerTimestamp } from './server_timestamps'; import { TypeOrder } from './type_order'; -import { isMapValue, typeOrder, valueEquals } from './values'; +import { deepClone, isMapValue, typeOrder, valueEquals } from './values'; export interface JsonObject { [name: string]: T; } - -/** - * An Overlay, which contains an update to apply. Can either be Value proto, a - * map of Overlay values (to represent additional nesting at the given key) or - * `null` (to represent field deletes). - */ -type Overlay = Map | ProtoValue | null; - /** * An ObjectValue represents a MapValue in the Firestore Proto and offers the * ability to add and remove fields (via the ObjectValueBuilder). */ export class ObjectValue { - /** - * The immutable Value proto for this object. Local mutations are stored in - * `overlayMap` and only applied when `buildProto()` is invoked. - */ - private partialValue: { mapValue: ProtoMapValue }; - - /** - * A nested map that contains the accumulated changes that haven't yet been - * applied to `partialValue`. Values can either be `Value` protos, Map values (to represent additional nesting) or `null` (to represent - * field deletes). - */ - private overlayMap = new Map(); + private value: { mapValue: ProtoMapValue }; constructor(proto: { mapValue: ProtoMapValue }) { debugAssert( !isServerTimestamp(proto), 'ServerTimestamps should be converted to ServerTimestampValue' ); - this.partialValue = proto; + this.value = proto; } static empty(): ObjectValue { @@ -77,12 +57,28 @@ export class ObjectValue { * @returns The value at the path or null if the path is not set. */ field(path: FieldPath): ProtoValue | null { - return ObjectValue.extractNestedValue(this.buildProto(), path); + if (path.isEmpty()) { + return this.value; + } else { + let currentLevel: ProtoValue = this.value; + for (let i = 0; i < path.length - 1; ++i) { + if (!currentLevel.mapValue!.fields) { + return null; + } + currentLevel = currentLevel.mapValue!.fields[path.get(i)]; + if (!isMapValue(currentLevel)) { + return null; + } + } + + currentLevel = (currentLevel.mapValue!.fields || {})[path.lastSegment()]; + return currentLevel || null; + } } /** Returns the full protobuf representation. */ toProto(): { mapValue: ProtoMapValue } { - return this.field(FieldPath.emptyPath()) as { mapValue: ProtoMapValue }; + return this.value; } /** @@ -96,7 +92,12 @@ export class ObjectValue { !path.isEmpty(), 'Cannot set field for empty path on ObjectValue' ); - this.setOverlay(path, value); + const parentMap = this.getParentMap(path.popLast()); + this.applyChanges( + parentMap, + { [path.lastSegment()]: value }, + /*deletes=*/ [] + ); } /** @@ -105,13 +106,30 @@ export class ObjectValue { * @param data - A map of fields to values (or null for deletes). */ setAll(data: Map): void { - data.forEach((value, fieldPath) => { + let parent = FieldPath.emptyPath(); + + let upserts: { [key: string]: ProtoValue } = {}; + let deletes: string[] = []; + + data.forEach((value, path) => { + if (!parent.isImmediateParentOf(path)) { + // Insert the accumulated changes at this parent location + const parent_map = this.getParentMap(parent); + this.applyChanges(parent_map, upserts, deletes); + upserts = {}; + deletes = []; + parent = path.popLast(); + } + if (value) { - this.set(fieldPath, value); + upserts[path.lastSegment()] = value; } else { - this.delete(fieldPath); + deletes.push(path.lastSegment()); } }); + + const parentMap = this.getParentMap(parent); + this.applyChanges(parentMap, upserts, deletes); } /** @@ -125,138 +143,69 @@ export class ObjectValue { !path.isEmpty(), 'Cannot delete field for empty path on ObjectValue' ); - this.setOverlay(path, null); + const nestedValue = this.field(path.popLast()); + if (nestedValue && nestedValue.mapValue) { + this.applyChanges(nestedValue.mapValue, /*upserts=*/ {}, [ + path.lastSegment() + ]); + } } isEqual(other: ObjectValue): boolean { - return valueEquals(this.buildProto(), other.buildProto()); + return valueEquals(this.value, other.value); } /** - * Adds `value` to the overlay map at `path`. Creates nested map entries if - * needed. + * Returns the map that contains the leaf element of `path`. If the parent + * entry does not yet exist, or if it is not a map, a new map will be created. */ - private setOverlay(path: FieldPath, value: ProtoValue | null): void { - let currentLevel = this.overlayMap; + private getParentMap(path: FieldPath): ProtoMapValue { + let parent = this.value; - for (let i = 0; i < path.length - 1; ++i) { + for (let i = 0; i < path.length; ++i) { const currentSegment = path.get(i); - let currentValue = currentLevel.get(currentSegment); - if (currentValue instanceof Map) { - // Re-use a previously created map - currentLevel = currentValue; - } else if ( - currentValue && - typeOrder(currentValue) === TypeOrder.ObjectValue - ) { - // Convert the existing Protobuf MapValue into a map - currentValue = new Map( - Object.entries(currentValue.mapValue!.fields || {}) - ); - currentLevel.set(currentSegment, currentValue); - currentLevel = currentValue; - } else { - // Create an empty map to represent the current nesting level - currentValue = new Map(); - currentLevel.set(currentSegment, currentValue); - currentLevel = currentValue; + if (!parent.mapValue.fields) { + parent.mapValue.fields = {}; } - } + let currentValue = parent.mapValue.fields[currentSegment]; - currentLevel.set(path.lastSegment(), value); - } - - /** - * Applies any overlays from `currentOverlays` that exist at `currentPath` - * and returns the merged data at `currentPath` (or null if there were no - * changes). - * - * @param currentPath - The path at the current nesting level. Can be set to - * FieldValue.emptyPath() to represent the root. - * @param currentOverlays - The overlays at the current nesting level in the - * same format as `overlayMap`. - * @returns The merged data at `currentPath` or null if no modifications - * were applied. - */ - private applyOverlay( - currentPath: FieldPath, - currentOverlays: Map - ): { mapValue: ProtoMapValue } | null { - let modified = false; - - const existingValue = ObjectValue.extractNestedValue( - this.partialValue, - currentPath - ); - const resultAtPath = isMapValue(existingValue) - ? // If there is already data at the current path, base our - // modifications on top of the existing data. - { ...existingValue.mapValue.fields } - : {}; - - currentOverlays.forEach((value, pathSegment) => { - if (value instanceof Map) { - const nested = this.applyOverlay(currentPath.child(pathSegment), value); - if (nested != null) { - resultAtPath[pathSegment] = nested; - modified = true; - } - } else if (value !== null) { - resultAtPath[pathSegment] = value; - modified = true; - } else if (resultAtPath.hasOwnProperty(pathSegment)) { - delete resultAtPath[pathSegment]; - modified = true; + if (!currentValue || typeOrder(currentValue) !== TypeOrder.ObjectValue) { + // Since the element is not a map value, free all existing data and + // change it to a map type. + currentValue = { mapValue: {} }; + parent.mapValue.fields[currentSegment] = currentValue; } - }); - return modified ? { mapValue: { fields: resultAtPath } } : null; + parent.mapValue.fields[currentSegment] = currentValue; + parent = currentValue as { mapValue: ProtoMapValue }; + } + + return parent.mapValue; } /** - * Builds the Protobuf that backs this ObjectValue. - * - * This method applies any outstanding modifications and memoizes the result. - * Further invocations are based on this memoized result. + * Modifies `parent_map` by adding, replacing or deleting the specified + * entries. */ - private buildProto(): { mapValue: ProtoMapValue } { - const mergedResult = this.applyOverlay( - FieldPath.emptyPath(), - this.overlayMap - ); - if (mergedResult != null) { - this.partialValue = mergedResult; - this.overlayMap.clear(); + private applyChanges( + parentMap: ProtoMapValue, + inserts: { [key: string]: ProtoValue }, + deletes: string[] + ): void { + if (!parentMap.fields) { + parentMap.fields = {}; } - return this.partialValue; - } - - private static extractNestedValue( - proto: ProtoValue, - path: FieldPath - ): ProtoValue | null { - if (path.isEmpty()) { - return proto; - } else { - let value: ProtoValue = proto; - for (let i = 0; i < path.length - 1; ++i) { - if (!value.mapValue!.fields) { - return null; - } - value = value.mapValue!.fields[path.get(i)]; - if (!isMapValue(value)) { - return null; - } - } - - value = (value.mapValue!.fields || {})[path.lastSegment()]; - return value || null; + forEach(inserts, (key, val) => (parentMap.fields![key] = val)); + for (const field of deletes) { + delete parentMap.fields[field]; } } clone(): ObjectValue { - return new ObjectValue(this.buildProto()); + return new ObjectValue( + deepClone(this.value) as { mapValue: ProtoMapValue } + ); } } diff --git a/packages/firestore/src/model/values.ts b/packages/firestore/src/model/values.ts index 04764b6caf1..9f5625ba52a 100644 --- a/packages/firestore/src/model/values.ts +++ b/packages/firestore/src/model/values.ts @@ -554,3 +554,27 @@ export function isMapValue( ): value is { mapValue: MapValue } { return !!value && 'mapValue' in value; } + +/** Creates a deep copy of `source`. */ +export function deepClone(source: Value): Value { + if (source.geoPointValue) { + return { geoPointValue: { ...source.geoPointValue } }; + } else if (source.timestampValue) { + return { timestampValue: { ...normalizeTimestamp(source.timestampValue) } }; + } else if (source.mapValue) { + const target: Value = { mapValue: { fields: {} } }; + forEach( + source.mapValue.fields || {}, + (key, val) => (target.mapValue!.fields![key] = deepClone(val)) + ); + return target; + } else if (source.arrayValue) { + const target: Value = { arrayValue: { values: [] } }; + for (let i = 0; i < (source.arrayValue.values || []).length; ++i) { + target.arrayValue!.values![i] = deepClone(source.arrayValue.values![i]); + } + return target; + } else { + return { ...source }; + } +} From f8ad4b3415ca38a6138adf50bfd07508aae01275 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 21 May 2021 20:24:20 -0600 Subject: [PATCH 02/11] Simplify --- packages/firestore/src/model/object_value.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/firestore/src/model/object_value.ts b/packages/firestore/src/model/object_value.ts index 7a27d450a34..d2443b0daac 100644 --- a/packages/firestore/src/model/object_value.ts +++ b/packages/firestore/src/model/object_value.ts @@ -36,14 +36,11 @@ export interface JsonObject { * ability to add and remove fields (via the ObjectValueBuilder). */ export class ObjectValue { - private value: { mapValue: ProtoMapValue }; - - constructor(proto: { mapValue: ProtoMapValue }) { + constructor(private value: { mapValue: ProtoMapValue }) { debugAssert( - !isServerTimestamp(proto), + !isServerTimestamp(value), 'ServerTimestamps should be converted to ServerTimestampValue' ); - this.value = proto; } static empty(): ObjectValue { From dea34067eda30ab3637657c62f81caf543e20476 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Sat, 22 May 2021 14:55:00 -0600 Subject: [PATCH 03/11] Simplify forEach --- packages/firestore/src/lite/user_data_writer.ts | 2 +- packages/firestore/src/model/object_value.ts | 2 +- packages/firestore/src/model/values.ts | 4 ++-- packages/firestore/src/util/obj.ts | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/firestore/src/lite/user_data_writer.ts b/packages/firestore/src/lite/user_data_writer.ts index 3f48d9094cf..2b30b970573 100644 --- a/packages/firestore/src/lite/user_data_writer.ts +++ b/packages/firestore/src/lite/user_data_writer.ts @@ -92,7 +92,7 @@ export abstract class AbstractUserDataWriter { serverTimestampBehavior: ServerTimestampBehavior ): DocumentData { const result: DocumentData = {}; - forEach(mapValue.fields || {}, (key, value) => { + forEach(mapValue.fields, (key, value) => { result[key] = this.convertValue(value, serverTimestampBehavior); }); return result; diff --git a/packages/firestore/src/model/object_value.ts b/packages/firestore/src/model/object_value.ts index d2443b0daac..920d2fa6c4c 100644 --- a/packages/firestore/src/model/object_value.ts +++ b/packages/firestore/src/model/object_value.ts @@ -211,7 +211,7 @@ export class ObjectValue { */ export function extractFieldMask(value: ProtoMapValue): FieldMask { const fields: FieldPath[] = []; - forEach(value!.fields || {}, (key, value) => { + forEach(value!.fields, (key, value) => { const currentPath = new FieldPath([key]); if (isMapValue(value)) { const nestedMask = extractFieldMask(value.mapValue!); diff --git a/packages/firestore/src/model/values.ts b/packages/firestore/src/model/values.ts index 9f5625ba52a..720d65137d4 100644 --- a/packages/firestore/src/model/values.ts +++ b/packages/firestore/src/model/values.ts @@ -479,7 +479,7 @@ export function estimateByteSize(value: Value): number { function estimateMapByteSize(mapValue: MapValue): number { let size = 0; - forEach(mapValue.fields || {}, (key, val) => { + forEach(mapValue.fields, (key, val) => { size += key.length + estimateByteSize(val); }); return size; @@ -564,7 +564,7 @@ export function deepClone(source: Value): Value { } else if (source.mapValue) { const target: Value = { mapValue: { fields: {} } }; forEach( - source.mapValue.fields || {}, + source.mapValue.fields, (key, val) => (target.mapValue!.fields![key] = deepClone(val)) ); return target; diff --git a/packages/firestore/src/util/obj.ts b/packages/firestore/src/util/obj.ts index 5081aed1867..53e05f48c3f 100644 --- a/packages/firestore/src/util/obj.ts +++ b/packages/firestore/src/util/obj.ts @@ -32,7 +32,7 @@ export function objectSize(obj: object): number { } export function forEach( - obj: Dict, + obj: Dict | undefined, fn: (key: string, val: V) => void ): void { for (const key in obj) { From 60ab6a3dab89bf1a32f463ceb8d68240f11ca7db Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 24 May 2021 09:08:50 -0600 Subject: [PATCH 04/11] Reduce code duplication --- packages/firestore/src/model/object_value.ts | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/packages/firestore/src/model/object_value.ts b/packages/firestore/src/model/object_value.ts index 920d2fa6c4c..466e9b79b93 100644 --- a/packages/firestore/src/model/object_value.ts +++ b/packages/firestore/src/model/object_value.ts @@ -111,8 +111,8 @@ export class ObjectValue { data.forEach((value, path) => { if (!parent.isImmediateParentOf(path)) { // Insert the accumulated changes at this parent location - const parent_map = this.getParentMap(parent); - this.applyChanges(parent_map, upserts, deletes); + const parentMap = this.getParentMap(parent); + this.applyChanges(parentMap, upserts, deletes); upserts = {}; deletes = []; parent = path.popLast(); @@ -160,22 +160,18 @@ export class ObjectValue { let parent = this.value; for (let i = 0; i < path.length; ++i) { - const currentSegment = path.get(i); + const segment = path.get(i); if (!parent.mapValue.fields) { parent.mapValue.fields = {}; } - let currentValue = parent.mapValue.fields[currentSegment]; - if (!currentValue || typeOrder(currentValue) !== TypeOrder.ObjectValue) { - // Since the element is not a map value, free all existing data and + if (!isMapValue(parent.mapValue.fields[segment])) { + // Since the element is not a map value, remove all existing data and // change it to a map type. - currentValue = { mapValue: {} }; - parent.mapValue.fields[currentSegment] = currentValue; + parent.mapValue.fields[segment] = { mapValue: {} }; } - - parent.mapValue.fields[currentSegment] = currentValue; - parent = currentValue as { mapValue: ProtoMapValue }; + parent = parent.mapValue.fields[segment] as { mapValue: ProtoMapValue }; } return parent.mapValue; From aab4e57314e0a7b96b2a607020519c3226df6c3a Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 24 May 2021 09:54:26 -0600 Subject: [PATCH 05/11] Reduce code --- packages/firestore/src/exp/snapshot.ts | 2 +- packages/firestore/src/lite/snapshot.ts | 4 +- .../firestore/src/local/local_store_impl.ts | 4 +- .../firestore/src/local/memory_persistence.ts | 2 +- packages/firestore/src/model/document.ts | 2 +- packages/firestore/src/model/object_value.ts | 71 ++++++++----------- packages/firestore/src/remote/serializer.ts | 4 +- .../test/unit/model/document.test.ts | 2 +- .../test/unit/model/object_value.test.ts | 2 +- .../test/unit/remote/serializer.helper.ts | 2 +- .../test/unit/specs/bundle_spec.test.ts | 4 +- .../firestore/test/unit/specs/spec_builder.ts | 10 +-- .../firestore/test/util/spec_test_helpers.ts | 2 +- 13 files changed, 47 insertions(+), 64 deletions(-) diff --git a/packages/firestore/src/exp/snapshot.ts b/packages/firestore/src/exp/snapshot.ts index c7a1f499527..576238af9a8 100644 --- a/packages/firestore/src/exp/snapshot.ts +++ b/packages/firestore/src/exp/snapshot.ts @@ -278,7 +278,7 @@ export class DocumentSnapshot< return this._converter.fromFirestore(snapshot, options); } else { return this._userDataWriter.convertValue( - this._document.data.toProto(), + this._document.data.value, options.serverTimestamps ) as T; } diff --git a/packages/firestore/src/lite/snapshot.ts b/packages/firestore/src/lite/snapshot.ts index 01ab14d976b..297f046eb44 100644 --- a/packages/firestore/src/lite/snapshot.ts +++ b/packages/firestore/src/lite/snapshot.ts @@ -174,9 +174,7 @@ export class DocumentSnapshot { ); return this._converter.fromFirestore(snapshot); } else { - return this._userDataWriter.convertValue( - this._document.data.toProto() - ) as T; + return this._userDataWriter.convertValue(this._document.data.value) as T; } } diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index 8dae27bc990..aa83ac17f74 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -16,7 +16,7 @@ */ import { User } from '../auth/user'; -import { BundledDocuments, NamedQuery, BundleConverter } from '../core/bundle'; +import { BundleConverter, BundledDocuments, NamedQuery } from '../core/bundle'; import { newQueryForPath, Query, queryToTarget } from '../core/query'; import { SnapshotVersion } from '../core/snapshot_version'; import { canonifyTarget, Target, targetEquals } from '../core/target'; @@ -330,7 +330,7 @@ export function localStoreWriteLocally( new PatchMutation( mutation.key, baseValue, - extractFieldMask(baseValue.toProto().mapValue!), + extractFieldMask(baseValue.value.mapValue!), Precondition.exists(true) ) ); diff --git a/packages/firestore/src/local/memory_persistence.ts b/packages/firestore/src/local/memory_persistence.ts index 6437955c035..740ac5114c7 100644 --- a/packages/firestore/src/local/memory_persistence.ts +++ b/packages/firestore/src/local/memory_persistence.ts @@ -474,7 +474,7 @@ export class MemoryLruDelegate implements ReferenceDelegate, LruDelegate { documentSize(document: Document): number { let documentSize = document.key.toString().length; if (document.isFoundDocument()) { - documentSize += estimateByteSize(document.data.toProto()); + documentSize += estimateByteSize(document.data.value); } return documentSize; } diff --git a/packages/firestore/src/model/document.ts b/packages/firestore/src/model/document.ts index 951d2d9c9d6..fa9dd8af977 100644 --- a/packages/firestore/src/model/document.ts +++ b/packages/firestore/src/model/document.ts @@ -331,7 +331,7 @@ export class MutableDocument implements Document { toString(): string { return ( `Document(${this.key}, ${this.version}, ${JSON.stringify( - this.data.toProto() + this.data.value )}, ` + `{documentType: ${this.documentType}}), ` + `{documentState: ${this.documentState}})` diff --git a/packages/firestore/src/model/object_value.ts b/packages/firestore/src/model/object_value.ts index 466e9b79b93..c0eb2cfda48 100644 --- a/packages/firestore/src/model/object_value.ts +++ b/packages/firestore/src/model/object_value.ts @@ -16,17 +16,17 @@ */ import { + firestoreV1ApiClientInterfaces, MapValue as ProtoMapValue, Value as ProtoValue } from '../protos/firestore_proto_api'; import { debugAssert } from '../util/assert'; import { forEach } from '../util/obj'; - import { FieldMask } from './field_mask'; import { FieldPath } from './path'; import { isServerTimestamp } from './server_timestamps'; -import { TypeOrder } from './type_order'; -import { deepClone, isMapValue, typeOrder, valueEquals } from './values'; +import { deepClone, isMapValue, valueEquals } from './values'; +import Value = firestoreV1ApiClientInterfaces.Value; export interface JsonObject { [name: string]: T; @@ -36,7 +36,7 @@ export interface JsonObject { * ability to add and remove fields (via the ObjectValueBuilder). */ export class ObjectValue { - constructor(private value: { mapValue: ProtoMapValue }) { + constructor(readonly value: { mapValue: ProtoMapValue }) { debugAssert( !isServerTimestamp(value), 'ServerTimestamps should be converted to ServerTimestampValue' @@ -73,11 +73,6 @@ export class ObjectValue { } } - /** Returns the full protobuf representation. */ - toProto(): { mapValue: ProtoMapValue } { - return this.value; - } - /** * Sets the field to the provided value. * @@ -89,12 +84,8 @@ export class ObjectValue { !path.isEmpty(), 'Cannot set field for empty path on ObjectValue' ); - const parentMap = this.getParentMap(path.popLast()); - this.applyChanges( - parentMap, - { [path.lastSegment()]: value }, - /*deletes=*/ [] - ); + const fieldsMap = this.getFieldsMap(path.popLast()); + fieldsMap[path.lastSegment()] = value; } /** @@ -111,8 +102,8 @@ export class ObjectValue { data.forEach((value, path) => { if (!parent.isImmediateParentOf(path)) { // Insert the accumulated changes at this parent location - const parentMap = this.getParentMap(parent); - this.applyChanges(parentMap, upserts, deletes); + const fieldsMap = this.getFieldsMap(parent); + this.applyChanges(fieldsMap, upserts, deletes); upserts = {}; deletes = []; parent = path.popLast(); @@ -125,8 +116,8 @@ export class ObjectValue { } }); - const parentMap = this.getParentMap(parent); - this.applyChanges(parentMap, upserts, deletes); + const fieldsMap = this.getFieldsMap(parent); + this.applyChanges(fieldsMap, upserts, deletes); } /** @@ -141,10 +132,8 @@ export class ObjectValue { 'Cannot delete field for empty path on ObjectValue' ); const nestedValue = this.field(path.popLast()); - if (nestedValue && nestedValue.mapValue) { - this.applyChanges(nestedValue.mapValue, /*upserts=*/ {}, [ - path.lastSegment() - ]); + if (isMapValue(nestedValue) && nestedValue.mapValue.fields) { + delete nestedValue.mapValue.fields[path.lastSegment()]; } } @@ -156,25 +145,24 @@ export class ObjectValue { * Returns the map that contains the leaf element of `path`. If the parent * entry does not yet exist, or if it is not a map, a new map will be created. */ - private getParentMap(path: FieldPath): ProtoMapValue { - let parent = this.value; - - for (let i = 0; i < path.length; ++i) { - const segment = path.get(i); + private getFieldsMap(path: FieldPath): Record { + let current = this.value; - if (!parent.mapValue.fields) { - parent.mapValue.fields = {}; - } + if (!current.mapValue!.fields) { + current.mapValue = { fields: {} }; + } - if (!isMapValue(parent.mapValue.fields[segment])) { - // Since the element is not a map value, remove all existing data and - // change it to a map type. - parent.mapValue.fields[segment] = { mapValue: {} }; + for (let i = 0; i < path.length; ++i) { + let next = + current.mapValue!.fields && current.mapValue!.fields![path.get(i)]; + if (!isMapValue(next) || !next.mapValue.fields) { + next = { mapValue: { fields: {} } }; + current.mapValue!.fields![path.get(i)] = next; } - parent = parent.mapValue.fields[segment] as { mapValue: ProtoMapValue }; + current = next as { mapValue: ProtoMapValue }; } - return parent.mapValue; + return current.mapValue!.fields!; } /** @@ -182,16 +170,13 @@ export class ObjectValue { * entries. */ private applyChanges( - parentMap: ProtoMapValue, + fieldsMap: Record, inserts: { [key: string]: ProtoValue }, deletes: string[] ): void { - if (!parentMap.fields) { - parentMap.fields = {}; - } - forEach(inserts, (key, val) => (parentMap.fields![key] = val)); + forEach(inserts, (key, val) => (fieldsMap[key] = val)); for (const field of deletes) { - delete parentMap.fields[field]; + delete fieldsMap[field]; } } diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 06045a21900..7f4b71e8f2b 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -379,7 +379,7 @@ export function toMutationDocument( ): ProtoDocument { return { name: toName(serializer, key), - fields: fields.toProto().mapValue.fields + fields: fields.value.mapValue.fields }; } @@ -393,7 +393,7 @@ export function toDocument( ); return { name: toName(serializer, document.key), - fields: document.data.toProto().mapValue.fields, + fields: document.data.value.mapValue.fields, updateTime: toTimestamp(serializer, document.version.toTimestamp()) }; } diff --git a/packages/firestore/test/unit/model/document.test.ts b/packages/firestore/test/unit/model/document.test.ts index 431f77d0ca2..cfb93d15e6f 100644 --- a/packages/firestore/test/unit/model/document.test.ts +++ b/packages/firestore/test/unit/model/document.test.ts @@ -34,7 +34,7 @@ describe('Document', () => { const document = doc('rooms/Eros', 1, data); const value = document.data; - expect(value.toProto()).to.deep.equal( + expect(value.value).to.deep.equal( wrap({ desc: 'Discuss all the project related stuff', owner: 'Jonny' diff --git a/packages/firestore/test/unit/model/object_value.test.ts b/packages/firestore/test/unit/model/object_value.test.ts index 32e1036cb22..d2f3140a191 100644 --- a/packages/firestore/test/unit/model/object_value.test.ts +++ b/packages/firestore/test/unit/model/object_value.test.ts @@ -170,7 +170,7 @@ describe('ObjectValue', () => { 'map.nested.d', 'emptymap' ); - const actualMask = extractFieldMask(objValue.toProto().mapValue); + const actualMask = extractFieldMask(objValue.value.mapValue); expect(actualMask.isEqual(expectedMask)).to.be.true; }); diff --git a/packages/firestore/test/unit/remote/serializer.helper.ts b/packages/firestore/test/unit/remote/serializer.helper.ts index 56ccfd9aefc..f06509fb7fd 100644 --- a/packages/firestore/test/unit/remote/serializer.helper.ts +++ b/packages/firestore/test/unit/remote/serializer.helper.ts @@ -797,7 +797,7 @@ export function serializerTest( const d = doc('foo/bar', 42, { a: 5, b: 'b' }); const proto = { name: toName(s, d.key), - fields: d.data.toProto().mapValue.fields, + fields: d.data.value.mapValue.fields, updateTime: toVersion(s, d.version) }; const serialized = toDocument(s, d); diff --git a/packages/firestore/test/unit/specs/bundle_spec.test.ts b/packages/firestore/test/unit/specs/bundle_spec.test.ts index 5d0ac79eb3c..3efa1be4d63 100644 --- a/packages/firestore/test/unit/specs/bundle_spec.test.ts +++ b/packages/firestore/test/unit/specs/bundle_spec.test.ts @@ -22,8 +22,8 @@ import { LimitType } from '../../../src/protos/firestore_bundle_proto'; import { toVersion } from '../../../src/remote/serializer'; import { doc, - query, filter, + query, TestSnapshotVersion, version, wrapObject @@ -76,7 +76,7 @@ function bundleWithDocumentAndQuery( testDoc.key, toVersion(JSON_SERIALIZER, version(testDoc.createTime)), toVersion(JSON_SERIALIZER, version(testDoc.updateTime!)), - wrapObject(testDoc.content!).toProto().mapValue.fields! + wrapObject(testDoc.content!).value.mapValue.fields! ); } return builder.build( diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index 26993992107..9bba02260f3 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -17,12 +17,12 @@ import { UserDataWriter } from '../../../src/api/database'; import { + hasLimitToFirst, + hasLimitToLast, + newQueryForPath, Query, queryEquals, - newQueryForPath, - queryToTarget, - hasLimitToLast, - hasLimitToFirst + queryToTarget } from '../../../src/core/query'; import { canonifyTarget, @@ -1044,7 +1044,7 @@ export class SpecBuilder { key: SpecBuilder.keyToSpec(doc.key), version: doc.version.toMicroseconds(), value: userDataWriter.convertValue( - doc.data.toProto() + doc.data.value ) as JsonObject, options: { hasLocalMutations: doc.hasLocalMutations, diff --git a/packages/firestore/test/util/spec_test_helpers.ts b/packages/firestore/test/util/spec_test_helpers.ts index 389112f3fd6..c721d0e0675 100644 --- a/packages/firestore/test/util/spec_test_helpers.ts +++ b/packages/firestore/test/util/spec_test_helpers.ts @@ -56,7 +56,7 @@ export function encodeWatchChange( documentChange: { document: { name: toName(serializer, doc.key), - fields: doc?.data?.toProto().mapValue.fields, + fields: doc?.data.value.mapValue.fields, updateTime: toVersion(serializer, doc.version) }, targetIds: watchChange.updatedTargetIds, From d2945f920f4517fc153f06c8d4bc51cb52b25576 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 24 May 2021 09:56:13 -0600 Subject: [PATCH 06/11] Lint --- packages/firestore/src/model/object_value.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/model/object_value.ts b/packages/firestore/src/model/object_value.ts index c0eb2cfda48..272f49c7606 100644 --- a/packages/firestore/src/model/object_value.ts +++ b/packages/firestore/src/model/object_value.ts @@ -22,11 +22,11 @@ import { } from '../protos/firestore_proto_api'; import { debugAssert } from '../util/assert'; import { forEach } from '../util/obj'; + import { FieldMask } from './field_mask'; import { FieldPath } from './path'; import { isServerTimestamp } from './server_timestamps'; import { deepClone, isMapValue, valueEquals } from './values'; -import Value = firestoreV1ApiClientInterfaces.Value; export interface JsonObject { [name: string]: T; From 25bfef4d89f54b0deb8a7ca9f0315a6002ffedd5 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 24 May 2021 10:01:05 -0600 Subject: [PATCH 07/11] Even less code --- packages/firestore/src/local/local_store_impl.ts | 2 +- packages/firestore/src/model/object_value.ts | 11 +++-------- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index aa83ac17f74..ba5782753d6 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -330,7 +330,7 @@ export function localStoreWriteLocally( new PatchMutation( mutation.key, baseValue, - extractFieldMask(baseValue.value.mapValue!), + extractFieldMask(baseValue.value.mapValue), Precondition.exists(true) ) ); diff --git a/packages/firestore/src/model/object_value.ts b/packages/firestore/src/model/object_value.ts index 272f49c7606..ac144829132 100644 --- a/packages/firestore/src/model/object_value.ts +++ b/packages/firestore/src/model/object_value.ts @@ -59,16 +59,12 @@ export class ObjectValue { } else { let currentLevel: ProtoValue = this.value; for (let i = 0; i < path.length - 1; ++i) { - if (!currentLevel.mapValue!.fields) { - return null; - } - currentLevel = currentLevel.mapValue!.fields[path.get(i)]; + currentLevel = (currentLevel.mapValue!.fields || {})[path.get(i)]; if (!isMapValue(currentLevel)) { return null; } } - - currentLevel = (currentLevel.mapValue!.fields || {})[path.lastSegment()]; + currentLevel = (currentLevel.mapValue!.fields! || {})[path.lastSegment()]; return currentLevel || null; } } @@ -153,8 +149,7 @@ export class ObjectValue { } for (let i = 0; i < path.length; ++i) { - let next = - current.mapValue!.fields && current.mapValue!.fields![path.get(i)]; + let next = current.mapValue!.fields![path.get(i)]; if (!isMapValue(next) || !next.mapValue.fields) { next = { mapValue: { fields: {} } }; current.mapValue!.fields![path.get(i)] = next; From 24240d1889154468822e8d033d453044a187b6ad Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 24 May 2021 10:21:08 -0600 Subject: [PATCH 08/11] Lint (again) --- packages/firestore/src/model/object_value.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/firestore/src/model/object_value.ts b/packages/firestore/src/model/object_value.ts index ac144829132..34fcccab215 100644 --- a/packages/firestore/src/model/object_value.ts +++ b/packages/firestore/src/model/object_value.ts @@ -16,7 +16,6 @@ */ import { - firestoreV1ApiClientInterfaces, MapValue as ProtoMapValue, Value as ProtoValue } from '../protos/firestore_proto_api'; From a5faa21726151834c309ab0b267eb122dfe2383e Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 24 May 2021 11:22:17 -0600 Subject: [PATCH 09/11] Clone --- packages/firestore/src/model/object_value.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/firestore/src/model/object_value.ts b/packages/firestore/src/model/object_value.ts index 34fcccab215..7c87cdc98a1 100644 --- a/packages/firestore/src/model/object_value.ts +++ b/packages/firestore/src/model/object_value.ts @@ -80,7 +80,7 @@ export class ObjectValue { 'Cannot set field for empty path on ObjectValue' ); const fieldsMap = this.getFieldsMap(path.popLast()); - fieldsMap[path.lastSegment()] = value; + fieldsMap[path.lastSegment()] = deepClone(value); } /** @@ -105,7 +105,7 @@ export class ObjectValue { } if (value) { - upserts[path.lastSegment()] = value; + upserts[path.lastSegment()] = deepClone(value); } else { deletes.push(path.lastSegment()); } From 4f4eeba71271a6fa1827887b865b0fc7cc7771bb Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 25 May 2021 09:59:16 -0600 Subject: [PATCH 10/11] Update object_value.ts --- packages/firestore/src/model/object_value.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/model/object_value.ts b/packages/firestore/src/model/object_value.ts index 7c87cdc98a1..b63f2677e02 100644 --- a/packages/firestore/src/model/object_value.ts +++ b/packages/firestore/src/model/object_value.ts @@ -160,7 +160,7 @@ export class ObjectValue { } /** - * Modifies `parent_map` by adding, replacing or deleting the specified + * Modifies `parentMap` by adding, replacing or deleting the specified * entries. */ private applyChanges( From c8a3f69ff7ddbd4885d5445f0bdb32269cf3ddcb Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 25 May 2021 09:59:41 -0600 Subject: [PATCH 11/11] Update object_value.ts --- packages/firestore/src/model/object_value.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/model/object_value.ts b/packages/firestore/src/model/object_value.ts index b63f2677e02..d5cb273eb9d 100644 --- a/packages/firestore/src/model/object_value.ts +++ b/packages/firestore/src/model/object_value.ts @@ -160,7 +160,7 @@ export class ObjectValue { } /** - * Modifies `parentMap` by adding, replacing or deleting the specified + * Modifies `fieldsMap` by adding, replacing or deleting the specified * entries. */ private applyChanges(