Skip to content

Commit accd742

Browse files
Debug and release asserts
This PR aims to differentiate between asserts that we should handle in production builds (e.g. we received invalid data) and asserts we handle only during testing (our local state is corrupt). Some of the distinctions are really obvious, and other are anything but. In a lot of cases, we actually have multiple asserts for a single condition. I opted to only make the initial assertion a hardAssert (e.g. when we reveive mutation results, we assert multiple times that the number of results matches the mutation count). softAssert is a temporary name that I will rename back to assert once this is done. It is meant to make the decisions here more obvious
1 parent 437c404 commit accd742

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

59 files changed

+451
-373
lines changed

packages/firestore/src/api/credentials.ts

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
22
* @license
3-
* Copyright 2017 Google Inc.
3+
* Copyright 2017 Google LLC
44
*
55
* Licensed under the Apache License, Version 2.0 (the "License");
66
* you may not use this file except in compliance with the License.
@@ -16,7 +16,7 @@
1616
*/
1717

1818
import { User } from '../auth/user';
19-
import { assert } from '../util/assert';
19+
import { hardAssert, softAssert } from '../util/assert';
2020
import { Code, FirestoreError } from '../util/error';
2121
import {
2222
FirebaseAuthInternal,
@@ -116,14 +116,14 @@ export class EmptyCredentialsProvider implements CredentialsProvider {
116116
invalidateToken(): void {}
117117

118118
setChangeListener(changeListener: CredentialChangeListener): void {
119-
assert(!this.changeListener, 'Can only call setChangeListener() once.');
119+
softAssert(!this.changeListener, 'Can only call setChangeListener() once.');
120120
this.changeListener = changeListener;
121121
// Fire with initial user.
122122
changeListener(User.UNAUTHENTICATED);
123123
}
124124

125125
removeChangeListener(): void {
126-
assert(
126+
softAssert(
127127
this.changeListener !== null,
128128
'removeChangeListener() when no listener registered'
129129
);
@@ -190,7 +190,7 @@ export class FirebaseCredentialsProvider implements CredentialsProvider {
190190
}
191191

192192
getToken(): Promise<Token | null> {
193-
assert(
193+
softAssert(
194194
this.tokenListener != null,
195195
'getToken cannot be called after listener removed.'
196196
);
@@ -217,7 +217,7 @@ export class FirebaseCredentialsProvider implements CredentialsProvider {
217217
);
218218
} else {
219219
if (tokenData) {
220-
assert(
220+
hardAssert(
221221
typeof tokenData.accessToken === 'string',
222222
'Invalid tokenData returned from getToken():' + tokenData
223223
);
@@ -234,7 +234,7 @@ export class FirebaseCredentialsProvider implements CredentialsProvider {
234234
}
235235

236236
setChangeListener(changeListener: CredentialChangeListener): void {
237-
assert(!this.changeListener, 'Can only call setChangeListener() once.');
237+
softAssert(!this.changeListener, 'Can only call setChangeListener() once.');
238238
this.changeListener = changeListener;
239239

240240
// Fire the initial event
@@ -244,8 +244,11 @@ export class FirebaseCredentialsProvider implements CredentialsProvider {
244244
}
245245

246246
removeChangeListener(): void {
247-
assert(this.tokenListener != null, 'removeChangeListener() called twice');
248-
assert(
247+
softAssert(
248+
this.tokenListener != null,
249+
'removeChangeListener() called twice'
250+
);
251+
softAssert(
249252
this.changeListener !== null,
250253
'removeChangeListener() called when no listener registered'
251254
);
@@ -263,7 +266,7 @@ export class FirebaseCredentialsProvider implements CredentialsProvider {
263266
// to guarantee to get the actual user.
264267
private getUser(): User {
265268
const currentUid = this.auth && this.auth.getUid();
266-
assert(
269+
hardAssert(
267270
currentUid === null || typeof currentUid === 'string',
268271
'Received invalid UID: ' + currentUid
269272
);
@@ -342,7 +345,7 @@ export function makeCredentialsProvider(
342345
case 'gapi':
343346
const client = credentials.client as Gapi;
344347
// Make sure this really is a Gapi client.
345-
assert(
348+
hardAssert(
346349
!!(
347350
typeof client === 'object' &&
348351
client !== null &&

packages/firestore/src/api/database.ts

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ import { isServerTimestamp } from '../model/server_timestamps';
4747
import { refValue } from '../model/values';
4848
import { PlatformSupport } from '../platform/platform';
4949
import { makeConstructorPrivate } from '../util/api';
50-
import { assert, fail } from '../util/assert';
50+
import { softAssert, fail } from '../util/assert';
5151
import { AsyncObserver } from '../util/async_observer';
5252
import { AsyncQueue } from '../util/async_queue';
5353
import { Code, FirestoreError } from '../util/error';
@@ -507,9 +507,12 @@ export class Firestore implements firestore.FirebaseFirestore, FirebaseService {
507507
persistenceProvider: PersistenceProvider,
508508
persistenceSettings: PersistenceSettings
509509
): Promise<void> {
510-
assert(!!this._settings.host, 'FirestoreSettings.host is not set');
510+
softAssert(!!this._settings.host, 'FirestoreSettings.host is not set');
511511

512-
assert(!this._firestoreClient, 'configureClient() called multiple times');
512+
softAssert(
513+
!this._firestoreClient,
514+
'configureClient() called multiple times'
515+
);
513516

514517
const databaseInfo = this.makeDatabaseInfo();
515518

@@ -1206,7 +1209,7 @@ export class DocumentReference<T = firestore.DocumentData>
12061209
const asyncObserver = new AsyncObserver<ViewSnapshot>({
12071210
next: snapshot => {
12081211
if (observer.next) {
1209-
assert(
1212+
softAssert(
12101213
snapshot.docs.size <= 1,
12111214
'Too many documents returned on a document query'
12121215
);
@@ -1451,7 +1454,7 @@ export class QueryDocumentSnapshot<T = firestore.DocumentData>
14511454
implements firestore.QueryDocumentSnapshot<T> {
14521455
data(options?: SnapshotOptions): T {
14531456
const data = super.data(options);
1454-
assert(
1457+
softAssert(
14551458
data !== undefined,
14561459
'Document in a QueryDocumentSnapshot should exist'
14571460
);
@@ -2533,11 +2536,11 @@ export function changesFromSnapshot<T>(
25332536
snapshot.mutatedKeys.has(change.doc.key),
25342537
converter
25352538
);
2536-
assert(
2539+
softAssert(
25372540
change.type === ChangeType.Added,
25382541
'Invalid event type for first snapshot'
25392542
);
2540-
assert(
2543+
softAssert(
25412544
!lastDoc || snapshot.query.docComparator(lastDoc, change.doc) < 0,
25422545
'Got added events in wrong order'
25432546
);
@@ -2570,7 +2573,7 @@ export function changesFromSnapshot<T>(
25702573
let newIndex = -1;
25712574
if (change.type !== ChangeType.Added) {
25722575
oldIndex = indexTracker.indexOf(change.doc.key);
2573-
assert(oldIndex >= 0, 'Index for document not found');
2576+
softAssert(oldIndex >= 0, 'Index for document not found');
25742577
indexTracker = indexTracker.delete(change.doc.key);
25752578
}
25762579
if (change.type !== ChangeType.Removed) {

packages/firestore/src/api/user_data_reader.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ import {
3232
TransformMutation
3333
} from '../model/mutation';
3434
import { FieldPath } from '../model/path';
35-
import { assert, fail } from '../util/assert';
35+
import { softAssert, fail } from '../util/assert';
3636
import { Code, FirestoreError } from '../util/error';
3737
import { isPlainObject, valueDescription } from '../util/input_validation';
3838
import { Dict, forEach, isEmpty } from '../util/obj';
@@ -494,8 +494,8 @@ export class UserDataReader {
494494
FieldPath.EMPTY_PATH
495495
);
496496
const parsed = this.parseData(input, context);
497-
assert(parsed != null, 'Parsed data should not be null.');
498-
assert(
497+
softAssert(parsed != null, 'Parsed data should not be null.');
498+
softAssert(
499499
context.fieldTransforms.length === 0,
500500
'Field transforms should have been disallowed.'
501501
);
@@ -633,7 +633,7 @@ export class UserDataReader {
633633
// fieldMask so it gets deleted.
634634
context.fieldMask.push(context.path);
635635
} else if (context.dataSource === UserDataSource.Update) {
636-
assert(
636+
softAssert(
637637
context.path.length > 0,
638638
'FieldValue.delete() at the top level should have already' +
639639
' been handled.'

packages/firestore/src/api/user_data_writer.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ import {
3535
getLocalWriteTime,
3636
getPreviousValue
3737
} from '../model/server_timestamps';
38-
import { assert, fail } from '../util/assert';
38+
import { fail, hardAssert } from '../util/assert';
3939
import { forEach } from '../util/obj';
4040
import { TypeOrder } from '../model/field_value';
4141
import { ResourcePath } from '../model/path';
@@ -130,7 +130,7 @@ export class UserDataWriter<T = firestore.DocumentData> {
130130

131131
private convertReference(name: string): DocumentReference<T> {
132132
const resourcePath = ResourcePath.fromString(name);
133-
assert(
133+
hardAssert(
134134
isValidResourceName(resourcePath),
135135
'ReferenceValue is not valid ' + name
136136
);

packages/firestore/src/core/event_manager.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
22
* @license
3-
* Copyright 2017 Google Inc.
3+
* Copyright 2017 Google LLC
44
*
55
* Licensed under the Apache License, Version 2.0 (the "License");
66
* you may not use this file except in compliance with the License.
@@ -15,7 +15,7 @@
1515
* limitations under the License.
1616
*/
1717

18-
import { assert } from '../util/assert';
18+
import { softAssert } from '../util/assert';
1919
import { EventHandler } from '../util/misc';
2020
import { ObjectMap } from '../util/obj_map';
2121
import { Query } from './query';
@@ -73,7 +73,7 @@ export class EventManager implements SyncEngineListener {
7373

7474
// Run global snapshot listeners if a consistent snapshot has been emitted.
7575
const raisedEvent = listener.applyOnlineStateChange(this.onlineState);
76-
assert(
76+
softAssert(
7777
!raisedEvent,
7878
"applyOnlineStateChange() shouldn't raise an event for brand-new listeners."
7979
);
@@ -226,7 +226,7 @@ export class QueryListener {
226226
* indeed raised.
227227
*/
228228
onViewSnapshot(snap: ViewSnapshot): boolean {
229-
assert(
229+
softAssert(
230230
snap.docChanges.length > 0 || snap.syncStateChanged,
231231
'We got a new snapshot with no changes?'
232232
);
@@ -288,7 +288,7 @@ export class QueryListener {
288288
snap: ViewSnapshot,
289289
onlineState: OnlineState
290290
): boolean {
291-
assert(
291+
softAssert(
292292
!this.raisedInitialEvent,
293293
'Determining whether to raise first event but already had first event'
294294
);
@@ -304,7 +304,7 @@ export class QueryListener {
304304
// Don't raise the event if we're online, aren't synced yet (checked
305305
// above) and are waiting for a sync.
306306
if (this.options.waitForSyncWhenOnline && maybeOnline) {
307-
assert(
307+
softAssert(
308308
snap.fromCache,
309309
'Waiting for sync, but snapshot is not from cache'
310310
);
@@ -337,7 +337,7 @@ export class QueryListener {
337337
}
338338

339339
private raiseInitialEvent(snap: ViewSnapshot): void {
340-
assert(
340+
softAssert(
341341
!this.raisedInitialEvent,
342342
'Trying to raise initial events for second time'
343343
);

0 commit comments

Comments
 (0)