diff --git a/packages/firestore/lite/src/api/field_path.ts b/packages/firestore/lite/src/api/field_path.ts index 64e435dec95..021ed57f36a 100644 --- a/packages/firestore/lite/src/api/field_path.ts +++ b/packages/firestore/lite/src/api/field_path.ts @@ -15,8 +15,11 @@ * limitations under the License. */ -import { _BaseFieldPath } from '../../../src/api/field_path'; -import { DOCUMENT_KEY_NAME } from '../../../src/model/path'; +import { + DOCUMENT_KEY_NAME, + FieldPath as InternalFieldPath +} from '../../../src/model/path'; +import { Code, FirestoreError } from '../../../src/util/error'; /** * A `FieldPath` refers to a field in a document. The path may consist of a @@ -26,12 +29,9 @@ import { DOCUMENT_KEY_NAME } from '../../../src/model/path'; * Create a `FieldPath` by providing field names. If more than one field * name is provided, the path will point to a nested field in a document. */ -export class FieldPath extends _BaseFieldPath { - // Note: This class is stripped down a copy of the FieldPath class in the - // legacy SDK. The changes are: - // - The `documentId()` static method has been removed - // - Input validation is limited to errors that cannot be caught by the - // TypeScript transpiler. +export class FieldPath { + /** Internal representation of a Firestore field path. */ + readonly _internalPath: InternalFieldPath; /** * Creates a FieldPath from the provided field names. If more than one field @@ -40,7 +40,17 @@ export class FieldPath extends _BaseFieldPath { * @param fieldNames A list of field names. */ constructor(...fieldNames: string[]) { - super(fieldNames); + for (let i = 0; i < fieldNames.length; ++i) { + if (fieldNames[i].length === 0) { + throw new FirestoreError( + Code.INVALID_ARGUMENT, + `Invalid field name at argument $(i + 1). ` + + 'Field names must not be empty.' + ); + } + } + + this._internalPath = new InternalFieldPath(fieldNames); } /** diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 834388c17d5..73905859ada 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -77,7 +77,7 @@ import { } from '../util/input_validation'; import { logWarn, setLogLevel as setClientLogLevel } from '../util/log'; import { AutoId } from '../util/misc'; -import { _BaseFieldPath, FieldPath as ExternalFieldPath } from './field_path'; +import { FieldPath as ExpFieldPath } from '../../lite/src/api/field_path'; import { CompleteFn, ErrorFn, @@ -119,6 +119,7 @@ import { DocumentData as PublicDocumentData, DocumentReference as PublicDocumentReference, DocumentSnapshot as PublicDocumentSnapshot, + FieldPath as PublicFieldPath, FirebaseFirestore as PublicFirestore, FirestoreDataConverter as PublicFirestoreDataConverter, GetOptions as PublicGetOptions, @@ -518,28 +519,33 @@ export class Transaction implements PublicTransaction { ): Transaction; update( documentRef: PublicDocumentReference, - field: string | ExternalFieldPath, + field: string | PublicFieldPath, value: unknown, ...moreFieldsAndValues: unknown[] ): Transaction; update( documentRef: PublicDocumentReference, - fieldOrUpdateData: string | ExternalFieldPath | PublicUpdateData, + fieldOrUpdateData: string | PublicFieldPath | PublicUpdateData, value?: unknown, ...moreFieldsAndValues: unknown[] ): Transaction { - let ref; - let parsed; + const ref = validateReference( + 'Transaction.update', + documentRef, + this._firestore + ); + // For Compat types, we have to "extract" the underlying types before + // performing validation. + if (fieldOrUpdateData instanceof Compat) { + fieldOrUpdateData = (fieldOrUpdateData as Compat)._delegate; + } + + let parsed; if ( typeof fieldOrUpdateData === 'string' || - fieldOrUpdateData instanceof ExternalFieldPath + fieldOrUpdateData instanceof ExpFieldPath ) { - ref = validateReference( - 'Transaction.update', - documentRef, - this._firestore - ); parsed = parseUpdateVarargs( this._dataReader, 'Transaction.update', @@ -549,11 +555,6 @@ export class Transaction implements PublicTransaction { moreFieldsAndValues ); } else { - ref = validateReference( - 'Transaction.update', - documentRef, - this._firestore - ); parsed = parseUpdateData( this._dataReader, 'Transaction.update', @@ -629,30 +630,34 @@ export class WriteBatch implements PublicWriteBatch { ): WriteBatch; update( documentRef: PublicDocumentReference, - field: string | ExternalFieldPath, + field: string | PublicFieldPath, value: unknown, ...moreFieldsAndValues: unknown[] ): WriteBatch; update( documentRef: PublicDocumentReference, - fieldOrUpdateData: string | ExternalFieldPath | PublicUpdateData, + fieldOrUpdateData: string | PublicFieldPath | PublicUpdateData, value?: unknown, ...moreFieldsAndValues: unknown[] ): WriteBatch { this.verifyNotCommitted(); + const ref = validateReference( + 'WriteBatch.update', + documentRef, + this._firestore + ); - let ref; - let parsed; + // For Compat types, we have to "extract" the underlying types before + // performing validation. + if (fieldOrUpdateData instanceof Compat) { + fieldOrUpdateData = (fieldOrUpdateData as Compat)._delegate; + } + let parsed; if ( typeof fieldOrUpdateData === 'string' || - fieldOrUpdateData instanceof ExternalFieldPath + fieldOrUpdateData instanceof ExpFieldPath ) { - ref = validateReference( - 'WriteBatch.update', - documentRef, - this._firestore - ); parsed = parseUpdateVarargs( this._dataReader, 'WriteBatch.update', @@ -662,11 +667,6 @@ export class WriteBatch implements PublicWriteBatch { moreFieldsAndValues ); } else { - ref = validateReference( - 'WriteBatch.update', - documentRef, - this._firestore - ); parsed = parseUpdateData( this._dataReader, 'WriteBatch.update', @@ -825,26 +825,25 @@ export class DocumentReference update(value: PublicUpdateData): Promise; update( - field: string | ExternalFieldPath, + field: string | PublicFieldPath, value: unknown, ...moreFieldsAndValues: unknown[] ): Promise; update( - fieldOrUpdateData: string | ExternalFieldPath | PublicUpdateData, + fieldOrUpdateData: string | PublicFieldPath | PublicUpdateData, value?: unknown, ...moreFieldsAndValues: unknown[] ): Promise { // For Compat types, we have to "extract" the underlying types before // performing validation. if (fieldOrUpdateData instanceof Compat) { - fieldOrUpdateData = (fieldOrUpdateData as Compat<_BaseFieldPath>) - ._delegate; + fieldOrUpdateData = (fieldOrUpdateData as Compat)._delegate; } let parsed; if ( typeof fieldOrUpdateData === 'string' || - fieldOrUpdateData instanceof _BaseFieldPath + fieldOrUpdateData instanceof ExpFieldPath ) { parsed = parseUpdateVarargs( this._dataReader, @@ -1080,7 +1079,7 @@ export class DocumentSnapshot } get( - fieldPath: string | ExternalFieldPath, + fieldPath: string | PublicFieldPath, options: PublicSnapshotOptions = {} ): unknown { if (this._document) { @@ -1556,7 +1555,7 @@ export class Query implements PublicQuery { } where( - field: string | ExternalFieldPath, + field: string | PublicFieldPath, opStr: PublicWhereFilterOp, value: unknown ): PublicQuery { @@ -1578,7 +1577,7 @@ export class Query implements PublicQuery { } orderBy( - field: string | ExternalFieldPath, + field: string | PublicFieldPath, directionStr?: PublicOrderByDirection ): PublicQuery { let direction: Direction; diff --git a/packages/firestore/src/api/field_path.ts b/packages/firestore/src/api/field_path.ts index 4008a9936c1..bf4e24491d2 100644 --- a/packages/firestore/src/api/field_path.ts +++ b/packages/firestore/src/api/field_path.ts @@ -17,44 +17,20 @@ import { FieldPath as PublicFieldPath } from '@firebase/firestore-types'; +import { FieldPath as ExpFieldPath } from '../../lite/src/api/field_path'; import { FieldPath as InternalFieldPath } from '../model/path'; -import { Code, FirestoreError } from '../util/error'; +import { Compat } from '../compat/compat'; // The objects that are a part of this API are exposed to third-parties as // compiled javascript so we want to flag our private members with a leading // underscore to discourage their use. -/** - * A field class base class that is shared by the lite, full and legacy SDK, - * which supports shared code that deals with FieldPaths. - */ -// Use underscore prefix to hide this class from our Public API. -// eslint-disable-next-line @typescript-eslint/naming-convention -export abstract class _BaseFieldPath { - /** Internal representation of a Firestore field path. */ - readonly _internalPath: InternalFieldPath; - - constructor(fieldNames: string[]) { - for (let i = 0; i < fieldNames.length; ++i) { - if (fieldNames[i].length === 0) { - throw new FirestoreError( - Code.INVALID_ARGUMENT, - `Invalid field name at argument $(i + 1). ` + - 'Field names must not be empty.' - ); - } - } - - this._internalPath = new InternalFieldPath(fieldNames); - } -} - /** * A `FieldPath` refers to a field in a document. The path may consist of a * single field name (referring to a top-level field in the document), or a list * of field names (referring to a nested field in the document). */ -export class FieldPath extends _BaseFieldPath implements PublicFieldPath { +export class FieldPath extends Compat implements PublicFieldPath { /** * Creates a FieldPath from the provided field names. If more than one field * name is provided, the path will point to a nested field in a document. @@ -62,7 +38,7 @@ export class FieldPath extends _BaseFieldPath implements PublicFieldPath { * @param fieldNames A list of field names. */ constructor(...fieldNames: string[]) { - super(fieldNames); + super(new ExpFieldPath(...fieldNames)); } static documentId(): FieldPath { @@ -76,37 +52,12 @@ export class FieldPath extends _BaseFieldPath implements PublicFieldPath { } isEqual(other: PublicFieldPath): boolean { - if (!(other instanceof FieldPath)) { + if (other instanceof Compat) { + other = other._delegate; + } + if (!(other instanceof ExpFieldPath)) { return false; } - return this._internalPath.isEqual(other._internalPath); - } -} - -/** - * Matches any characters in a field path string that are reserved. - */ -const RESERVED = new RegExp('[~\\*/\\[\\]]'); - -/** - * Parses a field path string into a FieldPath, treating dots as separators. - */ -export function fromDotSeparatedString(path: string): FieldPath { - const found = path.search(RESERVED); - if (found >= 0) { - throw new FirestoreError( - Code.INVALID_ARGUMENT, - `Invalid field path (${path}). Paths must not contain ` + - `'~', '*', '/', '[', or ']'` - ); - } - try { - return new FieldPath(...path.split('.')); - } catch (e) { - throw new FirestoreError( - Code.INVALID_ARGUMENT, - `Invalid field path (${path}). Paths must not be empty, ` + - `begin with '.', end with '.', or contain '..'` - ); + return this._delegate._internalPath.isEqual(other._internalPath); } } diff --git a/packages/firestore/src/api/user_data_reader.ts b/packages/firestore/src/api/user_data_reader.ts index a574191d365..bf63ab6cfa9 100644 --- a/packages/firestore/src/api/user_data_reader.ts +++ b/packages/firestore/src/api/user_data_reader.ts @@ -15,7 +15,11 @@ * limitations under the License. */ -import { DocumentData, SetOptions } from '@firebase/firestore-types'; +import { + DocumentData, + SetOptions, + FieldPath as PublicFieldPath +} from '@firebase/firestore-types'; import { Value as ProtoValue, @@ -33,7 +37,7 @@ import { SetMutation, TransformMutation } from '../model/mutation'; -import { FieldPath } from '../model/path'; +import { FieldPath as InternalFieldPath } from '../model/path'; import { debugAssert, fail } from '../util/assert'; import { Code, FirestoreError } from '../util/error'; import { isPlainObject, valueDescription } from '../util/input_validation'; @@ -46,13 +50,13 @@ import { toResourceName, toTimestamp } from '../remote/serializer'; -import { _BaseFieldPath, fromDotSeparatedString } from './field_path'; import { DeleteFieldValueImpl } from './field_value'; import { GeoPoint } from './geo_point'; import { newSerializer } from '../platform/serializer'; import { Bytes } from '../../lite/src/api/bytes'; import { Compat } from '../compat/compat'; import { FieldValue } from '../../lite/src/api/field_value'; +import { FieldPath } from '../../lite/src/api/field_path'; const RESERVED_FIELD_REGEX = /^__.*__$/; @@ -174,7 +178,7 @@ interface ContextSettings { * nonempty path (indicating the context represents a nested location within * the data). */ - readonly path?: FieldPath; + readonly path?: InternalFieldPath; /** * Whether or not this context corresponds to an element of an array. * If not set, elements are treated as if they were outside of arrays. @@ -190,7 +194,7 @@ interface ContextSettings { /** A "context" object passed around while parsing user data. */ export class ParseContext { readonly fieldTransforms: FieldTransform[]; - readonly fieldMask: FieldPath[]; + readonly fieldMask: InternalFieldPath[]; /** * Initializes a ParseContext with the given source and path. * @@ -215,7 +219,7 @@ export class ParseContext { readonly serializer: JsonProtoSerializer, readonly ignoreUndefinedProperties: boolean, fieldTransforms?: FieldTransform[], - fieldMask?: FieldPath[] + fieldMask?: InternalFieldPath[] ) { // Minor hack: If fieldTransforms is undefined, we assume this is an // external call and we need to validate the entire path. @@ -226,7 +230,7 @@ export class ParseContext { this.fieldMask = fieldMask || []; } - get path(): FieldPath | undefined { + get path(): InternalFieldPath | undefined { return this.settings.path; } @@ -253,7 +257,7 @@ export class ParseContext { return context; } - childContextForFieldPath(field: FieldPath): ParseContext { + childContextForFieldPath(field: InternalFieldPath): ParseContext { const childPath = this.path?.child(field); const context = this.contextWith({ path: childPath, arrayElement: false }); context.validatePath(); @@ -277,7 +281,7 @@ export class ParseContext { } /** Returns 'true' if 'fieldPath' was traversed when creating this context. */ - contains(fieldPath: FieldPath): boolean { + contains(fieldPath: InternalFieldPath): boolean { return ( this.fieldMask.find(field => fieldPath.isPrefixOf(field)) !== undefined || this.fieldTransforms.find(transform => @@ -334,7 +338,7 @@ export class UserDataReader { dataSource, methodName, targetDoc, - path: FieldPath.emptyPath(), + path: InternalFieldPath.emptyPath(), arrayElement: false, hasConverter }, @@ -372,23 +376,14 @@ export function parseSetData( fieldMask = new FieldMask(context.fieldMask); fieldTransforms = context.fieldTransforms; } else if (options.mergeFields) { - const validatedFieldPaths: FieldPath[] = []; + const validatedFieldPaths: InternalFieldPath[] = []; for (const stringOrFieldPath of options.mergeFields) { - let fieldPath: FieldPath; - - if (stringOrFieldPath instanceof _BaseFieldPath) { - fieldPath = stringOrFieldPath._internalPath; - } else if (typeof stringOrFieldPath === 'string') { - fieldPath = fieldPathFromDotSeparatedString( - methodName, - stringOrFieldPath, - targetDoc - ); - } else { - throw fail('Expected stringOrFieldPath to be a string or a FieldPath'); - } - + const fieldPath = fieldPathFromArgument( + methodName, + stringOrFieldPath, + targetDoc + ); if (!context.contains(fieldPath)) { throw new FirestoreError( Code.INVALID_ARGUMENT, @@ -431,7 +426,7 @@ export function parseUpdateData( ); validatePlainObject('Data must be an object, but it was:', context, input); - const fieldMaskPaths: FieldPath[] = []; + const fieldMaskPaths: InternalFieldPath[] = []; const updateData = new ObjectValueBuilder(); forEach(input as Dict, (key, value) => { const path = fieldPathFromDotSeparatedString(methodName, key, targetDoc); @@ -468,7 +463,7 @@ export function parseUpdateVarargs( userDataReader: UserDataReader, methodName: string, targetDoc: DocumentKey, - field: string | _BaseFieldPath, + field: string | PublicFieldPath | Compat, value: unknown, moreFieldsAndValues: unknown[] ): ParsedUpdateData { @@ -492,13 +487,13 @@ export function parseUpdateVarargs( keys.push( fieldPathFromArgument( methodName, - moreFieldsAndValues[i] as string | _BaseFieldPath + moreFieldsAndValues[i] as string | PublicFieldPath ) ); values.push(moreFieldsAndValues[i + 1]); } - const fieldMaskPaths: FieldPath[] = []; + const fieldMaskPaths: InternalFieldPath[] = []; const updateData = new ObjectValueBuilder(); // We iterate in reverse order to pick the last value for a field if the @@ -797,16 +792,16 @@ function validatePlainObject( */ export function fieldPathFromArgument( methodName: string, - path: string | _BaseFieldPath | Compat<_BaseFieldPath>, + path: string | PublicFieldPath | Compat, targetDoc?: DocumentKey -): FieldPath { +): InternalFieldPath { // If required, replace the FieldPath Compat class with with the firestore-exp // FieldPath. if (path instanceof Compat) { - path = (path as Compat<_BaseFieldPath>)._delegate; + path = (path as Compat)._delegate; } - if (path instanceof _BaseFieldPath) { + if (path instanceof FieldPath) { return path._internalPath; } else if (typeof path === 'string') { return fieldPathFromDotSeparatedString(methodName, path); @@ -822,6 +817,11 @@ export function fieldPathFromArgument( } } +/** + * Matches any characters in a field path string that are reserved. + */ +const FIELD_PATH_RESERVED = new RegExp('[~\\*/\\[\\]]'); + /** * Wraps fromDotSeparatedString with an error message about the method that * was thrown. @@ -834,13 +834,25 @@ export function fieldPathFromDotSeparatedString( methodName: string, path: string, targetDoc?: DocumentKey -): FieldPath { +): InternalFieldPath { + const found = path.search(FIELD_PATH_RESERVED); + if (found >= 0) { + throw createError( + `Invalid field path (${path}). Paths must not contain ` + + `'~', '*', '/', '[', or ']'`, + methodName, + /* hasConverter= */ false, + /* path= */ undefined, + targetDoc + ); + } + try { - return fromDotSeparatedString(path)._internalPath; + return new FieldPath(...path.split('.'))._internalPath; } catch (e) { - const message = errorMessage(e); throw createError( - message, + `Invalid field path (${path}). Paths must not be empty, ` + + `begin with '.', end with '.', or contain '..'`, methodName, /* hasConverter= */ false, /* path= */ undefined, @@ -853,7 +865,7 @@ function createError( reason: string, methodName: string, hasConverter: boolean, - path?: FieldPath, + path?: InternalFieldPath, targetDoc?: DocumentKey ): FirestoreError { const hasPath = path && !path.isEmpty(); @@ -883,15 +895,10 @@ function createError( ); } -/** - * Extracts the message from a caught exception, which should be an Error object - * though JS doesn't guarantee that. - */ -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 { +function fieldMaskContains( + haystack: InternalFieldPath[], + needle: InternalFieldPath +): boolean { return haystack.some(v => v.isEqual(needle)); } diff --git a/packages/firestore/test/integration/util/firebase_export.ts b/packages/firestore/test/integration/util/firebase_export.ts index 3dfd156c8eb..6024db21309 100644 --- a/packages/firestore/test/integration/util/firebase_export.ts +++ b/packages/firestore/test/integration/util/firebase_export.ts @@ -34,6 +34,7 @@ import firebaseAppCompat from '@firebase/app-compat'; import * as exp from '../../../exp/test/shim'; import { FieldValue } from '../../../src/compat/field_value'; +import { FieldPath } from '../../../src/api/field_path'; import { FirebaseApp } from '@firebase/app-types'; import { Firestore, @@ -115,9 +116,6 @@ export function newTestFirestore( // eslint-disable-next-line @typescript-eslint/no-explicit-any const legacyNamespace = (firebase as any).firestore; -const FieldPath = usesFunctionalApi() - ? exp.FieldPath - : legacyNamespace.FieldPath; const Timestamp = usesFunctionalApi() ? exp.Timestamp : legacyNamespace.Timestamp; diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index 67c63479b9d..855fe5afb76 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -22,7 +22,6 @@ import * as api from '../../src/protos/firestore_proto_api'; import { expect } from 'chai'; import { Blob } from '../../src/api/blob'; -import { fromDotSeparatedString } from '../../src/api/field_path'; import { UserDataWriter } from '../../src/api/user_data_writer'; import { parseQueryValue, @@ -203,7 +202,7 @@ export function path(path: string, offset?: number): ResourcePath { } export function field(path: string): FieldPath { - return fromDotSeparatedString(path)._internalPath; + return new FieldPath(path.split('.')); } export function mask(...paths: string[]): FieldMask {