mikebridge commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3364361511
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -140,21 +143,116 @@ def import_dataset( # noqa: C901
force_data: bool = False,
ignore_permissions: bool = False,
) -> SqlaTable:
+ """Import a dataset from a config dict, handling existing matches.
+
+ Permission model for an existing UUID match:
+
+ +--------------+---------------+---------------------+-----------------+
+ | Existing row | overwrite arg | Caller has perms? | Outcome |
+ +==============+===============+=====================+=================+
+ | alive | False | (n/a) | return existing |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write + owner | UPDATE in place |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write + owner | restore + UPDATE|
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | not can_write | raise (Case B) |
+ +--------------+---------------+---------------------+-----------------+
+
+ Re-importing a soft-deleted UUID is implicitly a restore-with-update:
+ the user is bringing the dataset back by uploading it again. We apply
+ the same ownership check as the explicit overwrite path so non-owners
+ cannot resurrect via re-import, and we raise rather than silently
+ returning a soft-deleted row to callers without write permission
+ (which would let them reattach charts/dashboards to a deleted dataset).
+ """
can_write = ignore_permissions or security_manager.can_access(
"can_write",
"Dataset",
)
- existing =
db.session.query(SqlaTable).filter_by(uuid=config["uuid"]).first()
+ # `user` is None for background / example-loader paths (no Flask request
+ # user). Combined with ``can_write=True`` (typically from
+ # ``ignore_permissions=True``), the ownership check below is intentionally
+ # skipped because the caller has already established trust at the command
+ # level. This matches pre-existing overwrite behaviour but now also applies
+ # to soft-deleted matches via ``needs_mutation``.
user = get_user()
- if existing:
- if overwrite and can_write and user:
+ # Tracks whether we entered the soft-deleted mutation path so the
+ # downstream `sync` decision (below) can reflect that an
+ # implicit-restore re-import is a clean replacement, not a merge.
+ is_soft_deleted_match = False
+
+ if existing := find_existing_for_import(SqlaTable, config["uuid"]):
+ is_soft_deleted = existing.deleted_at is not None
+ needs_mutation = overwrite or is_soft_deleted
Review Comment:
Follow-up before merge (noting here so it isn't lost): this fuses two
distinct operations — *overwrite an alive dataset* vs. *restore a soft-deleted
one* — into a single `needs_mutation` flag, then re-discriminates them with
`'restore' if is_soft_deleted else 'overwrite'` ternaries and the `if
is_soft_deleted:` side-effect block below.
**Plan:** extract an `_ImportOp` enum (`CREATE` / `RETURN_EXISTING` /
`OVERWRITE` / `RESTORE`) and a pure, unit-testable
`_resolve_import_op(existing, overwrite, can_write, user)` that owns the
permission/ownership gate and returns the operation, keeping the restore
side-effects (dup-check, `deleted_at` snapshot/flush, `MultipleResultsFound`
rollback) gated by `op is _ImportOp.RESTORE`. Behavior-preserving except one
fix: an alive row with `overwrite=True` but no `can_write` currently raises the
wrong message (`"Dataset was deleted and re-import requires can_write…"`).
The same collapse exists in the charts (#40129) and dashboards (#40128)
importers — refactor all three together so the siblings stay consistent.
--
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]
