You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "eschutho (via GitHub)" <gi...@apache.org> on 2026/09/15 05:17:29 UTC
[PR] feat(druid): support the dynamic connection form [superset]
eschutho opened a new pull request, #44279:
URL: https://github.com/apache/superset/pull/44279
### SUMMARY
Epic [sc-120993](https://app.shortcut.com/preset/story/120993) migrates DB connections to the new dynamic connection form, which appears when an engine spec defines a `parameters_schema`. This PR does that for **Apache Druid** (engine `druid`).
Story: https://app.shortcut.com/preset/story/120998
`DruidEngineSpec` now mixes in `BasicParametersMixin`, so `/api/v1/database/available/` returns individual connection parameters (host, port, username, password, encryption, extra query params) instead of the raw SQLAlchemy-URI box.
**Design decisions**
The pydruid SQLAlchemy URI is `druid://user:password@host:port/druid/v2/sql/`, which differs from the plain `BasicParametersMixin` pattern in two ways:
- **Fixed SQL endpoint path** — the path `/druid/v2/sql/` is constant (it is pydruid's default endpoint and is never entered by the user). A custom `DruidParametersSchema` therefore omits the `database` field, and `build_sqlalchemy_uri` injects the fixed path (`sqlalchemy_uri_database`).
- **http/https toggle via the driver** — pydruid registers `druid` (and `druid+http`) for plain HTTP and `druid+https` for TLS (the `DruidHTTPSDialect`, `scheme="https"`). The `encryption` toggle switches the driver name accordingly rather than adding a query parameter, and `get_parameters_from_uri` recovers it from the driver name so the form round-trips.
`username`/`password` are optional (anonymous Druid clusters are allowed); only `host` and `port` are required. The existing custom-certificate path (`get_extra_params` setting `connect_args.scheme=https`) is unchanged.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend engine-spec change; the frontend renders the dynamic form generically from `parameters_schema`).
### TESTING INSTRUCTIONS
- `pytest tests/unit_tests/db_engine_specs/test_druid.py`
- Manually: with `pydruid` installed, add a database → select **Apache Druid** → confirm the form now shows host/port/username/password/encryption fields (not the raw URI box), and that toggling **encryption** produces a `druid+https://…/druid/v2/sql/` URI.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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] feat(druid): support the dynamic connection form [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44279:
URL: https://github.com/apache/superset/pull/44279#issuecomment-5688924876
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] feat(druid): support the dynamic connection form [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44279:
URL: https://github.com/apache/superset/pull/44279#discussion_r4171702455
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/DruidParameters.tsx:
##########
@@ -0,0 +1,95 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { t } from '@apache-superset/core/translation';
+import { SupersetTheme } from '@apache-superset/core/theme';
+import { Switch } from '@superset-ui/core/components/Switch';
+import {
+ InfoTooltip,
+ LabeledErrorBoundInput as ValidatedInput,
+} from '@superset-ui/core/components';
+import { FieldPropTypes } from '../../types';
+import { toggleStyle, infoTooltip } from '../styles';
+
+/**
+ * Druid-specific variants of the common connection form fields.
+ *
+ * Superset connects to Druid's SQL endpoint on the broker (port 8082 by
+ * default) or the router (8888), and the encryption toggle switches the pydruid
+ * dialect from `druid://` (HTTP) to `druid+https://` rather than setting a
+ * Postgres-style SSL mode.
+ */
+export const DRUID_ENGINE = 'druid';
+
+export const druidPortField = ({
+ required,
+ changeMethods,
+ getValidation,
+ validationErrors,
+ db,
+ isValidating,
+}: FieldPropTypes) => (
+ <ValidatedInput
+ id="port"
+ name="port"
+ type="number"
+ isValidating={isValidating}
+ required={required}
+ value={db?.parameters?.port as number}
+ validationMethods={{ onBlur: getValidation }}
+ errorMessage={validationErrors?.port}
+ placeholder={t('e.g. 8082 (8888 for the router)')}
+ className="form-group-w-50"
+ label={t('Port')}
+ onChange={changeMethods.onParametersChange}
+ />
+);
+
+export const druidEncryptionField = ({
+ isEditMode,
+ changeMethods,
+ db,
+ sslForced,
+}: FieldPropTypes) => (
+ <div css={(theme: SupersetTheme) => infoTooltip(theme)}>
+ <Switch
+ disabled={sslForced && !isEditMode}
+ checked={db?.parameters?.encryption || sslForced}
+ onChange={changed => {
+ changeMethods.onParametersChange({
+ target: {
+ type: 'toggle',
+ name: 'encryption',
+ checked: true,
+ value: changed,
+ },
+ });
+ }}
+ />
+ <span css={toggleStyle}>{t('SSL')}</span>
+ <InfoTooltip
+ tooltip={t('HTTPS will be used to connect to the Druid SQL endpoint.')}
+ placement="right"
+ />
+ </div>
+);
+
+export const DRUID_FORM_FIELD_MAP = {
+ port: druidPortField,
+ encryption: druidEncryptionField,
+};
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated field components</b></div>
<div id="fix">
`druidPortField` and `druidEncryptionField` are near-verbatim copies of `portField` and `forceSSLField` in CommonParameters.tsx, differing only in placeholder/tooltip text. Duplicating the full component means future changes to the common field (validation, styling, disabled logic) must be made in two places and can silently diverge. Consider parameterizing placeholder/tooltip on the common fields and having the Druid override supply them.
</div>
</div>
<small><i>Code Review Run #9aca26</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/db_engine_specs/test_druid.py:
##########
@@ -258,3 +259,247 @@ def test_unmask_encrypted_extra() -> None:
assert DruidEngineSpec.unmask_encrypted_extra(old, new) == json.dumps(
{"connect_args": {"scheme": "http", "jwt": "old-token", "password": "new"}}
)
+
+
+# ---------------------------------------------------------------------------
+# Dynamic connection form (BasicParametersMixin)
+#
+# Druid connects over a fixed SQL endpoint (`/druid/v2/sql/`) and toggles TLS by
+# switching between pydruid's `druid` (http) and `druid+https` dialects, so the
+# form omits the database field and encodes encryption in the driver name rather
+# than a query parameter.
+# ---------------------------------------------------------------------------
+
+
+def _parameters(**overrides: Any) -> Any:
+ from superset.db_engine_specs.base import BasicParametersType
+
+ parameters: dict[str, Any] = {
+ "username": "user",
+ "password": "pwd",
+ "host": "localhost",
+ "port": 9088,
+ "query": {},
+ }
+ parameters.update(overrides)
+ return cast(BasicParametersType, parameters)
+
+
+def test_get_engine_spec_supports_parameters() -> None:
+ """
+ Druid must resolve to a spec that supports the dynamic connection form so
+ the ``/available`` endpoint returns individual parameters.
+ """
+ from superset.db_engine_specs import get_engine_spec
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ spec = cast("type[DruidEngineSpec]", get_engine_spec("druid"))
+ assert spec is DruidEngineSpec
+ assert spec.parameters_schema is not None
+ assert hasattr(spec, "build_sqlalchemy_uri")
+
+
+@pytest.mark.parametrize(
+ "encryption,expected_driver",
+ [
+ (False, "druid"),
+ (True, "druid+https"),
+ ],
+)
+def test_build_sqlalchemy_uri_toggles_scheme(
+ encryption: bool, expected_driver: str
+) -> None:
+ """
+ The encryption toggle selects the http vs. https pydruid dialect and always
+ injects the fixed SQL endpoint path.
+ """
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ uri = make_url(
+ DruidEngineSpec.build_sqlalchemy_uri(_parameters(encryption=encryption))
+ )
+
+ assert uri.drivername == expected_driver
+ assert uri.database == "druid/v2/sql/"
+ assert uri.host == "localhost"
+ assert uri.port == 9088
+
+
+def test_build_sqlalchemy_uri_preserves_query_params() -> None:
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ uri = make_url(
+ DruidEngineSpec.build_sqlalchemy_uri(_parameters(query={"header": "true"}))
+ )
+
+ assert uri.query["header"] == "true"
+
+
+def test_build_sqlalchemy_uri_renders_password() -> None:
+ """The stored URI is used to connect, so the password must not be masked."""
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ uri = DruidEngineSpec.build_sqlalchemy_uri(_parameters(password="s3cret")) # noqa: S106
+
+ assert "s3cret" in uri
+
+
+@pytest.mark.parametrize(
+ "uri,expected_encryption",
+ [
+ ("druid://user:pwd@localhost:9088/druid/v2/sql/", False),
+ ("druid+http://user:pwd@localhost:9088/druid/v2/sql/", False),
+ ("druid+https://user:pwd@localhost:9088/druid/v2/sql/", True),
+ ],
+)
+def test_get_parameters_from_uri_encryption(
+ uri: str, expected_encryption: bool
+) -> None:
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ parameters = DruidEngineSpec.get_parameters_from_uri(uri)
+
+ assert parameters["encryption"] is expected_encryption
+ assert parameters["host"] == "localhost"
+ assert parameters["port"] == 9088
+ assert parameters["database"] == "druid/v2/sql/"
+
+
+def test_get_parameters_from_uri_accepts_encrypted_extra_keyword() -> None:
+ """
+ ``Database.parameters`` passes ``encrypted_extra`` by keyword; a signature
+ mismatch would silently empty the connection form.
+ """
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ parameters = DruidEngineSpec.get_parameters_from_uri(
+ "druid+https://user:pwd@localhost:9088/druid/v2/sql/",
+ encrypted_extra={},
+ )
+
+ assert parameters["encryption"] is True
+
+
+@pytest.mark.parametrize("encryption", [True, False])
+def test_parameters_round_trip(encryption: bool) -> None:
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ uri = DruidEngineSpec.build_sqlalchemy_uri(
+ _parameters(encryption=encryption, query={"header": "true"})
+ )
+ parameters = DruidEngineSpec.get_parameters_from_uri(uri)
+
+ assert parameters["encryption"] is encryption
+ assert parameters["host"] == "localhost"
+ assert parameters["port"] == 9088
+ assert parameters["username"] == "user"
+ assert parameters["query"] == {"header": "true"}
+
+
+def test_parameters_json_schema_omits_database() -> None:
+ """
+ The SQL endpoint path is fixed, so ``database`` must not appear as a form
+ field; the encryption toggle must be present instead.
+ """
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ schema = DruidEngineSpec.parameters_json_schema()
+
+ assert "database" not in schema["properties"]
+ assert "encryption" in schema["properties"]
+ assert set(schema["required"]) == {"host", "port"}
+
+
+def test_parameters_schema_reloads_emitted_parameters() -> None:
+ """
+ ``get_parameters_from_uri`` emits the fixed ``database`` path, which is not a
+ form field. Re-loading that dict through the schema (the create/update path)
+ must not raise on the unknown ``database`` key.
+ """
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ parameters = DruidEngineSpec.get_parameters_from_uri(
+ "druid+https://user:pwd@localhost:9088/druid/v2/sql/"
+ )
+ assert parameters["database"] == "druid/v2/sql/"
+
+ loaded = DruidEngineSpec.parameters_schema.load(parameters)
+
+ assert "database" not in loaded
+ assert loaded["host"] == "localhost"
+ assert loaded["encryption"] is True
+
+
+def test_default_driver_matches_pydruid_dialects() -> None:
+ """
+ ``/available`` only returns form parameters when the spec's default driver is
+ among the detected drivers, so it must match what pydruid's dialects report.
+ """
+ sqlalchemy_dialect = pytest.importorskip("pydruid.db.sqlalchemy")
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ detected = {
+ sqlalchemy_dialect.DruidHTTPDialect.driver,
+ sqlalchemy_dialect.DruidHTTPSDialect.driver,
+ }
+
+ assert detected == {"rest"}
+ assert DruidEngineSpec.default_driver in detected
+
+
+def test_build_sqlalchemy_uri_uses_registered_dialect_names() -> None:
+ """
+ The default driver ("rest") is not a registered dialect name, so it must
+ never leak into the generated URI scheme.
+ """
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ for encryption in (True, False):
+ uri = DruidEngineSpec.build_sqlalchemy_uri(_parameters(encryption=encryption))
+ assert "rest" not in make_url(uri).drivername
+
+
+@pytest.mark.parametrize(
+ "uri",
+ [
+ "druid://user:pwd@localhost:8082/druid/v2/sql/",
+ "druid+https://user:pwd@localhost:8082/druid/v2/sql/",
+ ],
+)
+def test_existing_uris_resolve_to_druid_spec(uri: str) -> None:
+ """Existing Druid connections keep resolving to the Druid engine spec."""
+ from superset.db_engine_specs import get_engine_spec
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ url = make_url(uri)
+ driver = url.drivername.split("+")[1] if "+" in url.drivername else "rest"
+
+ assert get_engine_spec(url.get_backend_name(), driver) is DruidEngineSpec
+
+
+@pytest.mark.parametrize(
+ "denylist,expect_available",
+ [
+ ({"druid": {"rest"}}, False),
+ ({"druid": {"other"}}, True),
+ ({}, True),
+ ],
+)
+def test_available_engine_specs_druid_denylist(
+ mocker: Any, denylist: dict[str, set[str]], expect_available: bool
+) -> None:
+ """``DBS_AVAILABLE_DENYLIST`` disables Druid by its "rest" driver name."""
+ from superset.db_engine_specs import get_available_engine_specs
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ mocker.patch(
+ "superset.db_engine_specs.load_engine_specs",
+ return_value=[DruidEngineSpec],
+ )
+ mocker.patch("superset.db_engine_specs.entry_points", return_value=[])
+ app = mocker.patch("superset.db_engine_specs.app")
+ app.config = {"DBS_AVAILABLE_DENYLIST": dict(denylist)}
+
+ available = get_available_engine_specs()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test type annotations</b></div>
<div id="fix">
New tests leave locals and the mock unannotated: `sqlalchemy_dialect` (438), `detected` (441), `url`/`driver` (474-475), `available` (503), and mock `app` (500). Repo rules require explicit annotations on test locals and mock variables (`app: mock.MagicMock`); `mocker: Any` could be `MockerFixture` as in `test_init.py`. Keeps mypy coverage uniform with sibling tests.
</div>
</div>
<small><i>Code Review Run #9aca26</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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] feat(druid): support the dynamic connection form [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44279:
URL: https://github.com/apache/superset/pull/44279#issuecomment-5675277485
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44279?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 75.91%. Comparing base ([`b6de64f`](https://app.codecov.io/gh/apache/superset/commit/b6de64fce749f9d537a32893ef957a70b0b4cb25?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`abf54cb`](https://app.codecov.io/gh/apache/superset/commit/abf54cb4a5ae0c8a15c7be2fe1493c38b6e5c30d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44279 +/- ##
=======================================
Coverage 75.91% 75.91%
=======================================
Files 2926 2926
Lines 173023 173049 +26
Branches 40139 40139
=======================================
+ Hits 131351 131375 +24
- Misses 38993 38995 +2
Partials 2679 2679
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44279/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [python](https://app.codecov.io/gh/apache/superset/pull/44279/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `76.15% <100.00%> (+<0.01%)` | :arrow_up: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44279/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `76.15% <100.00%> (+<0.01%)` | :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/44279?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] feat(druid): support the dynamic connection form [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44279:
URL: https://github.com/apache/superset/pull/44279#issuecomment-5675142505
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] feat(druid): support the dynamic connection form [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 #44279:
URL: https://github.com/apache/superset/pull/44279#discussion_r4200285166
##########
superset/db_engine_specs/druid.py:
##########
@@ -162,6 +212,44 @@ def alter_new_orm_column(cls, orm_col: TableColumn) -> None:
if orm_col.column_name == "__time":
orm_col.is_dttm = True
+ @classmethod
+ def build_sqlalchemy_uri(
+ cls,
+ parameters: BasicParametersType,
+ encrypted_extra: dict[str, str] | None = None,
+ ) -> str:
+ query = parameters.get("query", {}).copy()
+ # `druid+https` selects pydruid's TLS dialect; the bare `druid` driver
+ # connects over plain HTTP.
+ driver = "https" if parameters.get("encryption") else cls.default_driver
+
+ # SQLAlchemy 2.0 hides the password from `URL.__str__()`, so render it
+ # explicitly since this URI is used to connect, not just displayed.
+ return URL.create(
+ f"{cls.engine}+{driver}".rstrip("+"),
+ username=parameters.get("username"),
+ password=parameters.get("password"),
+ host=parameters["host"],
+ port=parameters["port"],
+ database=cls.sqlalchemy_uri_database,
+ query=query,
+ ).render_as_string(hide_password=False)
+
+ @classmethod
+ def get_parameters_from_uri(
+ cls, uri: str, encrypted_extra: dict[str, Any] | None = None
+ ) -> BasicParametersType:
+ url = make_url_safe(uri)
+ return BasicParametersType(
+ username=url.username,
+ password=url.password,
+ host=url.host,
+ port=url.port,
+ database=url.database or cls.sqlalchemy_uri_database,
+ query=dict(url.query),
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `18d0aba`.
Added Marshmallow Meta configuration with `unknown = EXCLUDE`, allowing the `database` value returned from URI parsing to be ignored during dynamic-form validation.
<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] feat(druid): support the dynamic connection form [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 #44279:
URL: https://github.com/apache/superset/pull/44279#discussion_r4171069707
##########
superset/db_engine_specs/druid.py:
##########
@@ -162,6 +212,44 @@ def alter_new_orm_column(cls, orm_col: TableColumn) -> None:
if orm_col.column_name == "__time":
orm_col.is_dttm = True
+ @classmethod
+ def build_sqlalchemy_uri(
+ cls,
+ parameters: BasicParametersType,
+ encrypted_extra: dict[str, str] | None = None,
+ ) -> str:
+ query = parameters.get("query", {}).copy()
+ # `druid+https` selects pydruid's TLS dialect; the bare `druid` driver
+ # connects over plain HTTP.
+ driver = "https" if parameters.get("encryption") else cls.default_driver
+
+ # SQLAlchemy 2.0 hides the password from `URL.__str__()`, so render it
+ # explicitly since this URI is used to connect, not just displayed.
+ return URL.create(
+ f"{cls.engine}+{driver}".rstrip("+"),
+ username=parameters.get("username"),
+ password=parameters.get("password"),
+ host=parameters["host"],
+ port=parameters["port"],
+ database=cls.sqlalchemy_uri_database,
+ query=query,
+ ).render_as_string(hide_password=False)
+
+ @classmethod
+ def get_parameters_from_uri(
+ cls, uri: str, encrypted_extra: dict[str, Any] | None = None
+ ) -> BasicParametersType:
+ url = make_url_safe(uri)
+ return BasicParametersType(
+ username=url.username,
+ password=url.password,
+ host=url.host,
+ port=url.port,
+ database=url.database or cls.sqlalchemy_uri_database,
+ query=dict(url.query),
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `cb99cdd`.
Added Marshmallow Meta configuration with `unknown = EXCLUDE`, allowing the `database` value returned from URI parsing to be ignored during dynamic-form validation.
<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] feat(druid): support the dynamic connection form [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 #44279:
URL: https://github.com/apache/superset/pull/44279#discussion_r4261841039
##########
superset/db_engine_specs/druid.py:
##########
@@ -162,6 +212,44 @@ def alter_new_orm_column(cls, orm_col: TableColumn) -> None:
if orm_col.column_name == "__time":
orm_col.is_dttm = True
+ @classmethod
+ def build_sqlalchemy_uri(
+ cls,
+ parameters: BasicParametersType,
+ encrypted_extra: dict[str, str] | None = None,
+ ) -> str:
+ query = parameters.get("query", {}).copy()
+ # `druid+https` selects pydruid's TLS dialect; the bare `druid` driver
+ # connects over plain HTTP.
+ driver = "https" if parameters.get("encryption") else cls.default_driver
+
+ # SQLAlchemy 2.0 hides the password from `URL.__str__()`, so render it
+ # explicitly since this URI is used to connect, not just displayed.
+ return URL.create(
+ f"{cls.engine}+{driver}".rstrip("+"),
+ username=parameters.get("username"),
+ password=parameters.get("password"),
+ host=parameters["host"],
+ port=parameters["port"],
+ database=cls.sqlalchemy_uri_database,
+ query=query,
+ ).render_as_string(hide_password=False)
+
+ @classmethod
+ def get_parameters_from_uri(
+ cls, uri: str, encrypted_extra: dict[str, Any] | None = None
+ ) -> BasicParametersType:
+ url = make_url_safe(uri)
+ return BasicParametersType(
+ username=url.username,
+ password=url.password,
+ host=url.host,
+ port=url.port,
+ database=url.database or cls.sqlalchemy_uri_database,
+ query=dict(url.query),
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `dcf305a`.
Added `unknown = EXCLUDE` to `DruidParametersSchema.Meta`, so returned `database` values are ignored during schema validation.
<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] feat(druid): support the dynamic connection form [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44279:
URL: https://github.com/apache/superset/pull/44279#issuecomment-5965395765
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] feat(druid): support the dynamic connection form [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 #44279:
URL: https://github.com/apache/superset/pull/44279#discussion_r4201822225
##########
superset/db_engine_specs/druid.py:
##########
@@ -162,6 +212,44 @@ def alter_new_orm_column(cls, orm_col: TableColumn) -> None:
if orm_col.column_name == "__time":
orm_col.is_dttm = True
+ @classmethod
+ def build_sqlalchemy_uri(
+ cls,
+ parameters: BasicParametersType,
+ encrypted_extra: dict[str, str] | None = None,
+ ) -> str:
+ query = parameters.get("query", {}).copy()
+ # `druid+https` selects pydruid's TLS dialect; the bare `druid` driver
+ # connects over plain HTTP.
+ driver = "https" if parameters.get("encryption") else cls.default_driver
+
+ # SQLAlchemy 2.0 hides the password from `URL.__str__()`, so render it
+ # explicitly since this URI is used to connect, not just displayed.
+ return URL.create(
+ f"{cls.engine}+{driver}".rstrip("+"),
+ username=parameters.get("username"),
+ password=parameters.get("password"),
+ host=parameters["host"],
+ port=parameters["port"],
+ database=cls.sqlalchemy_uri_database,
+ query=query,
+ ).render_as_string(hide_password=False)
+
+ @classmethod
+ def get_parameters_from_uri(
+ cls, uri: str, encrypted_extra: dict[str, Any] | None = None
+ ) -> BasicParametersType:
+ url = make_url_safe(uri)
+ return BasicParametersType(
+ username=url.username,
+ password=url.password,
+ host=url.host,
+ port=url.port,
+ database=url.database or cls.sqlalchemy_uri_database,
+ query=dict(url.query),
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `8c57796`.
Added Marshmallow Meta configuration with `unknown = EXCLUDE`, allowing the `database` value returned from URI parsing to be ignored during dynamic-form validation.
<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] feat(druid): support the dynamic connection form [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 #44279:
URL: https://github.com/apache/superset/pull/44279#discussion_r4012218861
##########
superset/db_engine_specs/druid.py:
##########
@@ -40,14 +50,54 @@
_DRUID_FLOAT_SPECIAL = frozenset({"NaN", "Infinity", "-Infinity"})
-class DruidEngineSpec(BaseEngineSpec):
+class DruidParametersSchema(Schema):
+ """
+ Parameters for the dynamic connection form.
+
+ Druid connects over a fixed SQL endpoint (`/druid/v2/sql/`), so the schema
+ intentionally omits the ``database`` field. The ``encryption`` toggle selects
+ the http vs. https pydruid dialect rather than adding a query parameter.
+ """
+
+ username = fields.String(allow_none=True, metadata={"description": __("Username")})
+ password = fields.String(allow_none=True, metadata={"description": __("Password")})
+ host = fields.String(
+ required=True, metadata={"description": __("Hostname or IP address")}
+ )
+ port = fields.Integer(
+ required=True,
+ metadata={"description": __("Database port")},
+ validate=Range(min=0, max=2**16, max_inclusive=False),
+ )
Review Comment:
**Suggestion:** Existing Druid URIs without an explicit port produce `port=None`, but this required field rejects that value and prevents those connections from being edited through the dynamic form. [api mismatch]
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=62634a07213c47bf892937b94caaa447&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=62634a07213c47bf892937b94caaa447&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:** superset/db_engine_specs/druid.py
**Line:** 67:71
**Comment:**
*Api Mismatch: Existing Druid URIs without an explicit port produce `port=None`, but this required field rejects that value and prevents those connections from being edited through the dynamic form.
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%2F44279&comment_hash=0ad0e4e7f2fa9c6c30623e53f375ec88efc49ac68747f5974cc82fe69e88d838&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44279&comment_hash=0ad0e4e7f2fa9c6c30623e53f375ec88efc49ac68747f5974cc82fe69e88d838&reaction=dislike'>👎</a>
##########
superset/db_engine_specs/druid.py:
##########
@@ -162,6 +212,44 @@ def alter_new_orm_column(cls, orm_col: TableColumn) -> None:
if orm_col.column_name == "__time":
orm_col.is_dttm = True
+ @classmethod
+ def build_sqlalchemy_uri(
+ cls,
+ parameters: BasicParametersType,
+ encrypted_extra: dict[str, str] | None = None,
+ ) -> str:
+ query = parameters.get("query", {}).copy()
+ # `druid+https` selects pydruid's TLS dialect; the bare `druid` driver
+ # connects over plain HTTP.
+ driver = "https" if parameters.get("encryption") else cls.default_driver
+
+ # SQLAlchemy 2.0 hides the password from `URL.__str__()`, so render it
+ # explicitly since this URI is used to connect, not just displayed.
+ return URL.create(
+ f"{cls.engine}+{driver}".rstrip("+"),
+ username=parameters.get("username"),
+ password=parameters.get("password"),
+ host=parameters["host"],
+ port=parameters["port"],
+ database=cls.sqlalchemy_uri_database,
+ query=query,
+ ).render_as_string(hide_password=False)
+
+ @classmethod
+ def get_parameters_from_uri(
+ cls, uri: str, encrypted_extra: dict[str, Any] | None = None
+ ) -> BasicParametersType:
+ url = make_url_safe(uri)
+ return BasicParametersType(
+ username=url.username,
+ password=url.password,
+ host=url.host,
+ port=url.port,
+ database=url.database or cls.sqlalchemy_uri_database,
+ query=dict(url.query),
Review Comment:
**Suggestion:** `database` is returned even though `DruidParametersSchema` does not define it, so editing an existing dynamic-form connection fails Marshmallow validation on this unknown field. [api mismatch]
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Often`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ba5000074ca94add91d20f1f24590964&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=ba5000074ca94add91d20f1f24590964&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:** superset/db_engine_specs/druid.py
**Line:** 249:249
**Comment:**
*Api Mismatch: `database` is returned even though `DruidParametersSchema` does not define it, so editing an existing dynamic-form connection fails Marshmallow validation on this unknown field.
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%2F44279&comment_hash=67c3587b1076b3e5bda208d5bcf1dc021aa0b6ccd972aa30673575546541dc17&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44279&comment_hash=67c3587b1076b3e5bda208d5bcf1dc021aa0b6ccd972aa30673575546541dc17&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] feat(druid): support the dynamic connection form [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 #44279:
URL: https://github.com/apache/superset/pull/44279#discussion_r4210882860
##########
superset/db_engine_specs/druid.py:
##########
@@ -162,6 +212,44 @@ def alter_new_orm_column(cls, orm_col: TableColumn) -> None:
if orm_col.column_name == "__time":
orm_col.is_dttm = True
+ @classmethod
+ def build_sqlalchemy_uri(
+ cls,
+ parameters: BasicParametersType,
+ encrypted_extra: dict[str, str] | None = None,
+ ) -> str:
+ query = parameters.get("query", {}).copy()
+ # `druid+https` selects pydruid's TLS dialect; the bare `druid` driver
+ # connects over plain HTTP.
+ driver = "https" if parameters.get("encryption") else cls.default_driver
+
+ # SQLAlchemy 2.0 hides the password from `URL.__str__()`, so render it
+ # explicitly since this URI is used to connect, not just displayed.
+ return URL.create(
+ f"{cls.engine}+{driver}".rstrip("+"),
+ username=parameters.get("username"),
+ password=parameters.get("password"),
+ host=parameters["host"],
+ port=parameters["port"],
+ database=cls.sqlalchemy_uri_database,
+ query=query,
+ ).render_as_string(hide_password=False)
+
+ @classmethod
+ def get_parameters_from_uri(
+ cls, uri: str, encrypted_extra: dict[str, Any] | None = None
+ ) -> BasicParametersType:
+ url = make_url_safe(uri)
+ return BasicParametersType(
+ username=url.username,
+ password=url.password,
+ host=url.host,
+ port=url.port,
+ database=url.database or cls.sqlalchemy_uri_database,
+ query=dict(url.query),
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `2043403`.
Added `unknown = EXCLUDE` to `DruidParametersSchema.Meta`, so returned `database` values are ignored during schema validation.
<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] feat(druid): support the dynamic connection form [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #44279:
URL: https://github.com/apache/superset/pull/44279#discussion_r4261834483
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/DruidParameters.tsx:
##########
@@ -0,0 +1,95 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { t } from '@apache-superset/core/translation';
+import { SupersetTheme } from '@apache-superset/core/theme';
+import { Switch } from '@superset-ui/core/components/Switch';
+import {
+ InfoTooltip,
+ LabeledErrorBoundInput as ValidatedInput,
+} from '@superset-ui/core/components';
+import { FieldPropTypes } from '../../types';
+import { toggleStyle, infoTooltip } from '../styles';
+
+/**
+ * Druid-specific variants of the common connection form fields.
+ *
+ * Superset connects to Druid's SQL endpoint on the broker (port 8082 by
+ * default) or the router (8888), and the encryption toggle switches the pydruid
+ * dialect from `druid://` (HTTP) to `druid+https://` rather than setting a
+ * Postgres-style SSL mode.
+ */
+export const DRUID_ENGINE = 'druid';
+
+export const druidPortField = ({
+ required,
+ changeMethods,
+ getValidation,
+ validationErrors,
+ db,
+ isValidating,
+}: FieldPropTypes) => (
+ <ValidatedInput
+ id="port"
+ name="port"
+ type="number"
+ isValidating={isValidating}
+ required={required}
+ value={db?.parameters?.port as number}
+ validationMethods={{ onBlur: getValidation }}
+ errorMessage={validationErrors?.port}
+ placeholder={t('e.g. 8082 (8888 for the router)')}
+ className="form-group-w-50"
+ label={t('Port')}
+ onChange={changeMethods.onParametersChange}
+ />
+);
+
+export const druidEncryptionField = ({
+ isEditMode,
+ changeMethods,
+ db,
+ sslForced,
+}: FieldPropTypes) => (
+ <div css={(theme: SupersetTheme) => infoTooltip(theme)}>
+ <Switch
+ disabled={sslForced && !isEditMode}
+ checked={db?.parameters?.encryption || sslForced}
+ onChange={changed => {
+ changeMethods.onParametersChange({
+ target: {
+ type: 'toggle',
+ name: 'encryption',
+ checked: true,
+ value: changed,
+ },
+ });
+ }}
+ />
+ <span css={toggleStyle}>{t('SSL')}</span>
+ <InfoTooltip
+ tooltip={t('HTTPS will be used to connect to the Druid SQL endpoint.')}
+ placement="right"
+ />
+ </div>
+);
+
+export const DRUID_FORM_FIELD_MAP = {
+ port: druidPortField,
+ encryption: druidEncryptionField,
+};
Review Comment:
Fixed in dcf305a7b0. `portField` and `forceSSLField` in `CommonParameters.tsx` now take optional `portPlaceholder` / `sslTooltip` props. They default to the existing text, so other engines are unchanged. `druidPortField` / `druidEncryptionField` just render the common components with Druid's text, so validation, styling and disabled logic live in one place. Existing tests cover both the Druid text and the default text for other engines.
##########
tests/unit_tests/db_engine_specs/test_druid.py:
##########
@@ -258,3 +259,247 @@ def test_unmask_encrypted_extra() -> None:
assert DruidEngineSpec.unmask_encrypted_extra(old, new) == json.dumps(
{"connect_args": {"scheme": "http", "jwt": "old-token", "password": "new"}}
)
+
+
+# ---------------------------------------------------------------------------
+# Dynamic connection form (BasicParametersMixin)
+#
+# Druid connects over a fixed SQL endpoint (`/druid/v2/sql/`) and toggles TLS by
+# switching between pydruid's `druid` (http) and `druid+https` dialects, so the
+# form omits the database field and encodes encryption in the driver name rather
+# than a query parameter.
+# ---------------------------------------------------------------------------
+
+
+def _parameters(**overrides: Any) -> Any:
+ from superset.db_engine_specs.base import BasicParametersType
+
+ parameters: dict[str, Any] = {
+ "username": "user",
+ "password": "pwd",
+ "host": "localhost",
+ "port": 9088,
+ "query": {},
+ }
+ parameters.update(overrides)
+ return cast(BasicParametersType, parameters)
+
+
+def test_get_engine_spec_supports_parameters() -> None:
+ """
+ Druid must resolve to a spec that supports the dynamic connection form so
+ the ``/available`` endpoint returns individual parameters.
+ """
+ from superset.db_engine_specs import get_engine_spec
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ spec = cast("type[DruidEngineSpec]", get_engine_spec("druid"))
+ assert spec is DruidEngineSpec
+ assert spec.parameters_schema is not None
+ assert hasattr(spec, "build_sqlalchemy_uri")
+
+
+@pytest.mark.parametrize(
+ "encryption,expected_driver",
+ [
+ (False, "druid"),
+ (True, "druid+https"),
+ ],
+)
+def test_build_sqlalchemy_uri_toggles_scheme(
+ encryption: bool, expected_driver: str
+) -> None:
+ """
+ The encryption toggle selects the http vs. https pydruid dialect and always
+ injects the fixed SQL endpoint path.
+ """
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ uri = make_url(
+ DruidEngineSpec.build_sqlalchemy_uri(_parameters(encryption=encryption))
+ )
+
+ assert uri.drivername == expected_driver
+ assert uri.database == "druid/v2/sql/"
+ assert uri.host == "localhost"
+ assert uri.port == 9088
+
+
+def test_build_sqlalchemy_uri_preserves_query_params() -> None:
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ uri = make_url(
+ DruidEngineSpec.build_sqlalchemy_uri(_parameters(query={"header": "true"}))
+ )
+
+ assert uri.query["header"] == "true"
+
+
+def test_build_sqlalchemy_uri_renders_password() -> None:
+ """The stored URI is used to connect, so the password must not be masked."""
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ uri = DruidEngineSpec.build_sqlalchemy_uri(_parameters(password="s3cret")) # noqa: S106
+
+ assert "s3cret" in uri
+
+
+@pytest.mark.parametrize(
+ "uri,expected_encryption",
+ [
+ ("druid://user:pwd@localhost:9088/druid/v2/sql/", False),
+ ("druid+http://user:pwd@localhost:9088/druid/v2/sql/", False),
+ ("druid+https://user:pwd@localhost:9088/druid/v2/sql/", True),
+ ],
+)
+def test_get_parameters_from_uri_encryption(
+ uri: str, expected_encryption: bool
+) -> None:
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ parameters = DruidEngineSpec.get_parameters_from_uri(uri)
+
+ assert parameters["encryption"] is expected_encryption
+ assert parameters["host"] == "localhost"
+ assert parameters["port"] == 9088
+ assert parameters["database"] == "druid/v2/sql/"
+
+
+def test_get_parameters_from_uri_accepts_encrypted_extra_keyword() -> None:
+ """
+ ``Database.parameters`` passes ``encrypted_extra`` by keyword; a signature
+ mismatch would silently empty the connection form.
+ """
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ parameters = DruidEngineSpec.get_parameters_from_uri(
+ "druid+https://user:pwd@localhost:9088/druid/v2/sql/",
+ encrypted_extra={},
+ )
+
+ assert parameters["encryption"] is True
+
+
+@pytest.mark.parametrize("encryption", [True, False])
+def test_parameters_round_trip(encryption: bool) -> None:
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ uri = DruidEngineSpec.build_sqlalchemy_uri(
+ _parameters(encryption=encryption, query={"header": "true"})
+ )
+ parameters = DruidEngineSpec.get_parameters_from_uri(uri)
+
+ assert parameters["encryption"] is encryption
+ assert parameters["host"] == "localhost"
+ assert parameters["port"] == 9088
+ assert parameters["username"] == "user"
+ assert parameters["query"] == {"header": "true"}
+
+
+def test_parameters_json_schema_omits_database() -> None:
+ """
+ The SQL endpoint path is fixed, so ``database`` must not appear as a form
+ field; the encryption toggle must be present instead.
+ """
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ schema = DruidEngineSpec.parameters_json_schema()
+
+ assert "database" not in schema["properties"]
+ assert "encryption" in schema["properties"]
+ assert set(schema["required"]) == {"host", "port"}
+
+
+def test_parameters_schema_reloads_emitted_parameters() -> None:
+ """
+ ``get_parameters_from_uri`` emits the fixed ``database`` path, which is not a
+ form field. Re-loading that dict through the schema (the create/update path)
+ must not raise on the unknown ``database`` key.
+ """
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ parameters = DruidEngineSpec.get_parameters_from_uri(
+ "druid+https://user:pwd@localhost:9088/druid/v2/sql/"
+ )
+ assert parameters["database"] == "druid/v2/sql/"
+
+ loaded = DruidEngineSpec.parameters_schema.load(parameters)
+
+ assert "database" not in loaded
+ assert loaded["host"] == "localhost"
+ assert loaded["encryption"] is True
+
+
+def test_default_driver_matches_pydruid_dialects() -> None:
+ """
+ ``/available`` only returns form parameters when the spec's default driver is
+ among the detected drivers, so it must match what pydruid's dialects report.
+ """
+ sqlalchemy_dialect = pytest.importorskip("pydruid.db.sqlalchemy")
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ detected = {
+ sqlalchemy_dialect.DruidHTTPDialect.driver,
+ sqlalchemy_dialect.DruidHTTPSDialect.driver,
+ }
+
+ assert detected == {"rest"}
+ assert DruidEngineSpec.default_driver in detected
+
+
+def test_build_sqlalchemy_uri_uses_registered_dialect_names() -> None:
+ """
+ The default driver ("rest") is not a registered dialect name, so it must
+ never leak into the generated URI scheme.
+ """
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ for encryption in (True, False):
+ uri = DruidEngineSpec.build_sqlalchemy_uri(_parameters(encryption=encryption))
+ assert "rest" not in make_url(uri).drivername
+
+
+@pytest.mark.parametrize(
+ "uri",
+ [
+ "druid://user:pwd@localhost:8082/druid/v2/sql/",
+ "druid+https://user:pwd@localhost:8082/druid/v2/sql/",
+ ],
+)
+def test_existing_uris_resolve_to_druid_spec(uri: str) -> None:
+ """Existing Druid connections keep resolving to the Druid engine spec."""
+ from superset.db_engine_specs import get_engine_spec
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ url = make_url(uri)
+ driver = url.drivername.split("+")[1] if "+" in url.drivername else "rest"
+
+ assert get_engine_spec(url.get_backend_name(), driver) is DruidEngineSpec
+
+
+@pytest.mark.parametrize(
+ "denylist,expect_available",
+ [
+ ({"druid": {"rest"}}, False),
+ ({"druid": {"other"}}, True),
+ ({}, True),
+ ],
+)
+def test_available_engine_specs_druid_denylist(
+ mocker: Any, denylist: dict[str, set[str]], expect_available: bool
+) -> None:
+ """``DBS_AVAILABLE_DENYLIST`` disables Druid by its "rest" driver name."""
+ from superset.db_engine_specs import get_available_engine_specs
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ mocker.patch(
+ "superset.db_engine_specs.load_engine_specs",
+ return_value=[DruidEngineSpec],
+ )
+ mocker.patch("superset.db_engine_specs.entry_points", return_value=[])
+ app = mocker.patch("superset.db_engine_specs.app")
+ app.config = {"DBS_AVAILABLE_DENYLIST": dict(denylist)}
+
+ available = get_available_engine_specs()
Review Comment:
Fixed in dcf305a7b0. Added explicit annotations: `sqlalchemy_dialect: ModuleType`, `detected: set[str]`, `url: URL`, `driver: str`, `app: mock.MagicMock`, and `available: dict[type[BaseEngineSpec], set[str]]`. `mocker` is now `MockerFixture`, matching `test_init.py`. mypy passes on the file.
--
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] feat(druid): support the dynamic connection form [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 #44279:
URL: https://github.com/apache/superset/pull/44279#discussion_r4408511988
##########
superset/db_engine_specs/druid.py:
##########
@@ -40,14 +50,54 @@
_DRUID_FLOAT_SPECIAL = frozenset({"NaN", "Infinity", "-Infinity"})
-class DruidEngineSpec(BaseEngineSpec):
+class DruidParametersSchema(Schema):
+ """
+ Parameters for the dynamic connection form.
+
+ Druid connects over a fixed SQL endpoint (`/druid/v2/sql/`), so the schema
+ intentionally omits the ``database`` field. The ``encryption`` toggle selects
+ the http vs. https pydruid dialect rather than adding a query parameter.
+ """
+
+ username = fields.String(allow_none=True, metadata={"description": __("Username")})
+ password = fields.String(allow_none=True, metadata={"description": __("Password")})
+ host = fields.String(
+ required=True, metadata={"description": __("Hostname or IP address")}
+ )
+ port = fields.Integer(
+ required=True,
+ metadata={"description": __("Database port")},
+ validate=Range(min=0, max=2**16, max_inclusive=False),
+ )
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `7d06d41`.
Added a default Druid broker port and use it when the parsed URI has no explicit port, so the required form field receives an integer.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/db_engine_specs/druid.py:
##########
@@ -162,6 +212,44 @@ def alter_new_orm_column(cls, orm_col: TableColumn) -> None:
if orm_col.column_name == "__time":
orm_col.is_dttm = True
+ @classmethod
+ def build_sqlalchemy_uri(
+ cls,
+ parameters: BasicParametersType,
+ encrypted_extra: dict[str, str] | None = None,
+ ) -> str:
+ query = parameters.get("query", {}).copy()
+ # `druid+https` selects pydruid's TLS dialect; the bare `druid` driver
+ # connects over plain HTTP.
+ driver = "https" if parameters.get("encryption") else cls.default_driver
+
+ # SQLAlchemy 2.0 hides the password from `URL.__str__()`, so render it
+ # explicitly since this URI is used to connect, not just displayed.
+ return URL.create(
+ f"{cls.engine}+{driver}".rstrip("+"),
+ username=parameters.get("username"),
+ password=parameters.get("password"),
+ host=parameters["host"],
+ port=parameters["port"],
+ database=cls.sqlalchemy_uri_database,
+ query=query,
+ ).render_as_string(hide_password=False)
+
+ @classmethod
+ def get_parameters_from_uri(
+ cls, uri: str, encrypted_extra: dict[str, Any] | None = None
+ ) -> BasicParametersType:
+ url = make_url_safe(uri)
+ return BasicParametersType(
+ username=url.username,
+ password=url.password,
+ host=url.host,
+ port=url.port,
+ database=url.database or cls.sqlalchemy_uri_database,
+ query=dict(url.query),
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `7d06d41`.
Set the schema’s unknown-field handling to `EXCLUDE`, so the returned `database` value is ignored during form validation.
<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] feat(druid): support the dynamic connection form [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44279:
URL: https://github.com/apache/superset/pull/44279#issuecomment-6123929128
Thanks for this, the form round trip and the broker/router port hint are nice touches. One thing I'd want sorted before we merge: a saved Druid URI with no port (`druid://host/druid/v2/sql/`) gives `port=None` from `get_parameters_from_uri`, and the schema's required `port` will reject that, so those connections can't be edited through the form. Could we allow None there or fall back to a default?
Also worth a quick look at the two Bito nits (the duplicated field components and the test annotations). The `database` thread looks fixed by `unknown = EXCLUDE`, so that one can be resolved.
--
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] feat(druid): support the dynamic connection form [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 #44279:
URL: https://github.com/apache/superset/pull/44279#discussion_r4171087932
##########
superset/db_engine_specs/druid.py:
##########
@@ -162,6 +212,44 @@ def alter_new_orm_column(cls, orm_col: TableColumn) -> None:
if orm_col.column_name == "__time":
orm_col.is_dttm = True
+ @classmethod
+ def build_sqlalchemy_uri(
+ cls,
+ parameters: BasicParametersType,
+ encrypted_extra: dict[str, str] | None = None,
+ ) -> str:
+ query = parameters.get("query", {}).copy()
+ # `druid+https` selects pydruid's TLS dialect; the bare `druid` driver
+ # connects over plain HTTP.
+ driver = "https" if parameters.get("encryption") else cls.default_driver
+
+ # SQLAlchemy 2.0 hides the password from `URL.__str__()`, so render it
+ # explicitly since this URI is used to connect, not just displayed.
+ return URL.create(
+ f"{cls.engine}+{driver}".rstrip("+"),
+ username=parameters.get("username"),
+ password=parameters.get("password"),
+ host=parameters["host"],
+ port=parameters["port"],
+ database=cls.sqlalchemy_uri_database,
+ query=query,
+ ).render_as_string(hide_password=False)
+
+ @classmethod
+ def get_parameters_from_uri(
+ cls, uri: str, encrypted_extra: dict[str, Any] | None = None
+ ) -> BasicParametersType:
+ url = make_url_safe(uri)
+ return BasicParametersType(
+ username=url.username,
+ password=url.password,
+ host=url.host,
+ port=url.port,
+ database=url.database or cls.sqlalchemy_uri_database,
+ query=dict(url.query),
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `d3575ca`.
Added Marshmallow Meta configuration with `unknown = EXCLUDE`, allowing the `database` value returned from URI parsing to be ignored during dynamic-form validation.
<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] feat(druid): support the dynamic connection form [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 #44279:
URL: https://github.com/apache/superset/pull/44279#discussion_r4171051353
##########
superset/db_engine_specs/druid.py:
##########
@@ -162,6 +212,44 @@ def alter_new_orm_column(cls, orm_col: TableColumn) -> None:
if orm_col.column_name == "__time":
orm_col.is_dttm = True
+ @classmethod
+ def build_sqlalchemy_uri(
+ cls,
+ parameters: BasicParametersType,
+ encrypted_extra: dict[str, str] | None = None,
+ ) -> str:
+ query = parameters.get("query", {}).copy()
+ # `druid+https` selects pydruid's TLS dialect; the bare `druid` driver
+ # connects over plain HTTP.
+ driver = "https" if parameters.get("encryption") else cls.default_driver
+
+ # SQLAlchemy 2.0 hides the password from `URL.__str__()`, so render it
+ # explicitly since this URI is used to connect, not just displayed.
+ return URL.create(
+ f"{cls.engine}+{driver}".rstrip("+"),
+ username=parameters.get("username"),
+ password=parameters.get("password"),
+ host=parameters["host"],
+ port=parameters["port"],
+ database=cls.sqlalchemy_uri_database,
+ query=query,
+ ).render_as_string(hide_password=False)
+
+ @classmethod
+ def get_parameters_from_uri(
+ cls, uri: str, encrypted_extra: dict[str, Any] | None = None
+ ) -> BasicParametersType:
+ url = make_url_safe(uri)
+ return BasicParametersType(
+ username=url.username,
+ password=url.password,
+ host=url.host,
+ port=url.port,
+ database=url.database or cls.sqlalchemy_uri_database,
+ query=dict(url.query),
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `c08094a`.
Added Marshmallow Meta configuration with `unknown = EXCLUDE`, allowing the `database` value returned from URI parsing to be ignored during dynamic-form validation.
<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] feat(druid): support the dynamic connection form [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #44279:
URL: https://github.com/apache/superset/pull/44279#issuecomment-6124721498
Heya @eschutho, the `port=None` case for saved Druid URIs with no explicit port is the one thing keeping this from being ready. Allowing None there or falling back to the broker default (8082) would sort it. Otherwise this looks about ready, and the two Bito nits are worth a quick look too.
--
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] feat(druid): support the dynamic connection form [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44279:
URL: https://github.com/apache/superset/pull/44279#issuecomment-5686833935
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 9bf71150a62306e67d84a03646ae5eb579493cc0 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa99b662ee7eb000852f54a |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-44279--superset-docs-preview.netlify.app](https://deploy-preview-44279--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#q1YqKMrPLShRslJS0lFKKkrMS84AsouTdQ2NDCwtLXRTikozU3TzUst10_KLcoFqUlILcvIrPVOAqswSEy0tk8zMjFJTzVOTDAwMLEyN0kxNEpVqAQ) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44279--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
Re: [PR] feat(druid): support the dynamic connection form [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 #44279:
URL: https://github.com/apache/superset/pull/44279#discussion_r4286797813
##########
superset/db_engine_specs/druid.py:
##########
@@ -40,14 +50,54 @@
_DRUID_FLOAT_SPECIAL = frozenset({"NaN", "Infinity", "-Infinity"})
-class DruidEngineSpec(BaseEngineSpec):
+class DruidParametersSchema(Schema):
+ """
+ Parameters for the dynamic connection form.
+
+ Druid connects over a fixed SQL endpoint (`/druid/v2/sql/`), so the schema
+ intentionally omits the ``database`` field. The ``encryption`` toggle selects
+ the http vs. https pydruid dialect rather than adding a query parameter.
+ """
+
+ username = fields.String(allow_none=True, metadata={"description": __("Username")})
+ password = fields.String(allow_none=True, metadata={"description": __("Password")})
+ host = fields.String(
+ required=True, metadata={"description": __("Hostname or IP address")}
+ )
+ port = fields.Integer(
+ required=True,
+ metadata={"description": __("Database port")},
+ validate=Range(min=0, max=2**16, max_inclusive=False),
+ )
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `609cd6b`.
Added a default Druid broker port and use it when the parsed URI has no explicit port, so the required form field receives an integer.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/db_engine_specs/druid.py:
##########
@@ -162,6 +212,44 @@ def alter_new_orm_column(cls, orm_col: TableColumn) -> None:
if orm_col.column_name == "__time":
orm_col.is_dttm = True
+ @classmethod
+ def build_sqlalchemy_uri(
+ cls,
+ parameters: BasicParametersType,
+ encrypted_extra: dict[str, str] | None = None,
+ ) -> str:
+ query = parameters.get("query", {}).copy()
+ # `druid+https` selects pydruid's TLS dialect; the bare `druid` driver
+ # connects over plain HTTP.
+ driver = "https" if parameters.get("encryption") else cls.default_driver
+
+ # SQLAlchemy 2.0 hides the password from `URL.__str__()`, so render it
+ # explicitly since this URI is used to connect, not just displayed.
+ return URL.create(
+ f"{cls.engine}+{driver}".rstrip("+"),
+ username=parameters.get("username"),
+ password=parameters.get("password"),
+ host=parameters["host"],
+ port=parameters["port"],
+ database=cls.sqlalchemy_uri_database,
+ query=query,
+ ).render_as_string(hide_password=False)
+
+ @classmethod
+ def get_parameters_from_uri(
+ cls, uri: str, encrypted_extra: dict[str, Any] | None = None
+ ) -> BasicParametersType:
+ url = make_url_safe(uri)
+ return BasicParametersType(
+ username=url.username,
+ password=url.password,
+ host=url.host,
+ port=url.port,
+ database=url.database or cls.sqlalchemy_uri_database,
+ query=dict(url.query),
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `609cd6b`.
Set the schema’s unknown-field handling to `EXCLUDE`, so the returned `database` value is ignored during form validation.
<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] feat(druid): support the dynamic connection form [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #44279:
URL: https://github.com/apache/superset/pull/44279#issuecomment-6199354084
@rusackas thanks. Fixed in 609cd6b69c: saved Druid URIs without an explicit port now default to the broker port 8082 in `get_parameters_from_uri`, so the required port field loads and those connections can be edited through the form. An explicit port is still kept as is. There's a test covering both `druid://` and `druid+https://` URIs with no port. The two Bito nits are addressed in dcf305a7b0.
--
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] feat(druid): support the dynamic connection form [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #44279:
URL: https://github.com/apache/superset/pull/44279#issuecomment-6025161820
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging because existing Druid connections without an explicit port can fail to load in the dynamic form.
- get_parameters_from_uri returns url.port unchanged, while DruidParametersSchema requires a non-null integer port; handle omitted ports before enabling this form.
**Assessed commit:** `18d0aba84b03`
--
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