codeant-ai-for-open-source[bot] commented on code in PR #37945: URL: https://github.com/apache/superset/pull/37945#discussion_r2801726931
########## superset/db_engine_specs/doltdb.py: ########## @@ -0,0 +1,49 @@ +# 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. +from superset.db_engine_specs.base import DatabaseCategory +from superset.db_engine_specs.mysql import MySQLEngineSpec + + +class DoltDBEngineSpec(MySQLEngineSpec): + """ + Engine spec for DoltDB. + + DoltDB is a SQL database with Git-like version control for data and schema. + It is fully MySQL-compatible. + """ + + engine = "doltdb" + engine_name = "DoltDB" + + metadata = { + "description": ( + "DoltDB is a SQL database with Git-like version control for data " + "and schema. It is fully MySQL-compatible." + ), + "logo": "doltdb.png", + "homepage_url": "https://www.dolthub.com/";, + "categories": [ + DatabaseCategory.TRADITIONAL_RDBMS, + DatabaseCategory.OPEN_SOURCE, + ], + "pypi_packages": ["mysqlclient"], + "connection_string": "mysql://{username}:{password}@{host}:{port}/{database}", Review Comment: **Suggestion:** The `engine` for this spec is set to `doltdb` while the example `connection_string` uses the `mysql://` backend; if a user follows this template, Superset will treat the database as generic MySQL (using `MySQLEngineSpec`) rather than `DoltDBEngineSpec`, so the Dolt-specific metadata (logo, categories, etc.) will never be associated with that connection. Aligning the connection string backend with the engine name avoids this mismatch and ensures `get_engine_spec` and the UI refer to the same backend. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Dolt databases created from template use MySQL engine spec. - ⚠️ Dolt-specific metadata and logo unused for saved connections. - ⚠️ Future Dolt-specific overrides ignored; MySQL behavior always applied. ``` </details> ```suggestion "connection_string": "doltdb://{username}:{password}@{host}:{port}/{database}", ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In the Dolt engine spec at `superset/db_engine_specs/doltdb.py:32-45`, note that the metadata example `connection_string` is `mysql://{username}:{password}@{host}:{port}/{database}` while `engine = "doltdb"` at `superset/db_engine_specs/doltdb.py:29`. 2. From the database model in `superset/models/core.py:299-307` and `1021-1039`, observe that a `Database`'s backend is derived from its SQLAlchemy URI (`backend = url.get_backend_name()` at line 1031) and the engine spec is resolved via `db_engine_specs.get_engine_spec(backend, driver)` at line 1038. 3. Inspect engine specs: `MySQLEngineSpec` declares `engine = "mysql"` at `superset/db_engine_specs/mysql.py:78-80`, while `DoltDBEngineSpec` declares `engine = "doltdb"` at `superset/db_engine_specs/doltdb.py:29` and does not set any `engine_aliases`. `BaseEngineSpec.supports_backend` in `superset/db_engine_specs/base.py:855-870` returns `True` only when the URL backend matches `cls.engine` or `engine_aliases`. 4. When an admin creates a Dolt database and follows the Dolt metadata template (pasting a URI starting with `mysql://` into the SQLAlchemy URI field), the saved `Database.sqlalchemy_uri` has backend `mysql`, so `Database.get_db_engine_spec()` resolves to `MySQLEngineSpec` (engine `"mysql"`) instead of `DoltDBEngineSpec` (engine `"doltdb"`). As a result, that connection is treated as generic MySQL and never uses the Dolt-specific engine spec, despite having been configured from the Dolt metadata template. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/db_engine_specs/doltdb.py **Line:** 44:44 **Comment:** *Logic Error: The `engine` for this spec is set to `doltdb` while the example `connection_string` uses the `mysql://` backend; if a user follows this template, Superset will treat the database as generic MySQL (using `MySQLEngineSpec`) rather than `DoltDBEngineSpec`, so the Dolt-specific metadata (logo, categories, etc.) will never be associated with that connection. Aligning the connection string backend with the engine name avoids this mismatch and ensures `get_engine_spec` and the UI refer to the same backend. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37945&comment_hash=96d9f620de0032fa59a1b31f85b1ed4e735eb4bb3e87de22de981b6355e3e0e6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37945&comment_hash=96d9f620de0032fa59a1b31f85b1ed4e735eb4bb3e87de22de981b6355e3e0e6&reaction=dislike'>👎</a> ########## superset/db_engine_specs/tidb.py: ########## @@ -0,0 +1,74 @@ +# 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. +from superset.db_engine_specs.base import DatabaseCategory +from superset.db_engine_specs.mysql import MySQLEngineSpec + + +class TiDBEngineSpec(MySQLEngineSpec): + """ + Engine spec for TiDB. + + TiDB is an open-source, cloud-native, distributed SQL database designed for + hybrid transactional and analytical processing (HTAP) workloads. It is + MySQL-compatible. + """ + + engine = "tidb" + engine_name = "TiDB" + Review Comment: **Suggestion:** This engine spec currently inherits `default_driver` from `MySQLEngineSpec` (which is set to `"mysqldb"`), but the TiDB spec advertises the native SQLAlchemy dialect/driver `"tidb"` in its own metadata; keeping the default driver as `"mysqldb"` for the `"tidb"` backend will cause the driver metadata returned by `get_available_engine_specs` (used by the database connection UI and denylist logic) to be inconsistent with the actual dialect, potentially surfacing an unusable default driver for TiDB connections and breaking any configuration that relies on matching `default_driver` to installed drivers. Explicitly setting `default_driver = "tidb"` on `TiDBEngineSpec` aligns the default driver with the native TiDB dialect advertised in the spec. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Database available endpoint returns inconsistent TiDB default_driver metadata. - ⚠️ TiDB missing parameter-based configuration in new database modal. - ⚠️ UI engine+driver matching for TiDB connections becomes unreliable. - ⚠️ Denylist rules cannot reliably target TiDB's actual driver. ``` </details> ```suggestion default_driver = "tidb" ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Install Superset with this PR code and a TiDB SQLAlchemy dialect registered via the `sqlalchemy.dialects` entry point (so `get_available_engine_specs()` in `superset/db_engine_specs/__init__.py:124-193` discovers a backend named `"tidb"`; for third-party dialects, driver defaults to `dialect.name` per line 165, so the discovered driver will commonly be `"tidb"`). 2. Note that `TiDBEngineSpec` in `superset/db_engine_specs/tidb.py:21-31` subclasses `MySQLEngineSpec` from `superset/db_engine_specs/mysql.py:78-83`, inheriting `default_driver = "mysqldb"` (line 83) because `TiDBEngineSpec` does not override `default_driver`. 3. Call the database-availability API `GET /api/v1/database/available`, which executes `DatabasesApi.available` in `superset/databases/api.py:37-67` (snippet starting at line 37 in the OpenAPI description): this iterates over `get_available_engine_specs().items()`, builds `available_drivers` from the discovered drivers set (for TiDB this will be `{"tidb"}` from step 1), and unconditionally sets `payload["default_driver"] = engine_spec.default_driver` (line 53), yielding `default_driver: "mysqldb"` alongside `available_drivers: ["tidb"]` for TiDB. 4. Observe two concrete inconsistencies: (a) the metadata advertised in `TiDBEngineSpec.metadata["drivers"]` at `superset/db_engine_specs/tidb.py:48-69` lists a native `"tidb"` driver backed by `sqlalchemy-tidb`, while `default_driver` remains `"mysqldb"` (from MySQL); (b) the UI code in `superset-frontend/src/features/databases/DatabaseModal/index.tsx:7-23` relies on `available.default_driver === db?.driver` when matching an engine+driver pair and on `default_driver` being present in the `available_drivers` set to surface parameter-based configuration (see `engine_spec.default_driver in drivers` check in `superset/databases/api.py:57-61`), meaning TiDB will expose a default driver name (`"mysqldb"`) that does not correspond to any installed driver for the `"tidb"` backend, preventing the new-UI parameter schema from being enabled even when the native `tidb` dialect is installed and leading to misleading driver information for TiDB connections. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/db_engine_specs/tidb.py **Line:** 32:32 **Comment:** *Logic Error: This engine spec currently inherits `default_driver` from `MySQLEngineSpec` (which is set to `"mysqldb"`), but the TiDB spec advertises the native SQLAlchemy dialect/driver `"tidb"` in its own metadata; keeping the default driver as `"mysqldb"` for the `"tidb"` backend will cause the driver metadata returned by `get_available_engine_specs` (used by the database connection UI and denylist logic) to be inconsistent with the actual dialect, potentially surfacing an unusable default driver for TiDB connections and breaking any configuration that relies on matching `default_driver` to installed drivers. Explicitly setting `default_driver = "tidb"` on `TiDBEngineSpec` aligns the default driver with the native TiDB dialect advertised in the spec. 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. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37945&comment_hash=42376ac7504e9d76ea5a4f86ec7fb6eed6a58ac1d4ea9849e596b5eb037063cd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37945&comment_hash=42376ac7504e9d76ea5a4f86ec7fb6eed6a58ac1d4ea9849e596b5eb037063cd&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: [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]
