Display accurate message for INACCESSIBLE_SCHEMA error #7157
Conversation
fredzqm
changed the title
src/dataconnect/errors.ts
Outdated
| const incompatible = incompatibles[0]; | ||
| // Extract the violation type from the precondition error detail. | ||
| const preconditionErrs = errorDetails(err, PRECONDITION_ERROR_TYPESTRING); | ||
| incompatible.violationType = preconditionErrs[0].violations[0].type; |
There was a problem hiding this comment.
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";
}
There was a problem hiding this comment.
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.
src/dataconnect/schemaMigration.ts
Outdated
| } | ||
| break; | ||
| default: | ||
| throw new FirebaseError("Unknown schema violation type: " + error.violationType); |
There was a problem hiding this comment.
Include the error message here too - if this ever get hit, it will be very helpful to have it.
There was a problem hiding this comment.
Unfortunately, at this point, we don't have access to the original error message. I can include the whole diff error detail here.
src/dataconnect/errors.ts
Outdated
| const incompatible = incompatibles[0]; | ||
| // Extract the violation type from the precondition error detail. | ||
| const preconditionErrs = errorDetails(err, PRECONDITION_ERROR_TYPESTRING); | ||
| incompatible.violationType = preconditionErrs[0].violations[0].type; |
There was a problem hiding this comment.
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.
src/dataconnect/schemaMigration.ts
Outdated
| } | ||
| break; | ||
| default: | ||
| throw new FirebaseError("Unknown schema violation type: " + error.violationType); |
There was a problem hiding this comment.
Unfortunately, at this point, we don't have access to the original error message. I can include the whole diff error detail here.
* Cloud SQL create and update polish (#7159) * improve logged message execute SQL commands (#7156) * Error handling and logging when linking CSQL instance (#7158) * Display accurate message for INACCESSIBLE_SCHEMA error (#7157) * handle INACCESSIBLE_SCHEMA case separately * error msg * m * m * m * merge * feedbacks * Jh idx no metadata calls (#7179) * Avoid triggering metadata server calls in idx * Remove outdated service account check * Update Firebase Logo (#7180) * unleash turtles (#7174) * unleash turtles * Update CHANGELOG.md * Flag flip for gen kit (#7175) * Flag flip for gen kit * Update CHANGELOG.md * update icons --------- Co-authored-by: Bryan Kendall <bkend@google.com> Co-authored-by: joehan <joehanley@google.com> * Use client instead of size 1 pool (#7182) * Fix schema validation (#7185) * Fix schema validation * Also depend on redhat.yaml * fix typo --------- Co-authored-by: Fred Zhang <fredzqm@google.com> Co-authored-by: Harold Shen <hlshen@google.com> Co-authored-by: Bryan Kendall <bkend@google.com>
Description
INACCESSIBLE_SCHEMAfrom the backend. Print accurate error message. In practice,INACCESSIBLE_SCHEMAshould occur rarely:firebase deployafter deleting the P4SA IAM user, soINCOMPATIBLE_SCHEMAerror are returned in Apply after provisioning connectivity.There was a subtle typo
s/orignal/original, failed with:Scenarios Tested
firebase sql:migrate:diffwhen schema is accessible vs not.Sample Commands