fix(imports): Error when importing charts / dashboards with missing DB credentials

[fisjac]

SUMMARY

When importing charts, dashboards, and databases, certain databases (BigQuery) require credentials to be entered into the encrypted extra field for authorization. When importing this type of database, or any chart or dashboard that relies on this type of database, the db creation will fail generating an import failed error.

This fix implements a try except block to catch connection errors when attempting to populate db catalogs when importing a db. It prevents bigquery exceptions from being remapped to generic Exception types.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
https://www.loom.com/share/ca5efd11ba004c19aea9c83484bdab47?sid=6be35dfc-42e1-45b2-81dd-0a8df1d41dd6

After:
https://www.loom.com/share/a9cc247ddb434ee198f81503514f6776?sid=93c7481f-9ace-4def-9862-6567ecf2f32b

TESTING INSTRUCTIONS

Export a bigquery database or a chart/dashboard linked to a bigquery db.
Delete the underlying db
Attempt to import the db, chart, or dashboard.
The import should be successful, even though the db does not have auth creds

ADDITIONAL INFORMATION

• Has associated issue: Dashboard Import error #30383
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.92%. Comparing base (76d897e) to head (b1dc06a).
Report is 966 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #30503       +/-   ##
===========================================
+ Coverage   60.48%   83.92%   +23.43%     
===========================================
  Files        1931      533     -1398     
  Lines       76236    38538    -37698     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32344    -13770     
+ Misses      28017     6194    -21823     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	49.00% <33.33%> (-0.17%)	⬇️
javascript	?
mysql	76.76% <66.66%> (?)
postgres	76.89% <66.66%> (?)
presto	53.49% <33.33%> (-0.32%)	⬇️
python	83.92% <100.00%> (+20.43%)	⬆️
sqlite	76.34% <66.66%> (?)
unit	60.72% <100.00%> (+3.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Antonio-RiveroMartnez]

This might occur by other connection errors and not only the credentials one, right? So we would be affecting other engines too and not just BigQuery. Could we instead use the custom_errors in the db_engine_spec and map the credentials one to something we could target individually?

[fisjac]

Correct, this would affect other DBs. While this bug emerged in the specific case of BigQuery, there are other DBs that may error as well when trying to set up catalogs on import if it's unable to connect. Regardless of the cause of the connection error, be it creds or otherwise, we still shouldn't be blocking the import because it can't connect when looking to add catalogs, right?

[fisjac]

Currently the credentials error for BigQuery is being mapped to the SupersetDBAPIConnectionError

superset/superset/db_engine_specs/bigquery.py

Line 605 in 68c9a81

return {DefaultCredentialsError: SupersetDBAPIConnectionError}

[betodealmeida]

Ideally we'd only allow the exception if the database is configured for OAuth2. But even more ideally we'd do what the TODO says, and use CreateDatabaseCommand for the imports, since the command already handles edge cases like this. So I think this is OK for now.

[betodealmeida]

Looks good!

[betodealmeida]

This method parse_error_exception is very confusing... I think it's really unlikely that str(exception).splitlines()[0].strip() will raise an exception — it only happens if str(exception) is '' or if there's some shennanigans in the exception class __str__ method.

I think what the method is trying to do is limit the exception message to the first line, but there are better ways to do that. But it's OK, we can refactor later.

[fisjac]

The issue I was running into was when a mapped SupersetDBAPIError was being passed into this method, it would then be remapped to a base class Exception type with the message preserved. I was trying to preserve the Exception type as well as the message

[betodealmeida]

Ideally we'd only allow the exception if the database is configured for OAuth2. But even more ideally we'd do what the TODO says, and use CreateDatabaseCommand for the imports, since the command already handles edge cases like this. So I think this is OK for now.