diff --git a/packages/firestore/src/api/user_data_reader.ts b/packages/firestore/src/api/user_data_reader.ts index e7d78bb76a3..3d53204b954 100644 --- a/packages/firestore/src/api/user_data_reader.ts +++ b/packages/firestore/src/api/user_data_reader.ts @@ -44,7 +44,6 @@ import { ServerTimestampTransform } from '../model/transform_operation'; import { JsonProtoSerializer } from '../remote/serializer'; -import { SortedSet } from '../util/sorted_set'; import { Blob } from './blob'; import { FieldPath as ExternalFieldPath, @@ -337,10 +336,10 @@ export class UserDataReader { let fieldTransforms: FieldTransform[]; if (!fieldPaths) { - fieldMask = FieldMask.fromArray(context.fieldMask); + fieldMask = new FieldMask(context.fieldMask); fieldTransforms = context.fieldTransforms; } else { - let validatedFieldPaths = new SortedSet(FieldPath.comparator); + const validatedFieldPaths: FieldPath[] = []; for (const stringOrFieldPath of fieldPaths) { let fieldPath: FieldPath; @@ -365,10 +364,12 @@ export class UserDataReader { ); } - validatedFieldPaths = validatedFieldPaths.add(fieldPath); + if (!fieldMaskContains(validatedFieldPaths, fieldPath)) { + validatedFieldPaths.push(fieldPath); + } } - fieldMask = FieldMask.fromSet(validatedFieldPaths); + fieldMask = new FieldMask(validatedFieldPaths); fieldTransforms = context.fieldTransforms.filter(transform => fieldMask.covers(transform.field) ); @@ -389,7 +390,7 @@ export class UserDataReader { ); validatePlainObject('Data must be an object, but it was:', context, input); - let fieldMaskPaths = new SortedSet(FieldPath.comparator); + const fieldMaskPaths: FieldPath[] = []; const updateData = new ObjectValueBuilder(); forEach(input as Dict, (key, value) => { const path = fieldPathFromDotSeparatedString(methodName, key); @@ -398,17 +399,17 @@ export class UserDataReader { value = this.runPreConverter(value, childContext); if (value instanceof DeleteFieldValueImpl) { // Add it to the field mask, but don't add anything to updateData. - fieldMaskPaths = fieldMaskPaths.add(path); + fieldMaskPaths.push(path); } else { const parsedValue = this.parseData(value, childContext); if (parsedValue != null) { - fieldMaskPaths = fieldMaskPaths.add(path); + fieldMaskPaths.push(path); updateData.set(path, parsedValue); } } }); - const mask = FieldMask.fromSet(fieldMaskPaths); + const mask = new FieldMask(fieldMaskPaths); return new ParsedUpdateData( updateData.build(), mask, @@ -449,26 +450,30 @@ export class UserDataReader { values.push(moreFieldsAndValues[i + 1]); } - let fieldMaskPaths = new SortedSet(FieldPath.comparator); + const fieldMaskPaths: FieldPath[] = []; const updateData = new ObjectValueBuilder(); - for (let i = 0; i < keys.length; ++i) { - const path = keys[i]; - const childContext = context.childContextForFieldPath(path); - const value = this.runPreConverter(values[i], childContext); - if (value instanceof DeleteFieldValueImpl) { - // Add it to the field mask, but don't add anything to updateData. - fieldMaskPaths = fieldMaskPaths.add(path); - } else { - const parsedValue = this.parseData(value, childContext); - if (parsedValue != null) { - fieldMaskPaths = fieldMaskPaths.add(path); - updateData.set(path, parsedValue); + // We iterate in reverse order to pick the last value for a field if the + // user specified the field multiple times. + for (let i = keys.length - 1; i >= 0; --i) { + if (!fieldMaskContains(fieldMaskPaths, keys[i])) { + const path = keys[i]; + const childContext = context.childContextForFieldPath(path); + const value = this.runPreConverter(values[i], childContext); + if (value instanceof DeleteFieldValueImpl) { + // Add it to the field mask, but don't add anything to updateData. + fieldMaskPaths.push(path); + } else { + const parsedValue = this.parseData(value, childContext); + if (parsedValue != null) { + fieldMaskPaths.push(path); + updateData.set(path, parsedValue); + } } } } - const mask = FieldMask.fromSet(fieldMaskPaths); + const mask = new FieldMask(fieldMaskPaths); return new ParsedUpdateData( updateData.build(), mask, @@ -841,3 +846,8 @@ function fieldPathFromDotSeparatedString( function errorMessage(error: Error | object): string { return error instanceof Error ? error.message : error.toString(); } + +/** Checks `haystack` if FieldPath `needle` is present. Runs in O(n). */ +function fieldMaskContains(haystack: FieldPath[], needle: FieldPath): boolean { + return haystack.some(v => v.isEqual(needle)); +} diff --git a/packages/firestore/src/model/mutation.ts b/packages/firestore/src/model/mutation.ts index 8547235fdea..9777ba25229 100644 --- a/packages/firestore/src/model/mutation.ts +++ b/packages/firestore/src/model/mutation.ts @@ -20,7 +20,6 @@ import * as api from '../protos/firestore_proto_api'; import { Timestamp } from '../api/timestamp'; import { SnapshotVersion } from '../core/snapshot_version'; import { debugAssert, fail, hardAssert } from '../util/assert'; -import { SortedSet } from '../util/sorted_set'; import { Document, @@ -45,18 +44,15 @@ import { arrayEquals } from '../util/misc'; * containing foo */ export class FieldMask { - constructor(readonly fields: SortedSet) { + constructor(readonly fields: FieldPath[]) { // TODO(dimond): validation of FieldMask - } - - static fromSet(fields: SortedSet): FieldMask { - return new FieldMask(fields); - } - - static fromArray(fields: FieldPath[]): FieldMask { - let fieldsAsSet = new SortedSet(FieldPath.comparator); - fields.forEach(fieldPath => (fieldsAsSet = fieldsAsSet.add(fieldPath))); - return new FieldMask(fieldsAsSet); + // Sort the field mask to support `FieldMask.isEqual()` and assert below. + fields.sort(FieldPath.comparator); + debugAssert( + !fields.some((v, i) => i !== 0 && v.isEqual(fields[i - 1])), + 'FieldMask contains field that is not unique: ' + + fields.find((v, i) => i !== 0 && v.isEqual(fields[i - 1]))! + ); } /** @@ -66,17 +62,16 @@ export class FieldMask { * This is an O(n) operation, where `n` is the size of the field mask. */ covers(fieldPath: FieldPath): boolean { - let found = false; - this.fields.forEach(fieldMaskPath => { + for (const fieldMaskPath of this.fields) { if (fieldMaskPath.isPrefixOf(fieldPath)) { - found = true; + return true; } - }); - return found; + } + return false; } isEqual(other: FieldMask): boolean { - return this.fields.isEqual(other.fields); + return arrayEquals(this.fields, other.fields, (l, r) => l.isEqual(r)); } } diff --git a/packages/firestore/src/model/object_value.ts b/packages/firestore/src/model/object_value.ts index ed91379a7c3..3e3c6fa94c9 100644 --- a/packages/firestore/src/model/object_value.ts +++ b/packages/firestore/src/model/object_value.ts @@ -23,7 +23,6 @@ import { FieldPath } from './path'; import { isServerTimestamp } from './server_timestamps'; import { valueEquals, isMapValue, typeOrder } from './values'; import { forEach } from '../util/obj'; -import { SortedSet } from '../util/sorted_set'; export interface JsonObject { [name: string]: T; @@ -243,27 +242,27 @@ export class ObjectValueBuilder { * Returns a FieldMask built from all fields in a MapValue. */ export function extractFieldMask(value: api.MapValue): FieldMask { - let fields = new SortedSet(FieldPath.comparator); + const fields: FieldPath[] = []; 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()) { + if (nestedFields.length === 0) { // Preserve the empty map by adding it to the FieldMask. - fields = fields.add(currentPath); + fields.push(currentPath); } else { // For nested and non-empty ObjectValues, add the FieldPath of the // leaf nodes. - nestedFields.forEach(nestedPath => { - fields = fields.add(currentPath.child(nestedPath)); - }); + for (const nestedPath of nestedFields) { + fields.push(currentPath.child(nestedPath)); + } } } else { // For nested and non-empty ObjectValues, add the FieldPath of the leaf // nodes. - fields = fields.add(currentPath); + fields.push(currentPath); } }); - return FieldMask.fromSet(fields); + return new FieldMask(fields); } diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 6cbb61258f3..de29942ffc2 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -1086,8 +1086,7 @@ export class JsonProtoSerializer { fromDocumentMask(proto: api.DocumentMask): FieldMask { const paths = proto.fieldPaths || []; - const fields = paths.map(path => FieldPath.fromServerFormat(path)); - return FieldMask.fromArray(fields); + return new FieldMask(paths.map(path => FieldPath.fromServerFormat(path))); } } diff --git a/packages/firestore/test/integration/api/database.test.ts b/packages/firestore/test/integration/api/database.test.ts index 7da9bb8af7c..4db08e29634 100644 --- a/packages/firestore/test/integration/api/database.test.ts +++ b/packages/firestore/test/integration/api/database.test.ts @@ -531,6 +531,18 @@ apiDescribe('Database', (persistence: boolean) => { }); }); + it('can specify updated field multiple times', () => { + return withTestDoc(persistence, doc => { + return doc + .set({}) + .then(() => doc.update('field', 100, new FieldPath('field'), 200)) + .then(() => doc.get()) + .then(docSnap => { + expect(docSnap.data()).to.deep.equal({ field: 200 }); + }); + }); + }); + describe('documents: ', () => { const invalidDocValues = [undefined, null, 0, 'foo', ['a'], new Date()]; for (const val of invalidDocValues) { diff --git a/packages/firestore/test/unit/remote/serializer.helper.ts b/packages/firestore/test/unit/remote/serializer.helper.ts index bd7ea39326c..391909058a4 100644 --- a/packages/firestore/test/unit/remote/serializer.helper.ts +++ b/packages/firestore/test/unit/remote/serializer.helper.ts @@ -540,7 +540,7 @@ export function serializerTest( const expected: api.DocumentMask = { fieldPaths: ['foo.`bar.baz\\qux`'] }; - const mask = FieldMask.fromArray([ + const mask = new FieldMask([ FieldPath.fromServerFormat('foo.bar\\.baz\\\\qux') ]); const actual = s.toDocumentMask(mask); @@ -554,7 +554,7 @@ export function serializerTest( // TODO(b/34988481): Implement correct escaping // eslint-disable-next-line no-restricted-properties it.skip('converts a weird path', () => { - const expected = FieldMask.fromArray([ + const expected = new FieldMask([ FieldPath.fromServerFormat('foo.bar\\.baz\\\\qux') ]); const proto: api.DocumentMask = { fieldPaths: ['foo.`bar.baz\\qux`'] }; diff --git a/packages/firestore/test/unit/util/misc.test.ts b/packages/firestore/test/unit/util/misc.test.ts index 6627d9c6110..ab59f29393d 100644 --- a/packages/firestore/test/unit/util/misc.test.ts +++ b/packages/firestore/test/unit/util/misc.test.ts @@ -18,6 +18,7 @@ import { expect } from 'chai'; import { immediateSuccessor } from '../../../src/util/misc'; import { debugCast } from '../../../src/util/assert'; +import { mask } from '../../util/helpers'; describe('immediateSuccessor', () => { it('generates the correct immediate successors', () => { @@ -43,3 +44,11 @@ describe('typeCast', () => { ); }); }); + +describe('FieldMask', () => { + it('cannot contain duplicate fields', () => { + expect(() => mask('a', 'b', 'a')).to.throw( + 'FieldMask contains field that is not unique: a' + ); + }); +}); diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index 577cc296d61..ec8bb419ff9 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -212,11 +212,7 @@ export function field(path: string): FieldPath { } export function mask(...paths: string[]): FieldMask { - let fieldPaths = new SortedSet(FieldPath.comparator); - for (const path of paths) { - fieldPaths = fieldPaths.add(field(path)); - } - return FieldMask.fromSet(fieldPaths); + return new FieldMask(paths.map(v => field(v))); } export function blob(...bytes: number[]): Blob {