EnxDev commented on code in PR #44736:
URL: https://github.com/apache/superset/pull/44736#discussion_r4137523495
##########
superset/db_engine_specs/doris.py:
##########
@@ -112,7 +113,10 @@ class DorisEngineSpec(MySQLEngineSpec):
engine_aliases = {"doris"}
engine_name = "Apache Doris"
max_column_name_length = 64
- default_driver = "pydoris"
+ # pydoris registers its dialect (a ``MySQLDialect_mysqldb`` subclass) as
+ # ``doris`` and ``pydoris``, so the installed driver is ``mysqldb``. The
+ # connection form is only offered when ``default_driver`` is installed.
+ default_driver = "mysqldb"
Review Comment:
This is what turns the form on, which also puts the SSL switch in front of
users for the first time. With `encryption_parameters = {"ssl": "0"}` a few
lines down, flipping it on doesn't enable TLS (#44718 walks through why), so
the connection goes out in cleartext while the UI says it's encrypted.
Could this land after #44718, or pull that `encryption_parameters` fix in
here? The `encryption=True` case in the new test round-trips `ssl=0` as
encrypted, so it'll need updating either way.
##########
superset/db_engine_specs/doris.py:
##########
@@ -278,6 +282,21 @@ class DorisEngineSpec(MySQLEngineSpec):
),
}
+ @classmethod
+ def build_sqlalchemy_uri(
+ cls,
+ parameters: BasicParametersType,
+ encrypted_extra: Optional[dict[str, str]] = None,
+ ) -> str:
+ uri = super().build_sqlalchemy_uri(parameters, encrypted_extra)
+ # ``engine+default_driver`` would be ``pydoris+mysqldb``, which no
+ # SQLAlchemy entry point provides; ``doris`` is the dialect's scheme.
+ return (
+ make_url_safe(uri)
+ .set(drivername="doris")
+ .render_as_string(hide_password=False)
+ )
Review Comment:
Haven't clicked through it, but I think `doris` breaks editing a DB made
with this form. The saved URL's backend comes back as `doris`, and the modal
picks its form model by matching `available.engine` (`pydoris`) against
`db.backend` (`DatabaseModal/index.tsx:771`), so nothing matches, `dbModel` is
`{}`, and `DatabaseConnectionForm` renders an empty Basic tab.
Since pydoris also registers `pydoris` (your fixture covers it), emitting
that keeps the backend equal to `engine`. Worth asserting
`make_url(uri).get_backend_name() == DorisEngineSpec.engine` in the test too.
```suggestion
# ``engine+default_driver`` would be ``pydoris+mysqldb``, which no
# SQLAlchemy entry point provides. ``pydoris`` is registered, and
keeps
# the URL backend equal to ``engine`` so the edit modal finds the
form.
return (
make_url_safe(uri)
.set(drivername=cls.engine)
.render_as_string(hide_password=False)
)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
