bito-code-review[bot] commented on code in PR #44718:
URL: https://github.com/apache/superset/pull/44718#discussion_r4129933000
##########
superset/db_engine_specs/doris.py:
##########
@@ -300,7 +373,7 @@ def adjust_engine_params(
database = ".".join(part for part in (catalog, schema) if part)
uri = uri.set(database=database)
- return uri, connect_args
+ return require_mysqlclient_tls(uri, connect_args)
Review Comment:
<!-- Bito Reply -->
The reviewer's concern regarding potential query parameter loss was
addressed by the implementation of regression tests that verify the
preservation of charset, CA, and repeated query values across all TLS modes.
The user confirmed that `URL.set` and `update_query_dict` correctly handle the
query parameters as intended, and these tests validate that the original query
parameters are indeed preserved.
##########
superset/db_engine_specs/doris.py:
##########
@@ -116,7 +130,8 @@ class DorisEngineSpec(MySQLEngineSpec):
sqlalchemy_uri_placeholder = (
"doris://user:password@host:port/catalog.db[?key=value&key=value...]"
)
- encryption_parameters = {"ssl": "0"}
+ # REQUIRED can fall back with MariaDB Connector/C; verification fails
closed.
+ encryption_parameters = {"ssl_mode": "VERIFY_CA"}
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion regarding the potential for `ssl_mode=VERIFY_CA`
to cause issues with existing saved URIs was addressed by the user. The user
confirmed that the change is intended, and they have implemented regression
tests to ensure that encryption flags are correctly preserved and upgraded
during editing. The implementation in the provided code snippet correctly
updates the `encryption_parameters` to `VERIFY_CA` as requested.
**superset/db_engine_specs/doris.py**
```
# REQUIRED can fall back with MariaDB Connector/C; verification fails closed.
encryption_parameters = {"ssl_mode": "VERIFY_CA"}
```
##########
tests/unit_tests/db_engine_specs/test_doris.py:
##########
@@ -280,3 +280,173 @@ def test_get_catalog_names(
# Verify the returned catalog names
assert catalogs == expected_result
+
+
[email protected](
+ "native_type,generic_type",
+ [
+ ("variant", GenericDataType.STRING),
+ ("ipv4", GenericDataType.STRING),
+ ("ipv6", GenericDataType.STRING),
+ # MySQL protocol type names reported for SQL Lab result columns
+ ("NEWDECIMAL", GenericDataType.NUMERIC),
+ ("TINY", GenericDataType.NUMERIC),
+ ("SHORT", GenericDataType.NUMERIC),
+ ("BLOB", GenericDataType.STRING),
+ ],
+)
+def test_get_column_spec_extra_types(
+ native_type: str, generic_type: GenericDataType
+) -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ spec = DorisEngineSpec.get_column_spec(native_type)
+ assert spec is not None
+ assert spec.generic_type == generic_type
+
+
+def test_quarter_time_grain_avoids_interval_quarter() -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ expression = DorisEngineSpec.get_time_grain_expressions()["P3M"]
+ assert expression == (
+ "MAKEDATE(YEAR({col}), 1) + INTERVAL (QUARTER({col}) - 1) * 3 MONTH"
+ )
+ assert "INTERVAL 1 QUARTER" not in expression
+
+
[email protected](
+ "message,error_type",
+ [
+ (
+ "(2002, \"Can't connect to server on '127.0.0.1' (115)\")",
+ "CONNECTION_HOST_DOWN_ERROR",
+ ),
+ (
+ "(2003, \"Can't connect to MySQL server on 'db' (111)\")",
+ "CONNECTION_HOST_DOWN_ERROR",
+ ),
+ (
+ "(2005, \"Unknown server host 'no-such-host.invalid' (-2)\")",
+ "CONNECTION_INVALID_HOSTNAME_ERROR",
+ ),
+ (
+ "(1105, \"errCode = 2, detailMessage = \\nmismatched input 'SELEC'
"
+ "expecting {<EOF>, ';'}\")",
+ "SYNTAX_ERROR",
+ ),
+ (
+ "(1105, 'errCode = 2, detailMessage = Table [missing] does not
exist "
+ "in database [db].(line 1, pos 14)')",
+ "TABLE_DOES_NOT_EXIST_ERROR",
+ ),
+ (
+ "(1105, 'errCode = 2, detailMessage = Database [nodb] does not
exist."
+ "(line 1, pos 14)')",
+ "SCHEMA_DOES_NOT_EXIST_ERROR",
+ ),
+ (
+ "(1105, \"errCode = 2, detailMessage = Unknown column 'nope' in "
+ "'table list' in PROJECT clause(line 1, pos 7)\")",
+ "COLUMN_DOES_NOT_EXIST_ERROR",
+ ),
+ (
+ "(1045, \"Access denied for user '[email protected]' (using password:
YES)\")",
+ "CONNECTION_ACCESS_DENIED_ERROR",
+ ),
+ (
+ "(1049, \"errCode = 2, detailMessage = Unknown database 'nodb'\")",
+ "CONNECTION_UNKNOWN_DATABASE_ERROR",
+ ),
+ ],
+)
+def test_extract_errors(message: str, error_type: str) -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ errors = DorisEngineSpec.extract_errors(Exception(message))
+ assert errors[0].error_type.name == error_type
+
+
+def test_build_sqlalchemy_uri() -> None:
+ from superset.db_engine_specs.base import BasicParametersType
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ parameters: BasicParametersType = {
+ "username": "user",
+ "password": "p@ss",
+ "host": "doris.example.com",
+ "port": 9030,
+ "database": "internal.db",
+ "query": {},
+ }
+ encrypted = make_url(
+ DorisEngineSpec.build_sqlalchemy_uri({**parameters, "encryption":
True})
+ )
+ assert encrypted.drivername == "doris"
+ assert dict(encrypted.query) == {"ssl_mode": "VERIFY_CA"}
+ assert encrypted.password == "p@ss" # noqa: S105
+
+ plain = make_url(
+ DorisEngineSpec.build_sqlalchemy_uri({**parameters, "encryption":
False})
+ )
+ assert plain.drivername == "doris"
+ assert dict(plain.query) == {}
+
+ round_trip = DorisEngineSpec.get_parameters_from_uri(
+ encrypted.render_as_string(hide_password=False)
+ )
+ assert round_trip["encryption"] is True
+ assert round_trip["query"] == {}
+
+
[email protected]("source", ["toggle", "ssl=1", "ssl_mode=REQUIRED"])
+def test_doris_tls_request_uses_verification(source: str) -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ if source == "toggle":
+ uri = make_url(
+ DorisEngineSpec.build_sqlalchemy_uri(
+ {
+ "username": "root",
+ "host": "localhost",
+ "port": 9030,
+ "database": "internal.db",
+ "encryption": True,
+ }
+ )
+ )
+ else:
+ uri = make_url(f"doris://root@localhost/internal.db?{source}")
+ url, args = DorisEngineSpec.adjust_engine_params(
+ uri, {}, catalog="external", schema="other"
+ )
+ assert args.get("ssl_mode", url.query.get("ssl_mode")) == "VERIFY_CA"
+ assert url.drivername == "doris"
+ assert url.database == "external.other"
+
+
[email protected]("mode", ["DISABLED", "PREFERRED", None])
+def test_doris_toggle_cannot_be_cancelled(mode: Optional[str]) -> None:
Review Comment:
<!-- Bito Reply -->
The docstring for the test function `test_doris_toggle_cannot_be_cancelled`
has been verified as present in the codebase, addressing the reviewer's
previous suggestion.
##########
tests/unit_tests/db_engine_specs/test_doris.py:
##########
@@ -280,3 +280,173 @@ def test_get_catalog_names(
# Verify the returned catalog names
assert catalogs == expected_result
+
+
[email protected](
+ "native_type,generic_type",
+ [
+ ("variant", GenericDataType.STRING),
+ ("ipv4", GenericDataType.STRING),
+ ("ipv6", GenericDataType.STRING),
+ # MySQL protocol type names reported for SQL Lab result columns
+ ("NEWDECIMAL", GenericDataType.NUMERIC),
+ ("TINY", GenericDataType.NUMERIC),
+ ("SHORT", GenericDataType.NUMERIC),
+ ("BLOB", GenericDataType.STRING),
+ ],
+)
+def test_get_column_spec_extra_types(
+ native_type: str, generic_type: GenericDataType
+) -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ spec = DorisEngineSpec.get_column_spec(native_type)
+ assert spec is not None
+ assert spec.generic_type == generic_type
+
+
+def test_quarter_time_grain_avoids_interval_quarter() -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ expression = DorisEngineSpec.get_time_grain_expressions()["P3M"]
+ assert expression == (
+ "MAKEDATE(YEAR({col}), 1) + INTERVAL (QUARTER({col}) - 1) * 3 MONTH"
+ )
+ assert "INTERVAL 1 QUARTER" not in expression
+
+
[email protected](
+ "message,error_type",
+ [
+ (
+ "(2002, \"Can't connect to server on '127.0.0.1' (115)\")",
+ "CONNECTION_HOST_DOWN_ERROR",
+ ),
+ (
+ "(2003, \"Can't connect to MySQL server on 'db' (111)\")",
+ "CONNECTION_HOST_DOWN_ERROR",
+ ),
+ (
+ "(2005, \"Unknown server host 'no-such-host.invalid' (-2)\")",
+ "CONNECTION_INVALID_HOSTNAME_ERROR",
+ ),
+ (
+ "(1105, \"errCode = 2, detailMessage = \\nmismatched input 'SELEC'
"
+ "expecting {<EOF>, ';'}\")",
+ "SYNTAX_ERROR",
+ ),
+ (
+ "(1105, 'errCode = 2, detailMessage = Table [missing] does not
exist "
+ "in database [db].(line 1, pos 14)')",
+ "TABLE_DOES_NOT_EXIST_ERROR",
+ ),
+ (
+ "(1105, 'errCode = 2, detailMessage = Database [nodb] does not
exist."
+ "(line 1, pos 14)')",
+ "SCHEMA_DOES_NOT_EXIST_ERROR",
+ ),
+ (
+ "(1105, \"errCode = 2, detailMessage = Unknown column 'nope' in "
+ "'table list' in PROJECT clause(line 1, pos 7)\")",
+ "COLUMN_DOES_NOT_EXIST_ERROR",
+ ),
+ (
+ "(1045, \"Access denied for user '[email protected]' (using password:
YES)\")",
+ "CONNECTION_ACCESS_DENIED_ERROR",
+ ),
+ (
+ "(1049, \"errCode = 2, detailMessage = Unknown database 'nodb'\")",
+ "CONNECTION_UNKNOWN_DATABASE_ERROR",
+ ),
+ ],
+)
+def test_extract_errors(message: str, error_type: str) -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ errors = DorisEngineSpec.extract_errors(Exception(message))
+ assert errors[0].error_type.name == error_type
+
+
+def test_build_sqlalchemy_uri() -> None:
+ from superset.db_engine_specs.base import BasicParametersType
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ parameters: BasicParametersType = {
+ "username": "user",
+ "password": "p@ss",
+ "host": "doris.example.com",
+ "port": 9030,
+ "database": "internal.db",
+ "query": {},
+ }
+ encrypted = make_url(
+ DorisEngineSpec.build_sqlalchemy_uri({**parameters, "encryption":
True})
+ )
+ assert encrypted.drivername == "doris"
+ assert dict(encrypted.query) == {"ssl_mode": "VERIFY_CA"}
+ assert encrypted.password == "p@ss" # noqa: S105
+
+ plain = make_url(
+ DorisEngineSpec.build_sqlalchemy_uri({**parameters, "encryption":
False})
+ )
+ assert plain.drivername == "doris"
+ assert dict(plain.query) == {}
+
+ round_trip = DorisEngineSpec.get_parameters_from_uri(
+ encrypted.render_as_string(hide_password=False)
+ )
+ assert round_trip["encryption"] is True
+ assert round_trip["query"] == {}
+
+
[email protected]("source", ["toggle", "ssl=1", "ssl_mode=REQUIRED"])
+def test_doris_tls_request_uses_verification(source: str) -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ if source == "toggle":
+ uri = make_url(
+ DorisEngineSpec.build_sqlalchemy_uri(
+ {
+ "username": "root",
+ "host": "localhost",
+ "port": 9030,
+ "database": "internal.db",
+ "encryption": True,
+ }
+ )
+ )
+ else:
+ uri = make_url(f"doris://root@localhost/internal.db?{source}")
+ url, args = DorisEngineSpec.adjust_engine_params(
+ uri, {}, catalog="external", schema="other"
+ )
+ assert args.get("ssl_mode", url.query.get("ssl_mode")) == "VERIFY_CA"
+ assert url.drivername == "doris"
+ assert url.database == "external.other"
+
+
[email protected]("mode", ["DISABLED", "PREFERRED", None])
+def test_doris_toggle_cannot_be_cancelled(mode: Optional[str]) -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ with pytest.raises(ValueError, match="conflicts"):
+ DorisEngineSpec.adjust_engine_params(
+ make_url("doris://root@localhost/internal.db?ssl_mode=REQUIRED"),
+ {"ssl_mode": mode},
+ )
+
+
+def test_doris_tls_preserves_verified_mode_and_ca() -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ uri =
make_url("doris://root@localhost/internal.db?ssl_mode=VERIFY_IDENTITY")
+ _, args = DorisEngineSpec.adjust_engine_params(uri, {"ssl": {"ca":
"/ca.pem"}})
+ assert args["ssl_mode"] == "VERIFY_IDENTITY"
+ assert args["ssl"] == {"ca": "/ca.pem"}
+
+
+def test_unrequested_doris_connection_unchanged() -> None:
Review Comment:
<!-- Bito Reply -->
The docstring for `test_unrequested_doris_connection_unchanged` has been
successfully added in the latest update, addressing the previous review comment.
##########
docs/user_docs_versioned_docs/version-6.1.0/databases/supported/apache-doris.mdx:
##########
@@ -161,3 +161,11 @@ export const databaseInfo = {
};
<DatabasePage name="Apache Doris" database={databaseInfo} />
+
+### TLS connections
Review Comment:
<!-- Bito Reply -->
The change to a level-two heading (##) is appropriate. It correctly aligns
the documentation structure with the existing MySQL page, ensuring consistency
for top-level sections following the database page component.
**docs/user_docs_versioned_docs/version-6.1.0/databases/supported/apache-doris.mdx**
```
+## TLS connections
```
##########
superset/db_engine_specs/doris.py:
##########
@@ -276,8 +311,64 @@ class DorisEngineSpec(MySQLEngineSpec):
SupersetErrorType.SYNTAX_ERROR,
{},
),
+ DORIS_SYNTAX_ERROR_REGEX: (
+ _(
+ 'Please check your query for syntax errors near
"%(server_error)s". '
+ "Then, try running your query again."
+ ),
+ SupersetErrorType.SYNTAX_ERROR,
+ {},
+ ),
+ TABLE_DOES_NOT_EXIST_REGEX: (
+ _('The table "%(table_name)s" does not exist.'),
+ SupersetErrorType.TABLE_DOES_NOT_EXIST_ERROR,
+ {},
+ ),
+ SCHEMA_DOES_NOT_EXIST_REGEX: (
+ _('The schema "%(schema_name)s" does not exist.'),
+ SupersetErrorType.SCHEMA_DOES_NOT_EXIST_ERROR,
+ {},
+ ),
+ COLUMN_DOES_NOT_EXIST_REGEX: (
+ _('We can\'t seem to resolve the column "%(column_name)s".'),
+ SupersetErrorType.COLUMN_DOES_NOT_EXIST_ERROR,
+ {},
+ ),
}
+ @classmethod
+ def build_sqlalchemy_uri(
+ cls,
+ parameters: BasicParametersType,
+ encrypted_extra: Optional[dict[str, str]] = None,
+ ) -> str:
+ # Keep explicit hostname verification when rebuilding an encrypted URI.
+ if parameters.get("query", {}).get("ssl_mode") == "VERIFY_IDENTITY":
Review Comment:
<!-- Bito Reply -->
The suggestion to use named constants for TLS mode strings is appropriate.
Centralizing these literals improves maintainability and reduces the risk of
typos or configuration drift in security-critical connection settings, which is
a recommended best practice.
**superset/db_engine_specs/doris.py**
```
# Keep explicit hostname verification when rebuilding an encrypted URI.
if parameters.get("query", {}).get("ssl_mode") == "VERIFY_IDENTITY":
```
##########
tests/unit_tests/db_engine_specs/test_doris.py:
##########
@@ -280,3 +280,173 @@ def test_get_catalog_names(
# Verify the returned catalog names
assert catalogs == expected_result
+
+
[email protected](
+ "native_type,generic_type",
+ [
+ ("variant", GenericDataType.STRING),
+ ("ipv4", GenericDataType.STRING),
+ ("ipv6", GenericDataType.STRING),
+ # MySQL protocol type names reported for SQL Lab result columns
+ ("NEWDECIMAL", GenericDataType.NUMERIC),
+ ("TINY", GenericDataType.NUMERIC),
+ ("SHORT", GenericDataType.NUMERIC),
+ ("BLOB", GenericDataType.STRING),
+ ],
+)
+def test_get_column_spec_extra_types(
+ native_type: str, generic_type: GenericDataType
+) -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ spec = DorisEngineSpec.get_column_spec(native_type)
+ assert spec is not None
+ assert spec.generic_type == generic_type
+
+
+def test_quarter_time_grain_avoids_interval_quarter() -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ expression = DorisEngineSpec.get_time_grain_expressions()["P3M"]
+ assert expression == (
+ "MAKEDATE(YEAR({col}), 1) + INTERVAL (QUARTER({col}) - 1) * 3 MONTH"
+ )
+ assert "INTERVAL 1 QUARTER" not in expression
+
+
[email protected](
+ "message,error_type",
+ [
+ (
+ "(2002, \"Can't connect to server on '127.0.0.1' (115)\")",
+ "CONNECTION_HOST_DOWN_ERROR",
+ ),
+ (
+ "(2003, \"Can't connect to MySQL server on 'db' (111)\")",
+ "CONNECTION_HOST_DOWN_ERROR",
+ ),
+ (
+ "(2005, \"Unknown server host 'no-such-host.invalid' (-2)\")",
+ "CONNECTION_INVALID_HOSTNAME_ERROR",
+ ),
+ (
+ "(1105, \"errCode = 2, detailMessage = \\nmismatched input 'SELEC'
"
+ "expecting {<EOF>, ';'}\")",
+ "SYNTAX_ERROR",
+ ),
+ (
+ "(1105, 'errCode = 2, detailMessage = Table [missing] does not
exist "
+ "in database [db].(line 1, pos 14)')",
+ "TABLE_DOES_NOT_EXIST_ERROR",
+ ),
+ (
+ "(1105, 'errCode = 2, detailMessage = Database [nodb] does not
exist."
+ "(line 1, pos 14)')",
+ "SCHEMA_DOES_NOT_EXIST_ERROR",
+ ),
+ (
+ "(1105, \"errCode = 2, detailMessage = Unknown column 'nope' in "
+ "'table list' in PROJECT clause(line 1, pos 7)\")",
+ "COLUMN_DOES_NOT_EXIST_ERROR",
+ ),
+ (
+ "(1045, \"Access denied for user '[email protected]' (using password:
YES)\")",
+ "CONNECTION_ACCESS_DENIED_ERROR",
+ ),
+ (
+ "(1049, \"errCode = 2, detailMessage = Unknown database 'nodb'\")",
+ "CONNECTION_UNKNOWN_DATABASE_ERROR",
+ ),
+ ],
+)
+def test_extract_errors(message: str, error_type: str) -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ errors = DorisEngineSpec.extract_errors(Exception(message))
+ assert errors[0].error_type.name == error_type
+
+
+def test_build_sqlalchemy_uri() -> None:
+ from superset.db_engine_specs.base import BasicParametersType
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ parameters: BasicParametersType = {
+ "username": "user",
+ "password": "p@ss",
+ "host": "doris.example.com",
+ "port": 9030,
+ "database": "internal.db",
+ "query": {},
+ }
+ encrypted = make_url(
+ DorisEngineSpec.build_sqlalchemy_uri({**parameters, "encryption":
True})
+ )
+ assert encrypted.drivername == "doris"
+ assert dict(encrypted.query) == {"ssl_mode": "VERIFY_CA"}
+ assert encrypted.password == "p@ss" # noqa: S105
+
+ plain = make_url(
+ DorisEngineSpec.build_sqlalchemy_uri({**parameters, "encryption":
False})
+ )
+ assert plain.drivername == "doris"
+ assert dict(plain.query) == {}
+
+ round_trip = DorisEngineSpec.get_parameters_from_uri(
+ encrypted.render_as_string(hide_password=False)
+ )
+ assert round_trip["encryption"] is True
+ assert round_trip["query"] == {}
+
+
[email protected]("source", ["toggle", "ssl=1", "ssl_mode=REQUIRED"])
+def test_doris_tls_request_uses_verification(source: str) -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ if source == "toggle":
+ uri = make_url(
+ DorisEngineSpec.build_sqlalchemy_uri(
+ {
+ "username": "root",
+ "host": "localhost",
+ "port": 9030,
+ "database": "internal.db",
+ "encryption": True,
+ }
+ )
+ )
+ else:
+ uri = make_url(f"doris://root@localhost/internal.db?{source}")
+ url, args = DorisEngineSpec.adjust_engine_params(
+ uri, {}, catalog="external", schema="other"
+ )
+ assert args.get("ssl_mode", url.query.get("ssl_mode")) == "VERIFY_CA"
+ assert url.drivername == "doris"
+ assert url.database == "external.other"
+
+
[email protected]("mode", ["DISABLED", "PREFERRED", None])
+def test_doris_toggle_cannot_be_cancelled(mode: Optional[str]) -> None:
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ with pytest.raises(ValueError, match="conflicts"):
+ DorisEngineSpec.adjust_engine_params(
+ make_url("doris://root@localhost/internal.db?ssl_mode=REQUIRED"),
+ {"ssl_mode": mode},
+ )
+
+
+def test_doris_tls_preserves_verified_mode_and_ca() -> None:
Review Comment:
<!-- Bito Reply -->
The docstring for `test_doris_tls_preserves_verified_mode_and_ca` has been
successfully added in the latest update. It correctly documents that the test
verifies the preservation of explicit `ssl_mode=VERIFY_IDENTITY` and native
`ssl` CA configurations by `adjust_engine_params`.
**tests/unit_tests/db_engine_specs/test_doris.py**
```
def test_doris_tls_preserves_verified_mode_and_ca() -> None:
"""Test that explicit ssl_mode=VERIFY_IDENTITY and native ssl CA are
preserved."""
```
--
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]
