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.

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

refactor: dbapi exception mapping for dbapi's #12869

Merged
merged 4 commits into from
Feb 2, 2021

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Feb 1, 2021

SUMMARY

Lays the foundation for implementing superset's custom DBAPI exceptions from driver exceptions (following a @villebro idea/proposal).

DBAPI drivers raise custom exceptions or straight unexpected exceptions, this causes several problems e.g.: UI messages with sensitive text, use of broad exceptions, not possible to use more fine grain actions when an error occurs at the DBAPI.

Fixes: #11822

ADDITIONAL INFORMATION

@dpgaspar dpgaspar requested review from villebro and removed request for villebro February 1, 2021 14:41
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

This is beautiful! 🌮

@codecov-io
Copy link

codecov-io commented Feb 1, 2021

Codecov Report

Merging #12869 (c99864b) into master (9a7fba8) will decrease coverage by 3.62%.
The diff coverage is 86.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12869      +/-   ##
==========================================
- Coverage   67.04%   63.41%   -3.63%     
==========================================
  Files        1022      490     -532     
  Lines       50186    30245   -19941     
  Branches     5204        0    -5204     
==========================================
- Hits        33647    19181   -14466     
+ Misses      16404    11064    -5340     
+ Partials      135        0     -135     
Flag Coverage Δ
cypress ?
javascript ?
python 63.41% <86.53%> (-0.67%) ⬇️

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

Impacted Files Coverage Δ
superset/db_engine_specs/elasticsearch.py 86.95% <66.66%> (-7.49%) ⬇️
superset/db_engine_specs/clickhouse.py 87.09% <78.57%> (-7.35%) ⬇️
superset/db_engine_specs/base.py 80.09% <89.47%> (-5.88%) ⬇️
superset/db_engine_specs/exceptions.py 100.00% <100.00%> (ø)
superset/sql_validators/postgres.py 50.00% <0.00%> (-50.00%) ⬇️
superset/views/database/views.py 62.69% <0.00%> (-24.88%) ⬇️
superset/databases/commands/create.py 83.67% <0.00%> (-8.17%) ⬇️
superset/databases/commands/update.py 85.71% <0.00%> (-8.17%) ⬇️
superset/sql_validators/base.py 93.33% <0.00%> (-6.67%) ⬇️
... and 551 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 9a7fba8...c99864b. Read the comment docs.

Copy link
Member

@willbarrett willbarrett left a comment

Choose a reason for hiding this comment

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

This is pretty cool! I'm wondering if we want to have some manner of fallback error so that we can remove all the broad exceptions? With this there's still a chance of an un-mapped exception making it to the top of the stack.

@classmethod
def get_dbapi_exception_mapping(cls) -> Dict[Type[Exception], Type[Exception]]:
"""
Each engine can implement and converge it's own specific exceptions into
Copy link
Member

Choose a reason for hiding this comment

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

Grammar nit: "its" not "it's"

Copy link
Member Author

@dpgaspar dpgaspar Feb 2, 2021

Choose a reason for hiding this comment

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

Fixed, yes that's a good point, a simple way of doing is:

if not new_exception:
    return SupersetDBAPIError(str(exception))

But can have broader implications, I want to place the base for this, and just interfere on the defined exceptions for Elasticsearch and clickhouse (for now).

@dpgaspar dpgaspar merged commit 6c018c0 into apache:master Feb 2, 2021
@dpgaspar dpgaspar deleted the fix/dbapi-exception-remap branch February 2, 2021 15:07
amitmiran137 pushed a commit to nielsen-oss/superset that referenced this pull request Feb 3, 2021
* master: (23 commits)
  feat(explore): clear search on dataset change (apache#12909)
  chore: remove SIP-38 feature flag (apache#12894)
  fix: Config for dataset health check (apache#12906)
  fix(chart): allow null for most query object props (apache#12905)
  feat: add separate endpoint to fetch function names for autocomplete (apache#12840)
  chore: add required review on master (apache#12694)
  fix: comment typo (apache#12898)
  Migrates Radio component from Bootstrap to AntD. (apache#12738)
  fix: allow users to reset their passwords (apache#12886)
  fix(explore): missing select when groupby without metrics (apache#12890)
  refactor: dbapi exception mapping for dbapi's (apache#12869)
  feat(style-theme): add support for custom superset themes (apache#12858)
  chore(lint): fix pre-commit error (apache#12884)
  refactor(color-schemes): refactor setting of color schemes (apache#12857)
  feat(native-filters): Add defaultValue for Native filters modal (apache#12199)
  feat(release): add github token to changelog script (apache#12872)
  fix(menu): always show settings dropdown (apache#12877)
  Migrates Label component from Bootstrap to AntD. (apache#12774)
  [Helm] Automate datasource import (apache#10771)
  build: Skip loading example data from configs in CI (apache#12610)
  ...

:return: A map of driver specific exception to superset custom exceptions
"""
return {}
Copy link
Member

Choose a reason for hiding this comment

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

@dpgaspar you should be able to build this automatically, since the SQLAlchemy dialect exposts the DB API module, which should have the exceptions at the top level with standard names.

Something like this (untested):

dbapi = cls.get_engine(database).dialect.dbapi()

return {
  getattr(dbapi, name): getattr(superset.exceptions, f"SupersetDBAPI{name}"
  for name in {"ProgrammingError", "DatabaseError", etc}
}

Maybe have this as the base method and allow subclasses to define their own?

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/L 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shouldn't print datasouce user&&password when datasource crash
6 participants