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 1cd3a9c3cc513937e981d854baf26c50e5e64c83 Author: Mehmet Salih Yavuz <[email protected]> AuthorDate: Thu Oct 1 12:38:18 2026 +0300 feat(canvas): upgrade stored definitions and tolerate widget rule changes Stored definitions are upgraded to the current schema version on load through a chain of per-version upgrades; the next write stores the upgraded form and restarts the operation log, so clients on an older base reload. A definition newer than the server supports is refused with a 422 that says Superset needs upgrading. Widget rules (nesting, sizes, child layouts, filter roles) are enforced only on the nodes a write touches. Nodes saved under rules that have since tightened keep their stored placement and no longer block edits elsewhere on the canvas. --- superset/canvas/api.py | 3 ++ superset/canvas/definition/ops.py | 5 ++- superset/canvas/definition/render.py | 17 +++++++- superset/canvas/definition/upgrades.py | 74 ++++++++++++++++++++++++++++++++ superset/canvas/definition/validation.py | 45 +++++++++++++++---- superset/commands/canvas/apply_ops.py | 9 +++- superset/commands/canvas/create.py | 11 ++++- superset/daos/canvas.py | 16 ++++--- tests/unit_tests/canvas/api_test.py | 30 +++++++++++++ tests/unit_tests/canvas/ops_test.py | 50 +++++++++++++++++++++ tests/unit_tests/canvas/upgrades_test.py | 62 ++++++++++++++++++++++++++ 11 files changed, 301 insertions(+), 21 deletions(-) diff --git a/superset/canvas/api.py b/superset/canvas/api.py index 2c73de6e1fd..e365154663c 100644 --- a/superset/canvas/api.py +++ b/superset/canvas/api.py @@ -40,6 +40,7 @@ from superset.canvas.definition.schemas import ( Operation, ) from superset.canvas.definition.scopes import resolve_filter_scopes +from superset.canvas.definition.upgrades import DefinitionVersionError from superset.canvas.filters import ( CanvasAccessFilter, CanvasAllTextFilter, @@ -94,6 +95,8 @@ def _handle_canvas_errors( return self.response_422(message=ex.normalized_messages()) except (DefinitionConflictError, DefinitionInvalidError) as ex: return self.response(ex.status, **ex.to_payload()) + except DefinitionVersionError as ex: + return self.response_422(message=str(ex)) except ( CanvasCreateFailedError, CanvasUpdateFailedError, diff --git a/superset/canvas/definition/ops.py b/superset/canvas/definition/ops.py index 64fb3b10df0..e17e964cb19 100644 --- a/superset/canvas/definition/ops.py +++ b/superset/canvas/definition/ops.py @@ -277,4 +277,7 @@ def apply_operations( applied.append(applier.apply(op)) except ValueError as ex: raise OperationError(index, str(ex)) from ex - return normalize_definition(applier.canvas, rules, resolver), applied + # Widget rules are enforced on what these operations touched; the rest of + # the canvas keeps its stored placement even if a rule changed since. + touched = {touch.node_id for op in applied for touch in op.touched} + return normalize_definition(applier.canvas, rules, resolver, touched), applied diff --git a/superset/canvas/definition/render.py b/superset/canvas/definition/render.py index 49e9497df38..069ac2cc751 100644 --- a/superset/canvas/definition/render.py +++ b/superset/canvas/definition/render.py @@ -30,7 +30,8 @@ from __future__ import annotations from typing import Any -from superset_core.canvas import WidgetResolver +from pydantic import ValidationError +from superset_core.canvas import GridPlacement, WidgetResolver from superset.canvas.definition.grid import resolve_grid from superset.canvas.definition.registry import ( @@ -70,7 +71,8 @@ def render_context( placements: dict[str, dict[str, int]] = {} for children, columns in grids: - rects, _ = resolve_grid([nodes[c]["layout"] for c in children], columns) + layouts = [_grid_layout(nodes[c]["layout"]) for c in children] + rects, _ = resolve_grid(layouts, columns) for child, rect in zip(children, rects, strict=True): placements[child] = { "col": rect.col, @@ -83,3 +85,14 @@ def render_context( "placements": placements, "gridColumns": grid_columns, } + + +def _grid_layout(layout: dict[str, Any]) -> dict[str, Any]: + """A stored layout as grid placement; one kept from another layout model + after a container's rules changed is auto-placed instead.""" + try: + return GridPlacement.model_validate(layout).model_dump( + mode="json", by_alias=True, exclude_none=True + ) + except ValidationError: + return {} diff --git a/superset/canvas/definition/upgrades.py b/superset/canvas/definition/upgrades.py new file mode 100644 index 00000000000..1f63cc8b020 --- /dev/null +++ b/superset/canvas/definition/upgrades.py @@ -0,0 +1,74 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Upgrading stored definitions to the current schema version. + +Every change to the definition schema bumps ``DEFINITION_VERSION`` and adds an +upgrade from the previous version to ``UPGRADES``. Definitions are upgraded +when loaded; the next write stores the upgraded form. A definition newer than +this server understands is refused rather than read with a partial schema. +""" + +from __future__ import annotations + +import copy +from collections.abc import Callable, Mapping +from typing import Any + +from superset.canvas.definition.schemas import DEFINITION_VERSION + +Upgrade = Callable[[dict[str, Any]], dict[str, Any]] + +# Upgrades keyed by the version they upgrade from; each returns the definition +# at the next version. +UPGRADES: dict[int, Upgrade] = {} + + +class DefinitionVersionError(ValueError): + def __init__(self, version: object, current: int) -> None: + self.version = version + self.current = current + if isinstance(version, int) and version > current: + message = ( + f"This canvas was saved with definition version {version}; this " + f"Superset supports up to {current}. Upgrade Superset to open it." + ) + else: + message = f"Unsupported canvas definition version {version!r}" + super().__init__(message) + + +def upgrade_definition( + definition: dict[str, Any], + current: int = DEFINITION_VERSION, + upgrades: Mapping[int, Upgrade] | None = None, +) -> dict[str, Any]: + """Return ``definition`` at version ``current``, upgrading it if older.""" + upgrades = UPGRADES if upgrades is None else upgrades + version = definition.get("version", 1) + if not isinstance(version, int) or isinstance(version, bool): + raise DefinitionVersionError(version, current) + if version > current or version < 1: + raise DefinitionVersionError(version, current) + if version == current: + return definition + upgraded = copy.deepcopy(definition) + while version < current: + upgraded = upgrades[version](upgraded) + version += 1 + upgraded["version"] = version + return upgraded diff --git a/superset/canvas/definition/validation.py b/superset/canvas/definition/validation.py index d4f70617ea7..ec4b342a940 100644 --- a/superset/canvas/definition/validation.py +++ b/superset/canvas/definition/validation.py @@ -25,10 +25,16 @@ A node whose widget no longer exists, or whose container type is no longer registered (e.g. its extension was removed), is kept as an unresolved placeholder: it and its children stay as stored, so the rest of the canvas can still be edited. Whether a new widget may be placed is checked by the caller. + +Widget types' rules (nesting, sizes, child layouts, filter roles) can change +after a canvas is saved. With ``strict_nodes`` given, only those nodes are +checked against the rules; the others keep their stored placement, so a +tightened rule never blocks edits elsewhere. Tree integrity is always checked. """ from __future__ import annotations +from collections.abc import Iterable from dataclasses import dataclass from typing import Any @@ -81,10 +87,12 @@ class _Validator: raw: dict[str, Any], rules: LayoutRulesRegistry, resolver: WidgetResolver, + strict_nodes: Iterable[str] | None, ) -> None: self.raw = raw self.rules = rules self.resolver = resolver + self.strict = None if strict_nodes is None else set(strict_nodes) self.issues: list[Issue] = [] self.parents: dict[str, str] = {} self.node_rules: dict[str, type[CanvasLayoutRules]] = {} @@ -94,6 +102,9 @@ class _Validator: def fail(self, path: str, message: str) -> None: self.issues.append(Issue(path, message)) + def is_strict(self, node_id: str) -> bool: + return self.strict is None or node_id in self.strict + def run(self) -> dict[str, Any]: try: canvas = CanvasDefinition.model_validate(self.raw) @@ -179,7 +190,7 @@ class _Validator: def _check_nesting(self) -> None: for node_id, parent_id in self.parents.items(): rules = self.node_rules.get(node_id) - if rules is None: + if rules is None or not self.is_strict(node_id): continue accepted: frozenset[str] | None = None if parent_id == ROOT_ID: @@ -206,8 +217,9 @@ class _Validator: if filter_id not in nodes: self.fail(pointer(*path), f"unknown node {filter_id!r}") continue + strict = self.is_strict(filter_id) rules = self.node_rules.get(filter_id) - if rules is not None and not rules.is_filter: + if strict and rules is not None and not rules.is_filter: self.fail(pointer(*path), f"node {filter_id!r} is not a filter") for field in ("targets", "exclude"): for index, target in enumerate(scope[field]): @@ -216,7 +228,11 @@ class _Validator: self.fail( pointer(*path, field, index), f"unknown node {target!r}" ) - elif target_rules is not None and not target_rules.is_filterable: + elif ( + strict + and target_rules is not None + and not target_rules.is_filterable + ): self.fail( pointer(*path, field, index), f"node {target!r} cannot be filtered", @@ -255,16 +271,24 @@ class _Validator: model = GridPlacement if columns is not None else child_model assert model is not None # noqa: S101 layouts: list[dict[str, Any]] = [] + # Children whose stored layout no longer fits a changed rule, kept as + # stored and auto-placed when resolving the grid. + kept: set[str] = set() for child_id in children: layout_path = ("nodes", child_id, "layout") + strict = self.is_strict(child_id) try: layout = model.model_validate( doc["nodes"][child_id]["layout"] ).model_dump(mode="json", by_alias=True, exclude_none=True) except ValidationError as ex: - self.issues.extend(_pydantic_issues(layout_path, ex)) + if strict: + self.issues.extend(_pydantic_issues(layout_path, ex)) + continue + kept.add(child_id) + layouts.append({}) continue - if columns is not None: + if columns is not None and strict: for message in [ *span_errors(layout, columns), *self._size_errors(child_id, layout, columns), @@ -276,7 +300,8 @@ class _Validator: if columns is not None and len(layouts) == len(children): _, stored = resolve_grid(layouts, columns) for child_id, layout in zip(children, stored, strict=True): - doc["nodes"][child_id]["layout"] = layout + if child_id not in kept: + doc["nodes"][child_id]["layout"] = layout def _size_errors( self, node_id: str, layout: dict[str, Any], columns: int @@ -309,13 +334,15 @@ def normalize_definition( raw: dict[str, Any], rules: LayoutRulesRegistry | None = None, resolver: WidgetResolver | None = None, + strict_nodes: Iterable[str] | None = None, ) -> dict[str, Any]: """ Validate ``raw`` and return the normalized canvas to store. - Raises ``DefinitionValidationError`` listing every problem found, each with a - JSON-pointer path into the canvas. + ``strict_nodes`` limits the widget rule checks to those nodes; ``None`` + checks every node. Raises ``DefinitionValidationError`` listing every + problem found, each with a JSON-pointer path into the canvas. """ return _Validator( - raw, rules or layout_rules, resolver or get_widget_resolver() + raw, rules or layout_rules, resolver or get_widget_resolver(), strict_nodes ).run() diff --git a/superset/commands/canvas/apply_ops.py b/superset/commands/canvas/apply_ops.py index 9e4d1e7268b..790af1883b6 100644 --- a/superset/commands/canvas/apply_ops.py +++ b/superset/commands/canvas/apply_ops.py @@ -35,7 +35,7 @@ from superset.canvas.definition.ops import ( ) from superset.canvas.definition.registry import get_widget_resolver from superset.canvas.definition.render import render_context -from superset.canvas.definition.schemas import AddOp, Operation +from superset.canvas.definition.schemas import AddOp, DEFINITION_VERSION, Operation from superset.canvas.definition.scopes import resolve_filter_scopes from superset.canvas.definition.validation import ( DefinitionValidationError, @@ -111,7 +111,12 @@ class ApplyCanvasOperationsCommand(BaseCommand): self.validate() canvas = CanvasDAO.lock(self._canvas_id) current = canvas.revision + # Logged operations predate the stored definition's upgrade, so they + # can't be checked for overlap; the log restarts with this write. + upgrading = canvas.definition_version != DEFINITION_VERSION if self._base_revision != current: + if upgrading: + raise DefinitionConflictError(current, [], stale=True) self._check_overlap(current, named_touches(self._ops)) try: @@ -130,6 +135,8 @@ class ApplyCanvasOperationsCommand(BaseCommand): canvas.definition = json.dumps(definition) canvas.definition_version = definition["version"] canvas.revision = revision + if upgrading: + CanvasDAO.prune(canvas.id) CanvasDAO.log(canvas.id, revision, applied, get_user_id()) CanvasDAO.prune(canvas.id, revision - OP_LOG_RETAINED_REVISIONS) return ApplyResult( diff --git a/superset/commands/canvas/create.py b/superset/commands/canvas/create.py index 6b5b856c8ab..d02af52e913 100644 --- a/superset/commands/canvas/create.py +++ b/superset/commands/canvas/create.py @@ -25,6 +25,10 @@ from marshmallow import ValidationError from superset.canvas.definition.registry import get_widget_resolver from superset.canvas.definition.schemas import empty_definition +from superset.canvas.definition.upgrades import ( + DefinitionVersionError, + upgrade_definition, +) from superset.canvas.definition.validation import ( DefinitionValidationError, normalize_definition, @@ -61,7 +65,12 @@ class CreateCanvasCommand(CreateMixin, BaseCommand): def _validate_definition(self, exceptions: list[ValidationError]) -> None: raw = self._properties.pop("definition", None) or empty_definition() try: - definition = normalize_definition(raw) + definition = normalize_definition(upgrade_definition(raw)) + except DefinitionVersionError as ex: + exceptions.append( + ValidationError({"/version": [str(ex)]}, field_name="definition") + ) + return except DefinitionValidationError as ex: exceptions.append( ValidationError( diff --git a/superset/daos/canvas.py b/superset/daos/canvas.py index 069f66f3bbc..f4c6a810900 100644 --- a/superset/daos/canvas.py +++ b/superset/daos/canvas.py @@ -24,6 +24,7 @@ from sqlalchemy import func from superset import db from superset.canvas.definition.ops import AppliedOperation +from superset.canvas.definition.upgrades import upgrade_definition from superset.canvas.filters import CanvasAccessFilter from superset.daos.base import BaseDAO from superset.models.canvas import Canvas, CanvasOp @@ -46,7 +47,8 @@ class CanvasDAO(BaseDAO[Canvas]): @staticmethod def load(canvas: Canvas) -> dict[str, Any]: - return json.loads(canvas.definition) + """The stored definition, upgraded to the current schema version.""" + return upgrade_definition(json.loads(canvas.definition)) @staticmethod def ops_since(canvas_id: int, revision: int) -> list[CanvasOp]: @@ -88,9 +90,9 @@ class CanvasDAO(BaseDAO[Canvas]): ) @staticmethod - def prune(canvas_id: int, keep_after_revision: int) -> None: - """Drop logged operations at or before ``keep_after_revision``.""" - db.session.query(CanvasOp).filter( - CanvasOp.canvas_id == canvas_id, - CanvasOp.revision <= keep_after_revision, - ).delete(synchronize_session=False) + def prune(canvas_id: int, keep_after_revision: int | None = None) -> None: + """Drop logged operations at or before ``keep_after_revision``, or all.""" + query = db.session.query(CanvasOp).filter(CanvasOp.canvas_id == canvas_id) + if keep_after_revision is not None: + query = query.filter(CanvasOp.revision <= keep_after_revision) + query.delete(synchronize_session=False) diff --git a/tests/unit_tests/canvas/api_test.py b/tests/unit_tests/canvas/api_test.py index 4a09f277abd..18185df60d4 100644 --- a/tests/unit_tests/canvas/api_test.py +++ b/tests/unit_tests/canvas/api_test.py @@ -308,6 +308,36 @@ def test_changes_since_a_pruned_revision_are_stale( assert client.get(f"{URL}/changes?since=3").json["result"]["changes"] == [] +def test_definition_from_a_newer_superset_is_refused( + client: Any, full_api_access: None, canvas: Any, published: MagicMock +) -> None: + canvas.definition = json.dumps({**json.loads(canvas.definition), "version": 99}) + + for response in (client.get(URL), patch_ops(client, 1, add("chart-1"))): + assert response.status_code == 422 + assert "Upgrade Superset" in response.json["message"] + assert canvas.revision == 1 + + +def test_write_that_upgrades_the_definition_restarts_the_op_log( + client: Any, + full_api_access: None, + canvas: Any, + published: MagicMock, + session: Session, +) -> None: + patch_ops(client, 1, add("chart-1")) + # Stored by an older schema version. + canvas.definition_version = 0 + session.commit() + + assert patch_ops(client, 1, add("chart-2")).json["stale"] is True + assert patch_ops(client, 2, add("chart-2")).status_code == 200 + assert canvas.definition_version == 1 + assert client.get(f"{URL}/changes?since=1").status_code == 409 + assert client.get(f"{URL}/changes?since=2").status_code == 200 + + def test_schema_is_published( client: Any, full_api_access: None, widgets: FakeResolver ) -> None: diff --git a/tests/unit_tests/canvas/ops_test.py b/tests/unit_tests/canvas/ops_test.py index 85a0521609e..5449adb2733 100644 --- a/tests/unit_tests/canvas/ops_test.py +++ b/tests/unit_tests/canvas/ops_test.py @@ -114,6 +114,56 @@ def test_add_into_a_leaf_is_rejected() -> None: build(add("chart-1"), add("chart-2", parent="n0")) +def changed_rules(widget_type: str, **changes: Any) -> LayoutRulesRegistry: + """The test rules, with ``widget_type``'s rules changed after saving.""" + rules = canvas_rules() + base = rules.get(widget_type) + rules.unregister(widget_type) + rules.register(type(f"Changed{base.__name__}", (base,), changes)) + return rules + + +def apply_with( + rules: LayoutRulesRegistry, doc: dict[str, Any], *raw: dict[str, Any] +) -> dict[str, Any]: + return apply_operations( + doc, ops(*raw), rules=rules, resolver=FakeResolver(), new_id=sequential_ids() + )[0] + + +def test_a_tightened_size_rule_only_applies_to_touched_nodes() -> None: + doc = build(add("chart-1", layout={"colSpan": 24}), add("chart-2")) + rules = changed_rules("chart", max_col_span=12) + + result = apply_with( + rules, doc, {"op": "place", "id": "n1", "layout": {"colSpan": 6}} + ) + + assert result["nodes"]["n0"]["layout"]["colSpan"] == 24 + with pytest.raises(DefinitionValidationError, match="above the maximum of 12"): + apply_with(rules, doc, {"op": "place", "id": "n0", "layout": {"colSpan": 24}}) + + +def test_a_tightened_nesting_rule_only_applies_to_touched_nodes() -> None: + doc = build(add("group"), add("chart-1", parent="n0")) + rules = changed_rules("group", accepted_children=frozenset({"filter"})) + + result = apply_with(rules, doc, add("chart-2", id=GROUP_ID)) + + assert result["nodes"]["n0"]["children"] == ["n1"] + with pytest.raises(DefinitionValidationError, match="cannot be placed in group"): + apply_with(rules, doc, {"op": "move", "id": "n1", "parent": "n0", "index": 0}) + + +def test_a_layout_from_a_replaced_child_model_is_kept_as_stored() -> None: + doc = build(add("board"), add("chart-1", parent="n0", layout={"lane": "todo"})) + rules = changed_rules("board", child_layout_model=None, grid_columns=12) + + result = apply_with(rules, doc, add("chart-2", id=GROUP_ID)) + + assert result["nodes"]["n1"]["layout"] == {"lane": "todo"} + + def test_unresolved_container_keeps_its_children_but_takes_no_new_ones() -> None: doc = build(add("group"), add("chart-1", parent="n0"), add("chart-2")) # The group's widget type is no longer registered. diff --git a/tests/unit_tests/canvas/upgrades_test.py b/tests/unit_tests/canvas/upgrades_test.py new file mode 100644 index 00000000000..04d40d9c621 --- /dev/null +++ b/tests/unit_tests/canvas/upgrades_test.py @@ -0,0 +1,62 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from typing import Any + +import pytest + +from superset.canvas.definition.upgrades import ( + DefinitionVersionError, + upgrade_definition, +) + + +def add_title(definition: dict[str, Any]) -> dict[str, Any]: + return {**definition, "title": "untitled"} + + +def rename_title(definition: dict[str, Any]) -> dict[str, Any]: + definition["name"] = definition.pop("title") + return definition + + +UPGRADES = {1: add_title, 2: rename_title} + + +def test_current_definition_is_returned_as_is() -> None: + definition = {"version": 3} + + assert upgrade_definition(definition, current=3, upgrades=UPGRADES) is definition + + +def test_older_definition_runs_every_upgrade_in_order() -> None: + stored = {"version": 1, "nodes": {}} + + upgraded = upgrade_definition(stored, current=3, upgrades=UPGRADES) + + assert upgraded == {"version": 3, "nodes": {}, "name": "untitled"} + assert stored == {"version": 1, "nodes": {}} + + +def test_newer_definition_is_refused() -> None: + with pytest.raises(DefinitionVersionError, match="Upgrade Superset"): + upgrade_definition({"version": 4}, current=3, upgrades=UPGRADES) + + [email protected]("version", [0, "1", True, None]) +def test_malformed_version_is_refused(version: Any) -> None: + with pytest.raises(DefinitionVersionError, match="Unsupported"): + upgrade_definition({"version": version}, current=3, upgrades=UPGRADES)
