Skip to content

fix(firestore-send-email): map DATABASE_REGION to a Cloud Run region for function placement - #3102

Open
cabljac wants to merge 1 commit into
kitsfrom
fix/kits-send-email-database-region-mapping
Open

fix(firestore-send-email): map DATABASE_REGION to a Cloud Run region for function placement#3102
cabljac wants to merge 1 commit into
kitsfrom
fix/kits-send-email-database-region-mapping

Conversation

@cabljac

@cabljac cabljac commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Fixes #3069. The kit passed the raw DATABASE_REGION value as the function's region, and the param's select offers the Firestore multi-regions eur3/nam5/nam7, which are not Cloud Run regions, so multi-region deploys hard-failed.

The issue proposed dropping the region option (the #3066 pattern). This PR deliberately deviates: by agreement with the firebase-tools team, DATABASE_REGION stays and is mapped instead, because the CLI's own inference is unreliable for param-declared databases (firebase/firebase-tools#11020) and the extension-to-kit migration exports DATABASE_REGION into the user's .env. Multi-regions map to a region inside them (nam5/nam7 to us-central1, eur3 to europe-west1, mirroring the CLI's FIRESTORE_DUAL_REGION_TO_REGION_MAPPING); regional locations pass through; unset or empty means no region option and the CLI fallback. #3101 is the sibling fix for firestore-bigquery-export.

Tests pin all mapping cases plus the unset case; breaking the mapping fails them. Caveats: no live multi-region deploy was run for this kit, and the module-load process.env read needs firebase-tools >= 15.28.0 during discovery (older CLIs degrade to the no-region fallback). The approved #3089 also touches src/config.ts; this change edits only envDeployOptions at the end of the file, so overlap is limited to a trivial CHANGELOG conflict.

…for function placement

Stop passing the raw DATABASE_REGION value as the function region. Firestore
multi-region locations (nam5, nam7, eur3) are not Cloud Run regions, so any
multi-region database made every deploy fail. Map them to a region inside the
multi-region (nam5/nam7 -> us-central1, eur3 -> europe-west1); regional
locations pass through unchanged. With the parameter unset or empty the
function declares no region and the CLI resolves one at deploy time.

Fixes #3069.

@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 introduces a mapping from Firestore multi-region locations (such as nam5, nam7, and eur3) to valid Cloud Run regions (us-central1 and europe-west1) to prevent deployment failures. It updates the configuration logic, documentation, and adds corresponding unit tests. Feedback suggests normalizing the input location string by trimming and lowercasing it to make the mapping more robust against formatting variations.

Comment on lines +30 to +37
export function firestoreLocationToFunctionRegion(
location: string | undefined
): string | undefined {
if (!location) {
return undefined;
}
return MULTI_REGION_TO_FUNCTION_REGION[location] ?? location;
}

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

To make the region mapping more robust against accidental leading/trailing whitespace or casing differences in the .env file (for example, DATABASE_REGION= nam5 or DATABASE_REGION=NAM5), it is recommended to trim and lowercase the input location before performing the lookup.

Suggested change
export function firestoreLocationToFunctionRegion(
location: string | undefined
): string | undefined {
if (!location) {
return undefined;
}
return MULTI_REGION_TO_FUNCTION_REGION[location] ?? location;
}
export function firestoreLocationToFunctionRegion(
location: string | undefined
): string | undefined {
if (!location) {
return undefined;
}
const normalized = location.trim().toLowerCase();
if (!normalized) {
return undefined;
}
return MULTI_REGION_TO_FUNCTION_REGION[normalized] ?? normalized;
}

@cabljac cabljac mentioned this pull request Sep 2, 2026
62 tasks
@cabljac
cabljac requested review from CorieW and IzaakGough and removed request for CorieW September 7, 2026 09:48
@@ -158,15 +158,25 @@ value, and on OAuth2 auth do the same for `SMTP_PASSWORD`.

### DATABASE_REGION now decides where the function runs

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.

Worth documenting the firebase-tools floor in this section. Placement reads process.env.DATABASE_REGION at discovery, so on 15.25.x to 15.27.x the value is not loaded, no region is declared, and the CLI falls back to the existing region or us-central1. On those versions the CHANGELOG line about nam5 and nam7 deploying to us-central1 does not hold, and the deploy section above tells users 15.25.1 is enough for kits, so someone on that version gets the old behaviour with nothing to tell them.

The other case for the same paragraph: on a fresh interactive install the prompted value only takes effect from the second deploy, since resolveDefaultRegionsForBuild runs before param resolution. The first deploy lands in us-central1 and the next one moves the function. The bigquery-export README covers both points, so mirroring that wording here would do it.

}

export function envDeployOptions(): DeployTimeOptions {
// The region option cannot be a param expression, so the value is read from

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.

One note on the reasoning here: the region option does accept a param expression. firebase-functions types it as SupportedRegion | string | Expression<string> | ResetValue, firebase-tools resolves CEL in region via params.resolveList/resolveString in deploy/functions/build.js, and the code this replaces passed params.databaseRegion straight in. The constraint that actually forces the env read is that the multi-region lookup cannot be expressed in CEL, so the value has to be concrete at discovery time.

Rewording it that way also makes the trade-off visible, since the env read is what makes placement a silent no-op on CLIs older than 15.28.0.

`FIREBASE_FUNCTIONS_DEFAULT_REGION` environment variable when running
`firebase deploy`. Careful with that variable: it applies to every no-region
function in the deploy, not just this kit. Note that changing an existing
install's function region deletes and recreates the function in the new

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.

Could be worth spelling out what the recreate costs for this kit specifically. processQueue is the only function and there is no reconciliation pass, so mail documents written while the function is gone produce no Firestore event and are never delivered. They stay in the collection with no delivery state and nothing retries them.

Both paths above reach it: the first interactive deploy that moves the function on the second run, and upgrading the CLI past 15.28.0 with DATABASE_REGION already in .env. A sentence suggesting people drain the mail collection before a region change would help.

if (!location) {
return undefined;
}
return MULTI_REGION_TO_FUNCTION_REGION[location] ?? location;

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.

This lookup is case-sensitive, where the CLI equivalent it mirrors lowercases first (getDefaultRegion does db.locationId.toLowerCase() before indexing the same table). The select input keeps the prompted flow safe, but .env gets hand-edited and hand-copied during migration, and DATABASE_REGION=NAM5 passes NAM5 through as the function region, which is the same hard deploy failure this change removes.

location.trim().toLowerCase() before the lookup covers it. The trim is only belt and braces, values are already trimmed by the CLI's dotenv parser.

expect(envDeployOptions()).not.toHaveProperty("region");
});

test("serialized deploy-time options do not contain undefined", () => {

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.

Two coverage gaps worth closing while this is open.

Nothing imports src/index.ts, so the new spread on processQueue is unexercised: deleting ...(deploy.region ? { region: deploy.region } : {}) leaves the whole suite green, and that line is what actually places the function.

This test also now forces DATABASE_REGION unset, so the case it was written for, a region present in the serialized options, is no longer covered. The bigquery-export tests assert per-function options through a triggerOptions() helper, and the same shape would work here.

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.

fix(firestore-send-email): DATABASE_REGION used as function region breaks multi-region deploys

2 participants