codeant-ai-for-open-source[bot] commented on code in PR #41980:
URL: https://github.com/apache/superset/pull/41980#discussion_r3576033987
##########
superset/utils/core.py:
##########
@@ -641,9 +641,7 @@ def generic_find_constraint_name(
table: str, columns: set[str], referenced: str, database: SQLAlchemy
) -> str | None:
"""Utility to find a constraint name in alembic migrations"""
- tbl = sa.Table(
- table, database.metadata, autoload=True, autoload_with=database.engine
- )
+ tbl = sa.Table(table, database.metadata, autoload_with=database.engine)
Review Comment:
**Suggestion:** Using `database.metadata` for reflection can break
migrations when that metadata already contains ORM-declared tables (the common
case in Superset). Calling `sa.Table(..., autoload_with=...)` against an
already-registered table can raise `InvalidRequestError` or reuse model-time
constraints instead of the live migration schema, causing constraint lookup to
fail. Reflect into a fresh `MetaData()` (or use an inspector-based lookup) tied
to the migration connection so constraint discovery always reads the actual
database state. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
❌ Alembic migration `sync_db_with_models` may fail constraint lookup.
❌ Database upgrades can leave stale foreign-key constraints untouched.
⚠️ Future migrations relying on constraints may behave unpredictably.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Superset constructs the global SQLAlchemy `db` instance in
`superset/extensions/__init__.py:157` via `db = get_sqla_class()()`, and ORM
models such
as `Slice` register their tables on the shared metadata (`Model.metadata`)
with
`__tablename__ = "slices"` in `superset/models/slice.py:63-71`, so the
`"slices"` table is
present in `db.metadata.tables`.
2. Run Alembic migrations using Superset’s migration flow (for example
`flask db upgrade`
or the integration tests), which loads
`superset/migrations/versions/2016-09-22_10-21_3b626e2a6783_sync_db_with_models.py`;
this
script imports `db` from `superset` and `generic_find_constraint_name` from
`superset.utils.core` at lines 5-6.
3. During the `upgrade()` function in that migration
(`sync_db_with_models.py:15-26`), the
code calls `generic_find_constraint_name(table="slices",
columns={"druid_datasource_id"},
referenced="datasources", database=db)` and
`generic_find_constraint_name(table="slices",
columns={"table_id"}, referenced="tables", database=db)`, which reach
`generic_find_constraint_name()` in `superset/utils/core.py:640-644` and
execute `tbl =
sa.Table(table, database.metadata, autoload_with=database.engine)`.
4. Because `database.metadata` already contains a `Table` named `"slices"`
from the ORM
mapping, SQLAlchemy’s `sa.Table()` either raises
`sqlalchemy.exc.InvalidRequestError`
about the table already being defined for that `MetaData`, or reuses the
ORM-defined
constraints instead of reflecting the live schema; in both cases
`generic_find_constraint_name` cannot reliably discover the actual foreign
key constraint
names, causing the migration to log warnings and skip dropping or altering
the intended
constraints (`sync_db_with_models.py:28-35`, `sync_db_with_models.py:120-9`)
and
potentially leaving stale constraints in production databases.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f31a2a5defb64731bd1dee1250847ccc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f31a2a5defb64731bd1dee1250847ccc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/core.py
**Line:** 644:644
**Comment:**
*Api Mismatch: Using `database.metadata` for reflection can break
migrations when that metadata already contains ORM-declared tables (the common
case in Superset). Calling `sa.Table(..., autoload_with=...)` against an
already-registered table can raise `InvalidRequestError` or reuse model-time
constraints instead of the live migration schema, causing constraint lookup to
fail. Reflect into a fresh `MetaData()` (or use an inspector-based lookup) tied
to the migration connection so constraint discovery always reads the actual
database state.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41980&comment_hash=4581bb4ec2a5a4ee93d890c7a111709e4d15b48a34b198d9bfd5f96f3f0cfbc4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41980&comment_hash=4581bb4ec2a5a4ee93d890c7a111709e4d15b48a34b198d9bfd5f96f3f0cfbc4&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]
