EnxDev commented on code in PR #44307:
URL: https://github.com/apache/superset/pull/44307#discussion_r4066189040
##########
superset/commands/dashboard/importers/v1/__init__.py:
##########
@@ -75,6 +75,48 @@ 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")
+ if not slug or not file_name.startswith(self.prefix):
+ continue
Review Comment:
I checked this case against the PR head with a focused test: seed an active
dashboard with `slug=""`, then validate a bundle with a fresh UUID, `slug=""`,
and `overwrite=False`. Validation succeeds, and `import_dashboard()` returns
the seeded dashboard and rewrites the config UUID to match it. So this still
misses the confirmation this PR is adding.
Could we change the guard to `slug is None` and add an empty-string
collision case alongside the nonempty-slug test? `ImportV1DashboardSchema`
accepts the empty string, and the import lookup explicitly checks `is not
None`. I verified that the regression test fails on this head and passes with
that one-line change; the existing collision tests still pass. This would keep
validation consistent with the import lookup while preserving the
`None`/missing-slug behavior.
--
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]
