You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/09/28 06:06:09 UTC
[PR] fix(doris): offer the connection form by matching the installed driver [superset]
aminghadersohi opened a new pull request, #44736:
URL: https://github.com/apache/superset/pull/44736
### SUMMARY
The database picker never offers the Apache Doris connection form; users have to type a SQLAlchemy URI.
`/api/v1/database/available/` includes the `parameters` schema (which is what makes the modal render the form) only when the engine spec's `default_driver` is one of the drivers installed for its engine. `DorisEngineSpec.default_driver` was `"pydoris"`, but pydoris registers a `MySQLDialect_mysqldb` subclass (named `pydoris`) under the `doris` and `pydoris` entry points, so the only installed driver Superset detects is `mysqldb`:
```
available_drivers: ['mysqldb'], default_driver: 'pydoris' -> no "parameters", no form
```
This sets `default_driver = "mysqldb"`. A URI built from the form would then be `pydoris+mysqldb://`, which is not a SQLAlchemy entry point either (`NoSuchModuleError`), so `build_sqlalchemy_uri` now emits the registered `doris://` scheme. (#44718 carries the same `build_sqlalchemy_uri` change for the old `pydoris+pydoris` default; whichever merges second can drop the duplicate.)
### TESTING INSTRUCTIONS
`pytest tests/unit_tests/db_engine_specs/test_doris.py` โ the three new cases fail on master and pass here.
Live, against a local Apache Doris 4.1.3 all-in-one container with pydoris 1.2.0 / mysqlclient: with this change `get_available_engine_specs()` reports `mysqldb` for Doris and `default_driver` matches it, so the form is offered; the URI built from the form parameters (`doris://root:@127.0.0.1:<port>/internal.information_schema`) loads the pydoris dialect and connects.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(doris): offer the connection form by matching the installed driver [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44736:
URL: https://github.com/apache/superset/pull/44736#issuecomment-5864466460
<!-- Bito Reply -->
The review comment is correct. In the test `test_build_sqlalchemy_uri_uses_the_doris_scheme`, using `make_url` from `sqlalchemy.engine.url` only parses the URI string into a URL object; it does not verify if the `drivername` (in this case, `doris`) corresponds to a registered SQLAlchemy dialect. Consequently, the test passes even if the `doris` dialect is not properly registered or cannot create an engine.
To resolve this, you should verify that the dialect is actually loadable by SQLAlchemy. You can update the test to attempt to create an engine or load the dialect explicitly using `sqlalchemy.engine.create_engine` or `sqlalchemy.dialects.registry.load`.
Would you like me to implement this fix for the test, and would you like me to check and address the rest of the comments on this PR?
**tests/unit_tests/db_engine_specs/test_doris.py**
```
uri = DorisEngineSpec.build_sqlalchemy_uri(parameters)
# Verify the dialect is registered and loadable
from sqlalchemy.engine import create_engine
create_engine(uri)
url = make_url(uri)
assert url.drivername == "doris"
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(doris): offer the connection form by matching the installed driver [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44736:
URL: https://github.com/apache/superset/pull/44736#issuecomment-5927331904
**Bito Automatic Review Skipped โ PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(doris): offer the connection form by matching the installed driver [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44736:
URL: https://github.com/apache/superset/pull/44736#issuecomment-5864633712
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44736?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `71.42857%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 66.14%. Comparing base ([`b6be1dd`](https://app.codecov.io/gh/apache/superset/commit/b6be1dd5548c49a82aa0fda4cc6ac9b95e431999?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`bae1d08`](https://app.codecov.io/gh/apache/superset/commit/bae1d082882041dc461eff3f1776faa53e4b0e94?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44736?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/db\_engine\_specs/doris.py](https://app.codecov.io/gh/apache/superset/pull/44736?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Fdoris.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2RvcmlzLnB5) | 71.42% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44736?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44736 +/- ##
==========================================
+ Coverage 57.18% 66.14% +8.95%
==========================================
Files 2968 2968
Lines 179363 179368 +5
Branches 41597 41597
==========================================
+ Hits 102564 118635 +16071
+ Misses 75812 58153 -17659
- Partials 987 2580 +1593
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44736/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44736/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.74% <71.42%> (+<0.01%)` | :arrow_up: |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44736/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.07% <71.42%> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44736/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.64% <71.42%> (+<0.01%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/44736/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.16% <71.42%> (+17.47%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44736?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(doris): offer the connection form by matching the installed driver [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #44736:
URL: https://github.com/apache/superset/pull/44736#issuecomment-5905636319
@fitzee thanks for the thorough check. Both notes are addressed in 1da7d19f96, after merging master in 5bd233a8c3.
1. SSL switch: the `encryption_parameters` comment and the Doris docs metadata (engine spec `notes` and `databases.json`) now say that `VERIFY_CA` also verifies the hostname with MariaDB Connector/C. The switch needs `ssl_ca=<path>` in Additional parameters and an FE certificate matching the host, so a default self-signed FE fails closed.
2. Denylist: UPDATING.md notes that hiding Doris now takes `{"pydoris": {"mysqldb"}}`. `test_denylist_matches_the_default_driver` covers the new key and the old one.
StarRocks is left for a separate change.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(doris): offer the connection form by matching the installed driver [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(doris): offer the connection form by matching the installed driver [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #44736:
URL: https://github.com/apache/superset/pull/44736
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(doris): offer the connection form by matching the installed driver [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44736:
URL: https://github.com/apache/superset/pull/44736#issuecomment-5906318669
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(doris): offer the connection form by matching the installed driver [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44736:
URL: https://github.com/apache/superset/pull/44736#discussion_r4119093940
##########
tests/unit_tests/db_engine_specs/test_doris.py:
##########
@@ -280,3 +280,77 @@ def test_get_catalog_names(
# Verify the returned catalog names
assert catalogs == expected_result
+
+
+def test_connection_form_default_driver_is_installed(mocker: MockerFixture) -> None:
+ """
+ The database picker offers the connection form only when ``default_driver``
+ is among the drivers installed for the engine. pydoris registers a
+ ``MySQLDialect_mysqldb`` subclass named ``pydoris`` under both the ``doris``
+ and ``pydoris`` entry points, so its driver is ``mysqldb``.
+ """
+ from sqlalchemy.dialects.mysql.mysqldb import MySQLDialect_mysqldb
+
+ from superset.db_engine_specs import get_available_engine_specs
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ class PyDorisDialect(MySQLDialect_mysqldb):
+ name = "pydoris"
+
+ def entry_point(name: str) -> Any:
+ ep = mocker.MagicMock()
+ ep.name = name
+ ep.value = "pydoris.sqlalchemy.dialect:DorisDialect"
+ ep.load.return_value = PyDorisDialect
+ return ep
+
+ mocker.patch(
+ "superset.db_engine_specs.load_engine_specs",
+ return_value=iter([DorisEngineSpec]),
+ )
+ mocker.patch(
+ "superset.db_engine_specs.entry_points",
+ side_effect=lambda group: (
+ [entry_point("doris"), entry_point("pydoris")]
+ if group == "sqlalchemy.dialects"
+ else []
+ ),
+ )
+
+ drivers = get_available_engine_specs()[DorisEngineSpec]
+
+ assert drivers == {"mysqldb"}
+ assert DorisEngineSpec.default_driver in drivers
+
+
+@pytest.mark.parametrize("encryption", [False, True])
+def test_build_sqlalchemy_uri_uses_the_doris_scheme(encryption: bool) -> None:
+ """
+ A URI built from the connection form must name a registered dialect:
+ ``pydoris+mysqldb`` (``engine+default_driver``) is not one, ``doris`` is.
+ """
+ from superset.db_engine_specs.base import BasicParametersType
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ parameters: BasicParametersType = {
+ "username": "user",
+ "password": "p@ss",
+ "host": "doris.example.com",
+ "port": 9030,
+ "database": "internal.sales",
+ "query": {},
+ "encryption": encryption,
+ }
+
+ uri = DorisEngineSpec.build_sqlalchemy_uri(parameters)
+
+ url = make_url(uri)
Review Comment:
**Suggestion:** Parsing with `make_url` does not resolve the dialect, so this test passes even when the emitted `doris` URI cannot create a SQLAlchemy engine.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Often` ยท ๐ท๏ธ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e041a82ebaae4535a338a9ff11cca86a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e041a82ebaae4535a338a9ff11cca86a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/db_engine_specs/test_doris.py
**Line:** 347:347
**Comment:**
*Api Mismatch: Parsing with `make_url` does not resolve the dialect, so this test passes even when the emitted `doris` URI cannot create a SQLAlchemy engine.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44736&comment_hash=a18ccea38230e2433ada837fc369fecafd6881c7865693f82d962448fc5c416b&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44736&comment_hash=a18ccea38230e2433ada837fc369fecafd6881c7865693f82d962448fc5c416b&reaction=dislike'>๐</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(doris): offer the connection form by matching the installed driver [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44736:
URL: https://github.com/apache/superset/pull/44736#discussion_r4129351982
##########
tests/unit_tests/db_engine_specs/test_doris.py:
##########
@@ -280,3 +280,77 @@ def test_get_catalog_names(
# Verify the returned catalog names
assert catalogs == expected_result
+
+
+def test_connection_form_default_driver_is_installed(mocker: MockerFixture) -> None:
+ """
+ The database picker offers the connection form only when ``default_driver``
+ is among the drivers installed for the engine. pydoris registers a
+ ``MySQLDialect_mysqldb`` subclass named ``pydoris`` under both the ``doris``
+ and ``pydoris`` entry points, so its driver is ``mysqldb``.
+ """
+ from sqlalchemy.dialects.mysql.mysqldb import MySQLDialect_mysqldb
+
+ from superset.db_engine_specs import get_available_engine_specs
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ class PyDorisDialect(MySQLDialect_mysqldb):
+ name = "pydoris"
+
+ def entry_point(name: str) -> Any:
+ ep = mocker.MagicMock()
+ ep.name = name
+ ep.value = "pydoris.sqlalchemy.dialect:DorisDialect"
+ ep.load.return_value = PyDorisDialect
+ return ep
+
+ mocker.patch(
+ "superset.db_engine_specs.load_engine_specs",
+ return_value=iter([DorisEngineSpec]),
+ )
+ mocker.patch(
+ "superset.db_engine_specs.entry_points",
+ side_effect=lambda group: (
+ [entry_point("doris"), entry_point("pydoris")]
+ if group == "sqlalchemy.dialects"
+ else []
+ ),
+ )
+
+ drivers = get_available_engine_specs()[DorisEngineSpec]
+
+ assert drivers == {"mysqldb"}
+ assert DorisEngineSpec.default_driver in drivers
+
+
+@pytest.mark.parametrize("encryption", [False, True])
+def test_build_sqlalchemy_uri_uses_the_doris_scheme(encryption: bool) -> None:
+ """
+ A URI built from the connection form must name a registered dialect:
+ ``pydoris+mysqldb`` (``engine+default_driver``) is not one, ``doris`` is.
+ """
+ from superset.db_engine_specs.base import BasicParametersType
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ parameters: BasicParametersType = {
+ "username": "user",
+ "password": "p@ss",
+ "host": "doris.example.com",
+ "port": 9030,
+ "database": "internal.sales",
+ "query": {},
+ "encryption": encryption,
+ }
+
+ uri = DorisEngineSpec.build_sqlalchemy_uri(parameters)
+
+ url = make_url(uri)
Review Comment:
โ
**CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `0b87d32`.
The test now calls `url.get_dialect()` and asserts it resolves to `MySQLDialect_mysqldb`, verifying the emitted `doris` URI can resolve a SQLAlchemy dialect.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(doris): offer the connection form by matching the installed driver [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44736:
URL: https://github.com/apache/superset/pull/44736#discussion_r4140699509
##########
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:
Pulled the `encryption_parameters` fix in here in 43e385bce7 rather than waiting on #44718: the switch now sets `ssl_mode=VERIFY_CA` (the same value #44718 uses, verified there against Doris 4.1.3 with mysqlclient/MariaDB Connector/C), so it requires TLS instead of sending `ssl=0`. The `encryption=True` test case now asserts that mysqlclient's connect kwargs carry `ssl_mode=VERIFY_CA` and no `ssl` key, and that it still round-trips as encrypted. The normalization of saved `ssl=1` / `REQUIRED` URLs stays in #44718.
##########
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:
Right, thanks. Fixed in 43e385bce7: `build_sqlalchemy_uri` now sets the drivername to `cls.engine` (`pydoris`, which pydoris registers), and the test asserts `make_url(uri).get_backend_name() == DorisEngineSpec.engine` alongside resolving the dialect.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(doris): offer the connection form by matching the installed driver [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44736:
URL: https://github.com/apache/superset/pull/44736#issuecomment-5864413637
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(doris): offer the connection form by matching the installed driver [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44736:
URL: https://github.com/apache/superset/pull/44736#issuecomment-5885003999
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(doris): offer the connection form by matching the installed driver [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44736:
URL: https://github.com/apache/superset/pull/44736#discussion_r4129381329
##########
tests/unit_tests/db_engine_specs/test_doris.py:
##########
@@ -280,3 +280,77 @@ def test_get_catalog_names(
# Verify the returned catalog names
assert catalogs == expected_result
+
+
+def test_connection_form_default_driver_is_installed(mocker: MockerFixture) -> None:
+ """
+ The database picker offers the connection form only when ``default_driver``
+ is among the drivers installed for the engine. pydoris registers a
+ ``MySQLDialect_mysqldb`` subclass named ``pydoris`` under both the ``doris``
+ and ``pydoris`` entry points, so its driver is ``mysqldb``.
+ """
+ from sqlalchemy.dialects.mysql.mysqldb import MySQLDialect_mysqldb
+
+ from superset.db_engine_specs import get_available_engine_specs
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ class PyDorisDialect(MySQLDialect_mysqldb):
+ name = "pydoris"
+
+ def entry_point(name: str) -> Any:
+ ep = mocker.MagicMock()
+ ep.name = name
+ ep.value = "pydoris.sqlalchemy.dialect:DorisDialect"
+ ep.load.return_value = PyDorisDialect
+ return ep
+
+ mocker.patch(
+ "superset.db_engine_specs.load_engine_specs",
+ return_value=iter([DorisEngineSpec]),
+ )
+ mocker.patch(
+ "superset.db_engine_specs.entry_points",
+ side_effect=lambda group: (
+ [entry_point("doris"), entry_point("pydoris")]
+ if group == "sqlalchemy.dialects"
+ else []
+ ),
+ )
+
+ drivers = get_available_engine_specs()[DorisEngineSpec]
+
+ assert drivers == {"mysqldb"}
+ assert DorisEngineSpec.default_driver in drivers
+
+
+@pytest.mark.parametrize("encryption", [False, True])
+def test_build_sqlalchemy_uri_uses_the_doris_scheme(encryption: bool) -> None:
+ """
+ A URI built from the connection form must name a registered dialect:
+ ``pydoris+mysqldb`` (``engine+default_driver``) is not one, ``doris`` is.
+ """
+ from superset.db_engine_specs.base import BasicParametersType
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ parameters: BasicParametersType = {
+ "username": "user",
+ "password": "p@ss",
+ "host": "doris.example.com",
+ "port": 9030,
+ "database": "internal.sales",
+ "query": {},
+ "encryption": encryption,
+ }
+
+ uri = DorisEngineSpec.build_sqlalchemy_uri(parameters)
+
+ url = make_url(uri)
Review Comment:
Good catch, make_url only parses the string. Added a fixture that registers the two dialects pydoris ships and the test now asserts through url.get_dialect(), which is the resolution step create_engine does first, plus a companion test that the pydoris+mysqldb form raises NoSuchModuleError. Fixed in 0b87d323a6.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(doris): offer the connection form by matching the installed driver [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44736:
URL: https://github.com/apache/superset/pull/44736#issuecomment-5883128429
### <span aria-hidden="true">โ
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">๐จ</span> Latest commit | 0b87d323a6be494f5d10d2e6e8852b9b50440776 |
|<span aria-hidden="true">๐</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6abb315d07d1940008213986 |
|<span aria-hidden="true">๐</span> Deploy Preview | [https://deploy-preview-44736--superset-docs-preview.netlify.app](https://deploy-preview-44736--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">๐ฑ</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">๐ค</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#q1YqKMrPLShRslJS0lFKKkrMS84AslPyizKLdVNS0xJLc0p0U4oyy1KLgPIpqQU5-ZWeKUAVZolJScaGpikG5imGliYGBgYWRobGlhZmSrUA) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44736--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org