Skip to content

fix(delete-user-data): resolve the RTDB client on first use - #2955

Draft
IzaakGough wants to merge 1 commit into
kits-delete-user-data-testsfrom
kits-delete-user-data-rtdb-lazy
Draft

fix(delete-user-data): resolve the RTDB client on first use#2955
IzaakGough wants to merge 1 commit into
kits-delete-user-data-testsfrom
kits-delete-user-data-rtdb-lazy

Conversation

@IzaakGough

Copy link
Copy Markdown

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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines 74 to +81
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();
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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().

Suggested change
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());
},

@IzaakGough
IzaakGough force-pushed the kits-delete-user-data-tests branch from 166026d to 9391e3f Compare August 19, 2026 09:34
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.
@IzaakGough
IzaakGough force-pushed the kits-delete-user-data-rtdb-lazy branch from b109bf9 to 64d6498 Compare August 19, 2026 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants