Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Display accurate message for INACCESSIBLE_SCHEMA error #7157

Merged
merged 11 commits into from
May 14, 2024
Merged

Display accurate message for INACCESSIBLE_SCHEMA error #7157

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
403cc7c
handle INACCESSIBLE_SCHEMA case separately
fredzqm May 10, 2024
bbf2f0d
error msg
fredzqm May 10, 2024
f0d44af
m
fredzqm May 10, 2024
3033065
m
fredzqm May 10, 2024
f10dcc6
m
fredzqm May 10, 2024
ff205aa
Merge branch 'master' into fz/inaccessible-schema
fredzqm May 10, 2024
5bf25ee
merge
fredzqm May 13, 2024
f18f94a
Merge remote-tracking branch 'origin/dataconnect' into fz/inaccessibl…
fredzqm May 13, 2024
87943a9
Merge remote-tracking branch 'origin/master' into fz/inaccessible-schema
fredzqm May 13, 2024
932be9c
feedbacks
fredzqm May 13, 2024
70d9d69
Merge remote-tracking branch 'origin/dataconnect' into fz/inaccessibl…
fredzqm May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 15 additions & 14 deletions src/dataconnect/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,31 +4,26 @@
const PRECONDITION_ERROR_TYPESTRING = "type.googleapis.com/google.rpc.PreconditionFailure";
const INCOMPATIBLE_CONNECTOR_TYPE = "INCOMPATIBLE_CONNECTOR";

