codeant-ai-for-open-source[bot] commented on code in PR #39469:
URL: https://github.com/apache/superset/pull/39469#discussion_r3596015301
##########
superset/migrations/versions/2026-06-23_03-23_a7d3f1b9c2e4_cleanup_stale_can_import_pvm.py:
##########
@@ -101,9 +101,9 @@ def upgrade() -> None:
session = Session(bind=bind)
do_upgrade(session)
try:
- session.commit()
+ session.commit() # pylint: disable=consider-using-transaction
Review Comment:
**Suggestion:** Calling `session.commit()` inside an Alembic migration
breaks the outer Alembic-managed transaction boundary. If a failure happens
after this commit (for example while stamping migration state), the permission
changes can be permanently committed while the revision is not fully recorded,
leaving migration state inconsistent on reruns. Keep this work inside Alembic’s
transaction and flush the session instead of committing here. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Migration data committed while Alembic version not recorded.
- ⚠️ Rerunning migrations sees inconsistent permissions versus revision.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. From a Superset instance using this PR code, run `superset db upgrade` so
Alembic
executes migrations via `superset/migrations/env.py:64-84`, where
`context.run_migrations()` is wrapped inside `context.begin_transaction()`.
2. During the run, Alembic loads revision module
`superset/migrations/versions/2026-06-23_03-23_a7d3f1b9c2e4_cleanup_stale_can_import_pvm.py`
and calls its `upgrade()` function defined around line 99 in the PR hunk.
3. Inside `upgrade()`, `Session(bind=op.get_bind())` is created,
`do_upgrade(session)`
mutates permission rows, and `session.commit()` at line 104 commits those
changes directly
on the Alembic-managed connection, independently of Alembic’s outer
transaction in
`env.py:82`.
4. Introduce a later migration that intentionally raises an exception after
this revision
(for example, a failing DDL in another script), rerun `superset db upgrade`,
and observe
that Alembic aborts and may not fully record the revision chain while the
permission
updates committed by `upgrade()` remain persisted, demonstrating
desynchronized data
versus migration version tracking.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c29c0acdb3734f718b560003b706e33c&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=c29c0acdb3734f718b560003b706e33c&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/migrations/versions/2026-06-23_03-23_a7d3f1b9c2e4_cleanup_stale_can_import_pvm.py
**Line:** 104:104
**Comment:**
*Logic Error: Calling `session.commit()` inside an Alembic migration
breaks the outer Alembic-managed transaction boundary. If a failure happens
after this commit (for example while stamping migration state), the permission
changes can be permanently committed while the revision is not fully recorded,
leaving migration state inconsistent on reruns. Keep this work inside Alembic’s
transaction and flush the session instead of committing here.
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%2F39469&comment_hash=406d3b1f01c02afd44f6b79954a7efdd9332128f62e1269201ce67ca38e44f27&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=406d3b1f01c02afd44f6b79954a7efdd9332128f62e1269201ce67ca38e44f27&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]
