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
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.
| 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()); | |
| }, |
166026d to
9391e3f
Compare
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.
b109bf9 to
64d6498
Compare
Stacked on #2949, which adds the test suites this uses. Review that one first.
What was broken
getContextcalledadmin.database()eagerly. With no RTDB instance configured,SELECTED_DATABASE_INSTANCEdefaults to empty,getDatabaseUrlreturns null, andinitializeAppgets nodatabaseURL, so the call throwsCan't determine Firebase Database URL. That happens before any work, on every invocation ofclearData,handleSearchandhandleDeletion. A user deleting only Firestore data gets a kit that never runs.Passing explicit options to
initializeAppalso suppresses theFIREBASE_CONFIGfallback, so a deployed instance does not pick up the project's default database URL. The extension avoids this by callingadmin.database()inside the RTDB deletion path rather than up front.What changed
databasebecomes 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.