bito-code-review[bot] commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3432645313
##########
superset/db_engine_specs/databricks.py:
##########
@@ -746,7 +746,7 @@ def get_catalog_names(
database: Database,
inspector: Inspector,
) -> set[str]:
- return {catalog for (catalog,) in inspector.bind.execute("SHOW
CATALOGS")}
+ return {catalog for (catalog,) in inspector.bind.execute(text("SHOW
CATALOGS"))}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate adjust_engine_params method</b></div>
<div id="fix">
The `adjust_engine_params` method contains duplicate logic across Databricks
and Presto engine specs for handling catalog and schema parameters. Consider
refactoring into a shared base class or utility to reduce duplication and
ensure consistent behavior.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
@@ -1,15 +1,10 @@
-# Duplicated in both databricks.py (lines 749-759) and presto.py (lines
329-339)
-
- return {catalog for (catalog,) in inspector.bind.execute(text("SHOW
CATALOGS"))}
-
- @classmethod
- def adjust_engine_params(
- cls,
- uri: URL,
- connect_args: dict[str, Any],
- catalog: str | None = None,
- schema: str | None = None,
- ) -> tuple[URL, dict[str, Any]]:
- if catalog:
+ @classmethod
+ def adjust_engine_params(
+ cls,
+ uri: URL,
+ connect_args: dict[str, Any],
+ catalog: str | None = None,
+ schema: str | None = None,
+ ) -> tuple[URL, dict[str, Any]]:
+ return cls._adjust_engine_params_with_catalog(uri, connect_args,
catalog, schema)
```
</div>
</details>
</div>
<small><i>Code Review Run #d38cb5</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
##########
superset/db_engine_specs/hive.py:
##########
@@ -234,7 +234,7 @@ def df_to_sql(
catalog=table.catalog,
schema=table.schema,
) as engine:
- engine.execute(f"DROP TABLE IF EXISTS {str(table)}")
+ engine.execute(text(f"DROP TABLE IF EXISTS {str(table)}"))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test assertion mismatch</b></div>
<div id="fix">
In `tests/integration_tests/db_engine_specs/hive_tests.py`, import `text`
from SQLAlchemy and change the two `mock_execute.assert_any_call(f"DROP TABLE
IF EXISTS …")` assertions to `mock_execute.assert_any_call(text(f"DROP TABLE IF
EXISTS …"))` so they match the code change.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset/db_engine_specs/hive.py
+++ b/superset/db_engine_specs/hive.py
@@ -237 +237 @@
-mock_execute.assert_any_call(f"DROP TABLE IF EXISTS {table_name}")
+mock_execute.assert_any_call(text(f"DROP TABLE IF EXISTS {table_name}"))
```
</div>
</details>
</div>
<small><i>Code Review Run #d38cb5</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
##########
superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py:
##########
@@ -892,14 +892,14 @@ def print_update_count():
def reset_postgres_id_sequence(table: str) -> None:
"""Reset PostgreSQL sequence ID for a table's id column."""
op.execute(
- """
+ text("""
SELECT setval(
pg_get_serial_sequence(:table, 'id'),
COALESCE(max(id) + 1, 1),
false
)
FROM :table;
- """,
+ """),
{"table": table},
)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead code introduced</b></div>
<div id="fix">
The `reset_postgres_id_sequence` function is dead code — it is defined but
never called. The `text` import added at line 32 is only used by this unused
function. Both should be removed to avoid maintenance confusion.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
---
superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
+++
superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
@@ -29,7 +29,7 @@
import sqlalchemy as sa
from alembic import op
-from sqlalchemy import select, text
+from sqlalchemy import select
from sqlalchemy.exc import NoSuchModuleError
from sqlalchemy.ext.declarative import declarative_base, declared_attr
from sqlalchemy.orm import backref, relationship, Session
@@ -889,16 +889,6 @@
from sqlalchemy.orm import aliased
-def reset_postgres_id_sequence(table: str) -> None:
- """Reset PostgreSQL sequence ID for a table's id column."""
- op.execute(
- text("""
- SELECT setval(
- pg_get_serial_sequence(:table, 'id'),
- COALESCE(max(id) + 1, 1),
- false
- )
- FROM :table;
- """),
- {"table": table},
- )
```
</div>
</details>
</div>
<small><i>Code Review Run #8d1147</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>CWE-89: SQL Parameter Binding Table Name</b></div>
<div id="fix">
The `text()` wrapper alone does not fix the fundamental issue: `:table` bind
parameters cannot be used for table names in FROM clauses in SQLAlchemy. The
parameter binding only works for VALUES, not identifiers like table names. This
migration will fail at runtime on PostgreSQL with an error like "can't adapt
type 'str'" or similar. Use f-string interpolation with quoted identifiers
instead. (See also: [CWE-89](https://cwe.mitre.org/data/definitions/89.html))
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
---
a/superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
+++
b/superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
@@ -892,12 +892,10 @@ def reset_postgres_id_sequence(table: str) -> None:
"""Reset PostgreSQL sequence ID for a table's id column."""
op.execute(
- text("""
+ text(f"""
SELECT setval(
- pg_get_serial_sequence(:table, 'id'),
- COALESCE(max(id) + 1, 1),
- false
- )
- FROM :table;
- """),
- {"table": table},
- )+ pg_get_serial_sequence('{table}', 'id'),
+ COALESCE(max(id) + 1, 1),
+ false
+ )
+ FROM \"{table}\";
+ """),
+ )
```
</div>
</details>
</div>
<small><i>Code Review Run #d38cb5</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/integration_tests/conftest.py:
##########
@@ -213,12 +214,12 @@ def setup_presto_if_needed():
database = get_example_database()
with database.get_sqla_engine() as engine:
drop_from_schema(engine, CTAS_SCHEMA_NAME)
- engine.execute(f"DROP SCHEMA IF EXISTS {CTAS_SCHEMA_NAME}")
- engine.execute(f"CREATE SCHEMA {CTAS_SCHEMA_NAME}")
+ engine.execute(text(f"DROP SCHEMA IF EXISTS {CTAS_SCHEMA_NAME}"))
+ engine.execute(text(f"CREATE SCHEMA {CTAS_SCHEMA_NAME}"))
drop_from_schema(engine, ADMIN_SCHEMA_NAME)
- engine.execute(f"DROP SCHEMA IF EXISTS {ADMIN_SCHEMA_NAME}")
- engine.execute(f"CREATE SCHEMA {ADMIN_SCHEMA_NAME}")
+ engine.execute(text(f"DROP SCHEMA IF EXISTS {ADMIN_SCHEMA_NAME}"))
+ engine.execute(text(f"CREATE SCHEMA {ADMIN_SCHEMA_NAME}"))
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate with_feature_flags decorator</b></div>
<div id="fix">
The `with_feature_flags` decorator implementation is duplicated across
integration and unit test conftest files. Consider centralizing this utility in
a shared location to maintain a single source of truth for feature flag mocking
behavior.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
@@ -1,38 +1,8 @@
-# Duplicated decorator in tests/integration_tests/conftest.py (lines
222-257)
-# and tests/unit_tests/conftest.py (lines 182-217)
-
-def with_feature_flags(**mock_feature_flags):
- """
- Use this decorator to mock feature flags in tests.integration_tests.
-
- Usage:
-
- class TestYourFeature(SupersetTestCase):
-
- @with_feature_flags(YOUR_FEATURE=True)
- def test_your_feature_enabled(self):
- self.assertEqual(is_feature_enabled("YOUR_FEATURE"), True)
-
- @with_feature_flags(YOUR_FEATURE=False)
- def test_your_feature_disabled(self):
- self.assertEqual(is_feature_enabled("YOUR_FEATURE"), False)
- """
-
- def mock_get_feature_flags():
- feature_flags = feature_flag_manager._feature_flags or {}
- return {**feature_flags, **mock_feature_flags}
-
- def decorate(test_fn):
- def wrapper(*args, **kwargs):
- with patch.object(
- feature_flag_manager,
- "get_feature_flags",
- side_effect=mock_get_feature_flags,
- ):
- test_fn(*args, **kwargs)
-
- return functools.update_wrapper(wrapper, test_fn)
-
- return decorate
+from tests.shared.feature_flag_helpers import with_feature_flags
+
+__all__ = [
+ "with_feature_flags",
+ # ... other exports
+]
```
</div>
</details>
</div>
<small><i>Code Review Run #d38cb5</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
##########
pytest.ini:
##########
@@ -18,5 +18,30 @@
testpaths =
tests
python_files = *_test.py test_*.py *_tests.py *viz/utils.py
-addopts = -p no:warnings
+# `-p no:warnings` temporarily disabled in favor of more finely tuned
`filterwarnings`.
+#addopts = -p no:warnings
asyncio_mode = auto
+
+# `ignore` virtually reproduces to `-p no:warnings`.
+# Always print RemovedIn20Warning when SQLALCHEMY_WARN_20=1.
+# Additionally, raise errors for refactored RemovedIn20Warning cases to
prevent regression.
+filterwarnings =
+ ignore
+ always::sqlalchemy.exc.RemovedIn20Warning
+ error:Passing a string to Connection.execute\(\) is
deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:"Query" object is being merged into a
Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"SavedQuery" object is being merged into a
Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"SqlaTable" object is being merged into a
Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"SqlMetric" object is being merged into a
Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"TableColumn" object is being merged into a
Session:sqlalchemy.exc.RemovedIn20Warning
+# error:"TaggedObject" object is being merged into a
Session:sqlalchemy.exc.RemovedIn20Warning
+# error:The ``as_declarative\(\)`` function is now
available:sqlalchemy.exc.RemovedIn20Warning
+# error:The autoload parameter is
deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:The connection.execute\(\) method:sqlalchemy.exc.RemovedIn20Warning
+# error:The current statement is being autocommitted using implicit
autocommit:sqlalchemy.exc.RemovedIn20Warning
+# error:The `database` package is
deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:The ``declarative_base\(\)`` function is now
available:sqlalchemy.exc.RemovedIn20Warning
+# error:The Engine.execute\(\) method is considered
legacy:sqlalchemy.exc.RemovedIn20Warning
+# error:The legacy calling style of select\(\) is
deprecated:sqlalchemy.exc.RemovedIn20Warning
+# error:The "whens" argument to case:sqlalchemy.exc.RemovedIn20Warning
+# error:"User" object is being merged into a
Session:sqlalchemy.exc.RemovedIn20Warning
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead code: commented-out rules</b></div>
<div id="fix">
Dead code: 16 commented-out `error:` rules added in commit 13cef4fd84 remain
unused. These create confusion about which warnings should be converted to
errors and add maintenance burden. Either activate them (uncomment) or remove
them entirely.
</div>
</div>
<small><i>Code Review Run #d38cb5</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]
