Skip to content

Firestore: query.test.ts: use a different Firestore instance instead of a WriteBatch #7486

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.

Sign up for GitHub

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 1 commit into from
Jul 24, 2023
Merged

Firestore: query.test.ts: use a different Firestore instance instead of a WriteBatch #7486

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 17 additions & 14 deletions packages/firestore/test/integration/api/query.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ import {
toChangesArray,
toDataArray,
toIds,
PERSISTENCE_MODE_UNSPECIFIED,
withEmptyTestCollection,
withRetry,
withTestCollection,
Expand Down Expand Up @@ -2106,16 +2107,18 @@ apiDescribe('Queries', persistence => {
snapshot => snapshot.ref
);

// Delete 50 of the 100 documents. Use a WriteBatch, rather than
// deleteDoc(), to avoid affecting the local cache.
// Delete 50 of the 100 documents. Use a different Firestore
// instance to avoid affecting the local cache.
const deletedDocumentIds = new Set<string>();
const writeBatchForDocumentDeletes = writeBatch(db);
for (let i = 0; i < createdDocuments.length; i += 2) {
const documentToDelete = createdDocuments[i];
writeBatchForDocumentDeletes.delete(documentToDelete);
deletedDocumentIds.add(documentToDelete.id);
}
await writeBatchForDocumentDeletes.commit();
await withTestDb(PERSISTENCE_MODE_UNSPECIFIED, async db2 => {
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't this be returning the same Firestore instance(db === db2)? How it would help us avoid affecting cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question (I had to look into this too). But the answer is no because withTestDb() creates and returns a newly-created, distinct Firestore instance. Therefore, db and db2 have distinct caches and do not share anything, and using db2 to delete/update documents does not affect the local cache of db.

Here is the definition of withTestDb():

firebase-js-sdk/packages/firestore/test/lite/helpers.ts

Lines 57 to 61 in 223ac18

export function withTestDb(
fn: (db: Firestore) => void | Promise<void>
): Promise<void> {
return withTestDbSettings(DEFAULT_PROJECT_ID, DEFAULT_SETTINGS, fn);
}

which calls withTestDbSettings():

firebase-js-sdk/packages/firestore/test/lite/helpers.ts

Lines 43 to 55 in 223ac18

export async function withTestDbSettings(
projectId: string,
settings: FirestoreSettings,
fn: (db: Firestore) => void | Promise<void>
): Promise<void> {
const app = initializeApp(
{ apiKey: 'fake-api-key', projectId },
'test-app-' + appCount++
);
const firestore = initializeFirestore(app, settings);
return fn(firestore);
}

which creates a brand new FirestoreApp with a unique name (the appCount counter increments for each invocation) and then creates a brand new Firestore object from it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. So they are instances to a same firestore, but from different apps.

    { apiKey: 'fake-api-key', projectId },
    'test-app-' + appCount++
  );```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, exactly.

const batch = writeBatch(db2);
for (let i = 0; i < createdDocuments.length; i += 2) {
const documentToDelete = doc(db2, createdDocuments[i].path);
batch.delete(documentToDelete);
deletedDocumentIds.add(documentToDelete.id);
}
await batch.commit();
});

// Wait for 10 seconds, during which Watch will stop tracking the
// query and will send an existence filter rather than "delete"
Expand Down Expand Up @@ -2258,12 +2261,12 @@ apiDescribe('Queries', persistence => {
);

// Delete one of the documents so that the next call to getDocs() will
// experience an existence filter mismatch. Use a WriteBatch, rather
// than deleteDoc(), to avoid affecting the local cache.
// experience an existence filter mismatch. Use a different Firestore
// instance to avoid affecting the local cache.
const documentToDelete = doc(coll, 'DocumentToDelete');
const writeBatchForDocumentDeletes = writeBatch(db);
writeBatchForDocumentDeletes.delete(documentToDelete);
await writeBatchForDocumentDeletes.commit();
await withTestDb(PERSISTENCE_MODE_UNSPECIFIED, async db2 => {
await deleteDoc(doc(db2, documentToDelete.path));
});

// Wait for 10 seconds, during which Watch will stop tracking the query
// and will send an existence filter rather than "delete" events when
Expand Down