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") + + [email protected]( + "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 + + [email protected]( + "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 + + [email protected]("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 + + [email protected]( + "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 + + [email protected]( + "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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
