Repository: firebase/firebase-tools (ts)

Issue:
Multi-region function deploy fails on v11.4.0
<!-- DO NOT DELETE
validate_template=true
template_path=.github/ISSUE_TEMPLATE/bug_report.md
-->

<!--
Thank you for contributing to the Firebase community!

Think you found a bug?
=======================
Yeah, we're definitely not perfect! Please use this template and include a minimal repro when opening the issue. If you know how to solve the issue, please create a Pull Request, and we'd be happy to review it!

Have a feature request?
========================
Great, we love hearing how we can improve our products! However, do not use this template to submit a feature request. Please submit your feature requests to: https://firebase.google.com/support/contact/bugs-features/

Have a usage question?
=======================
We get lots of those and we love helping you, but GitHub is not the best place for them and they will be closed. Please take a look at the guide first: https://firebase.google.com/docs/cli/

If the official documentation doesn't help, try asking through our official support channel: https://firebase.google.com/support/

Additional locations to check for solutions or assistance from the community:
- Stack Overflow: https://stackoverflow.com/
- Firebase Slack Community: https://firebase.community/

*Please avoid duplicate posting across multiple channels!*
-->

### [REQUIRED] Environment info

<!-- What version of the Firebase CLI (`firebase-tools`) are you using? Note that your issue may already be fixed in the latest versions. The latest version can be found at https://github.com/firebase/firebase-tools/releases -->

<!-- Output of `firebase --version` -->

**firebase-tools:** 11.4.0 and 11.4.1 (works fine in 11.3.0)

<!-- e.g. macOS, Windows, Ubuntu -->

**Platform:** MacOS, TypeScript

### [REQUIRED] Test case

