fix(dataset): fetch metadata on dataset creation may raise broad exceptions

[dpgaspar]

SUMMARY

fetch_metadata may raise broad exceptions from database drivers, the try/catch block was too narrow

Example pydruid can raise when a column type is not supported:

pydruid.db.exceptions.NotSupportedError: Type BLOB is not supported

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

[codecov-io]

Codecov Report

Merging #11973 (82e9ab0) into master (cc44a2c) will decrease coverage by 5.97%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #11973      +/-   ##
==========================================
- Coverage   66.26%   60.29%   -5.98%     
==========================================
  Files         941      897      -44     
  Lines       45715    43912    -1803     
  Branches     4395     3889     -506     
==========================================
- Hits        30293    26475    -3818     
- Misses      15287    17437    +2150     
+ Partials      135        0     -135     

Flag	Coverage Δ
cypress	54.43% <ø> (+10.62%)	⬆️
javascript	?
python	63.39% <0.00%> (-0.87%)	⬇️

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

Impacted Files	Coverage Δ
superset/connectors/sqla/models.py	90.36% <0.00%> (-0.42%)	⬇️
...uperset-frontend/src/dashboard/util/dnd-reorder.js	0.00% <0.00%> (-100.00%)	⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js	0.00% <0.00%> (-100.00%)	⬇️
...dashboard/components/resizable/ResizableHandle.jsx	0.00% <0.00%> (-100.00%)	⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js	0.00% <0.00%> (-93.48%)	⬇️
...src/dashboard/components/gridComponents/Header.jsx	10.52% <0.00%> (-86.85%)	⬇️
...rc/dashboard/components/gridComponents/Divider.jsx	13.33% <0.00%> (-86.67%)	⬇️
...ontend/src/dashboard/util/getDashboardFilterKey.ts	14.28% <0.00%> (-85.72%)	⬇️
...end/src/SqlLab/components/ExploreResultsButton.jsx	8.00% <0.00%> (-84.00%)	⬇️
...nd/src/views/CRUD/data/query/QueryPreviewModal.tsx	14.70% <0.00%> (-82.97%)	⬇️
... and 398 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc44a2c...82e9ab0. Read the comment docs.

[villebro]

It feels pretty harsh to catch Exception, I wonder if there is some slightly more specific exception (or small set of exceptions) that could be used to catch the majority of these. Another option could be adding db specific exceptions to the db engine spec and then checking for those. Something like

def get_dbapi_exceptions(): Tuple[Exception, ...]:
    from pydruid import CustomPydryidException
    return (CustomPydryidException, )

[dpgaspar]

It feels pretty harsh to catch Exception, I wonder if there is some slightly more specific exception (or small set of exceptions) that could be used to catch the majority of these. Another option could be adding db specific exceptions to the db engine spec and then checking for those. Something like

def get_dbapi_exceptions(): Tuple[Exception, ...]:
    from pydruid import CustomPydryidException
    return (CustomPydryidException, )

I really don't like introducing broad exceptions, but dbapi exception's are raising custom exceptions from Exception. Yet created this related PR on pydruid to set SQLAlchemy visit_* methods raise from CompileError: druid-io/pydruid#243
I think is what we should expect here. But other drivers will raise childs of Exception.
Introducing all these imports inside superset will increase complexity and maintainability, IMO better to live with the broad except

[betodealmeida]

LGTM.

I'm taking a look at pydruid and trying to repro the problem and see what needs to be done to support BLOBs.

[john-bodley]

I thought SQLAlchemy should have the mechanism to catch all DB-API exceptions. Here is the SQLAlchemy wrapping of the NotSupportedError which is a child of sqlalchemy.exc.DatabaseError which is a child of sqlalchemy.exc.DBAPIError which eventually rolls up to a sqlalchemy.exc.SQLAlchemyError exception.

Either DBAPIError or SQLAlchemyError seems like a better candidate that Exception for broad SQLAlchemy issues.

[dpgaspar]

I thought SQLAlchemy should have the mechanism to catch all DB-API exceptions. Here is the SQLAlchemy wrapping of the NotSupportedError which is a child of sqlalchemy.exc.DatabaseError which is a child of sqlalchemy.exc.DBAPIError which eventually rolls up to a sqlalchemy.exc.SQLAlchemyError exception.

Either DBAPIError or SQLAlchemyError seems like a better candidate that Exception for broad SQLAlchemy issues.

True, I was following the logic directly on pydruid: https://github.com/druid-io/pydruid/blob/master/pydruid/db/exceptions.py#L37

That ends up on Exception, SQLAlchemy wraps db-api exceptions, but this one was slipping and ended up being a 500 on the API. Will test it further

[betodealmeida]

FIY, I fixed the BLOB issuse in pydruid here: druid-io/pydruid#244

[dpgaspar]

Thats awesome @betodealmeida I think this approach here still makes sense since other drivers may raise on unsupported types. I'll double check if the exception is narrowed by SQLAlchemy or if we have to content with a broad exception

[dpgaspar]

@john-bodley unfortunately these exceptions from sqlalchemy dialects (at SQL compile) do not raise any SQLAlchemy exception and SQLAlchemy does not wrap these up. Used elasticsearch-dbapi that follows the same exception pattern has pydruid and done an inpect MRO on the exception on our external_metadata method:

                except Exception as ex:  # pylint: disable=broad-except
                    import inspect
                    print(f"{inspect.getmro(ex.__class__)}")
                    col["type"] = "UNKNOWN"

The output:

(<class 'es.exceptions.NotSupportedError'>,
 <class 'es.exceptions.DatabaseError'>,
 <class 'es.exceptions.Error'>,
 <class 'Exception'>, <class 'BaseException'>, <class 'object'>
)

So it seems that we are bound to catching whatever exceptions the dialects may raise