bito-code-review[bot] commented on code in PR #40277:
URL: https://github.com/apache/superset/pull/40277#discussion_r3435728791
##########
tests/integration_tests/sqllab_tests.py:
##########
@@ -184,18 +185,18 @@ def test_sql_json_cta_dynamic_db(self, ctas_method:
CTASMethod) -> None:
examples_db = get_example_database()
with examples_db.get_sqla_engine() as engine:
data = engine.execute(
- f"SELECT * FROM admin_database.{tmp_table_name}" # noqa:
S608
+ text(f"SELECT * FROM admin_database.{tmp_table_name}") #
noqa: S608
).fetchall()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>SQLAlchemy engine.execute deprecated</b></div>
<div id="fix">
`engine.execute()` is deprecated in SQLAlchemy 1.4 (connectionless
execution). Use `with engine.connect() as conn: conn.execute(text(...))`
instead. This applies to all 5 changed `engine.execute()` calls in this diff.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- tests/integration_tests/sqllab_tests.py (lines 186-202) ---
186: with examples_db.get_sqla_engine() as engine:
187: - data = engine.execute(
188: - text(f"SELECT * FROM
admin_database.{tmp_table_name}") # noqa: S608
189: - ).fetchall()
190: - names_count = engine.execute(
191: - text(f"SELECT COUNT(*) FROM birth_names") #
noqa: F541, S608
192: - ).first()
193: + with engine.connect() as conn:
194: + data = conn.execute(
195: + text(f"SELECT * FROM
admin_database.{tmp_table_name}") # noqa: S608
196: + ).fetchall()
197: + names_count = conn.execute(
198: + text(f"SELECT COUNT(*) FROM birth_names") #
noqa: F541, S608
199: + ).first()
200: assert names_count[0] == len(
201: data
202: ) # SQL_MAX_ROW not applied due to the
SQLLAB_CTAS_NO_LIMIT set to True
203:
204: # cleanup
205: - engine.execute(
206: - text(f"DROP {ctas_method.name}
admin_database.{tmp_table_name}")
207: - )
208: + with engine.connect() as conn:
209: + conn.execute(
210: + text(f"DROP {ctas_method.name}
admin_database.{tmp_table_name}")
211: + )
212: examples_db.allow_ctas = old_allow_ctas
213: db.session.commit()
--- tests/integration_tests/sqllab_tests.py (lines 296-302) ---
296: with examples_db.get_sqla_engine() as engine:
297: - engine.execute(
298: - text(f"""
299: - CREATE TABLE IF NOT EXISTS
{CTAS_SCHEMA_NAME}.test_table AS
300: - SELECT 1 as c1, 2 as c2
301: - """) # noqa: E501
302: - )
303: + with engine.connect() as conn:
304: + conn.execute(
305: + text(f"""
306: + CREATE TABLE IF NOT EXISTS
{CTAS_SCHEMA_NAME}.test_table AS
307: + SELECT 1 as c1, 2 as c2
308: + """) # noqa: E501
309: + )
--- tests/integration_tests/sqllab_tests.py (lines 320-322) ---
320: db.session.query(Query).delete()
321: with get_example_database().get_sqla_engine() as engine:
322: - engine.execute(text(f"DROP TABLE IF EXISTS
{CTAS_SCHEMA_NAME}.test_table"))
323: + with engine.connect() as conn:
324: + conn.execute(text(f"DROP TABLE IF EXISTS
{CTAS_SCHEMA_NAME}.test_table"))
325: db.session.commit()
```
</div>
</details>
</div>
<small><i>Code Review Run #3d1d0b</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><div>
<div id="suggestion">
<div id="issue"><b>CWE-89: SQL Identifier Binding Error</b></div>
<div id="fix">
PostgreSQL does not allow table names as bind parameters. Instead of `FROM
:table`, build the SQL with the table name interpolated as an identifier. For
example, use SQLAlchemy's `quoted_name` or `identifier` helpers and something
like:
```python
sql = text(f"""
SELECT setval(
pg_get_serial_sequence('{table}', 'id'),
COALESCE(max(id) + 1, 1),
false
)
FROM {quoted_name(table)};
""")
op.execute(sql)
```
This ensures the table name is a literal identifier and avoids runtime
errors. (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">
```
--- tests/integration_tests/db_engine_specs/hive_tests.py (lines 201, 229)
---
201: from sqlalchemy import text
201: mock_execute.assert_any_call(text(f"DROP TABLE IF EXISTS
{table_name}"))
229: mock_execute.assert_any_call(text(f"DROP TABLE IF EXISTS
{schema}.{table_name}"))
```
</div>
</details>
</div>
<small><i>Code Review Run #3d1d0b</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]