<!-- Provide a minimal, complete, and verifiable example (http://stackoverflow.com/help/mcve) -->
```
export const theFuncName = functions
	.region('us-central1', 'australia-southeast1')
	.runWith({
	    maxInstances: 50,
        })
	.https.onRequest(async (request: functions.https.Request, response: functions.Response) => {
            // function body
        })
```

### [REQUIRED] Steps to reproduce

<!-- Provide the steps needed to reproduce the issue with the above test case. -->
`firebase deploy`

### [REQUIRED] Expected behavior
Deploy succeeds.

<!-- What is the expected behavior? -->

### [REQUIRED] Actual behavior

<!-- Run the command with --debug flag, and include the logs below. -->

Deploy error:
```Error: More than one backend found for function name: theFuncName. If the function is deployed in multiple regions, you must specify a region.```

Pull request description:
allows defaulting rewrites to us-central1 for multi-region functions
<!--

Thank you for contributing to the Firebase community! Please fill out the form below.

Run the linter and test suite
==============================
Run `npm test` to make sure your changes compile properly and the tests all pass on your local machine. We've hooked up this repo with continuous integration to double check those things for you.

-->

### Description

Since we had previously defaulted to `us-central1`, we should probably continue to default to `us-central1` if multiple regions for a rewrite function are available. Printing a nice note saying "hey, you can specify other regions now" as well, for visibility of that feature.

Added a test for this as well in the functional tests.

Fixes #4810

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8667ca3e4..f75b2d775 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,4 @@
 - Adds check for callable functions when discovering Hosting rewrite endpoints. (#4792)
+- Allows defaulting to a `us-central1` instance of a rewrite function if that function is deployed in multiple regions including `us-central1`. (#4810)
 - Fixes Storage emulator timeCreated metadata bug. (#4599)
 - Fixes Storage import failure due to missing directories. (#3823)
diff --git a/src/deploy/hosting/convertConfig.ts b/src/deploy/hosting/convertConfig.ts
index b7ebf5792..60edd2c82 100644
--- a/src/deploy/hosting/convertConfig.ts
+++ b/src/deploy/hosting/convertConfig.ts
@@ -9,7 +9,7 @@ import {
 import { Payload } from "./args";
 import * as backend from "../functions/backend";
 import { Context } from "../functions/args";
-import { logLabeledWarning } from "../../utils";
+import { logLabeledBullet, logLabeledWarning } from "../../utils";
 
 function has(obj: { [k: string]: unknown }, k: string): boolean {
   return obj[k] !== undefined;
@@ -87,6 +87,17 @@ export async function convertConfig(
     });
 
     if (matchingBackends.length > 1) {
+      // For now, if `us-central1` is specified, allow that to keep working.
+      for (const endpoint of matchingBackends) {
+        if (endpoint.region === "us-central1") {
+          logLabeledBullet(
+            `hosting[${config.site}]`,
+            `Function \`${functionsEndpointInfo.serviceId}\` found in multiple regions, defaulting to \`us-central1\`. ` +
+              `To rewrite to a different region, specify a \`region\` for the rewrite in \`firebase.json\`.`
+          );
+          return endpoint;
+        }
+      }
       throw new FirebaseError(
         `More than one backend found for function name: ${functionsEndpointInfo.serviceId}. If the function is deployed in multiple regions, you must specify a region.`
       );


Test patch:
diff --git a/scripts/hosting-tests/rewrites-tests/run.sh b/scripts/hosting-tests/rewrites-tests/run.sh
index df4a6bbd8..ab51f1d02 100755
--- a/scripts/hosting-tests/rewrites-tests/run.sh
+++ b/scripts/hosting-tests/rewrites-tests/run.sh
@@ -3,4 +3,3 @@
 source scripts/set-default-credentials.sh
 
 mocha scripts/hosting-tests/rewrites-tests/tests.ts
-
diff --git a/scripts/hosting-tests/rewrites-tests/tests.ts b/scripts/hosting-tests/rewrites-tests/tests.ts
index 64f255418..9fbc63d30 100644
--- a/scripts/hosting-tests/rewrites-tests/tests.ts
+++ b/scripts/hosting-tests/rewrites-tests/tests.ts
@@ -430,6 +430,51 @@ describe("deploy function-targeted rewrites And functions", () => {
     expect(await functionsResponse.text()).to.contain("Hello from Firebase");
   }).timeout(1000 * 1e3);
 
+  it("should rewrite to the default of us-central1 if multiple regions including us-central1 are available", async () => {
+    const firebaseJson = {
+      hosting: {
+        public: "hosting",
+        rewrites: [
+          {
+            source: "/helloWorld",
+            function: functionName,
+          },
+        ],
+      },
+    };
+
+    const firebaseJsonFilePath = join(tempDirInfo.tempDir.name, ".", "firebase.json");
+    writeFileSync(firebaseJsonFilePath, JSON.stringify(firebaseJson));
+    ensureDirSync(tempDirInfo.hostingDirPath);
+    writeBasicHostingFile(tempDirInfo.hostingDirPath);
+
+    writeHelloWorldFunctionWithRegions(
+      functionName,
+      join(tempDirInfo.tempDir.name, ".", "functions"),
+      ["asia-northeast1", "us-central1"]
+    );
+
+    await client.deploy({
+      project: process.env.FBTOOLS_TARGET_PROJECT,
+      cwd: tempDirInfo.tempDir.name,
+      only: "hosting,functions",
+      force: true,
+    });
+
+    const staticResponse = await fetch(
+      `https://${process.env.FBTOOLS_CLIENT_INTEGRATION_SITE}.web.app/index.html`
+    );
+    expect(await staticResponse.text()).to.contain("Rabbit");
+
+    const functionsRequest = new Request(
+      `https://${process.env.FBTOOLS_CLIENT_INTEGRATION_SITE}.web.app/helloWorld`
+    );
+
+    const functionsResponse = await fetch(functionsRequest);
+
+    expect(await functionsResponse.text()).to.contain("Hello from Firebase");
+  }).timeout(1000 * 1e3);
+
   it("should fail when rewrite points to an invalid region for a function with multiple regions", async () => {
     const firebaseJson = {
       hosting: {
diff --git a/src/test/deploy/hosting/convertConfig.spec.ts b/src/test/deploy/hosting/convertConfig.spec.ts
index 3615f9c45..21820132d 100644
--- a/src/test/deploy/hosting/convertConfig.spec.ts
+++ b/src/test/deploy/hosting/convertConfig.spec.ts
@@ -3,7 +3,6 @@ import { HostingConfig } from "../../../firebaseConfig";
 import { convertConfig } from "../../../deploy/hosting/convertConfig";
 import * as args from "../../../deploy/functions/args";
 import * as backend from "../../../deploy/functions/backend";
-import { FirebaseError } from "../../../error";
 
 const DEFAULT_CONTEXT = {
   loadedExistingBackend: true,
@@ -113,6 +112,70 @@ describe("convertConfig", () => {
         },
       },
     },
+    {
+      name: "defaults to a us-central1 rewrite if one is avaiable, v1 edition",
+      input: { rewrites: [{ glob: "/foo", function: "foofn" }] },
+      want: { rewrites: [{ glob: "/foo", function: "foofn", functionRegion: "us-central1" }] },
+      payload: {
+        functions: {
+          default: {
+            wantBackend: backend.of(
+              {
+                id: "foofn",
+                project: "my-project",
+                entryPoint: "foofn",
+                runtime: "nodejs14",
+                region: "europe-west2",
+                platform: "gcfv1",
+                httpsTrigger: {},
+              },
+              {
+                id: "foofn",
+                project: "my-project",
+                entryPoint: "foofn",
+                runtime: "nodejs14",
+                region: "us-central1",
+                platform: "gcfv1",
+                httpsTrigger: {},
+              }
+            ),
+            haveBackend: backend.empty(),
+          },
+        },
+      },
+    },
+    {
+      name: "defaults to a us-central1 rewrite if one is avaiable, v2 edition",
+      input: { rewrites: [{ glob: "/foo", function: "foofn" }] },
+      want: { rewrites: [{ glob: "/foo", run: { region: "us-central1", serviceId: "foofn" } }] },
+      payload: {
+        functions: {
+          default: {
+            wantBackend: backend.of(
+              {
+                id: "foofn",
+                project: "my-project",
+                entryPoint: "foofn",
+                runtime: "nodejs14",
+                region: "europe-west2",
+                platform: "gcfv1",
+                httpsTrigger: {},
+              },
+              {
+                id: "foofn",
+                project: "my-project",
+                entryPoint: "foofn",
+                runtime: "nodejs14",
+                region: "us-central1",
+                platform: "gcfv2",
+                httpsTrigger: {},
+              }
+            ),
+            haveBackend: backend.empty(),
+          },
+        },
+      },
+    },
     {
       name: "returns rewrites for regex CF3",
       input: { rewrites: [{ regex: "/foo$", function: "foofn", region: "us-central1" }] },
@@ -394,132 +457,3 @@ describe("convertConfig", () => {
     });
   }
 });
-
-describe("convertConfig throws expection if", () => {
-  const tests: Array<{
-    name: string;
-    input: HostingConfig | undefined;
-    errorString: string;
-    payload: args.Payload;
-    finalize?: boolean;
-    context: any;
-  }> = [
-    {
-      name: "multiple v2 endpoints are found for function and functionRegion is not specified",
-      input: { rewrites: [{ glob: "/foo", function: "foofn" }] },
-      errorString: "More than one backend found for function name",
-      context: {
-        loadedExistingBackend: true,
-        existingBackend: {
-          endpoints: {},
-        },
-      },
-      payload: {
-        functions: {
-          default: {
-            wantBackend: backend.of(
-              {
-                id: "foofn",
-                project: "my-project",
-                entryPoint: "foofn",
-                runtime: "nodejs14",
-                region: "us-central1",
-                platform: "gcfv2",
-                httpsTrigger: {},
-              },
-              {
-                id: "foofn",
-                project: "my-project",
-                entryPoint: "foofn",
-                runtime: "nodejs14",
-                region: "europe-west2",
-                platform: "gcfv2",
-                httpsTrigger: {},
-              }
-            ),
-            haveBackend: backend.empty(),
-          },
-        },
-      },
-      finalize: true,
-    },
-    {
-      name: "multiple v1 endpoints are found for function and functionRegion is not specified",
-      input: { rewrites: [{ glob: "/foo", function: "foofn" }] },
-      context: {
-        loadedExistingBackend: true,
-        existingBackend: {
-          endpoints: {},
-        },
-      },
-      payload: {
-        functions: {
-          default: {
-            wantBackend: backend.of(
-              {
-                id: "foofn",
-                project: "my-project",
-                entryPoint: "foofn",
-                runtime: "nodejs14",
-                region: "us-central1",
-                platform: "gcfv1",
-                httpsTrigger: {},
-              },
-              {
-                id: "foofn",
-                project: "my-project",
-                entryPoint: "foofn",
-                runtime: "nodejs14",
-                region: "europe-west2",
-                platform: "gcfv1",
-                httpsTrigger: {},
-              }
-            ),
-            haveBackend: backend.empty(),
-          },
-        },
-      },
-      finalize: true,
-      errorString: "More than one backend found for function name",
-    },
-    {
-      name: "multiple existing v1 function regions are found for rewrite and no region is specified",
-      input: { rewrites: [{ regex: "/foo$", function: "foofn" }] },
-      context: {
-        loadedExistingBackend: true,
-        existingBackend: {
-          endpoints: {
-            "us-central1": {
-              foofn: {
-                id: "foofn",
-                region: "us-central1",
-                platform: "gcfv1",
-                httpsTrigger: true,
-              },
-            },
-            "asia-northeast2": {
-              foofn: {
-                id: "foofn",
-                region: "asia-northeast2",
-                platform: "gcfv1",
-                httpsTrigger: true,
-              },
-            },
-          },
-        },
-      },
-      payload: {},
-      errorString: "More than one backend found for function name",
-      finalize: true,
-    },
-  ];
-
-  for (const { name, context, input, payload, errorString, finalize = true } of tests) {
-    it(name, async () => {
-      const config = async (): Promise<void> => {
-        await convertConfig(context, payload, input, finalize);
-      };
-      await expect(config()).to.eventually.be.rejectedWith(FirebaseError, errorString);
-    });
-  }
-});