fix(firestore): fallback to valid resource path in createSnapshotFromJson - #1942
fix(firestore): fallback to valid resource path in createSnapshotFromJson#1942ajperel wants to merge 2 commits into
Conversation
…Json
If the document payload is missing the `name` field (e.g., during an `onDocumentDeleted` trigger where the "after" snapshot payload is intentionally absent), `createSnapshotFromJson` construct a mocked snapshot and defaults the resource path to the `event.source`.
However, the `event.source` for Firestore CloudEvents is typically just the database prefix (e.g. `//cloudevents.googleapis.com/projects/YOUR_PROJECT/databases/(default)`), which is an invalid Firestore document resource path as it lacks the `documents/DOC_ID` suffix. Falling back to this caused `Firestore#snapshot_` to crash with invalid resource path errors in various edge cases (such as PubSub fallbacks).
This commit updates the fallback to use `getPath(event)` instead, synthesizing a valid `projects/{project}/databases/{database}/documents/{document}` resource path to correctly mock the snapshot.
Partially addresses #1922
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Firestore provider to use getPath(event) instead of event.source when creating snapshots from JSON, ensuring the correct path is resolved even when the document value is missing. It also adds corresponding unit tests and updates helper documentation. The review feedback highlights two main improvement opportunities: first, using optional chaining when casting and accessing event.data to prevent potential runtime TypeError crashes if event.data is null or undefined; second, refactoring the global firestoreInstance caching mechanism to prevent cross-database bugs in multi-database environments.
| const snapshot = firestore.createBeforeSnapshot(rawEvent); | ||
|
|
||
| expect(snapshot.exists).to.be.false; | ||
| expect(snapshot.ref.path).to.eq("foo/fGRodw71mHutZ4wGDuT8"); |
There was a problem hiding this comment.
extreme nit: i think pretty much everything in this codebase uses .equal, which is identical to .eq because javascript devs are just Like That
3 participants
Description
If the document payload is missing the
namefield (e.g., during anonDocumentDeletedtrigger where the "after" snapshot payload is intentionally absent),createSnapshotFromJsonconstruct a mocked snapshot and defaults the resource path to theevent.source.However, the
event.sourcefor Firestore CloudEvents is typically just the database prefix (e.g.//cloudevents.googleapis.com/projects/YOUR_PROJECT/databases/(default)), which is an invalid Firestore document resource path as it lacks thedocuments/DOC_IDsuffix. Falling back to this causedFirestore#snapshot_to crash with invalid resource path errors in various edge cases (such as PubSub fallbacks).This commit updates the fallback to use
getPath(event)instead, synthesizing a validprojects/{project}/databases/{database}/documents/{document}resource path to correctly mock the snapshot.Also removes our cache of the Firestore instance to rely instead on the better (per db) cache inside of Admin SDK.
Partially addresses #1922
Scenarios Tested
npm test
firebase deploy --only functions (with a bug repro case)
Release notes
relnote: fix(firestore): fallback to valid resource path in createSnapshotFromJson (#1922)