feat: add denylist for db engines #21486

eschutho · 2022-09-15T21:45:54Z

SUMMARY

Some db_engine_specs can have multiple db_engine_specs, Databricks for example. Since we build the database connection form available dbs dropdown list based on db_engine_specs that match installed drivers, there are times when one may not want to show all options. This is a simple optional denylist config that removes extra db engines from that list.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

With this code and no extra config added (this is what it looks like without this code):
Additional drivers installed:
databricks-sql-connector 2.0.2
sqlalchemy-databricks 0.2.0

With added local superset_config of:

DBS_AVAILABLE_DENYLIST: Dict[str, Tuple[str, ...]] = {
    "databricks": ("pyhive", "pyodbc")}

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
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 · 2022-09-15T21:50:50Z

Codecov Report

Merging #21486 (09afa27) into master (aef6217) will increase coverage by 9.81%.
The diff coverage is 83.33%.

❗ Current head 09afa27 differs from pull request most recent head 2b9c704. Consider uploading reports for the commit 2b9c704 to get more accurate results

@@            Coverage Diff             @@
##           master   #21486      +/-   ##
==========================================
+ Coverage   55.46%   65.28%   +9.81%     
==========================================
  Files        1800     1792       -8     
  Lines       68940    68671     -269     
  Branches     7334     7320      -14     
==========================================
+ Hits        38236    44830    +6594     
+ Misses      28812    21951    -6861     
+ Partials     1892     1890       -2

Flag	Coverage Δ
hive	`52.91% <83.33%> (-0.01%)`	⬇️
mysql	`78.21% <83.33%> (?)`
postgres	`78.28% <83.33%> (?)`
presto	`?`
python	`78.62% <83.33%> (+20.70%)`	⬆️
sqlite	`76.78% <83.33%> (?)`
unit	`?`

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

Impacted Files	Coverage Δ
superset/db_engine_specs/__init__.py	`84.52% <80.00%> (+0.97%)`	⬆️
superset/config.py	`91.61% <100.00%> (+0.64%)`	⬆️
superset/tables/schemas.py	`0.00% <0.00%> (-100.00%)`	⬇️
superset/columns/schemas.py	`0.00% <0.00%> (-100.00%)`	⬇️
...-chart-table/src/DataTable/utils/useMountedMemo.ts	`0.00% <0.00%> (-87.50%)`	⬇️
...set/advanced_data_type/plugins/internet_address.py	`16.32% <0.00%> (-79.60%)`	⬇️
superset/utils/pandas_postprocessing/boxplot.py	`20.51% <0.00%> (-79.49%)`	⬇️
superset/charts/post_processing.py	`11.76% <0.00%> (-77.95%)`	⬇️
...perset/advanced_data_type/plugins/internet_port.py	`18.75% <0.00%> (-77.09%)`	⬇️
superset/utils/pandas_postprocessing/rolling.py	`21.87% <0.00%> (-68.75%)`	⬇️
... and 474 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

superset/config.py

Antonio-RiveroMartnez · 2022-09-22T19:34:18Z

superset/db_engine_specs/__init__.py

+        if (
+            engine_spec.engine in dbs_denylist_engines
+            and hasattr(engine_spec, "default_driver")
+            and engine_spec.default_driver in dbs_denylist[engine_spec.engine]


Curious, why just checking if the default_driver is in the list and not any other that engine might have?

it's just a way to identify the db_engine_spec so that the default driver option doesn't show up in the database connection list.

eschutho · 2022-09-23T19:47:21Z

tests/unit_tests/conftest.py

+    if request and hasattr(request, "param"):
+        for key, val in request.param.items():
+            app.config[key] = val
+


@betodealmeida I extended this method so that you can pass in extra configs from tests.

Oh, I like the idea!

Should we give this a more descriptive name, just in case? Eg, override_app_config instead of param?

Also, how is this used? Is request a Flask request object?

I added typing here. Request is a pytest fixture sub request object. So I can’t change the name of param but agree it would read better.

Antonio-RiveroMartnez

LGTM!

betodealmeida

Looks great, I just left a couple comments!

superset/config.py

betodealmeida · 2022-09-26T23:42:08Z

tests/unit_tests/conftest.py

+    if request and hasattr(request, "param"):
+        for key, val in request.param.items():
+            app.config[key] = val
+


Oh, I like the idea!

Should we give this a more descriptive name, just in case? Eg, override_app_config instead of param?

Also, how is this used? Is request a Flask request object?

pull-request-size bot added the size/L label Sep 15, 2022

eschutho force-pushed the elizabeth/db_engine_blacklist branch from 23b44d9 to 1e932ff Compare September 15, 2022 21:50

eschutho requested a review from betodealmeida September 15, 2022 21:50

villebro reviewed Sep 16, 2022

View reviewed changes

superset/config.py Outdated Show resolved Hide resolved

eschutho changed the title ~~feat: add blacklist for db engines~~ feat: add denylist for db engines Sep 19, 2022

eschutho force-pushed the elizabeth/db_engine_blacklist branch 2 times, most recently from ad1a4e9 to b29799f Compare September 19, 2022 23:50

Antonio-RiveroMartnez reviewed Sep 22, 2022

View reviewed changes

eschutho commented Sep 23, 2022

View reviewed changes

Antonio-RiveroMartnez approved these changes Sep 23, 2022

View reviewed changes

betodealmeida approved these changes Sep 26, 2022

View reviewed changes

eschutho added 4 commits October 7, 2022 16:52

add denylist for db engines

ceade4e

clean up nomenclature

6477940

fix comment

14c237f

change some types

2b9c704

eschutho force-pushed the elizabeth/db_engine_blacklist branch from 40d08b6 to 2b9c704 Compare October 7, 2022 23:57

eschutho merged commit f58227a into apache:master Oct 9, 2022

eschutho deleted the elizabeth/db_engine_blacklist branch October 21, 2022 20:59

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add denylist for db engines #21486

feat: add denylist for db engines #21486

eschutho commented Sep 15, 2022 •

edited

Loading

codecov bot commented Sep 15, 2022 •

edited

Loading

Antonio-RiveroMartnez Sep 22, 2022

eschutho Sep 23, 2022

eschutho Sep 23, 2022

betodealmeida Sep 26, 2022

eschutho Oct 9, 2022 •

edited

Loading

Antonio-RiveroMartnez left a comment

betodealmeida left a comment

betodealmeida Sep 26, 2022

feat: add denylist for db engines #21486

feat: add denylist for db engines #21486

Conversation

eschutho commented Sep 15, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Sep 15, 2022 • edited Loading

Codecov Report

Antonio-RiveroMartnez Sep 22, 2022

Choose a reason for hiding this comment

eschutho Sep 23, 2022

Choose a reason for hiding this comment

eschutho Sep 23, 2022

Choose a reason for hiding this comment

betodealmeida Sep 26, 2022

Choose a reason for hiding this comment

eschutho Oct 9, 2022 • edited Loading

Choose a reason for hiding this comment

Antonio-RiveroMartnez left a comment

Choose a reason for hiding this comment

betodealmeida left a comment

Choose a reason for hiding this comment

betodealmeida Sep 26, 2022

Choose a reason for hiding this comment

eschutho commented Sep 15, 2022 •

edited

Loading

codecov bot commented Sep 15, 2022 •

edited

Loading

eschutho Oct 9, 2022 •

edited

Loading