chore: switching out ConnectorRegistry references for DatasourceDAO #20380

hughhhh · 2022-06-14T15:02:30Z

SUMMARY

Switching out all references of ConnectorRegistry to DatasourceDAO. In the PR, we are focusing on just removing the references and moving towards allowing other Datasources to power charts.

One issue is the circular dependencies that are blocking us from fully removing the logic to allow the app to run. So i've moved that code out the ConnectorRegistry for now and will prioritize a bigger refactor to stop the import issues in superset moving forward.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

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

…uperset into connreg-to-datadao_2

…reg-to-datadao_2

AAfghahi · 2022-06-14T17:20:32Z

tests/integration_tests/fixtures/energy_dashboard.py

@@ -82,7 +82,6 @@ def _create_energy_table():
        table.metrics.append(
            SqlMetric(metric_name="sum__value", expression=f"SUM({col})")
        )
-


nit: We could keep this in here.

eschutho · 2022-06-14T19:42:26Z

superset/initialization/__init__.py

+        # todo(hughhhh): fully remove the datasource config register
+        for module_name, class_names in module_datasource_map.items():
+            class_names = [str(s) for s in class_names]
+            __import__(module_name, fromlist=class_names)


I was thinking that we wouldn't have to init anything to make this work?

We should be able to remove the DEFAULT_MODULE_DS_MAP and ADDITIONAL_MODULE_DS_MAP for SIP68

for some reason the app won't without this code being here, can we put that into another ticket?

eschutho · 2022-06-14T19:44:57Z

superset/models/dashboard.py

