Skip to content

Add ignoreUndefinedProperties #3077

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions packages/firebase/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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;
}

/**
Expand Down
1 change: 1 addition & 0 deletions packages/firestore-types/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export interface Settings {
timestampsInSnapshots?: boolean;
cacheSizeBytes?: number;
experimentalForceLongPolling?: boolean;
ignoreUndefinedProperties?: boolean;
}

export interface PersistenceSettings {
Expand Down
5 changes: 5 additions & 0 deletions packages/firestore/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
40 changes: 33 additions & 7 deletions packages/firestore/src/api/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -169,7 +172,8 @@ class FirestoreSettings {
'credentials',
'timestampsInSnapshots',
'cacheSizeBytes',
'experimentalForceLongPolling'
'experimentalForceLongPolling',
'ignoreUndefinedProperties'
]);

validateNamedOptionalType(
Expand All @@ -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) {
Expand All @@ -202,6 +213,8 @@ class FirestoreSettings {
}
this.timestampsInSnapshots =
settings.timestampsInSnapshots ?? DEFAULT_TIMESTAMPS_IN_SNAPSHOTS;
this.ignoreUndefinedProperties =
settings.ignoreUndefinedProperties ?? DEFAULT_IGNORE_UNDEFINED_PROPERTIES;

validateNamedOptionalType(
'settings',
Expand Down Expand Up @@ -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 {
Expand All @@ -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
);
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
9 changes: 6 additions & 3 deletions packages/firestore/src/api/field_value.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)!
Expand Down Expand Up @@ -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)!
Expand All @@ -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(
Expand Down
15 changes: 13 additions & 2 deletions packages/firestore/src/api/user_data_reader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -172,6 +174,7 @@ export class ParseContext {
readonly settings: ContextSettings,
readonly databaseId: DatabaseId,
readonly serializer: JsonProtoSerializer,
readonly ignoreUndefinedProperties: boolean,
fieldTransforms?: FieldTransform[],
fieldMask?: FieldPath[]
) {
Expand All @@ -198,6 +201,7 @@ export class ParseContext {
{ ...this.settings, ...configuration },
this.databaseId,
this.serializer,
this.ignoreUndefinedProperties,
this.fieldTransforms,
this.fieldMask
);
Expand Down Expand Up @@ -276,6 +280,7 @@ export class UserDataReader {

constructor(
private readonly databaseId: DatabaseId,
private readonly ignoreUndefinedProperties: boolean,
serializer?: JsonProtoSerializer
) {
this.serializer =
Expand Down Expand Up @@ -458,7 +463,8 @@ export class UserDataReader {
arrayElement: false
},
this.databaseId,
this.serializer
this.serializer,
this.ignoreUndefinedProperties
);
}

Expand Down Expand Up @@ -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') {
Expand Down Expand Up @@ -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)}`
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/util/input_validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to align this error message with the rest of the validation messages.

)} argument to be a positive number, but it was: ${n}.`
);
Expand Down
60 changes: 59 additions & 1 deletion packages/firestore/test/integration/api/fields.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ import {
toDataArray,
withTestCollection,
withTestCollectionSettings,
withTestDoc
withTestDoc,
withTestDocAndSettings
} from '../util/helpers';

const FieldPath = firebase.firestore!.FieldPath;
Expand Down Expand Up @@ -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);
}
);
});
});
Loading