fix(delete-user-data): resolve the RTDB client on first use - #2955
fix(delete-user-data): resolve the RTDB client on first use#2955IzaakGough wants to merge 1 commit into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request updates the delete-user-data kit to support environments without a configured Realtime Database (RTDB) instance. It defers the initialization of the RTDB client using a getter to prevent startup crashes when a database URL is missing, adds a new emulator test app and test suite to verify behavior without RTDB, and includes a test for RTDB path deletion. Feedback suggests caching the resolved database instance in the getter to avoid redundant calls and potential issues if the context is destructured.
Sorry, something went wrong.
| ctx = { | ||
| firestore: getFirestore(resolved.firestoreDatabaseId), | ||
| storage: admin.storage(), | ||
| database: admin.database(), | ||
| // Resolved on first use. Without a configured RTDB instance there is no | ||
| // databaseURL to initialize the app with, and admin.database() throws. | ||
| get database() { | ||
| return admin.database(); | ||
| }, |
There was a problem hiding this comment.
While resolving the database client on first use is a great way to prevent eager initialization errors, calling admin.database() on every property access can be inefficient if accessed multiple times (e.g., in loops or multiple helper calls). Additionally, if ctx is ever destructured in the future (e.g., const { database } = ctx), it will trigger this getter and throw if RTDB is not configured.
We can optimize this and make it more robust by caching the resolved database instance in a local variable within getContext().
| ctx = { | |
| firestore: getFirestore(resolved.firestoreDatabaseId), | |
| storage: admin.storage(), | |
| database: admin.database(), | |
| // Resolved on first use. Without a configured RTDB instance there is no | |
| // databaseURL to initialize the app with, and admin.database() throws. | |
| get database() { | |
| return admin.database(); | |
| }, | |
| let db: admin.database.Database | undefined; | |
| ctx = { | |
| firestore: getFirestore(resolved.firestoreDatabaseId), | |
| storage: admin.storage(), | |
| // Resolved on first use. Without a configured RTDB instance there is no | |
| // databaseURL to initialize the app with, and admin.database() throws. | |
| get database() { | |
| return (db ??= admin.database()); | |
| }, |
Sorry, something went wrong.
getContext called admin.database() eagerly. With no RTDB instance configured there is no databaseURL to initialize the app with, so the call threw "Can't determine Firebase Database URL" and killed every invocation of clearData, handleSearch and handleDeletion, including for users who only delete Firestore data. Passing explicit options to initializeApp suppresses the FIREBASE_CONFIG fallback, so a deployed instance never picks up the project's default database URL either. Resolving the client on first use matches the extension, which calls admin.database() inside the RTDB deletion path. Adds an emulator codebase configured without an instance to cover the case, and asserts RTDB paths are still cleared when one is configured.
Stacked on #2949, which adds the test suites this uses. Review that one first.
What was broken
getContext called admin.database() eagerly. With no RTDB instance configured, SELECTED_DATABASE_INSTANCE defaults to empty, getDatabaseUrl returns null, and initializeApp gets no databaseURL, so the call throws Can't determine Firebase Database URL. That happens before any work, on every invocation of clearData, handleSearch and handleDeletion. A user deleting only Firestore data gets a kit that never runs.
Passing explicit options to initializeApp also suppresses the FIREBASE_CONFIG fallback, so a deployed instance does not pick up the project's default database URL. The extension avoids this by calling admin.database() inside the RTDB deletion path rather than up front.
What changed
database becomes a getter, resolved on first use. A second emulator codebase configured without an instance covers the case, and a new test asserts RTDB paths are still cleared when one is configured.
Verification
Reverting the getter fails the new test and nothing else. Skipping the RTDB branch fails the new RTDB test. Unit suite 54 passing, emulator suite 18.
The failure was also reproduced on a live deploy to a real project, on Node 24 2nd gen, with the same stack at /workspace/lib/index.js:97. The live check covered the failing direction only; the recovery direction is covered by the emulator.