diff --git a/packages/firebase/index.d.ts b/packages/firebase/index.d.ts index 90fd84b01fc..9987266aaad 100644 --- a/packages/firebase/index.d.ts +++ b/packages/firebase/index.d.ts @@ -6920,15 +6920,15 @@ declare namespace firebase.database.ServerValue { * ``` */ var TIMESTAMP: Object; - + /** - * Returns a placeholder value that can be used to atomically increment the + * Returns a placeholder value that can be used to atomically increment the * current database value by the provided delta. * * @param delta the amount to modify the current value atomically. * @return a placeholder value for modifying data atomically server-side. */ - function increment(delta: number) : Object; + function increment(delta: number): Object; } /** @@ -7743,6 +7743,14 @@ declare namespace firebase.firestore { * @webonly */ experimentalForceLongPolling?: boolean; + + /** + * Whether to skip nested properties that are set to `undefined` during + * object serialization. If set to `true`, these properties are skipped + * and not written to Firestore. If set `false` or omitted, the SDK throws + * an exception when it encounters properties of type `undefined`. + */ + ignoreUndefinedProperties?: boolean; } /** diff --git a/packages/firestore-types/index.d.ts b/packages/firestore-types/index.d.ts index d57e8637855..4b5e745f70a 100644 --- a/packages/firestore-types/index.d.ts +++ b/packages/firestore-types/index.d.ts @@ -29,6 +29,7 @@ export interface Settings { timestampsInSnapshots?: boolean; cacheSizeBytes?: number; experimentalForceLongPolling?: boolean; + ignoreUndefinedProperties?: boolean; } export interface PersistenceSettings { diff --git a/packages/firestore/CHANGELOG.md b/packages/firestore/CHANGELOG.md index fb6e861e858..da9fafb1fb8 100644 --- a/packages/firestore/CHANGELOG.md +++ b/packages/firestore/CHANGELOG.md @@ -1,4 +1,9 @@ # Unreleased +- [feature] Added support for calling `FirebaseFiresore.settings` with + `{ ignoreUndefinedProperties: true }`. When set, Firestore ignores + undefined properties inside objects rather than rejecting the API call. + +# Released - [fixed] Fixed a regression introduced in v7.14.2 that incorrectly applied a `FieldValue.increment` in combination with `set({...}, {merge: true})`. - [fixed] Firestore now rejects `onSnapshot()` listeners if they cannot be diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts index 4b82f0b0593..01b58340ca1 100644 --- a/packages/firestore/src/api/database.ts +++ b/packages/firestore/src/api/database.ts @@ -98,6 +98,7 @@ const DEFAULT_HOST = 'firestore.googleapis.com'; const DEFAULT_SSL = true; const DEFAULT_TIMESTAMPS_IN_SNAPSHOTS = true; const DEFAULT_FORCE_LONG_POLLING = false; +const DEFAULT_IGNORE_UNDEFINED_PROPERTIES = false; /** * Constant used to indicate the LRU garbage collection should be disabled. @@ -142,6 +143,8 @@ class FirestoreSettings { readonly forceLongPolling: boolean; + readonly ignoreUndefinedProperties: boolean; + // Can be a google-auth-library or gapi client. // eslint-disable-next-line @typescript-eslint/no-explicit-any credentials?: any; @@ -169,7 +172,8 @@ class FirestoreSettings { 'credentials', 'timestampsInSnapshots', 'cacheSizeBytes', - 'experimentalForceLongPolling' + 'experimentalForceLongPolling', + 'ignoreUndefinedProperties' ]); validateNamedOptionalType( @@ -187,6 +191,13 @@ class FirestoreSettings { settings.timestampsInSnapshots ); + validateNamedOptionalType( + 'settings', + 'boolean', + 'ignoreUndefinedProperties', + settings.ignoreUndefinedProperties + ); + // Nobody should set timestampsInSnapshots anymore, but the error depends on // whether they set it to true or false... if (settings.timestampsInSnapshots === true) { @@ -202,6 +213,8 @@ class FirestoreSettings { } this.timestampsInSnapshots = settings.timestampsInSnapshots ?? DEFAULT_TIMESTAMPS_IN_SNAPSHOTS; + this.ignoreUndefinedProperties = + settings.ignoreUndefinedProperties ?? DEFAULT_IGNORE_UNDEFINED_PROPERTIES; validateNamedOptionalType( 'settings', @@ -232,9 +245,7 @@ class FirestoreSettings { settings.experimentalForceLongPolling ); this.forceLongPolling = - settings.experimentalForceLongPolling === undefined - ? DEFAULT_FORCE_LONG_POLLING - : settings.experimentalForceLongPolling; + settings.experimentalForceLongPolling ?? DEFAULT_FORCE_LONG_POLLING; } isEqual(other: FirestoreSettings): boolean { @@ -244,7 +255,8 @@ class FirestoreSettings { this.timestampsInSnapshots === other.timestampsInSnapshots && this.credentials === other.credentials && this.cacheSizeBytes === other.cacheSizeBytes && - this.forceLongPolling === other.forceLongPolling + this.forceLongPolling === other.forceLongPolling && + this.ignoreUndefinedProperties === other.ignoreUndefinedProperties ); } } @@ -275,7 +287,7 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService { // TODO(mikelehen): Use modularized initialization instead. readonly _queue = new AsyncQueue(); - readonly _dataReader: UserDataReader; + _userDataReader: UserDataReader | undefined; // Note: We are using `MemoryComponentProvider` as a default // ComponentProvider to ensure backwards compatibility with the format @@ -310,7 +322,21 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService { this._componentProvider = componentProvider; this._settings = new FirestoreSettings({}); - this._dataReader = new UserDataReader(this._databaseId); + } + + get _dataReader(): UserDataReader { + debugAssert( + !!this._firestoreClient, + 'Cannot obtain UserDataReader before instance is intitialized' + ); + if (!this._userDataReader) { + // Lazy initialize UserDataReader once the settings are frozen + this._userDataReader = new UserDataReader( + this._databaseId, + this._settings.ignoreUndefinedProperties + ); + } + return this._userDataReader; } settings(settingsLiteral: firestore.Settings): void { diff --git a/packages/firestore/src/api/field_value.ts b/packages/firestore/src/api/field_value.ts index d03b6f34f52..1e895ba94c6 100644 --- a/packages/firestore/src/api/field_value.ts +++ b/packages/firestore/src/api/field_value.ts @@ -109,7 +109,8 @@ export class ArrayUnionFieldValueImpl extends FieldValueImpl { arrayElement: true }, context.databaseId, - context.serializer + context.serializer, + context.ignoreUndefinedProperties ); const parsedElements = this._elements.map( element => parseData(element, parseContext)! @@ -140,7 +141,8 @@ export class ArrayRemoveFieldValueImpl extends FieldValueImpl { arrayElement: true }, context.databaseId, - context.serializer + context.serializer, + context.ignoreUndefinedProperties ); const parsedElements = this._elements.map( element => parseData(element, parseContext)! @@ -167,7 +169,8 @@ export class NumericIncrementFieldValueImpl extends FieldValueImpl { methodName: this._methodName }, context.databaseId, - context.serializer + context.serializer, + context.ignoreUndefinedProperties ); const operand = parseData(this._operand, parseContext)!; const numericIncrement = new NumericIncrementTransformOperation( diff --git a/packages/firestore/src/api/user_data_reader.ts b/packages/firestore/src/api/user_data_reader.ts index f4e6b070f32..8588baf494c 100644 --- a/packages/firestore/src/api/user_data_reader.ts +++ b/packages/firestore/src/api/user_data_reader.ts @@ -158,6 +158,8 @@ export class ParseContext { * @param settings The settings for the parser. * @param databaseId The database ID of the Firestore instance. * @param serializer The serializer to use to generate the Value proto. + * @param ignoreUndefinedProperties Whether to ignore undefined properties + * rather than throw. * @param fieldTransforms A mutable list of field transforms encountered while * parsing the data. * @param fieldMask A mutable list of field paths encountered while parsing @@ -172,6 +174,7 @@ export class ParseContext { readonly settings: ContextSettings, readonly databaseId: DatabaseId, readonly serializer: JsonProtoSerializer, + readonly ignoreUndefinedProperties: boolean, fieldTransforms?: FieldTransform[], fieldMask?: FieldPath[] ) { @@ -198,6 +201,7 @@ export class ParseContext { { ...this.settings, ...configuration }, this.databaseId, this.serializer, + this.ignoreUndefinedProperties, this.fieldTransforms, this.fieldMask ); @@ -276,6 +280,7 @@ export class UserDataReader { constructor( private readonly databaseId: DatabaseId, + private readonly ignoreUndefinedProperties: boolean, serializer?: JsonProtoSerializer ) { this.serializer = @@ -458,7 +463,8 @@ export class UserDataReader { arrayElement: false }, this.databaseId, - this.serializer + this.serializer, + this.ignoreUndefinedProperties ); } @@ -613,7 +619,10 @@ function parseSentinelFieldValue( * * @return The parsed value */ -function parseScalarValue(value: unknown, context: ParseContext): api.Value { +function parseScalarValue( + value: unknown, + context: ParseContext +): api.Value | null { if (value === null) { return { nullValue: 'NULL_VALUE' }; } else if (typeof value === 'number') { @@ -659,6 +668,8 @@ function parseScalarValue(value: unknown, context: ParseContext): api.Value { value.firestore._databaseId ) }; + } else if (value === undefined && context.ignoreUndefinedProperties) { + return null; } else { throw context.createError( `Unsupported field value: ${valueDescription(value)}` diff --git a/packages/firestore/src/util/input_validation.ts b/packages/firestore/src/util/input_validation.ts index fe15c26c4d2..9318e66fa60 100644 --- a/packages/firestore/src/util/input_validation.ts +++ b/packages/firestore/src/util/input_validation.ts @@ -461,7 +461,7 @@ export function validatePositiveNumber( if (n <= 0) { throw new FirestoreError( Code.INVALID_ARGUMENT, - `Function "${functionName}()" requires its ${ordinal( + `Function ${functionName}() requires its ${ordinal( position )} argument to be a positive number, but it was: ${n}.` ); diff --git a/packages/firestore/test/integration/api/fields.test.ts b/packages/firestore/test/integration/api/fields.test.ts index ee8d55acf18..2a4aae355f4 100644 --- a/packages/firestore/test/integration/api/fields.test.ts +++ b/packages/firestore/test/integration/api/fields.test.ts @@ -23,7 +23,8 @@ import { toDataArray, withTestCollection, withTestCollectionSettings, - withTestDoc + withTestDoc, + withTestDocAndSettings } from '../util/helpers'; const FieldPath = firebase.firestore!.FieldPath; @@ -433,3 +434,60 @@ apiDescribe('Timestamp Fields in snapshots', (persistence: boolean) => { }); }); }); + +apiDescribe('`undefined` properties', (persistence: boolean) => { + const settings = { ...DEFAULT_SETTINGS }; + settings.ignoreUndefinedProperties = true; + + it('are ignored in set()', () => { + return withTestDocAndSettings(persistence, settings, async doc => { + await doc.set({ foo: 'foo', 'bar': undefined }); + const docSnap = await doc.get(); + expect(docSnap.data()).to.deep.equal({ foo: 'foo' }); + }); + }); + + it('are ignored in update()', () => { + return withTestDocAndSettings(persistence, settings, async doc => { + await doc.set({}); + await doc.update({ a: { foo: 'foo', 'bar': undefined } }); + await doc.update('b', { foo: 'foo', 'bar': undefined }); + const docSnap = await doc.get(); + expect(docSnap.data()).to.deep.equal({ + a: { foo: 'foo' }, + b: { foo: 'foo' } + }); + }); + }); + + it('are ignored in Query.where()', () => { + return withTestCollectionSettings( + persistence, + settings, + { 'doc1': { nested: { foo: 'foo' } } }, + async coll => { + const query = coll.where('nested', '==', { + foo: 'foo', + 'bar': undefined + }); + const querySnap = await query.get(); + expect(querySnap.size).to.equal(1); + } + ); + }); + + it('are ignored in Query.startAt()', () => { + return withTestCollectionSettings( + persistence, + settings, + { 'doc1': { nested: { foo: 'foo' } } }, + async coll => { + const query = coll + .orderBy('nested') + .startAt({ foo: 'foo', 'bar': undefined }); + const querySnap = await query.get(); + expect(querySnap.size).to.equal(1); + } + ); + }); +}); diff --git a/packages/firestore/test/integration/api/validation.test.ts b/packages/firestore/test/integration/api/validation.test.ts index d43483df9ec..9e20e7d195f 100644 --- a/packages/firestore/test/integration/api/validation.test.ts +++ b/packages/firestore/test/integration/api/validation.test.ts @@ -421,7 +421,8 @@ apiDescribe('Validation:', (persistence: boolean) => { new Date(), null, () => {}, - new TestClass('foo') + new TestClass('foo'), + undefined ]; const errorDescriptions = [ '42', @@ -515,6 +516,7 @@ apiDescribe('Validation:', (persistence: boolean) => { }); validationIt(persistence, 'must not contain undefined.', db => { + // Note: This test uses the default setting for `ignoreUndefinedProperties`. return expectWriteToFail( db, { foo: undefined }, @@ -734,6 +736,11 @@ apiDescribe('Validation:', (persistence: boolean) => { 'Function FieldValue.arrayRemove() called with invalid data. ' + 'Unsupported field value: a custom TestClass object' ); + + expect(() => doc.set({ x: FieldValue.arrayRemove(undefined) })).to.throw( + 'Function FieldValue.arrayRemove() called with invalid data. ' + + 'Unsupported field value: undefined' + ); }); validationIt(persistence, 'reject arrays', db => { @@ -785,6 +792,12 @@ apiDescribe('Validation:', (persistence: boolean) => { 'Function FieldValue.increment() requires its first argument to ' + 'be of type number, but it was: "foo"' ); + expect(() => + doc.set({ x: FieldValue.increment(undefined as any) }) + ).to.throw( + 'Function FieldValue.increment() requires its first argument to ' + + 'be of type number, but it was: undefined' + ); }); validationIt(persistence, 'reject more than one argument', db => { @@ -802,11 +815,11 @@ apiDescribe('Validation:', (persistence: boolean) => { validationIt(persistence, 'with non-positive limit fail', db => { const collection = db.collection('test'); expect(() => collection.limit(0)).to.throw( - 'Function "Query.limit()" requires its first argument to be a positive number, ' + + 'Function Query.limit() requires its first argument to be a positive number, ' + 'but it was: 0.' ); expect(() => collection.limitToLast(-1)).to.throw( - 'Function "Query.limitToLast()" requires its first argument to be a positive number, ' + + 'Function Query.limitToLast() requires its first argument to be a positive number, ' + 'but it was: -1.' ); }); @@ -1348,6 +1361,16 @@ apiDescribe('Validation:', (persistence: boolean) => { ); } ); + + validationIt(persistence, 'cannot pass undefined as a field value', db => { + const collection = db.collection('test'); + expect(() => collection.where('foo', '==', undefined)).to.throw( + 'Function Query.where() requires a valid third argument, but it was undefined.' + ); + expect(() => collection.orderBy('foo').startAt(undefined)).to.throw( + 'Function Query.startAt() requires a valid first argument, but it was undefined.' + ); + }); }); }); diff --git a/packages/firestore/test/integration/util/helpers.ts b/packages/firestore/test/integration/util/helpers.ts index 152acd784d0..201622e7d02 100644 --- a/packages/firestore/test/integration/util/helpers.ts +++ b/packages/firestore/test/integration/util/helpers.ts @@ -233,7 +233,6 @@ export async function withTestDbsSettings( try { await fn(dbs); } finally { - await wipeDb(dbs[0]); for (const db of dbs) { await db.terminate(); if (persistence) { @@ -252,6 +251,22 @@ export function withTestDoc( }); } +export function withTestDocAndSettings( + persistence: boolean, + settings: firestore.Settings, + fn: (doc: firestore.DocumentReference) => Promise +): Promise { + return withTestDbsSettings( + persistence, + DEFAULT_PROJECT_ID, + settings, + 1, + ([db]) => { + return fn(db.collection('test-collection').doc()); + } + ); +} + // TODO(rsgowman): Modify withTestDoc to take in (an optional) initialData and // fix existing usages of it. Then delete this function. This makes withTestDoc // more analogous to withTestCollection and eliminates the pattern of @@ -310,9 +325,3 @@ export function withTestCollectionSettings( } ); } - -function wipeDb(db: firestore.FirebaseFirestore): Promise { - // TODO(dimond): actually wipe DB and assert or listenables have been turned - // off. We probably need deep queries for this. - return Promise.resolve(undefined); -} diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index e2de8705e72..8dc7545daf2 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -112,6 +112,7 @@ export function testUserDataReader(useProto3Json?: boolean): UserDataReader { const databaseId = new DatabaseId('test-project'); return new UserDataReader( databaseId, + /* ignoreUndefinedProperties= */ false, useProto3Json !== undefined ? new JsonProtoSerializer(databaseId, { useProto3Json }) : undefined