export function getIncompatibleSchemaError(err: any): IncompatibleSqlSchemaError | undefined {

Check warning on line 7 in src/dataconnect/errors.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing JSDoc comment

Check warning on line 7 in src/dataconnect/errors.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unexpected any. Specify a different type
const original = err.context?.body?.error || err.orignal;
if (!original) {
// If we can't get the original, rethrow so we don't cover up the original error.
throw err;
const incompatibles = errorDetails(err, INCOMPATIBLE_SCHEMA_ERROR_TYPESTRING);
if (incompatibles.length === 0) {
return undefined;
}
const details: any[] = original.details;
const incompatibles = details.filter((d) =>
d["@type"]?.includes(INCOMPATIBLE_SCHEMA_ERROR_TYPESTRING),
);
// Should never get multiple incompatible schema errors
return incompatibles[0];
const incompatible = incompatibles[0];

Check warning on line 13 in src/dataconnect/errors.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe assignment of an `any` value
// Extract the violation type from the precondition error detail.
const preconditionErrs = errorDetails(err, PRECONDITION_ERROR_TYPESTRING);
incompatible.violationType = preconditionErrs[0].violations[0].type;

Check warning on line 16 in src/dataconnect/errors.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe assignment of an `any` value

Check warning on line 16 in src/dataconnect/errors.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe member access .violationType on an `any` value

Check warning on line 16 in src/dataconnect/errors.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe member access .violations on an `any` value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fragile to future changes - are we sure that we'll always return only one precondition error detail (maybe)? Are we sure that it will always contain exactly 1 violation (probably not)?

Would prefer something at least like like:

if (preconditonErrs.length) {
const preconditionErr = preconditionErrs[0]
incompatible.violationType = preconditonErr.violations.some(v => v.type === "INACCESSIBLE_SCHEMA") ? "INACCESSIBLE_SCHEMA" : "INCOMPATIBLE_SCHEMA";
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true right now. If backend returns IncompatibleSqlSchemaError, the first PreconditionFailure should be that.

Though you made a good point on future changes. Let me make it detect the first violation type that match.

return incompatible;

Check warning on line 17 in src/dataconnect/errors.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe return of an `any` typed value
}

// Note - the backend just includes file name, not the name of the connector resource in the GQLerror extensions.
// so we don't use this yet. Ideally, we'd just include connector name in the extensions.
export function getInvalidConnectors(err: any): string[] {

Check warning on line 22 in src/dataconnect/errors.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Missing JSDoc comment

Check warning on line 22 in src/dataconnect/errors.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unexpected any. Specify a different type
const preconditionErrs = errorDetails(err, PRECONDITION_ERROR_TYPESTRING);
const invalidConns: string[] = [];
const original = err.context?.body?.error || err?.orignal;
const details: any[] = original?.details;
const preconditionErrs = details?.filter((d) =>
d["@type"]?.includes(PRECONDITION_ERROR_TYPESTRING),
);
for (const preconditionErr of preconditionErrs) {
const incompatibleConnViolation = preconditionErr?.violations?.filter(

Check warning on line 26 in src/dataconnect/errors.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe assignment of an `any` value
(v: { type: string }) => v.type === INCOMPATIBLE_CONNECTOR_TYPE,
);
const newConns = incompatibleConnViolation?.map((i: { subject: string }) => i.subject) ?? [];
Expand All @@ -36,3 +31,9 @@
}
return invalidConns;
}

function errorDetails(err: any, ofType: string): any[] {
const original = err.context?.body?.error || err?.original;
const details: any[] = original?.details;
return details?.filter((d) => d["@type"]?.includes(ofType)) || [];
}
30 changes: 24 additions & 6 deletions src/dataconnect/schemaMigration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export async function migrateSchema(args: {
if (!shouldDeleteInvalidConnectors && invalidConnectors.length) {
const cmd = suggestedCommand(serviceName, invalidConnectors);
throw new FirebaseError(
`Command aborted. Try deploying compatible connectors first with ${clc.bold(cmd)}`,
`Command aborted. Try deploying those connectors first with ${clc.bold(cmd)}`,
);
}
const migrationMode = incompatible
Expand Down Expand Up @@ -315,11 +315,29 @@ async function ensureServiceIsConnectedToCloudSql(
}

function displaySchemaChanges(error: IncompatibleSqlSchemaError) {
const message =
"Your new schema is incompatible with the schema of your CloudSQL database. " +
"The following SQL statements will migrate your database schema to match your new Data Connect schema.\n" +
error.diffs.map(toString).join("\n");
logLabeledWarning("dataconnect", message);
switch (error.violationType) {
case "INCOMPATIBLE_SCHEMA":
{
const message =
"Your new schema is incompatible with the schema of your CloudSQL database. " +
"The following SQL statements will migrate your database schema to match your new Data Connect schema.\n" +
error.diffs.map(toString).join("\n");
logLabeledWarning("dataconnect", message);
}
break;
case "INACCESSIBLE_SCHEMA":
{
const message =
"Cannot access your CloudSQL database to validate schema. " +
"The following SQL statements can setup a new database schema.\n" +
error.diffs.map(toString).join("\n");
logLabeledWarning("dataconnect", message);
logLabeledWarning("dataconnect", "Some SQL resources may already exist.");
}
break;
default:
throw new FirebaseError("Unknown schema violation type: " + error.violationType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include the error message here too - if this ever get hit, it will be very helpful to have it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, at this point, we don't have access to the original error message. I can include the whole diff error detail here.

}
}

function toString(diff: Diff) {
Expand Down
8 changes: 6 additions & 2 deletions src/dataconnect/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,16 @@ export interface File {
content: string;
}

// An error indicating that the SQL database schema is incomptible with a data connect schema.
// An error indicating that the SQL database schema is incompatible with a data connect schema.
export interface IncompatibleSqlSchemaError {
// A list of differences between the two schema with instrucitons how to resolve them.
// A list of differences between the two schema with instructions how to resolve them.
diffs: Diff[];
// Whether any of the changes included are destructive.
destructive: boolean;

// The failed precondition validation type.
// Either "INCOMPATIBLE_SCHEMA" or "INACCESSIBLE_SCHEMA".
violationType: string;
fredzqm marked this conversation as resolved.
Show resolved Hide resolved
}

export interface Diff {
Expand Down