-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
chore: switching out ConnectorRegistry references for DatasourceDAO #20149
Conversation
…uperset into connreg-to-datadao_2
…reg-to-datadao_2
@@ -282,7 +286,7 @@ def request_access(self) -> FlaskResponse: | |||
datasource_id = request.args.get("datasource_id") | |||
datasource_type = request.args.get("datasource_type") | |||
if datasource_id and datasource_type: | |||
ds_class = ConnectorRegistry.sources.get(datasource_type) | |||
ds_class = DatasourceDAO.sources.get(datasource_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use DatasourceType(datasource_type)
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DatasourceDAO.sources should be able to take either a string or the DatasourceType instance, so I think either works here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of course you'll get an error either way if the datasource_type is not valid.
superset/views/core.py
Outdated
@@ -2535,7 +2539,7 @@ def fetch_datasource_metadata(self) -> FlaskResponse: # pylint: disable=no-self | |||
""" | |||
|
|||
datasource_id, datasource_type = request.args["datasourceKey"].split("__") | |||
datasource = ConnectorRegistry.get_datasource( | |||
datasource = DatasourceDAO.get_datasource( | |||
datasource_type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use DatasourceType(datasource.type)
superset/views/datasource/views.py
Outdated
@@ -74,7 +75,7 @@ def save(self) -> FlaskResponse: | |||
datasource_id = datasource_dict.get("id") | |||
datasource_type = datasource_dict.get("type") | |||
database_id = datasource_dict["database"].get("id") | |||
orm_datasource = ConnectorRegistry.get_datasource( | |||
orm_datasource = DatasourceDAO.get_datasource( | |||
datasource_type, datasource_id, db.session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use DatasourceType(datasource.type)
@@ -132,7 +132,7 @@ def test_query_cache_key_changes_when_datasource_is_updated(self): | |||
cache_key_original = query_context.query_cache_key(query_object) | |||
|
|||
# make temporary change and revert it to refresh the changed_on property | |||
datasource = ConnectorRegistry.get_datasource( | |||
datasource = DatasourceDAO.get_datasource( | |||
datasource_type=payload["datasource"]["type"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use DatasourceType(datasource.type)
@@ -156,7 +156,7 @@ def test_query_cache_key_changes_when_metric_is_updated(self): | |||
payload = get_query_context("birth_names") | |||
|
|||
# make temporary change and revert it to refresh the changed_on property | |||
datasource = ConnectorRegistry.get_datasource( | |||
datasource = DatasourceDAO.get_datasource( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use DatasourceType(datasource.type)
superset/models/dashboard.py
Outdated
@@ -407,16 +408,19 @@ 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_) | |||
# todo(phillip): get datasource type for this method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this comment
# pylint: disable=import-outside-toplevel | ||
from superset.datasource.dao import DatasourceDAO | ||
|
||
return DatasourceDAO.sources[self.datasource_type] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use DatasourceType(datasource_type)
superset/cachekeys/api.py
Outdated
@@ -83,13 +83,14 @@ def invalidate(self) -> Response: | |||
return self.response_400(message=str(error)) | |||
datasource_uids = set(datasources.get("datasource_uids", [])) | |||
for ds in datasources.get("datasources", []): | |||
ds_obj = ConnectorRegistry.get_datasource_by_name( | |||
ds_obj = DatasourceDAO.get_datasource_by_name( | |||
session=db.session, | |||
datasource_type=ds.get("datasource_type"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update DatasourceType(datasource_type)
c9cd671
to
6a85df3
Compare
66cdb1c
to
95c5eb2
Compare
821da1c
to
ac2d8aa
Compare
efb667e
to
051c26c
Compare
051c26c
to
ec17f51
Compare
Decided to move all the functions into the model |
@@ -75,7 +76,7 @@ def run(self) -> Optional[str]: | |||
new_state: TemporaryExploreState = { | |||
"owner": owner, | |||
"datasource_id": datasource_id, | |||
"datasource_type": datasource_type, | |||
"datasource_type": DatasourceType(datasource_type), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this one different from the above one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you referencing this class definition?
https://github.com/apache/superset/pull/20149/files/b678a8ba981f7687c17bc68b4f529209bde2d961#diff-6029cd3077bd4123d18311935a0fef83a580ccf22f4a98b6085ed780fd7257e9R27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I am referring to this: https://github.com/apache/superset/pull/20149/files/b678a8ba981f7687c17bc68b4f529209bde2d961#diff-6029cd3077bd4123d18311935a0fef83a580ccf22f4a98b6085ed780fd7257e9R27
THis one has DatasourceType(datasource_type)
the version above has DatasourceType
def get_datasource( | ||
cls, session: Session, datasource_type: DatasourceType, datasource_id: int | ||
) -> Datasource: | ||
if datasource_type not in cls.sources: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're going to want to accept the string value as well here.
I don't think "table" in cls.sources
is going to work for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm currently casting all the values going into the get_datasource
function like this:
DatasourceDAO.get_datasource(
db.session, DatasourceType(datasource["type"]), int(datasource["id"])
)
So moving forward i think we should push for the enums since it's easy to cast via just wrapping it in a DatasourceType()
constructor. If we allow string we'd have to add logic to handle/report whenever the user sending us invalid values.
I'm okay with putting the Union[str, DatasourceType]
but don't think it takes much work to just wrap the string with the DatasourceType
constructor before passing it into the function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could also just wrap source = cls.sources[datasource_type]
in a try/catch block. I just think there's too much room for error here across the application with people who may not know that they can't pass in a string. This should work if datasource_type is either a string or the enum key.
superset/security/manager.py
Outdated
set | ||
) | ||
session = self.get_session | ||
all_datasources = SqlaTable.get_all_sqlatables_datasources(session) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, but any reason why you wouldn't name this method get_all_datasources
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most are small questions, but if datasource_type not in cls.sources:
could cause problems if a string is passed in. I personally think we could allow both types, but we could prob go either way. It's just going to be difficult to enforce with typing.
|
||
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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use the SqlaTable model directly since this is the only option here?
cls, session: Session, datasource_type: DatasourceType, datasource_id: int | ||
) -> Datasource: | ||
if datasource_type not in cls.sources: | ||
raise DatasourceTypeNotSupportedError() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notes: set this exception to return 422
|
||
@classmethod | ||
def get_datasource( | ||
cls, session: Session, datasource_type: DatasourceType, datasource_id: int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go with a Union[str, DatasourceType]
and add test to verify works both and will throw 422
Moving here #20380 |
SUMMARY
Switching out all references of
ConnectorRegistry
toDatasourceDAO
. 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