Implement FDC connector evolution warnings. (#8023)

* Implement FDC connector evolution warnings. * Pass project ID to build command in the connector evolution experiment. * Some initial handling of connector evolution issues. * Adjust control flow with some placeholder TODOs. * Fix warningLevel structure and output log statements. * Fix up some log formatting and prompt for connector evolution warnings in interactive mode. * Better formatting of prompts/logs and better messaging. * Format issues and workarounds as a table. * Address some review comments. * Add unit test. * Fix test? * Fix test!
firebase · Jan 3, 2025 · ba3fa99 · ba3fa99
1 parent b684155
commit ba3fa99
Show file tree

Hide file tree

Showing 8 changed files with 272 additions and 7 deletions.
diff --git a/src/apphosting/utils.spec.ts b/src/apphosting/utils.spec.ts
@@ -23,6 +23,9 @@ describe("utils", () => {
     beforeEach(() => {
       prompt = sinon.stub(promptImport);
     });
+    afterEach(() => {
+      sinon.verifyAndRestore();
+    });
     it("should prompt with the correct options", async () => {
       const apphostingFileNameToPathMap = new Map<string, string>([
         ["apphosting.yaml", "/parent/cwd/apphosting.yaml"],

diff --git a/src/dataconnect/build.spec.ts b/src/dataconnect/build.spec.ts
@@ -0,0 +1,142 @@
+import { expect } from "chai";
+import * as sinon from "sinon";
+import * as prompt from "../prompt";
+import { handleBuildErrors } from "./build";
+import { GraphqlError } from "./types";
+
+describe("handleBuildErrors", () => {
+  let promptOnceStub: sinon.SinonStub;
+  beforeEach(() => {
+    promptOnceStub = sinon
+      .stub(prompt, "promptOnce")
+      .throws("unexpected call to prompt.promptOnce");
+  });
+  afterEach(() => {
+    sinon.verifyAndRestore();
+  });
+  const cases: {
+    desc: string;
+    graphqlErr: GraphqlError[];
+    nonInteractive: boolean;
+    force: boolean;
+    dryRun: boolean;
+    promptAnswer?: string;
+    expectErr: boolean;
+  }[] = [
+    {
+      desc: "Only build error",
+      graphqlErr: [{ message: "build error" }],
+      nonInteractive: false,
+      force: true,
+      dryRun: false,
+      expectErr: true,
+    },
+    {
+      desc: "Build error with evolution error",
+      graphqlErr: [
+        { message: "build error" },
+        { message: "evolution error", extensions: { warningLevel: "INTERACTIVE_ACK" } },
+      ],
+      nonInteractive: false,
+      force: true,
+      dryRun: false,
+      expectErr: true,
+    },
+    {
+      desc: "Interactive ack evolution error, prompt and accept",
+      graphqlErr: [{ message: "evolution error", extensions: { warningLevel: "INTERACTIVE_ACK" } }],
+      nonInteractive: false,
+      force: false,
+      dryRun: false,
+      promptAnswer: "proceed",
+      expectErr: false,
+    },
+    {
+      desc: "Interactive ack evolution error, prompt and reject",
+      graphqlErr: [{ message: "evolution error", extensions: { warningLevel: "INTERACTIVE_ACK" } }],
+      nonInteractive: false,
+      force: false,
+      dryRun: false,
+      promptAnswer: "abort",
+      expectErr: true,
+    },
+    {
+      desc: "Interactive ack evolution error, nonInteractive=true",
+      graphqlErr: [{ message: "evolution error", extensions: { warningLevel: "INTERACTIVE_ACK" } }],
+      nonInteractive: true,
+      force: false,
+      dryRun: false,
+      expectErr: false,
+    },
+    {
+      desc: "Interactive ack evolution error, force=true",
+      graphqlErr: [{ message: "evolution error", extensions: { warningLevel: "INTERACTIVE_ACK" } }],
+      nonInteractive: false,
+      force: true,
+      dryRun: false,
+      expectErr: false,
+    },
+    {
+      desc: "Interactive ack evolution error, dryRun=true",
+      graphqlErr: [{ message: "evolution error", extensions: { warningLevel: "INTERACTIVE_ACK" } }],
+      nonInteractive: false,
+      force: false,
+      dryRun: true,
+      expectErr: false,
+    },
+    {
+      desc: "Required ack evolution error, prompt and accept",
+      graphqlErr: [{ message: "evolution error", extensions: { warningLevel: "REQUIRE_ACK" } }],
+      nonInteractive: false,
+      force: false,
+      dryRun: false,
+      promptAnswer: "proceed",
+      expectErr: false,
+    },
+    {
+      desc: "Required ack evolution error, prompt and reject",
+      graphqlErr: [{ message: "evolution error", extensions: { warningLevel: "REQUIRE_ACK" } }],
+      nonInteractive: false,
+      force: false,
+      dryRun: false,
+      promptAnswer: "abort",
+      expectErr: true,
+    },
+    {
+      desc: "Required ack evolution error, nonInteractive=true, force=false",
+      graphqlErr: [{ message: "evolution error", extensions: { warningLevel: "REQUIRE_ACK" } }],
+      nonInteractive: true,
+      force: false,
+      dryRun: false,
+      expectErr: true,
+    },
+    {
+      desc: "Required ack evolution error, nonInteractive=true, force=true",
+      graphqlErr: [{ message: "evolution error", extensions: { warningLevel: "REQUIRE_ACK" } }],
+      nonInteractive: true,
+      force: true,
+      dryRun: false,
+      expectErr: false,
+    },
+    {
+      desc: "Required ack evolution error, nonInteractive=false, force=true",
+      graphqlErr: [{ message: "evolution error", extensions: { warningLevel: "REQUIRE_ACK" } }],
+      nonInteractive: false,
+      force: true,
+      dryRun: false,
+      expectErr: false,
+    },
+  ];
+  for (const c of cases) {
+    it(c.desc, async () => {
+      try {
+        if (c.promptAnswer) {
+          promptOnceStub.resolves(c.promptAnswer);
+        }
+        await handleBuildErrors(c.graphqlErr, c.nonInteractive, c.force, c.dryRun);
+      } catch (err) {
+        expect(c.expectErr).to.be.true;
+      }
+    });
+  }
+});
diff --git a/src/dataconnect/build.ts b/src/dataconnect/build.ts
@@ -1,15 +1,90 @@
 import { DataConnectEmulator } from "../emulator/dataconnectEmulator";
 import { Options } from "../options";
 import { FirebaseError } from "../error";
-import { prettify } from "./graphqlError";
-import { DeploymentMetadata } from "./types";
+import * as experiments from "../experiments";
+import { promptOnce } from "../prompt";
+import * as utils from "../utils";
+import { prettify, prettifyWithWorkaround } from "./graphqlError";
+import { DeploymentMetadata, GraphqlError } from "./types";
 
-export async function build(options: Options, configDir: string): Promise<DeploymentMetadata> {
-  const buildResult = await DataConnectEmulator.build({ configDir });
+export async function build(
+  options: Options,
+  configDir: string,
+  dryRun?: boolean,
+): Promise<DeploymentMetadata> {
+  const args: { configDir: string; projectId?: string } = { configDir };
+  if (experiments.isEnabled("fdcconnectorevolution") && options.projectId) {
+    const flags = process.env["DATA_CONNECT_PREVIEW"];
+    if (flags) {
+      process.env["DATA_CONNECT_PREVIEW"] = flags + ",conn_evolution";
+    } else {
+      process.env["DATA_CONNECT_PREVIEW"] = "conn_evolution";
+    }
+    args.projectId = options.projectId;
+  }
+  const buildResult = await DataConnectEmulator.build(args);
   if (buildResult?.errors?.length) {
+    await handleBuildErrors(buildResult.errors, options.nonInteractive, options.force, dryRun);
+  }
+  return buildResult?.metadata ?? {};
+}
+
+export async function handleBuildErrors(
+  errors: GraphqlError[],
+  nonInteractive: boolean,
+  force: boolean,
+  dryRun?: boolean,
+) {
+  if (errors.filter((w) => !w.extensions?.warningLevel).length) {
+    // Throw immediately if there are any build errors in the GraphQL schema or connectors.
     throw new FirebaseError(
-      `There are errors in your schema and connector files:\n${buildResult.errors.map(prettify).join("\n")}`,
+      `There are errors in your schema and connector files:\n${errors.map(prettify).join("\n")}`,
     );
   }
-  return buildResult?.metadata ?? {};
+  const interactiveAcks = errors.filter((w) => w.extensions?.warningLevel === "INTERACTIVE_ACK");
+  const requiredAcks = errors.filter((w) => w.extensions?.warningLevel === "REQUIRE_ACK");
+  const choices = [
+    { name: "Acknowledge all changes and proceed", value: "proceed" },
+    { name: "Reject changes and abort", value: "abort" },
+  ];
+  if (requiredAcks.length) {
+    utils.logLabeledWarning(
+      "dataconnect",
+      `There are changes in your schema or connectors that may break your existing applications. These changes require explicit acknowledgement to proceed. You may either reject the changes and update your sources with the suggested workaround(s), if any, or acknowledge these changes and proceed with the deployment:\n` +
+        prettifyWithWorkaround(requiredAcks),
+    );
+    if (nonInteractive && !force) {
+      throw new FirebaseError(
+        "Explicit acknowledgement required for breaking schema or connector changes. Rerun this command with --force to deploy these changes.",
+      );
+    } else if (!nonInteractive && !force && !dryRun) {
+      const result = await promptOnce({
+        message: "Would you like to proceed with these breaking changes?",
+        type: "list",
+        choices,
+        default: "abort",
+      });
+      if (result === "abort") {
+        throw new FirebaseError(`Deployment aborted.`);
+      }
+    }
+  }
+  if (interactiveAcks.length) {
+    utils.logLabeledWarning(
+      "dataconnect",
+      `There are changes in your schema or connectors that may cause unexpected behavior in your existing applications:\n` +
+        interactiveAcks.map(prettify).join("\n"),
+    );
+    if (!nonInteractive && !force && !dryRun) {
+      const result = await promptOnce({
+        message: "Would you like to proceed with these changes?",
+        type: "list",
+        choices,
+        default: "proceed",
+      });
+      if (result === "abort") {
+        throw new FirebaseError(`Deployment aborted.`);
+      }
+    }
+  }
 }
diff --git a/src/dataconnect/graphqlError.ts b/src/dataconnect/graphqlError.ts
@@ -1,4 +1,5 @@
 import { GraphqlError } from "./types";
+const Table = require("cli-table");
 
 export function prettify(err: GraphqlError): string {
   const message = err.message;
@@ -11,3 +12,25 @@ export function prettify(err: GraphqlError): string {
   }
   return header.length ? `${header}: ${message}` : message;
 }
+
+export function prettifyWithWorkaround(errs: GraphqlError[]): string {
+  const table = new Table({
+    head: ["Issue", "Workaround", "Reason"],
+    style: { head: ["yellow"] },
+  });
+  for (const e of errs) {
+    if (!e.extensions?.workarounds?.length) {
+      table.push([prettify(e), "", ""]);
+    } else {
+      const workarounds = e.extensions.workarounds;
+      for (let i = 0; i < workarounds.length; i++) {
+        if (i === 0) {
+          table.push([prettify(e), workarounds[i].Description, workarounds[i].Reason]);
+        } else {
+          table.push(["", workarounds[i].Description, workarounds[i].Reason]);
+        }
+      }
+    }
+  }
+  return table.toString();
+}
diff --git a/src/dataconnect/types.ts b/src/dataconnect/types.ts
@@ -71,6 +71,15 @@ export interface Diff {
   destructive: boolean;
 }
 
+export type WarningLevel = "INTERACTIVE_ACK" | "REQUIRE_ACK";
+
+export interface Workaround {
+  // TODO: Make these lower-case after fixing the emulator, to match the style convention.
+  Description: string;
+  Reason: string;
+  ReplaceWith: string;
+}
+
 export interface GraphqlError {
   message: string;
   locations?: {
@@ -79,6 +88,8 @@ export interface GraphqlError {
   }[];
   extensions?: {
     file?: string;
+    warningLevel?: WarningLevel;
+    workarounds?: Workaround[];
     [key: string]: any;
   };
 }

diff --git a/src/deploy/dataconnect/prepare.ts b/src/deploy/dataconnect/prepare.ts
@@ -39,7 +39,7 @@ export default async function (context: any, options: DeployOptions): Promise<vo
     serviceCfgs.map((c) => load(projectId, options.config, c.source)),
   );
   for (const si of serviceInfos) {
-    si.deploymentMetadata = await build(options, si.sourceDirectory);
+    si.deploymentMetadata = await build(options, si.sourceDirectory, options.dryRun);
   }
   const unmatchedFilters = filters?.filter((f) => {
     // filter out all filters that match no service

diff --git a/src/emulator/dataconnectEmulator.ts b/src/emulator/dataconnectEmulator.ts
@@ -51,6 +51,7 @@ export interface DataConnectGenerateArgs {
 
 export interface DataConnectBuildArgs {
   configDir: string;
+  projectId?: string;
 }
 
 // TODO: More concrete typing for events. Can we use string unions?
@@ -249,6 +250,9 @@ export class DataConnectEmulator implements EmulatorInstance {
   static async build(args: DataConnectBuildArgs): Promise<BuildResult> {
     const commandInfo = await downloadIfNecessary(Emulators.DATACONNECT);
     const cmd = ["--logtostderr", "-v=2", "build", `--config_dir=${args.configDir}`];
+    if (args.projectId) {
+      cmd.push(`--project_id=${args.projectId}`);
+    }
 
     const res = childProcess.spawnSync(commandInfo.binary, cmd, { encoding: "utf-8" });
     if (isIncomaptibleArchError(res.error)) {

diff --git a/src/experiments.ts b/src/experiments.ts
@@ -137,6 +137,13 @@ export const ALL_EXPERIMENTS = experiments({
     default: true,
     public: false,
   },
+
+  fdcconnectorevolution: {
+    shortDescription: "Enable Data Connect connector evolution warnings.",
+    fullDescription: "Enable Data Connect connector evolution warnings.",
+    default: false,
+    public: false,
+  },
 });
 
 export type ExperimentName = keyof typeof ALL_EXPERIMENTS;