From df2595112dc1217a0e65b98f4d865053ff640e3f Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 29 Apr 2020 21:10:59 -0700 Subject: [PATCH 1/4] Untangle FieldValues --- packages/firestore/index.console.ts | 4 +- packages/firestore/src/api/database.ts | 35 +- packages/firestore/src/api/field_value.ts | 193 ++++-- .../firestore/src/api/user_data_reader.ts | 596 ++++++++---------- packages/firestore/src/platform/config.ts | 4 +- packages/firestore/src/remote/datastore.ts | 6 +- .../test/unit/api/field_value.test.ts | 2 +- .../firestore/test/unit/core/query.test.ts | 9 +- .../test/unit/local/local_store.test.ts | 2 +- .../test/unit/model/mutation.test.ts | 5 +- .../firestore/test/unit/model/values.test.ts | 4 +- .../test/unit/remote/serializer.helper.ts | 62 +- packages/firestore/test/util/helpers.ts | 62 +- 13 files changed, 475 insertions(+), 509 deletions(-) diff --git a/packages/firestore/index.console.ts b/packages/firestore/index.console.ts index 10b77517d1c..918e60fe8a5 100644 --- a/packages/firestore/index.console.ts +++ b/packages/firestore/index.console.ts @@ -23,11 +23,11 @@ export { PublicCollectionReference as CollectionReference, PublicDocumentReference as DocumentReference, PublicDocumentSnapshot as DocumentSnapshot, - PublicQuerySnapshot as QuerySnapshot + PublicQuerySnapshot as QuerySnapshot, + PublicFieldValue as FieldValue } from './src/api/database'; export { GeoPoint } from './src/api/geo_point'; export { PublicBlob as Blob } from './src/api/blob'; export { FirstPartyCredentialsSettings } from './src/api/credentials'; -export { PublicFieldValue as FieldValue } from './src/api/field_value'; export { FieldPath } from './src/api/field_path'; export { Timestamp } from './src/api/timestamp'; diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 89356b9d82c..a5e38ca87ec 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -89,14 +89,11 @@ import { PartialObserver, Unsubscribe } from './observer'; -import { - DocumentKeyReference, - fieldPathFromArgument, - UserDataReader -} from './user_data_reader'; +import { fieldPathFromArgument, UserDataReader } from './user_data_reader'; import { UserDataWriter } from './user_data_writer'; import { FirebaseAuthInternalName } from '@firebase/auth-interop-types'; import { Provider } from '@firebase/component'; +import { FieldValue } from './field_value'; // settings() defaults: const DEFAULT_HOST = 'firestore.googleapis.com'; @@ -315,7 +312,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService { this._componentProvider = componentProvider; this._settings = new FirestoreSettings({}); - this._dataReader = this.createDataReader(this._databaseId); + this._dataReader = new UserDataReader(this._databaseId); } settings(settingsLiteral: firestore.Settings): void { @@ -499,28 +496,6 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService { return this._firestoreClient.start(componentProvider, persistenceSettings); } - private createDataReader(databaseId: DatabaseId): UserDataReader { - const preConverter = (value: unknown): unknown => { - if (value instanceof DocumentReference) { - const thisDb = databaseId; - const otherDb = value.firestore._databaseId; - if (!otherDb.isEqual(thisDb)) { - throw new FirestoreError( - Code.INVALID_ARGUMENT, - 'Document reference is for database ' + - `${otherDb.projectId}/${otherDb.database} but should be ` + - `for database ${thisDb.projectId}/${thisDb.database}` - ); - } - return new DocumentKeyReference(databaseId, value._key); - } else { - return value; - } - }; - const serializer = PlatformSupport.getPlatform().newSerializer(databaseId); - return new UserDataReader(serializer, preConverter); - } - private static databaseIdFromApp(app: FirebaseApp): DatabaseId { if (!contains(app.options, 'projectId')) { throw new FirestoreError( @@ -2586,3 +2561,7 @@ export const PublicCollectionReference = makeConstructorPrivate( CollectionReference, 'Use firebase.firestore().collection() instead.' ); +export const PublicFieldValue = makeConstructorPrivate( + FieldValue, + 'Use FieldValue.() instead.' +); diff --git a/packages/firestore/src/api/field_value.ts b/packages/firestore/src/api/field_value.ts index 06032b11282..8823e09e6b0 100644 --- a/packages/firestore/src/api/field_value.ts +++ b/packages/firestore/src/api/field_value.ts @@ -16,99 +16,188 @@ */ import * as firestore from '@firebase/firestore-types'; - -import { makeConstructorPrivate } from '../util/api'; import { validateArgType, validateAtLeastNumberOfArgs, validateExactNumberOfArgs, validateNoArgs } from '../util/input_validation'; +import { FieldTransform } from '../model/mutation'; +import { + ArrayRemoveTransformOperation, + ArrayUnionTransformOperation, + NumericIncrementTransformOperation, + ServerTimestampTransform +} from '../model/transform_operation'; +import { ParseContext, parseData, UserDataSource } from './user_data_reader'; +import { debugAssert } from '../util/assert'; /** * An opaque base class for FieldValue sentinel objects in our public API, * with public static methods for creating said sentinel objects. */ -export abstract class FieldValueImpl implements firestore.FieldValue { +export abstract class FieldValueImpl { protected constructor(readonly _methodName: string) {} - static delete(): FieldValueImpl { - validateNoArgs('FieldValue.delete', arguments); - return DeleteFieldValueImpl.instance; - } + abstract toFieldTransform(context: ParseContext): FieldTransform | null; - static serverTimestamp(): FieldValueImpl { - validateNoArgs('FieldValue.serverTimestamp', arguments); - return ServerTimestampFieldValueImpl.instance; - } + abstract isEqual(other: FieldValue): boolean; +} - static arrayUnion(...elements: unknown[]): FieldValueImpl { - validateAtLeastNumberOfArgs('FieldValue.arrayUnion', arguments, 1); - // NOTE: We don't actually parse the data until it's used in set() or - // update() since we need access to the Firestore instance. - return new ArrayUnionFieldValueImpl(elements); +export class DeleteFieldValueImpl extends FieldValueImpl { + constructor() { + super('FieldValue.delete'); } - static arrayRemove(...elements: unknown[]): FieldValueImpl { - validateAtLeastNumberOfArgs('FieldValue.arrayRemove', arguments, 1); - // NOTE: We don't actually parse the data until it's used in set() or - // update() since we need access to the Firestore instance. - return new ArrayRemoveFieldValueImpl(elements); + toFieldTransform(context: ParseContext): null { + if (context.dataSource === UserDataSource.MergeSet) { + // No transform to add for a delete, but we need to add it to our + // fieldMask so it gets deleted. + context.fieldMask.push(context.path!); + } else if (context.dataSource === UserDataSource.Update) { + debugAssert( + context.path!.length > 0, + 'FieldValue.delete() at the top level should have already' + + ' been handled.' + ); + throw context.createError( + 'FieldValue.delete() can only appear at the top level ' + + 'of your update data' + ); + } else { + // We shouldn't encounter delete sentinels for queries or non-merge set() calls. + throw context.createError( + 'FieldValue.delete() cannot be used with set() unless you pass ' + + '{merge:true}' + ); + } + return null; } - static increment(n: number): FieldValueImpl { - validateArgType('FieldValue.increment', 'number', 1, n); - validateExactNumberOfArgs('FieldValue.increment', arguments, 1); - return new NumericIncrementFieldValueImpl(n); + isEqual(other: FieldValue): boolean { + return other instanceof DeleteFieldValueImpl; } +} - isEqual(other: FieldValueImpl): boolean { - return this === other; +export class ServerTimestampFieldValueImpl extends FieldValueImpl { + constructor() { + super('FieldValue.serverTimestamp'); } -} -export class DeleteFieldValueImpl extends FieldValueImpl { - private constructor() { - super('FieldValue.delete'); + toFieldTransform(context: ParseContext): FieldTransform { + return new FieldTransform(context.path!, ServerTimestampTransform.instance); } - /** Singleton instance. */ - static instance = new DeleteFieldValueImpl(); -} -export class ServerTimestampFieldValueImpl extends FieldValueImpl { - private constructor() { - super('FieldValue.serverTimestamp'); + isEqual(other: FieldValue): boolean { + return other instanceof ServerTimestampFieldValueImpl; } - /** Singleton instance. */ - static instance = new ServerTimestampFieldValueImpl(); } export class ArrayUnionFieldValueImpl extends FieldValueImpl { - constructor(readonly _elements: unknown[]) { + constructor(private readonly _elements: unknown[]) { super('FieldValue.arrayUnion'); } + + toFieldTransform(context: ParseContext): FieldTransform { + // Although array transforms are used with writes, the actual elements + // being uniomed or removed are not considered writes since they cannot + // contain any FieldValue sentinels, etc. + const parseContext = context.contextWith({ + dataSource: UserDataSource.Argument, + methodName: this._methodName + }); + const parsedElements = this._elements.map( + (element, i) => parseData(element, parseContext.childContextForArray(i))! + ); + const arrayUnion = new ArrayUnionTransformOperation(parsedElements); + return new FieldTransform(context.path!, arrayUnion); + } + + isEqual(other: FieldValue): boolean { + // TODO(mrschmidt): Implement isEquals + return false; + } } export class ArrayRemoveFieldValueImpl extends FieldValueImpl { constructor(readonly _elements: unknown[]) { super('FieldValue.arrayRemove'); } + + toFieldTransform(context: ParseContext): FieldTransform { + // Although array transforms are used with writes, the actual elements + // being unioned or removed are not considered writes since they cannot + // contain any FieldValue sentinels, etc. + const parseContext = context.contextWith({ + dataSource: UserDataSource.Argument, + methodName: this._methodName + }); + const parsedElements = this._elements.map( + (element, i) => parseData(element, parseContext.childContextForArray(i))! + ); + const arrayUnion = new ArrayRemoveTransformOperation(parsedElements); + return new FieldTransform(context.path!, arrayUnion); + } + + isEqual(other: FieldValue): boolean { + // TODO(mrschmidt): Implement isEquals + return false; + } } export class NumericIncrementFieldValueImpl extends FieldValueImpl { - constructor(readonly _operand: number) { + constructor(private readonly _operand: number) { super('FieldValue.increment'); } + + toFieldTransform(context: ParseContext): FieldTransform { + context.contextWith({ methodName: this._methodName }); + const operand = parseData(this._operand, context)!; + const numericIncrement = new NumericIncrementTransformOperation( + context.serializer, + operand + ); + return new FieldTransform(context.path!, numericIncrement); + } + + isEqual(other: FieldValue): boolean { + // TODO(mrschmidt): Implement isEquals + return false; + } } -// Public instance that disallows construction at runtime. This constructor is -// used when exporting FieldValueImpl on firebase.firestore.FieldValue and will -// be called FieldValue publicly. Internally we still use FieldValueImpl which -// has a type-checked private constructor. Note that FieldValueImpl and -// PublicFieldValue can be used interchangeably in instanceof checks. -// For our internal TypeScript code PublicFieldValue doesn't exist as a type, -// and so we need to use FieldValueImpl as type and export it too. -export const PublicFieldValue = makeConstructorPrivate( - FieldValueImpl, - 'Use FieldValue.() instead.' -); +export class FieldValue implements firestore.FieldValue { + static delete(): FieldValueImpl { + validateNoArgs('FieldValue.delete', arguments); + return new DeleteFieldValueImpl(); + } + + static serverTimestamp(): FieldValueImpl { + validateNoArgs('FieldValue.serverTimestamp', arguments); + return new ServerTimestampFieldValueImpl(); + } + + static arrayUnion(...elements: unknown[]): FieldValueImpl { + validateAtLeastNumberOfArgs('FieldValue.arrayUnion', arguments, 1); + // NOTE: We don't actually parse the data until it's used in set() or + // update() since we need access to the Firestore instance. + return new ArrayUnionFieldValueImpl(elements); + } + + static arrayRemove(...elements: unknown[]): FieldValueImpl { + validateAtLeastNumberOfArgs('FieldValue.arrayRemove', arguments, 1); + // NOTE: We don't actually parse the data until it's used in set() or + // update() since we need access to the Firestore instance. + return new ArrayRemoveFieldValueImpl(elements); + } + + static increment(n: number): FieldValueImpl { + validateArgType('FieldValue.increment', 'number', 1, n); + validateExactNumberOfArgs('FieldValue.increment', arguments, 1); + return new NumericIncrementFieldValueImpl(n); + } + + isEqual(other: FieldValue): boolean { + return this === other; + } +} diff --git a/packages/firestore/src/api/user_data_reader.ts b/packages/firestore/src/api/user_data_reader.ts index e7d78bb76a3..7ce262887ee 100644 --- a/packages/firestore/src/api/user_data_reader.ts +++ b/packages/firestore/src/api/user_data_reader.ts @@ -37,12 +37,6 @@ import { Code, FirestoreError } from '../util/error'; import { isPlainObject, valueDescription } from '../util/input_validation'; import { Dict, forEach, isEmpty } from '../util/obj'; import { ObjectValue, ObjectValueBuilder } from '../model/object_value'; -import { - ArrayRemoveTransformOperation, - ArrayUnionTransformOperation, - NumericIncrementTransformOperation, - ServerTimestampTransform -} from '../model/transform_operation'; import { JsonProtoSerializer } from '../remote/serializer'; import { SortedSet } from '../util/sorted_set'; import { Blob } from './blob'; @@ -50,15 +44,10 @@ import { FieldPath as ExternalFieldPath, fromDotSeparatedString } from './field_path'; -import { - ArrayRemoveFieldValueImpl, - ArrayUnionFieldValueImpl, - DeleteFieldValueImpl, - FieldValueImpl, - NumericIncrementFieldValueImpl, - ServerTimestampFieldValueImpl -} from './field_value'; +import { DeleteFieldValueImpl, FieldValueImpl } from './field_value'; import { GeoPoint } from './geo_point'; +import { PlatformSupport } from '../platform/platform'; +import { DocumentReference } from './database'; const RESERVED_FIELD_REGEX = /^__.*__$/; @@ -110,7 +99,7 @@ export class ParsedUpdateData { * for determining which error conditions apply during parsing and providing * better error messages. */ -const enum UserDataSource { +export const enum UserDataSource { Set, Update, MergeSet, @@ -140,22 +129,34 @@ function isWrite(dataSource: UserDataSource): boolean { } } +interface ParseSettings { + /** Indicates what kind of API method this data came from. */ + dataSource: UserDataSource; + /** The serializer to use for user data conversion. */ + serializer: JsonProtoSerializer; + /** The database ID of the instance. */ + databaseId: DatabaseId; + /** The name of the method the user called to create the ParseContext. */ + methodName: string; + /** + * A path within the object being parsed. This could be an empty path (in + * which case the context represents the root of the data being parsed), or a + * nonempty path (indicating the context represents a nested location within + * the data). + */ + path: FieldPath | null; + /** Whether or not this context corresponds to an element of an array. */ + arrayElement: boolean; +} + /** A "context" object passed around while parsing user data. */ -class ParseContext { +export class ParseContext { readonly fieldTransforms: FieldTransform[]; readonly fieldMask: FieldPath[]; /** * Initializes a ParseContext with the given source and path. * - * @param dataSource Indicates what kind of API method this data came from. - * @param methodName The name of the method the user called to create this - * ParseContext. - * @param path A path within the object being parsed. This could be an empty - * path (in which case the context represents the root of the data being - * parsed), or a nonempty path (indicating the context represents a nested - * location within the data). - * @param arrayElement Whether or not this context corresponds to an element - * of an array. + * @param settings The settings for the parser. * @param fieldTransforms A mutable list of field transforms encountered while * parsing the data. * @param fieldMask A mutable list of field paths encountered while parsing @@ -167,10 +168,7 @@ class ParseContext { * compromised). */ constructor( - readonly dataSource: UserDataSource, - readonly methodName: string, - readonly path: FieldPath | null, - readonly arrayElement?: boolean, + private readonly settings: ParseSettings, fieldTransforms?: FieldTransform[], fieldMask?: FieldPath[] ) { @@ -179,35 +177,49 @@ class ParseContext { if (fieldTransforms === undefined) { this.validatePath(); } - this.arrayElement = arrayElement !== undefined ? arrayElement : false; this.fieldTransforms = fieldTransforms || []; this.fieldMask = fieldMask || []; } - childContextForField(field: string): ParseContext { - const childPath = this.path == null ? null : this.path.child(field); - const context = new ParseContext( - this.dataSource, - this.methodName, - childPath, - /*arrayElement=*/ false, + get path(): FieldPath | null { + return this.settings.path; + } + + get dataSource(): UserDataSource { + return this.settings.dataSource; + } + + get arrayElement(): boolean { + return this.settings.arrayElement; + } + + get serializer(): JsonProtoSerializer { + return this.settings.serializer; + } + + get databaseId(): DatabaseId { + return this.settings.databaseId; + } + + /** Returns a new context with the specified settings overwritten. */ + contextWith(configuration: Partial): ParseContext { + return new ParseContext( + { ...this.settings, ...configuration }, this.fieldTransforms, this.fieldMask ); + } + + childContextForField(field: string): ParseContext { + const childPath = this.path == null ? null : this.path.child(field); + const context = this.contextWith({ path: childPath, arrayElement: false }); context.validatePathSegment(field); return context; } childContextForFieldPath(field: FieldPath): ParseContext { const childPath = this.path == null ? null : this.path.child(field); - const context = new ParseContext( - this.dataSource, - this.methodName, - childPath, - /*arrayElement=*/ false, - this.fieldTransforms, - this.fieldMask - ); + const context = this.contextWith({ path: childPath, arrayElement: false }); context.validatePath(); return context; } @@ -215,14 +227,7 @@ class ParseContext { childContextForArray(index: number): ParseContext { // TODO(b/34871131): We don't support array paths right now; so make path // null. - return new ParseContext( - this.dataSource, - this.methodName, - /*path=*/ null, - /*arrayElement=*/ true, - this.fieldTransforms, - this.fieldMask - ); + return this.contextWith({ path: null, arrayElement: true }); } createError(reason: string): Error { @@ -232,7 +237,7 @@ class ParseContext { : ` (found in field ${this.path.toString()})`; return new FirestoreError( Code.INVALID_ARGUMENT, - `Function ${this.methodName}() called with invalid data. ` + + `Function ${this.settings.methodName}() called with invalid data. ` + reason + fieldDescription ); @@ -268,49 +273,27 @@ class ParseContext { } } } -/** - * An interface that allows arbitrary pre-converting of user data. This - * abstraction allows for, e.g.: - * * The public API to convert DocumentReference objects to DocRef objects, - * avoiding a circular dependency between user_data_converter.ts and - * database.ts - * * Tests to convert test-only sentinels (e.g. '') into types - * compatible with UserDataReader. - * - * Returns the converted value (can return back the input to act as a no-op). - * - * It can also throw an Error which will be wrapped into a friendly message. - */ -export type DataPreConverter = (input: unknown) => unknown; - -/** - * A placeholder object for DocumentReferences in this file, in order to - * avoid a circular dependency. See the comments for `DataPreConverter` for - * the full context. - */ -export class DocumentKeyReference { - constructor(public databaseId: DatabaseId, public key: DocumentKey) {} -} /** * Helper for parsing raw user input (provided via the API) into internal model * classes. */ export class UserDataReader { + private readonly serializer: JsonProtoSerializer; + constructor( - private readonly serializer: JsonProtoSerializer, - private readonly preConverter: DataPreConverter - ) {} + private readonly databaseId: DatabaseId, + serializer?: JsonProtoSerializer + ) { + this.serializer = + serializer || PlatformSupport.getPlatform().newSerializer(databaseId); + } /** Parse document data from a non-merge set() call. */ parseSetData(methodName: string, input: unknown): ParsedSetData { - const context = new ParseContext( - UserDataSource.Set, - methodName, - FieldPath.EMPTY_PATH - ); + const context = this.createContext(UserDataSource.Set, methodName); validatePlainObject('Data must be an object, but it was:', context, input); - const updateData = this.parseObject(input, context)!; + const updateData = parseObject(input, context)!; return new ParsedSetData( new ObjectValue(updateData), @@ -325,13 +308,9 @@ export class UserDataReader { input: unknown, fieldPaths?: Array ): ParsedSetData { - const context = new ParseContext( - UserDataSource.MergeSet, - methodName, - FieldPath.EMPTY_PATH - ); + const context = this.createContext(UserDataSource.MergeSet, methodName); validatePlainObject('Data must be an object, but it was:', context, input); - const updateData = this.parseObject(input, context); + const updateData = parseObject(input, context); let fieldMask: FieldMask; let fieldTransforms: FieldTransform[]; @@ -382,11 +361,7 @@ export class UserDataReader { /** Parse update data from an update() call. */ parseUpdateData(methodName: string, input: unknown): ParsedUpdateData { - const context = new ParseContext( - UserDataSource.Update, - methodName, - FieldPath.EMPTY_PATH - ); + const context = this.createContext(UserDataSource.Update, methodName); validatePlainObject('Data must be an object, but it was:', context, input); let fieldMaskPaths = new SortedSet(FieldPath.comparator); @@ -395,12 +370,11 @@ export class UserDataReader { const path = fieldPathFromDotSeparatedString(methodName, key); const childContext = context.childContextForFieldPath(path); - 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); } else { - const parsedValue = this.parseData(value, childContext); + const parsedValue = parseData(value, childContext); if (parsedValue != null) { fieldMaskPaths = fieldMaskPaths.add(path); updateData.set(path, parsedValue); @@ -423,11 +397,7 @@ export class UserDataReader { value: unknown, moreFieldsAndValues: unknown[] ): ParsedUpdateData { - const context = new ParseContext( - UserDataSource.Update, - methodName, - FieldPath.EMPTY_PATH - ); + const context = this.createContext(UserDataSource.Update, methodName); const keys = [fieldPathFromArgument(methodName, field)]; const values = [value]; @@ -454,13 +424,13 @@ export class UserDataReader { for (let i = 0; i < keys.length; ++i) { const path = keys[i]; + const value = values[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); + const parsedValue = parseData(value, childContext); if (parsedValue != null) { fieldMaskPaths = fieldMaskPaths.add(path); updateData.set(path, parsedValue); @@ -476,6 +446,21 @@ export class UserDataReader { ); } + /** Creates a new top-level parse context. */ + private createContext( + dataSource: UserDataSource, + methodName: string + ): ParseContext { + return new ParseContext({ + dataSource, + databaseId: this.databaseId, + serializer: this.serializer, + methodName, + path: FieldPath.EMPTY_PATH, + arrayElement: false + }); + } + /** * Parse a "query value" (e.g. value in a where filter or a value in a cursor * bound). @@ -488,12 +473,11 @@ export class UserDataReader { input: unknown, allowArrays = false ): api.Value { - const context = new ParseContext( + const context = this.createContext( allowArrays ? UserDataSource.ArrayArgument : UserDataSource.Argument, - methodName, - FieldPath.EMPTY_PATH + methodName ); - const parsed = this.parseData(input, context); + const parsed = parseData(input, context); debugAssert(parsed != null, 'Parsed data should not be null.'); debugAssert( context.fieldTransforms.length === 0, @@ -501,257 +485,183 @@ export class UserDataReader { ); return parsed; } +} - /** Sends data through this.preConverter, handling any thrown errors. */ - private runPreConverter(input: unknown, context: ParseContext): unknown { - try { - return this.preConverter(input); - } catch (e) { - const message = errorMessage(e); - throw context.createError(message); +/** + * Parses user data to Protobuf Values. + * + * @param input Data to be parsed. + * @param context A context object representing the current path being parsed, + * the source of the data being parsed, etc. + * @return The parsed value, or null if the value was a FieldValue sentinel + * that should not be included in the resulting parsed data. + */ +export function parseData( + input: unknown, + context: ParseContext +): api.Value | null { + if (looksLikeJsonObject(input)) { + validatePlainObject('Unsupported field value:', context, input); + return parseObject(input, context); + } else if (input instanceof FieldValueImpl) { + // FieldValues usually parse into transforms (except FieldValue.delete()) + // in which case we do not want to include this field in our parsed data + // (as doing so will overwrite the field directly prior to the transform + // trying to transform it). So we don't add this location to + // context.fieldMask and we return null as our parsing result. + parseSentinelFieldValue(input, context); + return null; + } else { + // If context.path is null we are inside an array and we don't support + // field mask paths more granular than the top-level array. + if (context.path) { + context.fieldMask.push(context.path); } - } - - /** - * Internal helper for parsing user data. - * - * @param input Data to be parsed. - * @param context A context object representing the current path being parsed, - * the source of the data being parsed, etc. - * @return The parsed value, or null if the value was a FieldValue sentinel - * that should not be included in the resulting parsed data. - */ - private parseData(input: unknown, context: ParseContext): api.Value | null { - input = this.runPreConverter(input, context); - if (looksLikeJsonObject(input)) { - validatePlainObject('Unsupported field value:', context, input); - return this.parseObject(input, context); - } else if (input instanceof FieldValueImpl) { - // FieldValues usually parse into transforms (except FieldValue.delete()) - // in which case we do not want to include this field in our parsed data - // (as doing so will overwrite the field directly prior to the transform - // trying to transform it). So we don't add this location to - // context.fieldMask and we return null as our parsing result. - this.parseSentinelFieldValue(input, context); - return null; - } else { - // If context.path is null we are inside an array and we don't support - // field mask paths more granular than the top-level array. - if (context.path) { - context.fieldMask.push(context.path); - } - if (input instanceof Array) { - // TODO(b/34871131): Include the path containing the array in the error - // message. - // In the case of IN queries, the parsed data is an array (representing - // the set of values to be included for the IN query) that may directly - // contain additional arrays (each representing an individual field - // value), so we disable this validation. - if ( - context.arrayElement && - context.dataSource !== UserDataSource.ArrayArgument - ) { - throw context.createError('Nested arrays are not supported'); - } - return this.parseArray(input as unknown[], context); - } else { - return this.parseScalarValue(input, context); + if (input instanceof Array) { + // TODO(b/34871131): Include the path containing the array in the error + // message. + // In the case of IN queries, the parsed data is an array (representing + // the set of values to be included for the IN query) that may directly + // contain additional arrays (each representing an individual field + // value), so we disable this validation. + if ( + context.arrayElement && + context.dataSource !== UserDataSource.ArrayArgument + ) { + throw context.createError('Nested arrays are not supported'); } + return parseArray(input as unknown[], context); + } else { + return parseScalarValue(input, context); } } +} - private parseObject( - obj: Dict, - context: ParseContext - ): { mapValue: api.MapValue } { - const fields: Dict = {}; - - if (isEmpty(obj)) { - // If we encounter an empty object, we explicitly add it to the update - // mask to ensure that the server creates a map entry. - if (context.path && context.path.length > 0) { - context.fieldMask.push(context.path); - } - } else { - forEach(obj, (key: string, val: unknown) => { - const parsedValue = this.parseData( - val, - context.childContextForField(key) - ); - if (parsedValue != null) { - fields[key] = parsedValue; - } - }); +function parseObject( + obj: Dict, + context: ParseContext +): { mapValue: api.MapValue } { + const fields: Dict = {}; + + if (isEmpty(obj)) { + // If we encounter an empty object, we explicitly add it to the update + // mask to ensure that the server creates a map entry. + if (context.path && context.path.length > 0) { + context.fieldMask.push(context.path); } - - return { mapValue: { fields } }; + } else { + forEach(obj, (key: string, val: unknown) => { + const parsedValue = parseData(val, context.childContextForField(key)); + if (parsedValue != null) { + fields[key] = parsedValue; + } + }); } - private parseArray(array: unknown[], context: ParseContext): api.Value { - const values: api.Value[] = []; - let entryIndex = 0; - for (const entry of array) { - let parsedEntry = this.parseData( - entry, - context.childContextForArray(entryIndex) - ); - if (parsedEntry == null) { - // Just include nulls in the array for fields being replaced with a - // sentinel. - parsedEntry = { nullValue: 'NULL_VALUE' }; - } - values.push(parsedEntry); - entryIndex++; + return { mapValue: { fields } }; +} + +function parseArray(array: unknown[], context: ParseContext): api.Value { + const values: api.Value[] = []; + let entryIndex = 0; + for (const entry of array) { + let parsedEntry = parseData( + entry, + context.childContextForArray(entryIndex) + ); + if (parsedEntry == null) { + // Just include nulls in the array for fields being replaced with a + // sentinel. + parsedEntry = { nullValue: 'NULL_VALUE' }; } - return { arrayValue: { values } }; + values.push(parsedEntry); + entryIndex++; } + return { arrayValue: { values } }; +} - /** - * "Parses" the provided FieldValueImpl, adding any necessary transforms to - * context.fieldTransforms. - */ - private parseSentinelFieldValue( - value: FieldValueImpl, - context: ParseContext - ): void { - // Sentinels are only supported with writes, and not within arrays. - if (!isWrite(context.dataSource)) { - throw context.createError( - `${value._methodName}() can only be used with update() and set()` - ); - } - if (context.path === null) { - throw context.createError( - `${value._methodName}() is not currently supported inside arrays` - ); - } +/** + * "Parses" the provided FieldValueImpl, adding any necessary transforms to + * context.fieldTransforms. + */ +function parseSentinelFieldValue( + value: FieldValueImpl, + context: ParseContext +): void { + // Sentinels are only supported with writes, and not within arrays. + if (!isWrite(context.dataSource)) { + throw context.createError( + `${value._methodName}() can only be used with update() and set()` + ); + } + if (context.path === null) { + throw context.createError( + `${value._methodName}() is not currently supported inside arrays` + ); + } - if (value instanceof DeleteFieldValueImpl) { - if (context.dataSource === UserDataSource.MergeSet) { - // No transform to add for a delete, but we need to add it to our - // fieldMask so it gets deleted. - context.fieldMask.push(context.path); - } else if (context.dataSource === UserDataSource.Update) { - debugAssert( - context.path.length > 0, - 'FieldValue.delete() at the top level should have already' + - ' been handled.' - ); - throw context.createError( - 'FieldValue.delete() can only appear at the top level ' + - 'of your update data' - ); - } else { - // We shouldn't encounter delete sentinels for queries or non-merge set() calls. - throw context.createError( - 'FieldValue.delete() cannot be used with set() unless you pass ' + - '{merge:true}' - ); - } - } else if (value instanceof ServerTimestampFieldValueImpl) { - context.fieldTransforms.push( - new FieldTransform(context.path, ServerTimestampTransform.instance) - ); - } else if (value instanceof ArrayUnionFieldValueImpl) { - const parsedElements = this.parseArrayTransformElements( - value._methodName, - value._elements - ); - const arrayUnion = new ArrayUnionTransformOperation(parsedElements); - context.fieldTransforms.push( - new FieldTransform(context.path, arrayUnion) - ); - } else if (value instanceof ArrayRemoveFieldValueImpl) { - const parsedElements = this.parseArrayTransformElements( - value._methodName, - value._elements - ); - const arrayRemove = new ArrayRemoveTransformOperation(parsedElements); - context.fieldTransforms.push( - new FieldTransform(context.path, arrayRemove) - ); - } else if (value instanceof NumericIncrementFieldValueImpl) { - const operand = this.parseQueryValue( - 'FieldValue.increment', - value._operand - ); - const numericIncrement = new NumericIncrementTransformOperation( - this.serializer, - operand - ); - context.fieldTransforms.push( - new FieldTransform(context.path, numericIncrement) - ); - } else { - fail('Unknown FieldValue type: ' + value); - } + const fieldTransform = value.toFieldTransform(context); + if (fieldTransform) { + context.fieldTransforms.push(fieldTransform); } +} - /** - * Helper to parse a scalar value (i.e. not an Object, Array, or FieldValue) - * - * @return The parsed value - */ - private parseScalarValue(value: unknown, context: ParseContext): api.Value { - if (value === null) { - return { nullValue: 'NULL_VALUE' }; - } else if (typeof value === 'number') { - return this.serializer.toNumber(value); - } else if (typeof value === 'boolean') { - return { booleanValue: value }; - } else if (typeof value === 'string') { - return { stringValue: value }; - } else if (value instanceof Date) { - const timestamp = Timestamp.fromDate(value); - return { timestampValue: this.serializer.toTimestamp(timestamp) }; - } else if (value instanceof Timestamp) { - // Firestore backend truncates precision down to microseconds. To ensure - // offline mode works the same with regards to truncation, perform the - // truncation immediately without waiting for the backend to do that. - const timestamp = new Timestamp( - value.seconds, - Math.floor(value.nanoseconds / 1000) * 1000 - ); - return { timestampValue: this.serializer.toTimestamp(timestamp) }; - } else if (value instanceof GeoPoint) { - return { - geoPointValue: { - latitude: value.latitude, - longitude: value.longitude - } - }; - } else if (value instanceof Blob) { - return { bytesValue: this.serializer.toBytes(value) }; - } else if (value instanceof DocumentKeyReference) { - return { - referenceValue: this.serializer.toResourceName( - value.key.path, - value.databaseId - ) - }; - } else { +/** + * Helper to parse a scalar value (i.e. not an Object, Array, or FieldValue) + * + * @return The parsed value + */ +function parseScalarValue(value: unknown, context: ParseContext): api.Value { + if (value === null) { + return { nullValue: 'NULL_VALUE' }; + } else if (typeof value === 'number') { + return context.serializer.toNumber(value); + } else if (typeof value === 'boolean') { + return { booleanValue: value }; + } else if (typeof value === 'string') { + return { stringValue: value }; + } else if (value instanceof Date) { + const timestamp = Timestamp.fromDate(value); + return { timestampValue: context.serializer.toTimestamp(timestamp) }; + } else if (value instanceof Timestamp) { + // Firestore backend truncates precision down to microseconds. To ensure + // offline mode works the same with regards to truncation, perform the + // truncation immediately without waiting for the backend to do that. + const timestamp = new Timestamp( + value.seconds, + Math.floor(value.nanoseconds / 1000) * 1000 + ); + return { timestampValue: context.serializer.toTimestamp(timestamp) }; + } else if (value instanceof GeoPoint) { + return { + geoPointValue: { + latitude: value.latitude, + longitude: value.longitude + } + }; + } else if (value instanceof Blob) { + return { bytesValue: context.serializer.toBytes(value) }; + } else if (value instanceof DocumentReference) { + const thisDb = context.databaseId; + const otherDb = value.firestore._databaseId; + if (!otherDb.isEqual(thisDb)) { throw context.createError( - `Unsupported field value: ${valueDescription(value)}` + 'Document reference is for database ' + + `${otherDb.projectId}/${otherDb.database} but should be ` + + `for database ${thisDb.projectId}/${thisDb.database}` ); } - } - - private parseArrayTransformElements( - methodName: string, - elements: unknown[] - ): api.Value[] { - return elements.map((element, i) => { - // Although array transforms are used with writes, the actual elements - // being unioned or removed are not considered writes since they cannot - // contain any FieldValue sentinels, etc. - const context = new ParseContext( - UserDataSource.Argument, - methodName, - FieldPath.EMPTY_PATH - ); - return this.parseData(element, context.childContextForArray(i))!; - }); + return { + referenceValue: context.serializer.toResourceName( + value._key.path, + value.firestore._databaseId + ) + }; + } else { + throw context.createError( + `Unsupported field value: ${valueDescription(value)}` + ); } } @@ -771,7 +681,7 @@ function looksLikeJsonObject(input: unknown): boolean { !(input instanceof Timestamp) && !(input instanceof GeoPoint) && !(input instanceof Blob) && - !(input instanceof DocumentKeyReference) && + !(input instanceof DocumentReference) && !(input instanceof FieldValueImpl) ); } diff --git a/packages/firestore/src/platform/config.ts b/packages/firestore/src/platform/config.ts index f8dee256720..c5be1a03709 100644 --- a/packages/firestore/src/platform/config.ts +++ b/packages/firestore/src/platform/config.ts @@ -31,10 +31,10 @@ import { PublicQueryDocumentSnapshot, PublicQuerySnapshot, PublicTransaction, - PublicWriteBatch + PublicWriteBatch, + PublicFieldValue } from '../api/database'; import { FieldPath } from '../api/field_path'; -import { PublicFieldValue } from '../api/field_value'; import { GeoPoint } from '../api/geo_point'; import { Timestamp } from '../api/timestamp'; diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index 4eb8295dd65..c4ede19dd03 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -37,7 +37,11 @@ import { AsyncQueue } from '../util/async_queue'; * Cloud Datastore grpc API, which provides an interface that is more convenient * for the rest of the client SDK architecture to consume. */ -export class Datastore {} +export class Datastore { + // Make sure that the structural type of `Datastore` is unique. + // See https://github.com/microsoft/TypeScript/issues/5451 + private _ = undefined; +} /** * An implementation of Datastore that exposes additional state for internal diff --git a/packages/firestore/test/unit/api/field_value.test.ts b/packages/firestore/test/unit/api/field_value.test.ts index d5c5983e37e..e1445cdd336 100644 --- a/packages/firestore/test/unit/api/field_value.test.ts +++ b/packages/firestore/test/unit/api/field_value.test.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { PublicFieldValue as FieldValue } from '../../../src/api/field_value'; +import { PublicFieldValue as FieldValue } from '../../../src/api/database'; import { expectEqual, expectNotEqual } from '../../util/helpers'; describe('FieldValue', () => { diff --git a/packages/firestore/test/unit/core/query.test.ts b/packages/firestore/test/unit/core/query.test.ts index a07930dd543..a1e5f4f20f6 100644 --- a/packages/firestore/test/unit/core/query.test.ts +++ b/packages/firestore/test/unit/core/query.test.ts @@ -399,7 +399,7 @@ describe('Query', () => { doc('collection/1', 0, { sort: 'a' }), doc('collection/1', 0, { sort: 'ab' }), doc('collection/1', 0, { sort: 'b' }), - doc('collection/1', 0, { sort: ref('p', 'collection/id1') }) + doc('collection/1', 0, { sort: ref('collection/id1') }) ]; expectCorrectComparisons(docs, query.docComparator.bind(query)); @@ -500,9 +500,8 @@ describe('Query', () => { // .addOrderBy(orderBy(DOCUMENT_KEY_NAME, 'desc')) // .withUpperBound(lip3, 'exclusive'); - const relativeReference = ref('project1/database1', 'col/doc'); + const relativeReference = ref('col/doc'); const absoluteReference = ref( - 'project1/database1', 'projects/project1/databases/database1/documents/col/doc', 5 ); @@ -581,9 +580,7 @@ describe('Query', () => { 'collection|f:a==NaN|ob:__name__asc' ); assertCanonicalId( - baseQuery.addFilter( - filter('__name__', '==', ref('test-project', 'collection/id')) - ), + baseQuery.addFilter(filter('__name__', '==', ref('collection/id'))), 'collection|f:__name__==collection/id|ob:__name__asc' ); assertCanonicalId( diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index dcab1919868..8a9803d2129 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -18,7 +18,7 @@ import * as api from '../../../src/protos/firestore_proto_api'; import { expect } from 'chai'; -import { PublicFieldValue } from '../../../src/api/field_value'; +import { PublicFieldValue } from '../../../src/api/database'; import { Timestamp } from '../../../src/api/timestamp'; import { User } from '../../../src/auth/user'; import { Query } from '../../../src/core/query'; diff --git a/packages/firestore/test/unit/model/mutation.test.ts b/packages/firestore/test/unit/model/mutation.test.ts index 955b2cfe0fe..3914f07322e 100644 --- a/packages/firestore/test/unit/model/mutation.test.ts +++ b/packages/firestore/test/unit/model/mutation.test.ts @@ -16,7 +16,7 @@ */ import { expect } from 'chai'; -import { PublicFieldValue as FieldValue } from '../../../src/api/field_value'; +import { PublicFieldValue as FieldValue } from '../../../src/api/database'; import { Timestamp } from '../../../src/api/timestamp'; import { Document, MaybeDocument } from '../../../src/model/document'; import { serverTimestamp } from '../../../src/model/server_timestamps'; @@ -32,7 +32,6 @@ import { import { Dict } from '../../../src/util/obj'; import { addEqualityMatcher } from '../../util/equality_matcher'; import { - DELETE_SENTINEL, deletedDoc, deleteMutation, doc, @@ -136,7 +135,7 @@ describe('Mutation', () => { foo: { bar: 'bar-value', baz: 'baz-value' } }); const patch = patchMutation('collection/key', { - 'foo.bar': DELETE_SENTINEL + 'foo.bar': FieldValue.delete() }); const patchedDoc = patch.applyToLocalView(baseDoc, baseDoc, timestamp); diff --git a/packages/firestore/test/unit/model/values.test.ts b/packages/firestore/test/unit/model/values.test.ts index 67be9a08123..5af4e09ee67 100644 --- a/packages/firestore/test/unit/model/values.test.ts +++ b/packages/firestore/test/unit/model/values.test.ts @@ -76,8 +76,8 @@ describe('Values', () => { [serverTimestamp(Timestamp.fromDate(date2), null)], [wrap(new GeoPoint(0, 1)), wrap(new GeoPoint(0, 1))], [wrap(new GeoPoint(1, 0))], - [wrap(ref('project', 'coll/doc1')), wrap(ref('project', 'coll/doc1'))], - [wrap(ref('project', 'coll/doc2'))], + [wrap(ref('coll/doc1')), wrap(ref('coll/doc1'))], + [wrap(ref('coll/doc2'))], [wrap(['foo', 'bar']), wrap(['foo', 'bar'])], [wrap(['foo', 'bar', 'baz'])], [wrap(['foo'])], diff --git a/packages/firestore/test/unit/remote/serializer.helper.ts b/packages/firestore/test/unit/remote/serializer.helper.ts index bd7ea39326c..3ba1657061b 100644 --- a/packages/firestore/test/unit/remote/serializer.helper.ts +++ b/packages/firestore/test/unit/remote/serializer.helper.ts @@ -18,11 +18,12 @@ import { expect } from 'chai'; import { Blob } from '../../../src/api/blob'; -import { PublicFieldValue as FieldValue } from '../../../src/api/field_value'; +import { + PublicFieldValue as FieldValue, + DocumentReference +} from '../../../src/api/database'; import { GeoPoint } from '../../../src/api/geo_point'; import { Timestamp } from '../../../src/api/timestamp'; -import { DocumentKeyReference } from '../../../src/api/user_data_reader'; -import { DocumentReference } from '../../../src/api/database'; import { DatabaseId } from '../../../src/core/database_info'; import { ArrayContainsAnyFilter, @@ -47,7 +48,6 @@ import { VerifyMutation } from '../../../src/model/mutation'; import { DOCUMENT_KEY_NAME, FieldPath } from '../../../src/model/path'; -import { refValue } from '../../../src/model/values'; import * as api from '../../../src/protos/firestore_proto_api'; import { JsonProtoSerializer } from '../../../src/remote/serializer'; import { @@ -60,7 +60,6 @@ import { addEqualityMatcher } from '../../util/equality_matcher'; import { bound, byteStringFromString, - dbId, deletedDoc, deleteMutation, doc, @@ -145,7 +144,15 @@ export function serializerTest( const actualReturnFieldValue = userDataWriter.convertValue( actualJsonProto ); - expect(actualReturnFieldValue).to.deep.equal(value); + + if ( + actualReturnFieldValue instanceof DocumentReference && + value instanceof DocumentReference + ) { + expect(actualReturnFieldValue.isEqual(value)).to.be.true; + } else { + expect(actualReturnFieldValue).to.deep.equal(value); + } // Convert value to ProtoJs and verify. const actualProtoJsProto = protoJsReader.parseQueryValue( @@ -468,21 +475,12 @@ export function serializerTest( }); it('converts RefValue', () => { - // verifyFieldValueRoundTrip cannot be used for RefValues since the - // serializer takes a DocumentKeyReference but returns a DocumentReference - const example = new DocumentKeyReference( - dbId('project1', 'database1'), - key('docs/1') - ); - const actualValue = protoJsReader.parseQueryValue('refValue', example); - expect(actualValue).to.deep.equal( - refValue(dbId('project1', 'database1'), key('docs/1')) - ); - - const roundtripResult = userDataWriter.convertValue( - actualValue - ) as DocumentReference; - expect(roundtripResult._key.isEqual(key('docs/1'))).to.be.true; + verifyFieldValueRoundTrip({ + value: ref('docs/1'), + valueType: 'referenceValue', + jsonValue: + 'projects/test-project/databases/(default)/documents/docs/1' + }); }); }); @@ -807,11 +805,7 @@ export function serializerTest( }); it('converts key field', () => { - const input = filter( - DOCUMENT_KEY_NAME, - '==', - ref('project/database', 'coll/doc') - ); + const input = filter(DOCUMENT_KEY_NAME, '==', ref('coll/doc')); const actual = s.toUnaryOrFieldFilter(input); expect(actual).to.deep.equal({ fieldFilter: { @@ -819,7 +813,7 @@ export function serializerTest( op: 'EQUAL', value: { referenceValue: - 'projects/project/databases/database/documents/coll/doc' + 'projects/test-project/databases/(default)/documents/coll/doc' } } }); @@ -1198,13 +1192,13 @@ export function serializerTest( const q = Query.atPath(path('docs')) .withStartAt( bound( - [[DOCUMENT_KEY_NAME, ref('p/d', 'foo/bar'), 'asc']], + [[DOCUMENT_KEY_NAME, ref('foo/bar'), 'asc']], /*before=*/ true ) ) .withEndAt( bound( - [[DOCUMENT_KEY_NAME, ref('p/d', 'foo/bar'), 'asc']], + [[DOCUMENT_KEY_NAME, ref('foo/bar'), 'asc']], /*before=*/ false ) ) @@ -1223,13 +1217,19 @@ export function serializerTest( ], startAt: { values: [ - { referenceValue: 'projects/p/databases/d/documents/foo/bar' } + { + referenceValue: + 'projects/test-project/databases/(default)/documents/foo/bar' + } ], before: true }, endAt: { values: [ - { referenceValue: 'projects/p/databases/d/documents/foo/bar' } + { + referenceValue: + 'projects/test-project/databases/(default)/documents/foo/bar' + } ], before: false } diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index 577cc296d61..a3e095aeb93 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -23,12 +23,8 @@ import { expect } from 'chai'; import { Blob } from '../../src/api/blob'; import { fromDotSeparatedString } from '../../src/api/field_path'; -import { FieldValueImpl } from '../../src/api/field_value'; import { UserDataWriter } from '../../src/api/user_data_writer'; -import { - DocumentKeyReference, - UserDataReader -} from '../../src/api/user_data_reader'; +import { UserDataReader } from '../../src/api/user_data_reader'; import { DatabaseId } from '../../src/core/database_info'; import { Bound, @@ -84,7 +80,7 @@ import { } from '../../src/remote/watch_change'; import { debugAssert, fail } from '../../src/util/assert'; import { primitiveComparator } from '../../src/util/misc'; -import { Dict } from '../../src/util/obj'; +import { Dict, forEach } from '../../src/util/obj'; import { SortedMap } from '../../src/util/sorted_map'; import { SortedSet } from '../../src/util/sorted_set'; import { query } from './api_helpers'; @@ -92,41 +88,31 @@ import { ByteString } from '../../src/util/byte_string'; import { PlatformSupport } from '../../src/platform/platform'; import { JsonProtoSerializer } from '../../src/remote/serializer'; import { Timestamp } from '../../src/api/timestamp'; +import { DocumentReference, Firestore } from '../../src/api/database'; +import { DeleteFieldValueImpl } from '../../src/api/field_value'; /* eslint-disable no-restricted-globals */ -export type TestSnapshotVersion = number; - -/** - * A string sentinel that can be used with patchMutation() to mark a field for - * deletion. - */ -export const DELETE_SENTINEL = ''; +// A Firestore that can be used in DocumentReferences and UserDataWriter. +const fakeFirestore: Firestore = { + ensureClientConfigured: () => {}, + _databaseId: new DatabaseId('test-project') + // eslint-disable-next-line @typescript-eslint/no-explicit-any +} as any; -const preConverter = (input: unknown): unknown => { - return input === DELETE_SENTINEL ? FieldValueImpl.delete() : input; -}; +export type TestSnapshotVersion = number; export function testUserDataWriter(): UserDataWriter { - // We should pass in a proper Firestore instance, but for now, only - // `ensureClientConfigured()` and `_databaseId` is used in our test usage of - // UserDataWriter. - - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const firestore: any = { - ensureClientConfigured: () => {}, - _databaseId: new DatabaseId('test-project') - }; - return new UserDataWriter(firestore, /* timestampsInSnapshots= */ false); + return new UserDataWriter(fakeFirestore, /* timestampsInSnapshots= */ false); } export function testUserDataReader(useProto3Json?: boolean): UserDataReader { const databaseId = new DatabaseId('test-project'); return new UserDataReader( + databaseId, useProto3Json !== undefined ? new JsonProtoSerializer(databaseId, { useProto3Json }) - : PlatformSupport.getPlatform().newSerializer(databaseId), - preConverter + : undefined ); } @@ -136,14 +122,11 @@ export function version(v: TestSnapshotVersion): SnapshotVersion { return SnapshotVersion.fromTimestamp(new Timestamp(seconds, nanos)); } -export function ref( - dbIdStr: string, - keyStr: string, - offset?: number -): DocumentKeyReference { - const [project, database] = dbIdStr.split('/', 2); - const dbId = new DatabaseId(project, database); - return new DocumentKeyReference(dbId, new DocumentKey(path(keyStr, offset))); +export function ref(key: string, offset?: number): DocumentReference { + return new DocumentReference( + new DocumentKey(path(key, offset)), + fakeFirestore + ); } export function doc( @@ -251,7 +234,12 @@ export function patchMutation( if (precondition === undefined) { precondition = Precondition.exists(true); } - + // Replace '' from JSON with FieldValue + forEach(json, (k, v) => { + if (v === '') { + json[k] = new DeleteFieldValueImpl(); + } + }); const parsed = testUserDataReader().parseUpdateData('patchMutation', json); return new PatchMutation( key(keyStr), From 7bf4d3f3ed1598fc2ea2a5e62c27955994b1574d Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 1 May 2020 14:44:12 -0700 Subject: [PATCH 2/4] Review --- .../firestore/src/api/user_data_reader.ts | 59 ++++++++----------- 1 file changed, 26 insertions(+), 33 deletions(-) diff --git a/packages/firestore/src/api/user_data_reader.ts b/packages/firestore/src/api/user_data_reader.ts index 308599b9a5d..be3a1142045 100644 --- a/packages/firestore/src/api/user_data_reader.ts +++ b/packages/firestore/src/api/user_data_reader.ts @@ -128,24 +128,21 @@ function isWrite(dataSource: UserDataSource): boolean { } } -interface ParseSettings { +/** Contains the settings that are mutated as we parse user data. */ +interface ContextSettings { /** Indicates what kind of API method this data came from. */ - dataSource: UserDataSource; - /** The serializer to use for user data conversion. */ - serializer: JsonProtoSerializer; - /** The database ID of the instance. */ - databaseId: DatabaseId; + readonly dataSource: UserDataSource; /** The name of the method the user called to create the ParseContext. */ - methodName: string; + readonly methodName: string; /** * A path within the object being parsed. This could be an empty path (in * which case the context represents the root of the data being parsed), or a * nonempty path (indicating the context represents a nested location within * the data). */ - path: FieldPath | null; + readonly path: FieldPath | null; /** Whether or not this context corresponds to an element of an array. */ - arrayElement: boolean; + readonly arrayElement: boolean; } /** A "context" object passed around while parsing user data. */ @@ -156,6 +153,8 @@ export class ParseContext { * Initializes a ParseContext with the given source and path. * * @param settings The settings for the parser. + * @param databaseId The database ID of the Firestore instance. + * @param serializer The serialize to use to generate the Value proto. * @param fieldTransforms A mutable list of field transforms encountered while * parsing the data. * @param fieldMask A mutable list of field paths encountered while parsing @@ -167,7 +166,9 @@ export class ParseContext { * compromised). */ constructor( - private readonly settings: ParseSettings, + readonly settings: ContextSettings, + readonly databaseId: DatabaseId, + readonly serializer: JsonProtoSerializer, fieldTransforms?: FieldTransform[], fieldMask?: FieldPath[] ) { @@ -188,22 +189,12 @@ export class ParseContext { return this.settings.dataSource; } - get arrayElement(): boolean { - return this.settings.arrayElement; - } - - get serializer(): JsonProtoSerializer { - return this.settings.serializer; - } - - get databaseId(): DatabaseId { - return this.settings.databaseId; - } - /** Returns a new context with the specified settings overwritten. */ - contextWith(configuration: Partial): ParseContext { + contextWith(configuration: Partial): ParseContext { return new ParseContext( { ...this.settings, ...configuration }, + this.databaseId, + this.serializer, this.fieldTransforms, this.fieldMask ); @@ -422,7 +413,7 @@ export class UserDataReader { const fieldMaskPaths: FieldPath[] = []; const updateData = new ObjectValueBuilder(); - + // 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) { @@ -456,14 +447,16 @@ export class UserDataReader { dataSource: UserDataSource, methodName: string ): ParseContext { - return new ParseContext({ - dataSource, - databaseId: this.databaseId, - serializer: this.serializer, - methodName, - path: FieldPath.EMPTY_PATH, - arrayElement: false - }); + return new ParseContext( + { + dataSource, + methodName, + path: FieldPath.EMPTY_PATH, + arrayElement: false + }, + this.databaseId, + this.serializer + ); } /** @@ -531,7 +524,7 @@ export function parseData( // contain additional arrays (each representing an individual field // value), so we disable this validation. if ( - context.arrayElement && + context.settings.arrayElement && context.dataSource !== UserDataSource.ArrayArgument ) { throw context.createError('Nested arrays are not supported'); From 7b67401f8c0d0e803dcf057c0a60cfab4ea3f7ab Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 1 May 2020 14:51:16 -0700 Subject: [PATCH 3/4] Typo --- packages/firestore/src/api/user_data_reader.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/api/user_data_reader.ts b/packages/firestore/src/api/user_data_reader.ts index be3a1142045..3e9e5aadb6b 100644 --- a/packages/firestore/src/api/user_data_reader.ts +++ b/packages/firestore/src/api/user_data_reader.ts @@ -154,7 +154,7 @@ export class ParseContext { * * @param settings The settings for the parser. * @param databaseId The database ID of the Firestore instance. - * @param serializer The serialize to use to generate the Value proto. + * @param serializer The serializer to use to generate the Value proto. * @param fieldTransforms A mutable list of field transforms encountered while * parsing the data. * @param fieldMask A mutable list of field paths encountered while parsing From 22a41b6c4c319fbbafdddabac1cfca973994b62f Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 1 May 2020 15:06:58 -0700 Subject: [PATCH 4/4] Better isEqual --- packages/firestore/src/api/field_value.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/firestore/src/api/field_value.ts b/packages/firestore/src/api/field_value.ts index 8823e09e6b0..d3bb8102a14 100644 --- a/packages/firestore/src/api/field_value.ts +++ b/packages/firestore/src/api/field_value.ts @@ -115,7 +115,7 @@ export class ArrayUnionFieldValueImpl extends FieldValueImpl { isEqual(other: FieldValue): boolean { // TODO(mrschmidt): Implement isEquals - return false; + return this === other; } } @@ -141,7 +141,7 @@ export class ArrayRemoveFieldValueImpl extends FieldValueImpl { isEqual(other: FieldValue): boolean { // TODO(mrschmidt): Implement isEquals - return false; + return this === other; } } @@ -162,7 +162,7 @@ export class NumericIncrementFieldValueImpl extends FieldValueImpl { isEqual(other: FieldValue): boolean { // TODO(mrschmidt): Implement isEquals - return false; + return this === other; } }