From a29e34f49913181729513030774378dc3f6fe1e1 Mon Sep 17 00:00:00 2001 From: Jacob Cable Date: Fri, 8 May 2026 13:26:07 +0100 Subject: [PATCH 1/2] fix(firestore-bigquery-change-tracker): unmask BQ errors and stop spurious table updates Fixes #2801, fixes #2194. #2801 - handleFailedTransactions The dead-letter path called `db.settings({ ignoreUndefinedProperties: true })` on every invocation. `Firestore.settings()` throws if the singleton has already been used, so on warm Cloud Functions containers the second invocation crashed with "Firestore has already been initialized", masking the original BigQuery insert error that triggered the dead-letter path in the first place. Guard the call with a per-instance Set and a try/catch: settings is applied once per process per instance, and any throw is swallowed so the dead-letter write to the backup collection still runs and the real BQ error surfaces. #2194 - tableRequiresUpdate always returning true Two bugs: 1. Clustering check: `JSON.stringify(config.clustering)` returns "null" when clustering is not configured, but `JSON.stringify(metadata.clustering?.fields || [])` returns "[]". They never match, so the table is always flagged as needing update. Normalise null/undefined to `[]` before stringifying. 2. `pathParamsColExists` was assigned the result of `fields.find()` (which is `Field | undefined`), then strict-compared against a boolean (`!!config.wildcardIds !== pathParamsColExists`). When the column is absent, `false !== undefined` is always true. The function signature already declares these params as `boolean`; coerce in the caller. Same fix applied to `documentIdColExists` and `oldDataColExists` for type-honesty (they already worked correctly under truthy checks but the type was being violated). --- .../src/bigquery/checkUpdates.ts | 2 +- .../src/bigquery/handleFailedTransactions.ts | 23 +++++++++++++++---- .../src/bigquery/index.ts | 6 ++--- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/checkUpdates.ts b/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/checkUpdates.ts index 6313f32cc..e18af3d14 100644 --- a/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/checkUpdates.ts +++ b/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/checkUpdates.ts @@ -23,7 +23,7 @@ export async function tableRequiresUpdate({ const { metadata } = table; /** Check clustering */ - const configCluster = JSON.stringify(config.clustering); + const configCluster = JSON.stringify(config.clustering || []); const tableCluster = JSON.stringify(metadata.clustering?.fields || []); if (configCluster !== tableCluster) return true; diff --git a/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/handleFailedTransactions.ts b/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/handleFailedTransactions.ts index 4addaeec9..6c00d14b8 100644 --- a/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/handleFailedTransactions.ts +++ b/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/handleFailedTransactions.ts @@ -7,15 +7,30 @@ if (!admin.apps.length) { initializeApp(); } +const settingsApplied = new Set(); + +const getConfiguredFirestore = (instanceId: string) => { + const db = getFirestore(instanceId); + if (!settingsApplied.has(instanceId)) { + settingsApplied.add(instanceId); + try { + db.settings({ ignoreUndefinedProperties: true }); + } catch { + // Firestore.settings() throws if the singleton has already been used + // elsewhere in the process. The dead-letter path doesn't strictly + // depend on ignoreUndefinedProperties, so swallow rather than crash + // and mask the original BigQuery error that led us here. + } + } + return db; +}; + export default async ( rows: any[], config: ChangeTrackerConfig, e: Error ): Promise => { - const db = getFirestore(config.firestoreInstanceId!); - db.settings({ - ignoreUndefinedProperties: true, - }); + const db = getConfiguredFirestore(config.firestoreInstanceId!); const batchArray = [db.batch()]; let operationCounter = 0; diff --git a/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/index.ts b/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/index.ts index 7b6f8a15e..2c7a11b1a 100644 --- a/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/index.ts +++ b/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/index.ts @@ -341,14 +341,14 @@ export class FirestoreBigQueryEventHistoryTracker await clustering.updateClustering(metadata); - const documentIdColExists = fields.find( + const documentIdColExists = !!fields.find( (column) => column.name === "document_id" ); - const pathParamsColExists = fields.find( + const pathParamsColExists = !!fields.find( (column) => column.name === "path_params" ); - const oldDataColExists = fields.find( + const oldDataColExists = !!fields.find( (column) => column.name === "old_data" ); From c6b28057581fde8851c11e392c145e7185516abd Mon Sep 17 00:00:00 2001 From: Jacob Cable Date: Mon, 11 May 2026 08:41:18 +0100 Subject: [PATCH 2/2] refactor(firestore-bigquery-change-tracker): apply review nits - handleFailedTransactions: make instanceId optional and drop non-null assertion at the call site so the helper matches the optional type on ChangeTrackerConfig.firestoreInstanceId - bigquery/index.ts: use Array.prototype.some() instead of !!find() for the column-existence checks (documentId, pathParams, oldData) --- .../src/bigquery/handleFailedTransactions.ts | 8 ++++---- .../src/bigquery/index.ts | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/handleFailedTransactions.ts b/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/handleFailedTransactions.ts index 6c00d14b8..5bf894430 100644 --- a/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/handleFailedTransactions.ts +++ b/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/handleFailedTransactions.ts @@ -7,10 +7,10 @@ if (!admin.apps.length) { initializeApp(); } -const settingsApplied = new Set(); +const settingsApplied = new Set(); -const getConfiguredFirestore = (instanceId: string) => { - const db = getFirestore(instanceId); +const getConfiguredFirestore = (instanceId?: string) => { + const db = instanceId ? getFirestore(instanceId) : getFirestore(); if (!settingsApplied.has(instanceId)) { settingsApplied.add(instanceId); try { @@ -30,7 +30,7 @@ export default async ( config: ChangeTrackerConfig, e: Error ): Promise => { - const db = getConfiguredFirestore(config.firestoreInstanceId!); + const db = getConfiguredFirestore(config.firestoreInstanceId); const batchArray = [db.batch()]; let operationCounter = 0; diff --git a/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/index.ts b/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/index.ts index 2c7a11b1a..f6202970e 100644 --- a/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/index.ts +++ b/firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/index.ts @@ -341,14 +341,14 @@ export class FirestoreBigQueryEventHistoryTracker await clustering.updateClustering(metadata); - const documentIdColExists = !!fields.find( + const documentIdColExists = fields.some( (column) => column.name === "document_id" ); - const pathParamsColExists = !!fields.find( + const pathParamsColExists = fields.some( (column) => column.name === "path_params" ); - const oldDataColExists = !!fields.find( + const oldDataColExists = fields.some( (column) => column.name === "old_data" );