codeant-ai-for-open-source[bot] commented on code in PR #41075:
URL: https://github.com/apache/superset/pull/41075#discussion_r3538010756
##########
superset/commands/dashboard/importers/v1/__init__.py:
##########
@@ -201,7 +205,31 @@ def _import(
overwrite
or (dashboard.id, chart_id) not in
existing_relationships
):
- dashboard_chart_ids.append((dashboard.id, chart_id))
+ target_chart_ids.append(chart_id)
+
+ if overwrite:
+ # Replace the dashboard's chart membership entirely.
+ dashboard.slices = (
+ db.session.query(Slice)
+ .filter(Slice.id.in_(target_chart_ids))
+ .all()
+ if target_chart_ids
+ else []
+ )
+ # Flush eagerly so the M2M rows land in
+ # ``dashboard_slices`` before any subsequent
+ # autoflush fires an inner-flush event handler
+ # that would reset the relationship change.
+ db.session.flush()
+ elif target_chart_ids:
+ # Append only the new associations to existing ones.
+ new_slices = (
+ db.session.query(Slice)
+ .filter(Slice.id.in_(target_chart_ids))
Review Comment:
**Suggestion:** The new relationship reassignment loads `dashboard.slices`
through the default `Slice` visibility filter, so soft-deleted charts currently
attached to the dashboard are omitted from the baseline and then dropped on
flush when you assign a new list. This causes unintended deletion of existing
`dashboard_slices` links during import. Preserve/append associations under a
visibility-filter bypass (or use insert-only logic) so hidden existing links
are not removed. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Restoring soft-deleted charts loses dashboard membership after import.
- ⚠️ YAML dashboard imports can mutate dashboard-chart links unexpectedly.
- ⚠️ Soft-delete restore contract for dashboards and charts is broken.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable soft delete by setting feature flag SOFT_DELETE to True in
`superset/config.py:669` and restart Superset so the `SoftDeleteMixin`
listener in
`superset/models/helpers.py:742-219` starts filtering `Slice` queries by
`deleted_at IS
NULL`.
2. Create a dashboard and attach two charts (Slice records) to it via normal
UI or DAO
calls so `dashboard_slices` contains rows for both charts, as defined in
`superset/models/dashboard.py:96-111` and the `Dashboard.slices`
relationship at
`superset/models/dashboard.py:197-199`.
3. Soft-delete one of the charts using the DAO delete path (for charts this
routes through
`BaseDAO.delete` in `superset/daos/base.py:20-27`), which with SOFT_DELETE
enabled calls
`soft_delete()` on `Slice` rather than removing rows; the `dashboard_slices`
junction row
remains, but the global visibility filter now hides this chart from ORM
queries.
4. Import a dashboard bundle that updates the same dashboard using
`ImportDashboardsCommand._import` in
`superset/commands/dashboard/importers/v1/__init__.py:71-79, 160-168,
197-213, 220-228`
with `overwrite=False`. During this call:
- Existing relationships are loaded directly from `dashboard_slices` via
Core at lines
160-168, so the soft-deleted chart’s `(dashboard_id, slice_id)` pair is
present in
`existing_relationships`.
- When resolving new membership, the code enters the non-overwrite branch
at lines
205-213 and 220-228: `new_slices` is loaded via
`db.session.query(Slice).filter(Slice.id.in_(target_chart_ids)).all()`,
and
`dashboard.slices` is read and reassigned as `dashboard.slices =
list(dashboard.slices)
+ new_slices` followed by `db.session.flush()` (lines 227-228). Both the
`dashboard.slices` lazy load and the `new_slices` query are subject to
the SoftDelete
visibility filter, so they omit the soft-deleted chart.
- As documented in `DashboardDAO.set_dash_metadata` at
`superset/daos/dashboard.py:171-197`, reassigning `dashboard.slices`
while the
soft-delete visibility filter is active causes hidden members to be
“silently dropped —
deleting [their] dashboard_slices junction row” when the unit of work
diffs the new
collection against the existing one. Because the importer reassigns
`dashboard.slices`
without using the documented `skip_visibility_filter(db.session, Slice)`
bypass (see
`superset/models/helpers.py:221-239` and its use at
`superset/daos/dashboard.py:179-203`), the soft-deleted chart’s
`dashboard_slices` row
is removed during this import, breaking the intended restore-reattach
behavior for
soft-deleted charts.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=28e7688064fd4a0ab939aa481c6f8d62&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=28e7688064fd4a0ab939aa481c6f8d62&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/importers/v1/__init__.py
**Line:** 227:228
**Comment:**
*Logic Error: The new relationship reassignment loads
`dashboard.slices` through the default `Slice` visibility filter, so
soft-deleted charts currently attached to the dashboard are omitted from the
baseline and then dropped on flush when you assign a new list. This causes
unintended deletion of existing `dashboard_slices` links during import.
Preserve/append associations under a visibility-filter bypass (or use
insert-only logic) so hidden existing links are not removed.
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%2F41075&comment_hash=a1a07f0fc30d0baae0251833f429b013837c3682d1f21ce5be161e0e47ef54b5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=a1a07f0fc30d0baae0251833f429b013837c3682d1f21ce5be161e0e47ef54b5&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]
