bito-code-review[bot] commented on code in PR #44718:
URL: https://github.com/apache/superset/pull/44718#discussion_r4129963992
##########
tests/unit_tests/db_engine_specs/test_doris.py:
##########
@@ -280,3 +280,214 @@ 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:
+ """Driver errors map to the corresponding Superset error types."""
+ 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:
+ """The parameters form builds a valid Doris URI with optional verified
TLS."""
+ 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:
+ """Toggle, scalar SSL and legacy REQUIRED requests use verified TLS."""
+ 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:
+ """Connect arguments cannot cancel a REQUIRED request in the URI."""
+ 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:
+ """Explicit hostname verification and the native CA dictionary survive."""
+ 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:
+ """Connections without a TLS request retain their URI and arguments."""
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ uri = make_url("doris://root@localhost/internal.db")
+ assert DorisEngineSpec.adjust_engine_params(uri, {}) == (uri, {})
+
+
[email protected]("mode", ["REQUIRED", "VERIFY_CA", "VERIFY_IDENTITY"])
+def test_doris_tls_parameters_round_trip(mode: str) -> None:
+ """Editing TLS URIs keeps encryption enabled and preserves stronger
modes."""
+ from superset.db_engine_specs.doris import DorisEngineSpec
+
+ parameters = DorisEngineSpec.get_parameters_from_uri(
+
f"doris://user:p%40ss@localhost:9030/internal.db?ssl_mode={mode}&charset=utf8mb4"
+ )
+ assert parameters["encryption"] is True
+ expected_mode = "VERIFY_IDENTITY" if mode == "VERIFY_IDENTITY" else
"VERIFY_CA"
+ expected_query = {"charset": "utf8mb4"}
+ if mode == "VERIFY_IDENTITY":
+ expected_query["ssl_mode"] = mode
+ assert parameters["query"] == expected_query
+ uri = make_url(DorisEngineSpec.build_sqlalchemy_uri(parameters))
+ assert uri.query == {"ssl_mode": expected_mode, "charset": "utf8mb4"}
+ assert uri.password == "p@ss" # noqa: S105
+ assert parameters["encryption"] is True
+ assert parameters["query"] == expected_query
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Redundant duplicate assertions</b></div>
<div id="fix">
Lines 478-479 re-assert `parameters["encryption"] is True` and
`parameters["query"] == expected_query`, which are already asserted verbatim at
lines 469 and 474 in this same test. Redundant assertions add no coverage and
create a maintenance hazard: if the expected TLS behavior changes, both copies
must be updated in sync or the test silently diverges. Drop the duplicates.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
````
</div>
</details>
</div>
<small><i>Code Review Run #b882ae</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]