@@ -407,16 +408,18 @@ def export_dashboards(  # pylint: disable=too-many-locals
                    id_ = target.get("datasetId")
                    if id_ is None:
                        continue
-                    datasource = ConnectorRegistry.get_datasource_by_id(session, id_)
+                    datasource = DatasourceDAO.get_datasource(


Same as above.. I don't think we need to use the DatasourceDAO if there's only one type that will be used.

eschutho · 2022-06-14T19:45:25Z

superset/examples/helpers.py


 BASE_URL = "https://github.com/apache-superset/examples-data/blob/master/"

 misc_dash_slices: Set[str] = set()  # slices assembled in a 'Misc Chart' dashboard


 def get_table_connector_registry() -> Any:
-    return ConnectorRegistry.sources["table"]
+    return DatasourceDAO.sources[DatasourceType.TABLE]


can we use sqlatable lookup directly instead of the DatasourceDAO since it's only one type?

eschutho · 2022-06-14T19:47:42Z

tests/integration_tests/dashboard_utils.py

@@ -35,7 +36,7 @@ def get_table(
    schema: Optional[str] = None,
 ):
    schema = schema or get_example_default_schema()
-    table_source = ConnectorRegistry.sources["table"]
+    table_source = DatasourceDAO.sources[DatasourceType.TABLE]


why not just import SQLATable here directly?

eschutho · 2022-06-14T19:47:54Z

tests/integration_tests/dashboard_utils.py

@@ -54,7 +55,7 @@ def create_table_metadata(

    table = get_table(table_name, database, schema)
    if not table:
-        table_source = ConnectorRegistry.sources["table"]
+        table_source = DatasourceDAO.sources[DatasourceType.TABLE]


eschutho · 2022-06-14T19:48:22Z

tests/integration_tests/datasource_tests.py

@@ -256,8 +258,8 @@ def test_external_metadata_error_return_400(self, mock_get_datasource):

        pytest.raises(
            SupersetGenericDBErrorException,
-            lambda: ConnectorRegistry.get_datasource(
-                "table", tbl.id, db.session
+            lambda: DatasourceDAO.get_datasource(


can we query just sqlatable here?

codecov · 2022-06-15T07:55:37Z

Codecov Report

Merging #20380 (6830f9c) into master (41f33a3) will decrease coverage by 0.10%.
The diff coverage is 61.64%.

❗ Current head 6830f9c differs from pull request most recent head ed31bf1. Consider uploading reports for the commit ed31bf1 to get more accurate results

@@            Coverage Diff             @@
##           master   #20380      +/-   ##
==========================================
- Coverage   66.64%   66.53%   -0.11%     
==========================================
  Files        1729     1738       +9     
  Lines       64904    65062     +158     
  Branches     6842     6897      +55     
==========================================
+ Hits        43257    43292      +35     
- Misses      19897    20017     +120     
- Partials     1750     1753       +3

Flag	Coverage Δ
hive	`?`
mysql	`82.35% <93.10%> (+0.07%)`	⬆️
postgres	`82.42% <93.10%> (+0.07%)`	⬆️
presto	`?`
python	`82.47% <93.10%> (-0.31%)`	⬇️
sqlite	`?`
unit	`?`

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

Impacted Files	Coverage Δ
...art-controls/src/operators/contributionOperator.ts	`100.00% <ø> (ø)`
...ui-chart-controls/src/operators/flattenOperator.ts	`100.00% <ø> (ø)`
...ui-chart-controls/src/operators/prophetOperator.ts	`100.00% <ø> (ø)`
...i-chart-controls/src/operators/resampleOperator.ts	`100.00% <ø> (ø)`
...art-controls/src/sections/annotationsAndLayers.tsx	`100.00% <ø> (ø)`
.../shared-controls/components/RadioButtonControl.tsx	`0.00% <ø> (ø)`
...chart-controls/src/shared-controls/dndControls.tsx	`26.92% <0.00%> (-8.98%)`	⬇️
...ages/superset-ui-core/src/query/types/Dashboard.ts	`100.00% <ø> (ø)`
...egacy-plugin-chart-event-flow/src/controlPanel.tsx	`14.28% <0.00%> (ø)`
.../legacy-plugin-chart-world-map/src/controlPanel.ts	`25.00% <0.00%> (-8.34%)`	⬇️
... and 164 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 41f33a3...ed31bf1. Read the comment docs.

…uperset into connreg-to-datadao_2

hughhhh · 2022-06-16T16:30:28Z

tests/integration_tests/datasource_tests.py

-            DatasetNotFoundError,
-            lambda: ConnectorRegistry.get_datasource("table", 9999999, db.session),
+            DatasourceNotFound,
+            lambda: DatasourceDAO.get_datasource(db.session, "table", 9999999),


These are testing the actual function calls inside the views/datasource.py files which call DatasourceDAO now

https://github.com/apache/superset/pull/20380/files#diff-bea5cc1ab7cb7816bd17972ec665905f07a35f2c4cd77cd962d5db580a8640bcL77

…pache#20380) * rename and move dao file * Update dao.py * add cachekey * Update __init__.py * change reference in query context test * add utils ref * more ref changes * add helpers * add todo in dashboard.py * add cachekey * circular import error in dar.py * push rest of refs * fix linting * fix more linting * update enum * remove references for connector registry * big reafctor * take value * fix * test to see if removing value works * delete connectregistry * address concerns * address comments * fix merge conflicts * address concern II * address concern II * fix test Co-authored-by: Phillip Kelley-Dotson <pkelleydotson@yahoo.com>

pkdotson and others added 26 commits May 18, 2022 15:11

rename and move dao file

0fbde4a

Update dao.py

148f400

add cachekey

c63d575

Merge branch 'connreg-to-datadao_2' of https://github.com/preset-io/s…

139e8f4

…uperset into connreg-to-datadao_2

Update __init__.py

bb79bc6

change reference in query context test

8119f7a

add utils ref

7804433

more ref changes

7ea4d1e

add helpers

e691320

add todo in dashboard.py

d0a694a

add cachekey

c92443a

circular import error in dar.py

606a285

push rest of refs

f0d308f

fix linting

0e44849

Merge branch 'master' of https://github.com/apache/superset into conn…

ea7ee06

…reg-to-datadao_2

fix more linting

6439dfa

update enum

36bf229

remove references for connector registry

ba919fc

fixed merge conflicts

5dff58f

big reafctor

95c5eb2

take value

e08c4b8

fix

ec17f51

test to see if removing value works

19de44a

delete connectregistry

b678a8b

address concerns

748805e

address comments

eb0da19

pull-request-size bot added the size/XL label Jun 14, 2022

hughhhh changed the title ~~Connreg to datadao 2~~ chore: switching out ConnectorRegistry references for DatasourceDAO Jun 14, 2022

hughhhh mentioned this pull request Jun 14, 2022

chore: switching out ConnectorRegistry references for DatasourceDAO #20149

Closed

9 tasks

fix merge conflicts

b9b77b6

fix merge conflicts

a7555ee

hughhhh requested review from betodealmeida, eschutho and AAfghahi June 14, 2022 16:42

AAfghahi approved these changes Jun 14, 2022

View reviewed changes

eschutho reviewed Jun 14, 2022

View reviewed changes

hughhhh force-pushed the connreg-to-datadao_2 branch from d4c13be to fe33b5f Compare June 15, 2022 08:17

address concern II

64ecbfe

hughhhh force-pushed the connreg-to-datadao_2 branch from fe33b5f to 64ecbfe Compare June 15, 2022 08:41

hughhhh added 3 commits June 15, 2022 20:12

address concern II

f282ad2

Merge branch 'connreg-to-datadao_2' of https://github.com/preset-io/s…

0653a58

…uperset into connreg-to-datadao_2

fix test

ed31bf1

hughhhh commented Jun 16, 2022

View reviewed changes

eschutho approved these changes Jun 17, 2022

View reviewed changes

hughhhh merged commit e3e37cb into apache:master Jun 21, 2022

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: switching out ConnectorRegistry references for DatasourceDAO #20380

chore: switching out ConnectorRegistry references for DatasourceDAO #20380

hughhhh commented Jun 14, 2022 •

edited

Loading

AAfghahi Jun 14, 2022

eschutho Jun 14, 2022

eschutho Jun 14, 2022

hughhhh Jun 15, 2022

eschutho Jun 14, 2022

eschutho Jun 14, 2022

eschutho Jun 14, 2022

eschutho Jun 14, 2022

eschutho Jun 14, 2022

codecov bot commented Jun 15, 2022 •

edited

Loading

hughhhh Jun 16, 2022 •

edited

Loading

chore: switching out ConnectorRegistry references for DatasourceDAO #20380

chore: switching out ConnectorRegistry references for DatasourceDAO #20380

Conversation

hughhhh commented Jun 14, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 15, 2022 • edited Loading

Codecov Report

hughhhh Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

hughhhh commented Jun 14, 2022 •

edited

Loading

codecov bot commented Jun 15, 2022 •

edited

Loading

hughhhh Jun 16, 2022 •

edited

Loading