From 2666b97be0f94f4eda624e680dfe06b69e1e88fa Mon Sep 17 00:00:00 2001 From: Rosalyn Tan Date: Tue, 11 Feb 2025 15:43:16 -0800 Subject: [PATCH 1/4] Format INTERACTIVE_ACK issues as table as well and add extra "type" column to table. --- src/dataconnect/build.ts | 2 +- src/dataconnect/graphqlError.ts | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/dataconnect/build.ts b/src/dataconnect/build.ts index bd67e0760a0..954bc82764b 100644 --- a/src/dataconnect/build.ts +++ b/src/dataconnect/build.ts @@ -84,7 +84,7 @@ export async function handleBuildErrors( 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"), + prettifyWithWorkaround(interactiveAcks), ); if (!nonInteractive && !force && !dryRun) { const result = await promptOnce({ diff --git a/src/dataconnect/graphqlError.ts b/src/dataconnect/graphqlError.ts index 5f48a892102..7c2f9327267 100644 --- a/src/dataconnect/graphqlError.ts +++ b/src/dataconnect/graphqlError.ts @@ -13,11 +13,19 @@ export function prettify(err: GraphqlError): string { return header.length ? `${header}: ${message}` : message; } +function splitIssueMessage(err: GraphqlError): string[] { + const msg = err.message.split(": "); + if (msg.length >= 2) { + return [msg[0], msg.slice(1).join(":")]; + } + return ["", err.message]; +} + export function prettifyWithWorkaround(errs: GraphqlError[]): string { const table = new Table({ - head: ["Issue", "Workaround", "Reason"], + head: ["Type", "Issue", "Workaround", "Reason"], style: { head: ["yellow"] }, - colWidths: [50, 50, 50], + colWidths: [20, 50, 50, 50], wordWrap: true, }); for (const e of errs) { @@ -26,10 +34,12 @@ export function prettifyWithWorkaround(errs: GraphqlError[]): string { } else { const workarounds = e.extensions.workarounds; for (let i = 0; i < workarounds.length; i++) { + const msg = splitIssueMessage(e); + e.message = msg[1]; if (i === 0) { - table.push([prettify(e), workarounds[i].description, workarounds[i].reason]); + table.push([msg[0], prettify(e), workarounds[i].description, workarounds[i].reason]); } else { - table.push(["", workarounds[i].description, workarounds[i].reason]); + table.push(["", "", workarounds[i].description, workarounds[i].reason]); } } } From f282665170eaf2598e6749f90289496dcac6e362 Mon Sep 17 00:00:00 2001 From: Rosalyn Tan Date: Tue, 11 Feb 2025 16:11:58 -0800 Subject: [PATCH 2/4] Update warning and prompt wording to reflect insecure operations as well as connector evolution issues. --- src/dataconnect/build.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/dataconnect/build.ts b/src/dataconnect/build.ts index 954bc82764b..5222bca4ef7 100644 --- a/src/dataconnect/build.ts +++ b/src/dataconnect/build.ts @@ -44,6 +44,7 @@ export async function handleBuildErrors( const requiredForces = errors.filter((w) => w.extensions?.warningLevel === "REQUIRE_FORCE"); if (requiredForces.length && !force) { + // Only INACCESSIBLE issues fall in this category. utils.logLabeledError( "dataconnect", `There are changes in your schema or connectors that will result in broken behavior:\n` + @@ -59,18 +60,19 @@ export async function handleBuildErrors( { name: "Reject changes and abort", value: "abort" }, ]; if (requiredAcks.length) { + // This category contains BREAKING, POTENTIALLY_BREAKING, and some INSECURE issues. 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` + + `There are changes in your schema or connectors that may break your existing applications or introduce security vulnerabilities. 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.", + "Explicit acknowledgement required for breaking schema or connector changes and new insecure operations. 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?", + message: "Would you like to proceed with these changes?", type: "list", choices, default: "abort", @@ -81,9 +83,10 @@ export async function handleBuildErrors( } } if (interactiveAcks.length) { + // This category contains WARNING and some INSECURE issues. utils.logLabeledWarning( "dataconnect", - `There are changes in your schema or connectors that may cause unexpected behavior in your existing applications:\n` + + `There are existing insecure operations or changes in your schema or connectors that may cause unexpected behavior in your existing applications:\n` + prettifyWithWorkaround(interactiveAcks), ); if (!nonInteractive && !force && !dryRun) { From 32b214592f641c194765f9e08634752a5454ee17 Mon Sep 17 00:00:00 2001 From: Rosalyn Tan Date: Wed, 12 Feb 2025 11:17:33 -0800 Subject: [PATCH 3/4] Wording. --- src/dataconnect/build.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dataconnect/build.ts b/src/dataconnect/build.ts index 5222bca4ef7..7ae5c14ef48 100644 --- a/src/dataconnect/build.ts +++ b/src/dataconnect/build.ts @@ -63,7 +63,7 @@ export async function handleBuildErrors( // This category contains BREAKING, POTENTIALLY_BREAKING, and some INSECURE issues. utils.logLabeledWarning( "dataconnect", - `There are changes in your schema or connectors that may break your existing applications or introduce security vulnerabilities. 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` + + `There are changes in your schema or connectors that may break your existing applications or introduce operations that are insecure. 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) { From fcdba1245140090184216107d60add060877c0db Mon Sep 17 00:00:00 2001 From: Rosalyn Tan Date: Wed, 12 Feb 2025 11:59:56 -0800 Subject: [PATCH 4/4] Sort issues in table by category and some formatting fixes. --- src/dataconnect/build.ts | 12 ++++++------ src/dataconnect/graphqlError.ts | 10 ++++++---- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/dataconnect/build.ts b/src/dataconnect/build.ts index 7ae5c14ef48..2dfe7e8f832 100644 --- a/src/dataconnect/build.ts +++ b/src/dataconnect/build.ts @@ -4,7 +4,7 @@ import { FirebaseError } from "../error"; import * as experiments from "../experiments"; import { promptOnce } from "../prompt"; import * as utils from "../utils"; -import { prettify, prettifyWithWorkaround } from "./graphqlError"; +import { prettify, prettifyTable } from "./graphqlError"; import { DeploymentMetadata, GraphqlError } from "./types"; export async function build( @@ -48,7 +48,7 @@ export async function handleBuildErrors( utils.logLabeledError( "dataconnect", `There are changes in your schema or connectors that will result in broken behavior:\n` + - prettifyWithWorkaround(requiredForces), + prettifyTable(requiredForces), ); throw new FirebaseError("Rerun this command with --force to deploy these changes."); } @@ -60,11 +60,11 @@ export async function handleBuildErrors( { name: "Reject changes and abort", value: "abort" }, ]; if (requiredAcks.length) { - // This category contains BREAKING, POTENTIALLY_BREAKING, and some INSECURE issues. + // This category contains BREAKING and INSECURE issues. utils.logLabeledWarning( "dataconnect", `There are changes in your schema or connectors that may break your existing applications or introduce operations that are insecure. 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), + prettifyTable(requiredAcks), ); if (nonInteractive && !force) { throw new FirebaseError( @@ -83,11 +83,11 @@ export async function handleBuildErrors( } } if (interactiveAcks.length) { - // This category contains WARNING and some INSECURE issues. + // This category contains WARNING and EXISTING_INSECURE issues. utils.logLabeledWarning( "dataconnect", `There are existing insecure operations or changes in your schema or connectors that may cause unexpected behavior in your existing applications:\n` + - prettifyWithWorkaround(interactiveAcks), + prettifyTable(interactiveAcks), ); if (!nonInteractive && !force && !dryRun) { const result = await promptOnce({ diff --git a/src/dataconnect/graphqlError.ts b/src/dataconnect/graphqlError.ts index 7c2f9327267..8fbf7d8d954 100644 --- a/src/dataconnect/graphqlError.ts +++ b/src/dataconnect/graphqlError.ts @@ -21,21 +21,23 @@ function splitIssueMessage(err: GraphqlError): string[] { return ["", err.message]; } -export function prettifyWithWorkaround(errs: GraphqlError[]): string { +export function prettifyTable(errs: GraphqlError[]): string { const table = new Table({ head: ["Type", "Issue", "Workaround", "Reason"], style: { head: ["yellow"] }, colWidths: [20, 50, 50, 50], wordWrap: true, }); + // We want to present BREAKING before INSECURE changes. Ordering of other issues matters less, but we want to keep categories grouped together. + errs.sort((a, b) => a.message.localeCompare(b.message)); for (const e of errs) { + const msg = splitIssueMessage(e); + e.message = msg[1]; if (!e.extensions?.workarounds?.length) { - table.push([prettify(e), "", ""]); + table.push([msg[0], prettify(e), "", ""]); } else { const workarounds = e.extensions.workarounds; for (let i = 0; i < workarounds.length; i++) { - const msg = splitIssueMessage(e); - e.message = msg[1]; if (i === 0) { table.push([msg[0], prettify(e), workarounds[i].description, workarounds[i].reason]); } else {