codeant-ai-for-open-source[bot] commented on code in PR #41176:
URL: https://github.com/apache/superset/pull/41176#discussion_r3555106308
##########
superset/commands/dataset/duplicate.py:
##########
@@ -52,6 +52,16 @@ def __init__(self, data: dict[str, Any]) -> None:
@transaction(on_error=partial(on_error,
reraise=DatasetDuplicateFailedError))
def run(self) -> Model:
self.validate()
+ # Declare the high-level avenue before the duplicate touches
+ # the session. The change-record listener stamps
+ # ``version_transaction.action_kind = 'clone'`` so the new
+ # dataset's baseline records read as a clone in the timeline.
+ # Method-scoped import — defers the versioning bootstrap path
+ # out of this command's module-load graph; see ``changes.py``
+ # module docstring for the broader init-order rationale.
+ from superset.versioning.changes import ACTION_KIND_CLONE,
ACTION_KIND_KEY
+
+ db.session.info[ACTION_KIND_KEY] = ACTION_KIND_CLONE
Review Comment:
**Suggestion:** This writes a per-transaction marker into `session.info` but
relies on versioning listeners for cleanup; with capture gated off, those
listeners are detached, so the marker can persist and incorrectly label
subsequent transactions on the same session. Ensure the key is removed in the
command path itself (for both success and error paths). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dataset version history may show normal changes as clones.
- ⚠️ Dataset duplicate endpoint /api/v1/dataset/<pk>/duplicate impacted.
- ⚠️ Off->on capture toggles can inherit stale clone markers.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start a Superset app with `ENABLE_VERSIONING_CAPTURE=False` so
`SupersetAppInitializer.init_versioning()` returns at
`superset/initialization/__init__.py:36-45` without calling
`register_change_record_listener()`, meaning the cleanup hooks that pop
`ACTION_KIND_KEY`
in `reset_processed_after_commit` and `reset_action_kind_after_rollback`
(`superset/versioning/changes/listener.py:214-242`) are not attached.
2. Call the dataset duplicate endpoint `PUT /api/v1/dataset/<pk>/duplicate`
(OpenAPI-documented around `superset/datasets/api.py:790-829`), which loads
the payload
and invokes `DuplicateDatasetCommand(item).run()` at
`superset/datasets/api.py:18-20`.
Inside `DuplicateDatasetCommand.run` (decorated with `@transaction` at
`superset/commands/dataset/duplicate.py:52`), the code at
`superset/commands/dataset/duplicate.py:62-64` imports `ACTION_KIND_CLONE,
ACTION_KIND_KEY` and sets `db.session.info[ACTION_KIND_KEY] =
ACTION_KIND_CLONE` before
creating and adding the new `SqlaTable` and related objects.
3. The transaction decorator at `superset/utils/decorators.py:235-263`
commits on success
(`db.session.commit()`), but because no change-record listeners are
registered when
capture is disabled, no `after_commit` handler fires to clear
`ACTION_KIND_KEY`. As a
result, the session-scoped `db.session.info` retains
`"_versioning_action_kind": "clone"`
after the duplicate finishes, even though that transaction is complete.
4. Later in the same Python process, a second Superset app is initialized
with
`ENABLE_VERSIONING_CAPTURE=True`, causing `init_versioning()` to execute the
ON branch at
`superset/initialization/__init__.py:47-76`, which calls
`register_change_record_listener()` at line 859 and attaches
`flush_change_records` to
`db.session`. When a subsequent dataset update (for example, `PUT
/api/v1/dataset/<pk>`,
which calls `UpdateDatasetCommand` in `superset/datasets/api.py`) performs
the first
versioned flush, `flush_change_records` invokes
`_stamp_action_kind_on_transaction` at
`superset/versioning/changes/listener.py:49-72`. That helper pops the stale
`ACTION_KIND_KEY` and stamps `version_transaction.action_kind='clone'` for
this unrelated
update, causing the dataset version history to misclassify a normal edit as
a clone.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6b9f7f02e7c44379b79a3a0e7f435e8c&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=6b9f7f02e7c44379b79a3a0e7f435e8c&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/dataset/duplicate.py
**Line:** 64:64
**Comment:**
*Logic Error: This writes a per-transaction marker into `session.info`
but relies on versioning listeners for cleanup; with capture gated off, those
listeners are detached, so the marker can persist and incorrectly label
subsequent transactions on the same session. Ensure the key is removed in the
command path itself (for both success and error paths).
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%2F41176&comment_hash=622cbf9be28a0a1d0f1d8a885079c9237d5b2896bc7ae7bf3837e79f41621487&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=622cbf9be28a0a1d0f1d8a885079c9237d5b2896bc7ae7bf3837e79f41621487&reaction=dislike'>👎</a>
##########
superset/commands/dashboard/copy.py:
##########
@@ -40,6 +40,19 @@ def __init__(self, original_dash: Dashboard, data: dict[str,
Any]) -> None:
@transaction(on_error=partial(on_error, reraise=DashboardCopyError))
def run(self) -> Dashboard:
self.validate()
+ # Declare the high-level avenue before the copy touches the
+ # session. The change-record listener stamps
+ # ``version_transaction.action_kind = 'clone'`` so the new
+ # dashboard's baseline records read as "Cloned from <source>"
+ # in the timeline instead of "Dashboard created".
+ # Method-scoped imports — defer the versioning bootstrap path
+ # (``Model.metadata`` and Continuum-adjacent setup) out of this
+ # command's module-load graph; see ``changes.py`` module
+ # docstring for the broader init-order rationale.
+ from superset import db
+ from superset.versioning.changes import ACTION_KIND_CLONE,
ACTION_KIND_KEY
+
+ db.session.info[ACTION_KIND_KEY] = ACTION_KIND_CLONE
Review Comment:
**Suggestion:** Setting a session-scoped action marker without guaranteed
cleanup leaks state when versioning capture is disabled (the cleanup listeners
are not registered in that mode). On a reused SQLAlchemy session, a later
unrelated transaction can inherit this stale marker and be misclassified. Clear
this key in a local try/finally (or a shared context manager) after the copy
operation completes. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard version history mislabels later edits as clones.
- ⚠️ Operators misinterpret /versions timeline for affected dashboards.
- ⚠️ Multi-app off->on toggles risk polluted action_kind context.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Initialize a Superset app with `ENABLE_VERSIONING_CAPTURE=False`, so
`SupersetAppInitializer.init_versioning()` returns early at
`superset/initialization/__init__.py:36-45` and never calls
`register_change_record_listener()`; as a result, the `after_commit` /
`after_rollback`
cleanup handlers in `superset/versioning/changes/listener.py:214-247` are
not registered.
2. Trigger a dashboard copy via the MCP tool, which calls
`CopyDashboardCommand(source,
data).run()` at
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:26-27`. Inside
`CopyDashboardCommand.run` (decorated with `@transaction` at
`superset/commands/dashboard/copy.py:40`), the code at
`superset/commands/dashboard/copy.py:55` executes
`db.session.info[ACTION_KIND_KEY] =
ACTION_KIND_CLONE` before calling `DashboardDAO.copy_dashboard(...)` at line
56.
3. The `@transaction` decorator at `superset/utils/decorators.py:235-263`
then calls
`db.session.commit()` on success (line 2 of the second chunk), but because
the versioning
change-record listeners were never registered, no `after_commit` handler
runs, and nothing
pops `ACTION_KIND_KEY` from `db.session.info`. The marker
`"_versioning_action_kind":
"clone"` therefore persists on the long-lived SQLAlchemy session beyond the
end of this
transaction.
4. In the same Python process, initialize another Superset app (e.g. in
tests or multi-app
setups) with `ENABLE_VERSIONING_CAPTURE=True`, so `init_versioning()`
proceeds into the ON
branch at `superset/initialization/__init__.py:47-76` and calls
`register_change_record_listener()` at line 859, attaching
`flush_change_records` and
`_stamp_action_kind_on_transaction` from
`superset/versioning/changes/listener.py:157-188`
and :49-72 to the existing `db.session`. The next dashboard update (e.g. `PUT
/api/v1/dashboard/<id>`, which calls `UpdateDashboardCommand(pk,
item).run()` at
`superset/dashboards/api.py:11-18`) runs under this same session; on its
first
`after_flush`, `_stamp_action_kind_on_transaction` reads and pops the stale
`ACTION_KIND_KEY` value set by the earlier copy and stamps
`version_transaction.action_kind='clone'` for this unrelated update,
misclassifying the
transaction.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e5d18181ae1f47c9bf73f4b71feae3b2&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=e5d18181ae1f47c9bf73f4b71feae3b2&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/dashboard/copy.py
**Line:** 55:55
**Comment:**
*Logic Error: Setting a session-scoped action marker without guaranteed
cleanup leaks state when versioning capture is disabled (the cleanup listeners
are not registered in that mode). On a reused SQLAlchemy session, a later
unrelated transaction can inherit this stale marker and be misclassified. Clear
this key in a local try/finally (or a shared context manager) after the copy
operation completes.
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%2F41176&comment_hash=83fe20f1433ebc54e58c6b64b70eb91c3ce8e8d152c8c4193fa137c66e5c9fb9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=83fe20f1433ebc54e58c6b64b70eb91c3ce8e8d152c8c4193fa137c66e5c9fb9&reaction=dislike'>👎</a>
##########
superset/commands/importers/v1/__init__.py:
##########
@@ -86,6 +86,19 @@ def _get_uuids(cls) -> set[str]:
def run(self) -> None:
self.validate()
+ # Declare the high-level avenue before any session writes. The
+ # change-record listener reads this on its first after_flush
+ # for the resulting ``version_transaction`` row and stamps
+ # ``version_transaction.action_kind = 'import'``. Lets operators
+ # explain otherwise-confusing diffs ("Cleared default_filters")
+ # as "this was an import".
+ # Method-scoped import — defers the versioning bootstrap path
+ # out of this command's module-load graph; see ``changes.py``
+ # module docstring for the broader init-order rationale.
+ from superset.versioning.changes import ACTION_KIND_IMPORT,
ACTION_KIND_KEY
+
+ db.session.info[ACTION_KIND_KEY] = ACTION_KIND_IMPORT
Review Comment:
**Suggestion:** The import command sets a session-level action marker before
running import work, but this file does not guarantee key removal itself. In
the dark-launch default (`ENABLE_VERSIONING_CAPTURE=False`), listener-based
cleanup is not active, so this marker can leak to later operations on the same
scoped session and mis-tag them as imports. Add explicit cleanup around the
`_import` call. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard version audit may misclassify edits as imports.
- ⚠️ Dashboards import endpoint /api/v1/dashboard/import/ implicated.
- ⚠️ Off->on capture toggles risk stale import markers.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Boot a Superset app with `ENABLE_VERSIONING_CAPTURE=False`, so
`SupersetAppInitializer.init_versioning()` takes the OFF path at
`superset/initialization/__init__.py:36-45`, logging that capture is
disabled and
returning without calling `register_change_record_listener()` at line 859;
thus the
`after_commit` and `after_rollback` handlers that clear `ACTION_KIND_KEY` in
`superset/versioning/changes/listener.py:214-242` are not registered.
2. Use the dashboard import endpoint `POST /api/v1/dashboard/import/`, which
constructs an
`ImportDashboardsCommand` at `superset/dashboards/api.py:2176-2190`.
`ImportDashboardsCommand` subclasses `ImportModelsCommand`
(`superset/commands/dashboard/importers/v1/__init__.py:5-18`), whose `run`
method is
decorated with `@transaction()` at
`superset/commands/importers/v1/__init__.py:85-87`.
Inside `run`, after `self.validate()`, the code at
`superset/commands/importers/v1/__init__.py:98-100` imports
`ACTION_KIND_IMPORT,
ACTION_KIND_KEY` and sets `db.session.info[ACTION_KIND_KEY] =
ACTION_KIND_IMPORT` before
calling `self._import(...)` in the surrounding try/except block at lines
102-107.
3. The `@transaction` decorator in `superset/utils/decorators.py:235-263`
commits on
success (`db.session.commit()` at line 2 of the second chunk) or rolls back
on exception
(`db.session.rollback()` at line 5). Because no versioning change-record
listeners are
attached when capture is disabled, neither `reset_processed_after_commit` nor
`reset_action_kind_after_rollback` is invoked, so `db.session.info` retains
`"_versioning_action_kind": "import"` after this import transaction
completes.
4. In the same interpreter process (for example, in multi-app test setups),
initialize
another Superset app with `ENABLE_VERSIONING_CAPTURE=True`, allowing
`init_versioning()`
to execute the ON branch at `superset/initialization/__init__.py:47-76` and
call
`register_change_record_listener()` at line 859 to attach
`flush_change_records` to
`db.session`. On the first subsequent versioned write, such as a dashboard
update via `PUT
/api/v1/dashboard/<id>` (`superset/dashboards/api.py:936-975`), the
`after_flush` handler
calls `_stamp_action_kind_on_transaction` at
`superset/versioning/changes/listener.py:49-72`, which pops the stale
`ACTION_KIND_KEY`
and stamps `version_transaction.action_kind='import'` for this normal edit,
causing the
audit trail to present it as an import rather than a regular update.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d704ea220a3b4bfb9ddf1cf97077556e&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=d704ea220a3b4bfb9ddf1cf97077556e&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/__init__.py
**Line:** 100:100
**Comment:**
*Logic Error: The import command sets a session-level action marker
before running import work, but this file does not guarantee key removal
itself. In the dark-launch default (`ENABLE_VERSIONING_CAPTURE=False`),
listener-based cleanup is not active, so this marker can leak to later
operations on the same scoped session and mis-tag them as imports. Add explicit
cleanup around the `_import` call.
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%2F41176&comment_hash=41cf444166b8d58e62f5726e74fc4f4f1743aa57679a9d11ca851a6f788211ee&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=41cf444166b8d58e62f5726e74fc4f4f1743aa57679a9d11ca851a6f788211ee&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]
