codeant-ai-for-open-source[bot] commented on code in PR #42920:
URL: https://github.com/apache/superset/pull/42920#discussion_r3741647496
##########
superset/commands/importers/v1/utils.py:
##########
@@ -318,28 +318,35 @@ def import_tag(
for tag_name in target_tag_names:
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
+ # Isolate each tag operation in a SAVEPOINT so a failure (e.g. a
+ # concurrent unique-constraint violation) rolls back only the
failed
+ # tag and leaves the session usable for the remaining tags, instead
+ # of poisoning the session with a pending-rollback state.
+ with db_session.begin_nested():
+ 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()
)
- db_session.add(new_tagged_object)
+ if not tagged_object:
+ new_tagged_object = TaggedObject(
+ tag_id=tag.id, object_id=object_id,
object_type=object_type
+ )
+ db_session.add(new_tagged_object)
- new_tag_ids.append(tag.id)
+ new_tag_ids.append(tag.id)
Review 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]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Returned tag IDs can include rolled-back associations.
- โ ๏ธ Import cleanup can retain stale tag relationships.
- โ ๏ธ Concurrent tagged-object conflicts can trigger this path.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=05254bdfde1140d3bc3062df8e02d5f0&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=05254bdfde1140d3bc3062df8e02d5f0&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/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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42920&comment_hash=fb43dac12f76df0b6338e83cd6a9a6eb1ba1f9c3fdc2977d085c2d7af16afe8f&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42920&comment_hash=fb43dac12f76df0b6338e83cd6a9a6eb1ba1f9c3fdc2977d085c2d7af16afe8f&reaction=dislike'>๐</a>
##########
tests/unit_tests/charts/commands/importers/v1/import_test.py:
##########
@@ -506,3 +507,54 @@ def test_import_tag_logic_for_charts(session_with_schema:
Session):
.all()
)
assert len(associated_tags) == 0
+
+
+def test_import_tag_savepoint_keeps_session_usable(
+ mocker: MockerFixture, session_with_schema: Session
+) -> None:
+ """
+ When a single tag operation fails with a SQLAlchemyError (e.g. a unique
+ constraint violation from a concurrent import), the per-tag SAVEPOINT
+ isolates the failure so the session is not left in a pending-rollback
+ state, and the remaining tags still import successfully.
+ """
+ contents = {
+ "tags.yaml": yaml.dump(
+ {
+ "tags": [
+ {"tag_name": "tag_1", "description": "Description for
tag_1"},
+ {"tag_name": "tag_2", "description": "Description for
tag_2"},
+ ]
+ }
+ )
+ }
+
+ object_id = 1
+ object_type = "chart"
+
+ # Simulate a unique-constraint violation on the first TaggedObject insert.
+ # The second tag must still be imported, and the session must not be left
+ # in a pending-rollback state (which would raise PendingRollbackError on
+ # the next operation).
+ original_add = session_with_schema.add
+ add_count = 0
+
+ def flaky_add(obj: object) -> None:
+ nonlocal add_count
+ if isinstance(obj, TaggedObject):
+ add_count += 1
+ if add_count == 1:
+ raise SQLAlchemyError("UNIQUE constraint failed:
tagged_object")
Review 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]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Regression coverage misses actual database constraint handling.
- โ ๏ธ Broken flush-time rollback could pass this test.
- โ ๏ธ Import concurrency behavior remains insufficiently tested.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a6c59a6f696d4b3ebe1b499c47c2ebc0&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=a6c59a6f696d4b3ebe1b499c47c2ebc0&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:** 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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42920&comment_hash=b2c08082b5a53395bc752dc177784a640e97b8572cf64e978a039a111cd7997b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42920&comment_hash=b2c08082b5a53395bc752dc177784a640e97b8572cf64e978a039a111cd7997b&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]
