codeant-ai-for-open-source[bot] commented on code in PR #43187:
URL: https://github.com/apache/superset/pull/43187#discussion_r3787014322
##########
superset/commands/importers/v1/utils.py:
##########
@@ -316,28 +316,49 @@ def import_tag(
for tag in db_session.query(Tag).filter(Tag.name.in_(target_tag_names))
}
+ # tags named by the import that already resolved to a row, whether or not
this
+ # run managed to write their association
+ resolved_target_tag_ids: set[int] = set()
+
for tag_name in target_tag_names:
+ tag = None
try:
- tag = existing_tags.get(tag_name)
-
- # If tag does not exist, create it
- if tag is None:
- description = tag_descriptions.get(tag_name, None)
- tag = Tag(name=tag_name, description=description,
type="custom")
- db_session.add(tag)
- existing_tags[tag_name] = tag # Update the existing_tags
dictionary
-
- # Ensure the association with the object
- tagged_object = (
- db_session.query(TaggedObject)
- .filter_by(object_id=object_id, object_type=object_type,
tag_id=tag.id)
- .first()
- )
- if not tagged_object:
- new_tagged_object = TaggedObject(
- tag_id=tag.id, object_id=object_id, object_type=object_type
+ # A concurrent import can commit the same tag or the same
+ # `uix_tagged_object(tag_id, object_id, object_type)` association
+ # between the lookups above and these writes, so the flush fails
with
+ # an IntegrityError. Give each tag its own SAVEPOINT: the failure
then
+ # rolls back that tag alone, instead of leaving the session in
+ # SQLAlchemy's pending-rollback state where every later statement
--
+ # including the ones for tags that are perfectly fine -- raises
+ # PendingRollbackError. A plain `db_session.rollback()` here would
+ # discard the rest of the import along with it.
+ with db_session.begin_nested():
Review Comment:
**Suggestion:** Starting `begin_nested()` performs an unconditional flush of
all pending session changes before the SAVEPOINT is established. Import callers
can have pending chart, dashboard, or relationship changes in the shared
session; if that pre-SAVEPOINT flush fails, this handler catches the resulting
`SQLAlchemyError` even though no tag SAVEPOINT protected it, leaving the
session in pending-rollback state and causing later tags or the outer import to
fail. Establish isolation without allowing unrelated pending state to flush
outside the SAVEPOINT, or handle this failure as an outer-transaction failure.
[race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unrelated pending import writes can poison subsequent tag processing.
- ❌ `ImportAssetsCommand.run()` can still fail and roll back the whole
import.
- ⚠️ The tag SAVEPOINT does not protect pre-existing session changes.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=956ade14dcba4d649f379ae431a86dd4&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=956ade14dcba4d649f379ae431a86dd4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/importers/v1/utils.py
**Line:** 335:335
**Comment:**
*Race Condition: Starting `begin_nested()` performs an unconditional
flush of all pending session changes before the SAVEPOINT is established.
Import callers can have pending chart, dashboard, or relationship changes in
the shared session; if that pre-SAVEPOINT flush fails, this handler catches the
resulting `SQLAlchemyError` even though no tag SAVEPOINT protected it, leaving
the session in pending-rollback state and causing later tags or the outer
import to fail. Establish isolation without allowing unrelated pending state to
flush outside the SAVEPOINT, or handle this failure as an outer-transaction
failure.
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%2F43187&comment_hash=1333244ac543f2c585083ee57a2d01ae07ec1243521384153e88f17b2d914518&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43187&comment_hash=1333244ac543f2c585083ee57a2d01ae07ec1243521384153e88f17b2d914518&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]
