EnxDev commented on code in PR #44307:
URL: https://github.com/apache/superset/pull/44307#discussion_r4176710061
##########
superset/commands/dashboard/importers/v1/__init__.py:
##########
@@ -88,6 +88,51 @@ def __init__(self, contents: dict[str, str], *args: Any,
**kwargs: Any) -> None:
self.overwrite_all = kwargs.pop("overwrite_all", False)
super().__init__(contents, *args, **kwargs)
+ def _prevent_overwrite_existing_model( # pylint: disable=invalid-name
+ self, exceptions: list[ValidationError]
+ ) -> None:
+ """Dashboards are also identified by ``slug`` on import.
+
+ ``import_dashboard()`` resolves a config carrying a fresh UUID but a
+ slug owned by an existing *active* dashboard onto that row, so the
+ overwrite gate must treat that collision like a UUID match: without
+ ``overwrite`` the import would silently merge the bundle's charts
+ into the slug-owning dashboard, and with ``overwrite`` it would
+ replace a dashboard the user never saw in the confirmation prompt
+ (which lists files, not UUIDs). The message matches the UUID branch
+ verbatim so the ImportModal's ``already exists`` detection keeps
+ working; soft-deleted owners are left to the restore path.
+ """
+ super()._prevent_overwrite_existing_model(exceptions)
+ if self.overwrite:
+ return
+ for file_name, config in self._configs.items():
+ slug = config.get("slug")
+ # ``slug is None`` rather than ``not slug``: ``import_dashboard()``
+ # resolves a slug collision with ``is not None``, so an
empty-string
+ # slug is an identity value there and must be gated here too.
+ if slug is None or not file_name.startswith(self.prefix):
+ continue
Review Comment:
If this UUID belongs to a soft-deleted dashboard, `import_dashboard()` takes
the restore path and never resolves by slug, but this branch still fires when a
live dashboard has claimed the old slug in the meantime. The user gets the
OVERWRITE prompt, confirms, and then hits the "cannot be restored via
re-import" error anyway, which is exactly the re-import case that restore check
was written for.
Could we skip the slug check whenever a row, live or soft-deleted, already
has this UUID? That also stops the second identical error when the UUID matches
one live dashboard and the slug another. Needs `find_existing_for_import`
imported from `superset.commands.importers.v1.utils`, and a soft-deleted test
case would pin it.
```suggestion
if slug is None or not file_name.startswith(self.prefix):
continue
# import_dashboard() only resolves by slug when no row, live or
# soft-deleted, already carries this UUID.
if find_existing_for_import(Dashboard, config["uuid"]) is not
None:
continue
```
--
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]
