fitzee commented on code in PR #45472:
URL: https://github.com/apache/superset/pull/45472#discussion_r4321912194
##########
superset/daos/dashboard.py:
##########
@@ -857,7 +857,10 @@ def copy_dashboard(
]
)
value["meta"]["chartId"] = new_id
- else:
+ elif metadata.get("positions") is None:
+ # With a layout, set_dash_metadata links exactly the layout's
charts.
+ # Linking the source's charts first and then dropping some makes
the
+ # flush delete dashboard_slices rows that were never inserted.
dash.slices = original_dash.slices
dash.params = original_dash.params
Review Comment:
Non-blocking, pre-existing. `set_dash_metadata` starts from `dash.params`,
which is the source's *current* metadata here, and only overlays the keys in
the payload. For a version fork, any key the old snapshot lacks comes from
today's dashboard. Keys that are carried through also aren't pruned to the
layout's charts. With a layout that keeps only chart A, `chart_configuration`,
`timed_refresh_immune_slices` and `expanded_slices` entries for the omitted
chart B survive in the copy's `json_metadata`. Only `filter_scopes` and
`default_filters` are filtered by `slice_ids`. The frontend seems to tolerate
dangling cross-filter entries, but the fork isn't a faithful copy of the
version.
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -5381,6 +5382,96 @@ def
test_copy_dashboard_layout_error_returns_422_without_writes(
db.session.delete(other)
db.session.commit()
+ @parameterized.expand([(True, False), (False, False), (True, True),
(False, True)])
+ def test_copy_dashboard_layout_without_current_charts(
+ self, keep_first: bool, duplicate_slices: bool
+ ) -> None:
+ """Copy a layout that omits charts the source has, as a version fork
does."""
+ from superset.connectors.sqla.models import SqlaTable
+
+ admin: User = self.get_user(ADMIN_USERNAME)
+ dataset_id: int = (
+ db.session.query(SqlaTable.id).order_by(SqlaTable.id).first()[0]
+ )
+ first: Slice = self.insert_chart("copy-subset-first", [admin.id],
dataset_id)
+ second: Slice = self.insert_chart("copy-subset-second", [admin.id],
dataset_id)
+ dashboard: Dashboard = self.insert_dashboard(
+ "copy-subset-source", None, [admin.id], slices=[first, second]
+ )
+ source_members: set[int] = {first.id, second.id}
+ positions: dict[str, Any] = (
+ {
+ "CHART-first": {
+ "id": "CHART-first",
+ "type": "CHART",
+ "meta": {"chartId": first.id, "width": 4, "height": 50},
+ }
+ }
+ if keep_first
+ else {}
+ )
+ self.login(ADMIN_USERNAME)
+ copied_id: int | None = None
+ try:
+ response: TestResponse = self.client.post(
+ f"/api/v1/dashboard/{dashboard.id}/copy/",
+ json={
+ "dashboard_title": "copy-subset-result",
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps({"positions": positions}),
+ },
+ )
+ assert response.status_code == 200, response.data
+ copied_id = response.json["result"]["id"]
+ db.session.expire_all()
+ copied: Dashboard = db.session.get(Dashboard, copied_id)
+ expected: set[str] = {"copy-subset-first"} if keep_first else set()
+ assert {chart.slice_name for chart in copied.slices} == expected
+ assert {chart.id for chart in dashboard.slices} == source_members
+ finally:
+ db.session.rollback()
+ if copied_id is not None:
+ db.session.delete(db.session.get(Dashboard, copied_id))
+ db.session.delete(dashboard)
+ # Clones carry the source names, including ones the layout left
out.
Review Comment:
Non-blocking. This cleanup shows the side effect of the
`duplicate_slices=True` cases. The clone loop clones every non-archived member
of the source, whether or not the layout references it. Charts the layout
leaves out become orphan clones: owned by the caller, on no dashboard, and
recorded as added-then-removed in the new dashboard's association history. The
test only checks `copied.slices`, so it passes while leaving those orphans
behind. Skipping clones whose original ID isn't in the layout's chart IDs would
fix this. The remap and `filter_scopes` mapping already only use IDs from the
layout. At minimum, assert the orphan count so a later change to this behavior
is deliberate.
##########
superset/daos/dashboard.py:
##########
@@ -857,7 +857,10 @@ def copy_dashboard(
]
)
value["meta"]["chartId"] = new_id
- else:
+ elif metadata.get("positions") is None:
Review Comment:
Non-blocking, pre-existing. With this change the posted layout is the only
thing that decides the copy's membership. Nothing on the server checks that the
layout's chart IDs belong to the source or that the caller can access them. A
non-duplicate copy whose layout references a chart from an unrelated dashboard
links that chart to the new dashboard (confirmed in a SQLite DAO harness).
`createDashboardFromSnapshot` filters unreachable charts through
`fetchReachableChartIds`, but that runs in the browser and isn't access
control. `PUT` on a dashboard you own has the same gap, so this PR doesn't
widen it. That matters because dashboard membership feeds the `dashboardId`
datasource grant in `raise_for_access` (viewer-promiscuous / embedded paths).
It's worth a follow-up that limits layout chart IDs to the source's members,
plus charts from version history the caller can access.
--
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]
