This is an automated email from the ASF dual-hosted git repository. msyavuz pushed a commit to branch msyavuz/feat/canvas in repository https://gitbox.apache.org/repos/asf/superset.git
commit 7dc9fcbaa7247319da3dd0b04b8dd12a56c7c357 Author: Mehmet Salih Yavuz <[email protected]> AuthorDate: Thu Oct 1 11:48:04 2026 +0300 feat(canvas): let callers choose node ids on add An add operation may carry an id, a canonical lowercase UUID not already on the canvas; otherwise the server generates one as before. Later operations in the same request can then reference the new node, so a container and its children are built in one atomic request, a UI can draw a node before the response, and a retried add fails instead of duplicating the node. --- superset/canvas/definition/ops.py | 6 ++++-- superset/canvas/definition/schemas.py | 13 ++++++++++-- tests/unit_tests/canvas/api_test.py | 24 ++++++++++++++++++++++ tests/unit_tests/canvas/ops_test.py | 38 ++++++++++++++++++++++++++++++++--- 4 files changed, 74 insertions(+), 7 deletions(-) diff --git a/superset/canvas/definition/ops.py b/superset/canvas/definition/ops.py index d3e37baa170..64fb3b10df0 100644 --- a/superset/canvas/definition/ops.py +++ b/superset/canvas/definition/ops.py @@ -67,7 +67,7 @@ class Touch: @dataclass class AppliedOperation: - # The operation as logged; ``add`` carries the node id the server assigned. + # The operation as logged; ``add`` always carries the new node's id. op: dict[str, Any] touched: list[Touch] = field(default_factory=list) @@ -179,7 +179,9 @@ class _Applier: def apply(self, op: Operation) -> AppliedOperation: logged = op.model_dump(mode="json", by_alias=True, exclude_none=True) if isinstance(op, AddOp): - node_id = self.new_id() + node_id = op.id or self.new_id() + if node_id in self.nodes: + raise ValueError(f"node {node_id!r} already exists") self.insert(self.children(op.parent), node_id, op.index) self.nodes[node_id] = {"widget": op.widget, "layout": op.layout} return AppliedOperation( diff --git a/superset/canvas/definition/schemas.py b/superset/canvas/definition/schemas.py index b5f9b87df8e..feac87dd612 100644 --- a/superset/canvas/definition/schemas.py +++ b/superset/canvas/definition/schemas.py @@ -18,7 +18,7 @@ The dashboard canvas and the operations that change it. A canvas is where a dashboard's widgets sit: a flat ``nodes`` map keyed by -server-generated ids, with the tree expressed through ``children`` id lists. +UUID node ids, with the tree expressed through ``children`` id lists. A node references a widget by id and holds only its placement; widget configuration lives with the widget. The root is not a node: it always exists, is always a grid, and is addressed as ``"root"`` in operations. @@ -34,6 +34,9 @@ from pydantic.alias_generators import to_camel DEFINITION_VERSION = 1 ROOT_ID = "root" +# Caller-chosen node ids must be canonical lowercase UUIDs, like the ones the +# server generates, so every reference to a node spells it the same way. +NODE_ID_PATTERN = r"^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$" class _Model(BaseModel): @@ -107,9 +110,15 @@ def empty_definition() -> dict[str, Any]: class AddOp(_Model): - """Place a widget under ``parent``; the server assigns the node id.""" + """ + Place a widget under ``parent``. + + The node id is the caller's ``id`` when given, so later operations in the + same request can reference the new node; otherwise the server assigns one. + """ op: Literal["add"] + id: str | None = Field(default=None, pattern=NODE_ID_PATTERN) widget: str = Field(min_length=1, max_length=64) layout: dict[str, Any] = Field(default_factory=dict) parent: str = ROOT_ID diff --git a/tests/unit_tests/canvas/api_test.py b/tests/unit_tests/canvas/api_test.py index 0445f8b875f..4a09f277abd 100644 --- a/tests/unit_tests/canvas/api_test.py +++ b/tests/unit_tests/canvas/api_test.py @@ -193,6 +193,30 @@ def test_write_from_the_future_is_stale( assert response.json["stale"] is True +def test_caller_ids_let_one_request_build_a_nested_layout( + client: Any, full_api_access: None, canvas: Any, published: MagicMock +) -> None: + group_id = "6f1c2b7e-2d4a-4c1e-9a53-0f3b8d2e7a10" + + response = patch_ops( + client, + 1, + add("group", id=group_id), + add("chart-1", parent=group_id), + add("chart-2", parent=group_id), + ) + + assert response.status_code == 200 + assert response.json["result"]["revision"] == 2 + assert response.json["result"]["ops"][0]["id"] == group_id + nodes = json.loads(canvas.definition)["nodes"] + assert len(nodes[group_id]["children"]) == 2 + + retried = patch_ops(client, 2, add("group", id=group_id)) + assert retried.status_code == 422 + assert canvas.revision == 2 + + def test_invalid_operation_reports_its_index( client: Any, full_api_access: None, canvas: Any, published: MagicMock ) -> None: diff --git a/tests/unit_tests/canvas/ops_test.py b/tests/unit_tests/canvas/ops_test.py index c8eb6bf189e..85a0521609e 100644 --- a/tests/unit_tests/canvas/ops_test.py +++ b/tests/unit_tests/canvas/ops_test.py @@ -77,6 +77,38 @@ def test_add_into_a_container_added_in_the_same_batch() -> None: assert result["nodes"]["n0"]["children"] == ["n2", "n1"] +GROUP_ID = "6f1c2b7e-2d4a-4c1e-9a53-0f3b8d2e7a10" + + +def test_add_takes_a_caller_id_that_later_ops_can_reference() -> None: + result, applied = apply( + empty_definition(), + add("group", id=GROUP_ID), + add("chart-1", parent=GROUP_ID), + {"op": "place", "id": GROUP_ID, "layout": {"colSpan": 12}}, + ) + + assert result["root"]["children"] == [GROUP_ID] + assert result["nodes"][GROUP_ID]["children"] == ["n0"] + assert result["nodes"][GROUP_ID]["layout"]["colSpan"] == 12 + assert applied[0].op["id"] == GROUP_ID + + +def test_add_rejects_an_id_already_in_use() -> None: + doc = build(add("group", id=GROUP_ID)) + + with pytest.raises(OperationError, match="already exists"): + apply(doc, add("chart-1", id=GROUP_ID)) + + [email protected]( + "node_id", ["root", "n0", GROUP_ID.upper(), GROUP_ID.replace("-", "")] +) +def test_add_rejects_ids_that_are_not_canonical_uuids(node_id: str) -> None: + with pytest.raises(ValueError, match="should match pattern"): + ops(add("chart-1", id=node_id)) + + def test_add_into_a_leaf_is_rejected() -> None: with pytest.raises(OperationError, match="'n0' cannot hold children"): build(add("chart-1"), add("chart-2", parent="n0")) @@ -147,11 +179,11 @@ def test_place_updates_layout_and_resolves_collisions() -> None: def test_all_operations_apply_or_none_do() -> None: - doc = build(add("chart-1")) + doc = build(add("chart-1", id=GROUP_ID)) with pytest.raises(DefinitionValidationError): - apply(doc, add("chart-2"), add("missing-1")) - assert list(doc["nodes"]) == ["n0"] + apply(doc, add("chart-2"), add("tab")) + assert list(doc["nodes"]) == [GROUP_ID] def test_unknown_node() -> None:
