Skip to content

Commit 27757a1

Browse files
Mike Bridgeclaude
andcommitted
fix(dashboards): treat re-import of soft-deleted UUID as implicit restore (Option C)
Same shape as the charts and datasets importer fixes. The dashboard importer on this branch had three bugs from the soft-delete refactor: 1. **Null-user `config["id"]` fallthrough was lost.** The `if overwrite and can_write and user:` gate excluded the example-loader case (ignore_permissions=True, user=None) from id-preservation, so re-import over an existing live row would collide with the UUID unique index. 2. **Hard-delete-and-replace on soft-deleted overwrite.** The code called `clear_soft_deleted_for_import` for soft-deleted matches, cascading through dashboard_slices junctions and role/owner/tag associations. The import then had to reconstruct everything. 3. **Silent return of soft-deleted rows on non-overwrite.** The agent's "silent resurrection" concern — direct re-import returned the soft-deleted dashboard without restoring it, leaving the row hidden in the list. Adopts the Option-C shape from f4768c4 (charts) and 7b2aa39 (datasets): - Any mutation path (overwrite OR soft-delete restore) applies the overwrite path's ownership check (`can_access_dashboard` + owner-or- admin), so non-owners cannot resurrect via re-import. - Soft-deleted match + can_write: restore in place + UPDATE via `config["id"] = existing.id`. Preserves the PK and all relationship rows (slices junctions, owners, roles, tags). - Alive match + non-overwrite: returns existing unchanged (master behavior). - Null user path correctly falls through to the mutation path when `needs_mutation` is true, preserving the example-loader semantics. Four new tests pin the contracts: - test_import_soft_deleted_dashboard_overwrite_restores_in_place - test_import_soft_deleted_dashboard_non_overwrite_restores_for_owner - test_import_soft_deleted_dashboard_non_overwrite_raises_for_non_owner - test_import_soft_deleted_dashboard_ignore_permissions_restores_in_place This should clear the dashboard zip-import playwright failure on apache#40128. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent cfcc5a9 commit 27757a1

2 files changed

Lines changed: 167 additions & 15 deletions

File tree

  • superset/commands/dashboard/importers/v1
  • tests/unit_tests/dashboards/commands/importers/v1

superset/commands/dashboard/importers/v1/utils.py

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -252,33 +252,38 @@ def import_dashboard( # noqa: C901
252252
"can_write",
253253
"Dashboard",
254254
)
255-
from superset.commands.importers.v1.utils import (
256-
clear_soft_deleted_for_import,
257-
find_existing_for_import,
258-
)
255+
from superset.commands.importers.v1.utils import find_existing_for_import
259256

260257
user = get_user()
261258

262259
if existing := find_existing_for_import(Dashboard, config["uuid"]):
263-
if overwrite and can_write and user:
260+
is_soft_deleted = existing.deleted_at is not None
261+
# A re-import that matches a soft-deleted UUID is implicitly a
262+
# restore-with-overwrite: bringing the dashboard back by
263+
# uploading it again. Apply the same ownership check as the
264+
# explicit overwrite path so non-owners cannot resurrect via
265+
# re-import.
266+
needs_mutation = overwrite or is_soft_deleted
267+
if needs_mutation and can_write and user:
264268
if not security_manager.can_access_dashboard(existing) or (
265269
user not in existing.owners and not security_manager.is_admin()
266270
):
267271
raise ImportFailedError(
268272
"A dashboard already exists and user doesn't "
269273
"have permissions to overwrite it"
270274
)
271-
# Permission check passed. If the existing row is
272-
# soft-deleted, hard-delete it now so the fresh insert
273-
# below doesn't collide on the UUID unique constraint.
274-
# Destructive op happens *after* the permission check, not
275-
# as a side effect of the lookup.
276-
if existing.deleted_at is not None:
277-
clear_soft_deleted_for_import(existing)
278-
else:
279-
config["id"] = existing.id
280-
elif not overwrite or not can_write:
275+
if not needs_mutation or not can_write:
281276
return existing
277+
# Mutation path. Restore a soft-deleted match in place rather
278+
# than hard-delete-and-replace: a hard delete cascades through
279+
# dashboard_slices junctions and dashboard role / owner / tag
280+
# associations, breaking the relationships the import would then
281+
# need to reconstruct. Setting config["id"] = existing.id routes
282+
# import_from_dict through UPDATE, preserving the PK and the
283+
# dependent rows.
284+
if is_soft_deleted:
285+
existing.deleted_at = None
286+
config["id"] = existing.id
282287
elif not can_write:
283288
raise ImportFailedError(
284289
"Dashboard doesn't exist and user doesn't "

tests/unit_tests/dashboards/commands/importers/v1/import_test.py

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import copy
2020
from collections.abc import Generator
21+
from datetime import datetime
2122
from unittest.mock import patch
2223

2324
import pytest
@@ -245,6 +246,152 @@ def test_import_existing_dashboard_with_permission(
245246
mock_can_access_dashboard.assert_called_once_with(dashboard)
246247

247248

249+
def test_import_soft_deleted_dashboard_overwrite_restores_in_place(
250+
mocker: MockerFixture,
251+
session_with_data: Session,
252+
) -> None:
253+
"""
254+
Overwrite-importing a soft-deleted dashboard must restore the row in
255+
place rather than hard-delete-and-replace. A hard delete cascades
256+
through dashboard_slices junctions and role/owner/tag associations;
257+
in-place restore preserves them.
258+
"""
259+
mocker.patch.object(security_manager, "can_access", return_value=True)
260+
mocker.patch.object(
261+
security_manager, "can_access_dashboard", return_value=True
262+
)
263+
264+
existing = (
265+
session_with_data.query(Dashboard)
266+
.filter(Dashboard.uuid == dashboard_config["uuid"])
267+
.one_or_none()
268+
)
269+
assert existing is not None
270+
original_id = existing.id
271+
existing.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
272+
session_with_data.flush()
273+
274+
admin = User(
275+
first_name="Alice",
276+
last_name="Doe",
277+
email="adoe@example.org",
278+
username="admin",
279+
roles=[Role(name="Admin")],
280+
)
281+
282+
with override_user(admin):
283+
dashboard = import_dashboard(dashboard_config, overwrite=True)
284+
285+
assert dashboard.id == original_id
286+
assert dashboard.deleted_at is None
287+
288+
289+
def test_import_soft_deleted_dashboard_non_overwrite_restores_for_owner(
290+
mocker: MockerFixture,
291+
session_with_data: Session,
292+
) -> None:
293+
"""
294+
Non-overwrite re-import of a soft-deleted UUID is implicitly a
295+
restore-and-update: the user is bringing the dashboard back by
296+
uploading it again. The same ownership rule as the overwrite path
297+
applies, so an owner (or admin) succeeds without setting
298+
overwrite=True.
299+
"""
300+
mocker.patch.object(security_manager, "can_access", return_value=True)
301+
mocker.patch.object(
302+
security_manager, "can_access_dashboard", return_value=True
303+
)
304+
305+
existing = (
306+
session_with_data.query(Dashboard)
307+
.filter(Dashboard.uuid == dashboard_config["uuid"])
308+
.one_or_none()
309+
)
310+
assert existing is not None
311+
original_id = existing.id
312+
existing.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
313+
session_with_data.flush()
314+
315+
admin = User(
316+
first_name="Alice",
317+
last_name="Doe",
318+
email="adoe@example.org",
319+
username="admin",
320+
roles=[Role(name="Admin")],
321+
)
322+
323+
with override_user(admin):
324+
dashboard = import_dashboard(dashboard_config, overwrite=False)
325+
326+
assert dashboard.id == original_id
327+
assert dashboard.deleted_at is None
328+
329+
330+
def test_import_soft_deleted_dashboard_non_overwrite_raises_for_non_owner(
331+
mocker: MockerFixture,
332+
session_with_data: Session,
333+
) -> None:
334+
"""
335+
Non-overwrite re-import that would resurrect a soft-deleted dashboard
336+
must respect ownership: a non-owner without admin role cannot
337+
restore-via-import. Mirrors the explicit /restore endpoint's check.
338+
"""
339+
mocker.patch.object(security_manager, "can_access", return_value=True)
340+
mocker.patch.object(
341+
security_manager, "can_access_dashboard", return_value=True
342+
)
343+
344+
existing = (
345+
session_with_data.query(Dashboard)
346+
.filter(Dashboard.uuid == dashboard_config["uuid"])
347+
.one_or_none()
348+
)
349+
assert existing is not None
350+
existing.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
351+
session_with_data.flush()
352+
353+
non_owner = User(
354+
first_name="Bob",
355+
last_name="Roe",
356+
email="bob@example.org",
357+
username="bob",
358+
roles=[Role(name="Gamma")],
359+
)
360+
361+
with override_user(non_owner):
362+
with pytest.raises(ImportFailedError) as excinfo:
363+
import_dashboard(dashboard_config, overwrite=False)
364+
assert "permissions to overwrite" in str(excinfo.value)
365+
366+
367+
def test_import_soft_deleted_dashboard_ignore_permissions_restores_in_place(
368+
mocker: MockerFixture,
369+
session_with_data: Session,
370+
) -> None:
371+
"""
372+
The example loader path: ignore_permissions=True with no logged-in
373+
user. The needs_mutation gate must still trigger the overwrite path
374+
so config["id"] is preserved on the fallthrough, otherwise the
375+
example loader's re-import collides on the UUID unique index.
376+
"""
377+
existing = (
378+
session_with_data.query(Dashboard)
379+
.filter(Dashboard.uuid == dashboard_config["uuid"])
380+
.one_or_none()
381+
)
382+
assert existing is not None
383+
original_id = existing.id
384+
existing.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
385+
session_with_data.flush()
386+
387+
dashboard = import_dashboard(
388+
dashboard_config, overwrite=True, ignore_permissions=True
389+
)
390+
391+
assert dashboard.id == original_id
392+
assert dashboard.deleted_at is None
393+
394+
248395
def test_import_tag_logic_for_dashboards(session_with_schema: Session):
249396
contents = {
250397
"tags.yaml": yaml.dump(

0 commit comments

Comments
 (0)