fix(import): isolate per-tag import in a SAVEPOINT to avoid poisoned session (#42912) - #42920
fix(import): isolate per-tag import in a SAVEPOINT to avoid poisoned session (#42912)#42920waterWang wants to merge 2 commits into
Conversation
Code Review Agent Run #916fd7Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| db_session.add(new_tagged_object) | ||
|
|
||
| new_tag_ids.append(tag.id) | ||
| new_tag_ids.append(tag.id) |
There was a problem hiding this comment.
Suggestion: new_tag_ids is mutated before the nested transaction flushes and releases its SAVEPOINT. If the association insert or SAVEPOINT release raises a SQLAlchemyError, the database work is rolled back but this ID remains in the returned list, causing old-association cleanup to treat the failed tag as successfully imported. Append the ID only after the nested context exits successfully. [logic error]
Severity Level: Minor 🧹
- ⚠️ Returned tag IDs can include rolled-back associations.
- ⚠️ Import cleanup can retain stale tag relationships.
- ⚠️ Concurrent tagged-object conflicts can trigger this path.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/importers/v1/utils.py
**Line:** 349:349
**Comment:**
*Logic Error: `new_tag_ids` is mutated before the nested transaction flushes and releases its SAVEPOINT. If the association insert or SAVEPOINT release raises a `SQLAlchemyError`, the database work is rolled back but this ID remains in the returned list, causing old-association cleanup to treat the failed tag as successfully imported. Append the ID only after the nested context exits successfully.
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| add_count += 1 | ||
| if add_count == 1: | ||
| raise SQLAlchemyError("UNIQUE constraint failed: tagged_object") |
There was a problem hiding this comment.
Suggestion: This test raises SQLAlchemyError directly from the mocked Session.add method, before SQLAlchemy performs an INSERT, autoflush, SAVEPOINT creation, or SAVEPOINT release. Consequently it does not exercise the database failure mode the production change targets and would still pass if rollback behavior during an actual flush or context-manager exit were broken. Trigger the violation during flush or configure the test database with a real conflicting unique row instead of raising from add. [possible bug]
Severity Level: Minor 🧹
- ⚠️ Regression coverage misses actual database constraint handling.
- ⚠️ Broken flush-time rollback could pass this test.
- ⚠️ Import concurrency behavior remains insufficiently tested.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/charts/commands/importers/v1/import_test.py
**Line:** 545:547
**Comment:**
*Possible Bug: This test raises `SQLAlchemyError` directly from the mocked `Session.add` method, before SQLAlchemy performs an INSERT, autoflush, SAVEPOINT creation, or SAVEPOINT release. Consequently it does not exercise the database failure mode the production change targets and would still pass if rollback behavior during an actual flush or context-manager exit were broken. Trigger the violation during flush or configure the test database with a real conflicting unique row instead of raising from `add`.
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42920 +/- ##
==========================================
- Coverage 66.41% 66.41% -0.01%
==========================================
Files 2858 2858
Lines 161342 161343 +1
Branches 37162 37162
==========================================
Hits 107161 107161
- Misses 52154 52155 +1
Partials 2027 2027
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Description
Closes #42912.
import_tagcatchesSQLAlchemyErrorafter a query-triggered autoflushfailure (e.g. a concurrent unique-constraint violation) and continues
using the same SQLAlchemy Session without rolling back or isolating the
failed operation. The Session enters a pending-rollback state, and the
next operation raises
PendingRollbackError.Fix
Wrap each per-tag operation in a
db_session.begin_nested()SAVEPOINT.When a conflict occurs:
discarded
Test
Added
test_import_tag_savepoint_keeps_session_usablewhich simulates aunique-constraint violation on the first tag and verifies that: