diff --git a/docs/admin_docs/configuration/mcp-server.mdx b/docs/admin_docs/configuration/mcp-server.mdx index 9c827bcacbdc..7551d5efe055 100644 --- a/docs/admin_docs/configuration/mcp-server.mdx +++ b/docs/admin_docs/configuration/mcp-server.mdx @@ -426,6 +426,88 @@ writes to the existing chart commands and does not bypass that prerequisite or chart edit permissions. Link generation alone does not establish that a chart was saved or that a provider query executed successfully. +## Canvas authoring and recovery + +Canvas tools require the `CANVAS` feature flag and the caller's Canvas permissions. +`create_canvas` with omitted, `null`, or empty `ops` creates an empty canvas. +Supplying initial operations intentionally publishes their content in that call; +if an initial operation fails, the new canvas and widgets are not retained. +To stage changes privately, create a canvas, call `create_canvas_draft`, and use +the returned token and draft revision with `apply_canvas_draft_ops`. Only an +explicit `commit_canvas_draft` publishes those draft changes. + +The native tool input schemas and `search_tools` results configured with +`MCP_TOOL_SEARCH_CONFIG["include_schemas"] = True` expose the canonical operation +grammar: `add`, `move`, `remove`, `set_props`, `place`, `set_scope`, and +`set_settings`, selected by the required `op` field. Arguments remain flat +(`token`, `revision`, `ops` for draft operations), +and operation fields use camelCase. Draft operation batches must be nonempty. +Native compact descriptions retain the input schema. Search results with +`include_schemas=False` deliberately omit schemas; flat Canvas tools have no +separate `request` instructions channel. Retrieve the full schema before +authoring operations, rather than relying on a truncated description. + +An `add` operation supplies exactly one of `widgetId` (an existing widget) or +`widgetType` (a new widget). New-widget fields such as `props` and `name` cannot +be combined with `widgetId`. Use `get_widget_control_schema` for the selected +widget type's props. Omitting `move.layout` preserves the existing placement +layout when its parent is unchanged; moving to another parent allows reflow. +`set_scope.scope` and `set_settings.value` are required fields that accept `null` +to reset the specified override. Explicit resets survive a draft commit. + +Chart, table, and filter placements reference a `widgetId`. Their effective +content is in the response's `widgets` map, including staged draft content. +`set_props` replaces the entire props object. For a follow-up dimension edit, +read `get_canvas_draft`, copy the effective props, update `dataBinding.dimensions`, +and submit the complete replacement to preserve unrelated props. A new widget removed from all placements +before commit is not published; any surviving placement keeps that widget. + +Canvas error results add an optional `error_type` while retaining `error`, +validation `errors` with paths, and `operation` when an operation index is known. +Top-level MCP argument/transport errors, permission denials, missing canvases, and +unsupported definition versions retain their existing responses. + +| `error_type` | Meaning | Recovery | +| --- | --- | --- | +| `ValidationError` | Invalid canvas metadata, operations, or resulting definition. | Correct the reported fields; a rejected batch leaves the draft unchanged. | +| `DraftUnavailable` | The token is malformed, unknown, expired, committed, discarded, or owned by another caller. These cases are intentionally indistinguishable. | Obtain an authorized fresh draft if needed; do not infer a reason or replay automatically. | +| `DraftRevisionConflict` | The submitted draft revision differs from the loaded draft. | Reload the draft and base subsequent changes on its current revision. | +| `CanvasRevisionConflict` | The published canvas changed before a mutating commit or conflicted with initial operations. | Start a fresh draft after a rejected mutating commit. | +| `WidgetRevisionConflict` | A shared widget changed even though the canvas did not. `changed_widgets` identifies the conflicts and `canvas_changed` is `false`. | Start a fresh draft; the rejected commit publishes none of its changes. | + +Revision fields identify the state actually observed by the command: + +| Field | Meaning | +| --- | --- | +| `base_revision` | Published canvas revision from which the draft was forked. | +| `draft_revision` | Current loaded draft revision, when available. Use a successful draft response's `revision` for its next write. | +| `current_canvas_revision` | Published canvas revision observed at a canvas/widget conflict check, when available; never a widget revision. | +| `canvas_revision` | Published revision returned by a successful commit, including an empty draft close. | +| `revision` | Compatibility field: the draft revision for draft responses/revision conflicts, or the observed canvas revision for canvas/widget conflicts and direct canvas writes. Prefer the explicit fields when interpreting conflicts. | + +Unavailable revisions are omitted. Validation does not load a token merely to +enrich an error, and `DraftUnavailable` reveals no revisions. A stale draft +revision is checked before publishing or closing. For a mutating commit, a +canvas conflict takes precedence over any widget conflicts; missing widget IDs +must not be inferred from that response. An empty draft closes successfully +without writing or incrementing the canvas, even if the published canvas changed +since the fork, and reports the published revision observed by the command. +A draft whose changes cancel out, for example adding a new widget and then +removing it, is not empty: committing it publishes a new canvas revision, as +publishing the same operations without a draft would, and returns +`CanvasRevisionConflict` if the canvas changed since the fork. + +For ranked results, specify both ordering and a limit in `dataBinding`, for +example `"orderBy": [{"field": "total_amount", "descending": true}], "rowLimit": 5`. +Use a saved metric name or the equivalent ad hoc metric label as the sort field. +Ascending order selects the smallest aggregates. A limit alone does not imply +ordering. The canvas page and `preview_widget` execute the binding through the +same chart-data pipeline, including dataset access checks. Preview rows, columns, +and permitted query text are evidence of the query that ran; they do not establish +that the binding answers a natural-language request. Query text requires +`can_view_query` on `Dashboard`. Visual rendering is optional and separate from +query execution. + ## Quick Start Get the MCP server running locally and connect an AI client in three steps. diff --git a/docs/static/resources/openapi.json b/docs/static/resources/openapi.json index f76101877219..ab0c86da3109 100644 --- a/docs/static/resources/openapi.json +++ b/docs/static/resources/openapi.json @@ -17974,7 +17974,7 @@ }, "/api/v1/canvas/draft/commit": { "post": { - "description": "Replays the draft's operations on the canvas and deletes the draft. Rejected with 409 if the canvas changed since the draft started.", + "description": "Replays the draft's operations on the canvas and deletes the draft. Rejected with 409 for a stale draft revision, or if the canvas or a widget the draft changed was saved since the draft started. An empty draft closes without a write and returns the published revision observed by the command.", "responses": { "200": { "description": "The canvas revision after the commit" @@ -17983,7 +17983,7 @@ "$ref": "#/components/responses/404" }, "409": { - "description": "The draft or the canvas changed" + "description": "The draft, canvas, or a changed widget conflicts" }, "422": { "description": "An operation no longer applies" diff --git a/superset/canvas/api.py b/superset/canvas/api.py index 2d91075f530a..13d451a8f014 100644 --- a/superset/canvas/api.py +++ b/superset/canvas/api.py @@ -113,10 +113,11 @@ def wrapped(self: CanvasRestApi, *args: Any, **kwargs: Any) -> Response: return f(self, *args, **kwargs) except ( CanvasNotFoundError, - CanvasDraftNotFoundError, CanvasPlacementNotFoundError, ): return self.response_404() + except CanvasDraftNotFoundError as ex: + return self.response(404, message="Not found", error_type=ex.recovery_type) except CanvasForbiddenError: return self.response_403() except CanvasInvalidError as ex: @@ -853,14 +854,17 @@ def commit_draft(self) -> Response: summary: Commit a canvas draft description: >- Replays the draft's operations on the canvas and deletes the draft. - Rejected with 409 if the canvas changed since the draft started. + Rejected with 409 for a stale draft revision, or if the canvas or + a widget the draft changed was saved since the draft started. + An empty draft closes without a write and returns the published + revision observed by the command. responses: 200: description: The canvas revision after the commit 404: $ref: '#/components/responses/404' 409: - description: The draft or the canvas changed + description: The draft, canvas, or a changed widget conflicts 422: description: An operation no longer applies """ @@ -874,7 +878,7 @@ def commit_draft(self) -> Response: 200, result={ "canvasId": draft.canvas_id, - "revision": result.revision if result else draft.base_revision, + "revision": result.revision, }, ) diff --git a/superset/canvas/definition/ops.py b/superset/canvas/definition/ops.py index aafb45fc1f48..a396eb7370ea 100644 --- a/superset/canvas/definition/ops.py +++ b/superset/canvas/definition/ops.py @@ -248,7 +248,6 @@ def apply(self, op: Operation) -> AppliedOperation: removed = self.subtree(op.id) for node_id in removed: del self.nodes[node_id] - self._forget_unplaced() touched = [Touch(node_id, FieldGroup.TREE) for node_id in removed] return AppliedOperation( logged, @@ -262,6 +261,7 @@ def apply(self, op: Operation) -> AppliedOperation: self.node(op.id) overrides = self.scopes(op.kind) if op.scope is None: + logged["scope"] = None overrides.pop(op.id, None) else: overrides[op.id] = op.scope.model_dump( @@ -271,6 +271,7 @@ def apply(self, op: Operation) -> AppliedOperation: if isinstance(op, SetSettingsOp): settings = self.canvas.setdefault("settings", {}) if op.value is None: + logged["value"] = None settings.pop(op.key, None) else: settings[op.key] = op.value @@ -429,7 +430,7 @@ def widget_of(self, node_id: str) -> tuple[str, StoredWidget]: raise ValueError(f"{node_id!r} holds a widget that no longer exists") return widget_id, stored - def _forget_unplaced(self) -> None: + def forget_unplaced(self) -> None: """Drop widgets created here that no placement holds any more.""" placed = {node.get("widgetId") for node in self.nodes.values()} for widget_id in [ @@ -497,6 +498,7 @@ def apply_operations( resolver: WidgetResolver | None = None, new_id: Callable[[str, Container[str]], str] = new_placement_id, pending: dict[str, StoredWidget] | None = None, + keep_unplaced: bool = False, new_widget_id: Callable[[], str] = lambda: str(uuid4()), ) -> tuple[dict[str, Any], list[AppliedOperation]]: """ @@ -509,6 +511,8 @@ def apply_operations( Widgets the operations create or change are kept in ``pending``, keyed by UUID, which is updated only if every operation applies; the caller saves them with the canvas. ``pending`` may already hold a draft's widgets. + ``keep_unplaced`` retains new widgets needed to replay a draft's history; + otherwise new widgets without a final placement are dropped before saving. """ widget_types = get_widget_types() if widget_types is None else widget_types resolver = resolver or get_widget_resolver() @@ -535,6 +539,8 @@ def apply_operations( except DefinitionValidationError as ex: ex.operation = _culprit(ex, applied) raise + if not keep_unplaced: + applier.forget_unplaced() pending.clear() pending.update(applier.pending) return definition, applied diff --git a/superset/commands/canvas/apply_ops.py b/superset/commands/canvas/apply_ops.py index 80a6a766b477..9abf990ea780 100644 --- a/superset/commands/canvas/apply_ops.py +++ b/superset/commands/canvas/apply_ops.py @@ -112,13 +112,18 @@ def apply_or_raise( definition: dict[str, Any], ops: list[Operation], pending: dict[str, StoredWidget] | None = None, + *, + keep_unplaced: bool = False, ) -> tuple[dict[str, Any], list[AppliedOperation]]: """ Apply ``ops`` to a copy of ``definition``, as one atomic batch; widgets - they create or change are added to ``pending``. + they create or change are added to ``pending``. Drafts keep unplaced new + widgets privately so their complete operation history remains replayable. """ try: - return apply_operations(definition, ops, pending=pending) + return apply_operations( + definition, ops, pending=pending, keep_unplaced=keep_unplaced + ) except OperationError as ex: raise DefinitionInvalidError( str(ex), @@ -172,7 +177,9 @@ def _apply(self) -> ApplyResult: canvas = CanvasDAO.lock(self._canvas_id) current = canvas.revision if self._strict and self._base_revision != current: - raise DraftConflictError(current, canvas_changed=True) + raise DraftConflictError( + current, canvas_changed=True, current_canvas_revision=current + ) if self._base_revision != current: self._check_overlap(current, named_touches(self._ops)) @@ -265,7 +272,11 @@ def save_widgets(pending: dict[str, StoredWidget], canvas: Canvas) -> None: row.schema_version = widget.schema_version row.revision = widget.revision + 1 if changed: - raise DraftConflictError(canvas.revision, widgets=sorted(changed)) + raise DraftConflictError( + canvas.revision, + widgets=sorted(changed), + current_canvas_revision=canvas.revision, + ) db.session.flush() diff --git a/superset/commands/canvas/draft.py b/superset/commands/canvas/draft.py index 6593b6c21211..ede65d8d3370 100644 --- a/superset/commands/canvas/draft.py +++ b/superset/commands/canvas/draft.py @@ -53,7 +53,6 @@ from superset.commands.canvas.apply_ops import ( apply_or_raise, ApplyCanvasOperationsCommand, - ApplyResult, placement_issues, ) from superset.commands.canvas.exceptions import ( @@ -88,7 +87,8 @@ class CanvasDraft: # The operations applied so far that change the canvas; commit replays # them. ops: list[dict[str, Any]] = field(default_factory=list) - # Widgets created or changed so far, by UUID; commit saves them. + # Widgets created or changed so far, by UUID, including new widgets needed + # to replay earlier operations. Commit saves only new widgets still placed. widgets: dict[str, StoredWidget] = field(default_factory=dict) def resolver(self) -> PendingWidgetResolver: @@ -205,12 +205,14 @@ def __init__(self, token: str, revision: int, ops: list[Operation]) -> None: def run(self) -> tuple[CanvasDraft, list[dict[str, Any]]]: draft = load_draft(self._token) if self._revision != draft.revision: - raise DraftConflictError(draft.revision) + raise DraftConflictError(draft.revision, draft_revision=draft.revision) if issues := placement_issues(self._ops, draft.resolver()): raise DefinitionInvalidError( "The operations add widgets that cannot be placed", issues=issues ) - definition, applied = apply_or_raise(draft.definition, self._ops, draft.widgets) + definition, applied = apply_or_raise( + draft.definition, self._ops, draft.widgets, keep_unplaced=True + ) logged = [operation.op for operation in applied] draft.definition = definition draft.ops = [*draft.ops, *(a.op for a in applied if a.replay)] @@ -219,6 +221,13 @@ def run(self) -> tuple[CanvasDraft, list[dict[str, Any]]]: return draft, logged +@dataclass(frozen=True) +class CommitResult: + """The revision written by a commit, or observed while closing an empty draft.""" + + revision: int + + class CommitCanvasDraftCommand: """ Publish a draft: save its widgets and replay its operations on the canvas, @@ -230,19 +239,25 @@ def __init__(self, token: str, revision: int) -> None: self._token = token self._revision = revision - def run(self) -> ApplyResult | None: + def run(self) -> CommitResult: draft = load_draft(self._token) if self._revision != draft.revision: - raise DraftConflictError(draft.revision) - result = None + raise DraftConflictError(draft.revision, draft_revision=draft.revision) if draft.ops or draft.widgets: - result = ApplyCanvasOperationsCommand( - draft.canvas_id, - draft.base_revision, - OPERATIONS.validate_python(draft.ops), - strict=True, - widgets=draft.widgets, - ).run() + try: + applied = ApplyCanvasOperationsCommand( + draft.canvas_id, + draft.base_revision, + OPERATIONS.validate_python(draft.ops), + strict=True, + widgets=draft.widgets, + ).run() + except DraftConflictError as ex: + ex.draft_revision = draft.revision + raise + result = CommitResult(applied.revision) + else: + result = CommitResult(_editable_canvas(draft.canvas_id).revision) DeleteCanvasDraftCommand(self._token).run() return result diff --git a/superset/commands/canvas/exceptions.py b/superset/commands/canvas/exceptions.py index 549e27177f32..8b0ef1110f0b 100644 --- a/superset/commands/canvas/exceptions.py +++ b/superset/commands/canvas/exceptions.py @@ -61,9 +61,11 @@ class DefinitionConflictError(CommandException): """The write is based on a revision that later changes overlap.""" status = 409 + recovery_type = "CanvasRevisionConflict" def __init__(self, revision: int, conflicts: list[str], stale: bool) -> None: self.revision = revision + self.current_canvas_revision = revision self.conflicts = conflicts self.stale = stale message = ( @@ -74,15 +76,22 @@ def __init__(self, revision: int, conflicts: list[str], stale: bool) -> None: super().__init__(message) def to_payload(self) -> dict[str, Any]: + """The REST error body. Like the Canvas definition and draft responses + (``canvasId``, ``baseRevision``), its keys are camelCase, except + ``error_type``, named as in ``SupersetError.to_dict``.""" return { "message": str(self.message), "revision": self.revision, "conflicts": self.conflicts, "stale": self.stale, + "error_type": self.recovery_type, + "currentCanvasRevision": self.current_canvas_revision, } class CanvasDraftNotFoundError(ObjectNotFoundError): + recovery_type = "DraftUnavailable" + def __init__(self) -> None: super().__init__("Canvas draft") @@ -110,30 +119,45 @@ def __init__( revision: int, canvas_changed: bool = False, widgets: list[str] | None = None, + *, + draft_revision: int | None = None, + current_canvas_revision: int | None = None, ) -> None: self.revision = revision self.canvas_changed = canvas_changed self.widgets = widgets or [] + self.draft_revision = draft_revision + self.current_canvas_revision = current_canvas_revision if canvas_changed: + self.recovery_type = "CanvasRevisionConflict" message = _( "The canvas changed since this draft started; start a new draft." ) elif self.widgets: + self.recovery_type = "WidgetRevisionConflict" message = _( "Widgets this draft changes were changed since; start a new draft." ) else: + self.recovery_type = "DraftRevisionConflict" message = _("The draft changed since your revision; reload it.") super().__init__(message) def to_payload(self) -> dict[str, Any]: + """The REST error body: camelCase keys except ``error_type``, as for + ``DefinitionConflictError``. Revisions not observed are left out.""" payload: dict[str, Any] = { "message": str(self.message), "revision": self.revision, "canvasChanged": self.canvas_changed, + "error_type": self.recovery_type, } if self.widgets: payload["widgets"] = self.widgets + if self.draft_revision is not None: + payload["draftRevision"] = self.draft_revision + if self.current_canvas_revision is not None: + payload["currentCanvasRevision"] = self.current_canvas_revision return payload @@ -141,6 +165,7 @@ class DefinitionInvalidError(CommandException): """An operation could not apply, or its result is not a valid definition.""" status = 422 + recovery_type = "ValidationError" def __init__( self, @@ -155,6 +180,7 @@ def __init__( def to_payload(self) -> dict[str, Any]: payload: dict[str, Any] = { "message": str(self.message), + "error_type": self.recovery_type, "errors": [ {"path": issue.path, "message": issue.message} for issue in self.issues ], diff --git a/superset/mcp_service/canvas/schemas.py b/superset/mcp_service/canvas/schemas.py index 28751aa1b9fb..c337ce841875 100644 --- a/superset/mcp_service/canvas/schemas.py +++ b/superset/mcp_service/canvas/schemas.py @@ -15,14 +15,15 @@ # specific language governing permissions and limitations # under the License. -"""Response models for the canvas MCP tools.""" +"""Input and response models for the canvas MCP tools.""" from __future__ import annotations -from typing import Any +from typing import Annotated, Any, Literal from pydantic import ( BaseModel, + BeforeValidator, ConfigDict, Field, model_serializer, @@ -30,6 +31,36 @@ SerializerFunctionWrapHandler, ) +from superset.canvas.definition.schemas import DraftOperationsRequest, Operation + + +def _keep_raw_ops(value: Any) -> Any: + """Keep operation validation and its error response inside Canvas helpers.""" + return value + + +DraftOpsInput = Annotated[ + list[dict[str, Any]], + BeforeValidator( + _keep_raw_ops, + json_schema_input_type=DraftOperationsRequest.model_fields[ + "ops" + ].rebuild_annotation(), + ), +] +InitialOpsInput = Annotated[ + list[dict[str, Any]], + BeforeValidator(_keep_raw_ops, json_schema_input_type=list[Operation]), +] + +CanvasErrorType = Literal[ + "ValidationError", + "DraftUnavailable", + "DraftRevisionConflict", + "CanvasRevisionConflict", + "WidgetRevisionConflict", +] + class _Compact(BaseModel): """Leaves unset (``None``) fields out of the result, so agents don't pay @@ -94,6 +125,11 @@ class CanvasWriteResponse(_Compact): default=None, description="The initial operations applied, with their ids." ) error: str | None = None + error_type: CanvasErrorType | None = None + current_canvas_revision: int | None = Field( + default=None, + description="The published canvas revision observed at a conflict.", + ) errors: Any | None = None operation: int | None = Field( default=None, description="Index of the initial operation that failed." @@ -156,7 +192,20 @@ class CanvasDraftResponse(_Compact): default=None, description="The canvas revision the draft started from." ) revision: int | None = Field( - default=None, description="The draft's revision; send it with the next write." + default=None, + description="Legacy revision: the draft revision on draft responses and " + "draft conflicts, but the published canvas revision on canvas/widget " + "conflicts. Prefer draft_revision and current_canvas_revision.", + ) + draft_revision: int | None = Field( + default=None, + description="The draft revision observed by the command; send it with " + "the next draft write. Omitted when the command did not load a draft.", + ) + current_canvas_revision: int | None = Field( + default=None, + description="The published canvas revision observed at a conflict check; " + "not a widget revision or a later read of the canvas.", ) definition: dict[str, Any] | None = None ops: list[dict[str, Any]] | None = Field( @@ -179,13 +228,16 @@ class CanvasDraftResponse(_Compact): description="Commit was rejected because these widgets changed meanwhile.", ) canvas_revision: int | None = Field( - default=None, description="The canvas revision after a commit." + default=None, + description="The published revision returned by a successful commit " + "command, including an empty draft close without a published write.", ) canvas_changed: bool | None = Field( default=None, description="Commit was rejected because the canvas changed meanwhile.", ) error: str | None = None + error_type: CanvasErrorType | None = None errors: list[dict[str, Any]] | None = None operation: int | None = Field( default=None, description="Index of the operation that failed." diff --git a/superset/mcp_service/canvas/tool/apply_canvas_draft_ops.py b/superset/mcp_service/canvas/tool/apply_canvas_draft_ops.py index 5326a699bab2..39386c3e38a4 100644 --- a/superset/mcp_service/canvas/tool/apply_canvas_draft_ops.py +++ b/superset/mcp_service/canvas/tool/apply_canvas_draft_ops.py @@ -30,7 +30,7 @@ ApplyCanvasDraftOperationsCommand, load_draft, ) -from superset.mcp_service.canvas.schemas import CanvasDraftResponse +from superset.mcp_service.canvas.schemas import CanvasDraftResponse, DraftOpsInput from superset.mcp_service.canvas.tool.canvas_draft_common import describe, error_result @@ -44,6 +44,7 @@ def _apply_canvas_draft_ops_impl( except ValidationError as ex: return { "error": "Invalid operations", + "error_type": "ValidationError", "errors": [ {"path": request_pointer(error["loc"]), "message": error["msg"]} for error in ex.errors() @@ -93,42 +94,36 @@ def _apply_canvas_draft_ops_impl( ), ) def apply_canvas_draft_ops( - token: str, revision: int, ops: list[dict[str, Any]] + token: str, revision: int, ops: DraftOpsInput ) -> CanvasDraftResponse: - """Apply operations to a canvas draft, in order and atomically: on any - error nothing changes and the error names the operation and the path at - fault. Only the draft changes; ``commit_canvas_draft`` publishes it. - ``revision`` is the draft revision you last saw; the response carries - the next one. + """Apply operations to a canvas draft, in order and atomically. Only the + draft changes; ``commit_canvas_draft`` publishes it. Send the last + ``draft_revision`` as ``revision``; the response carries the next one. On + ``DraftRevisionConflict``, reload the draft before writing again. - Operations (placement ids are readable slugs; ``parent`` defaults to - ``"root"``): + On any error nothing changes, and the error names the operation and the + path at fault. ``ValidationError`` leaves the draft unchanged. - - ``{"op": "add", "widgetType": "", "props": {...}, - "layout": {...}, "parent": "", "index": n, "id": ""}`` - adds a widget; get its props schema with ``get_widget_control_schema``. - A chart, table or filter becomes a saved widget (``"name"`` optional), - created when the draft is published. Use ``"widgetId": ""`` - instead of ``widgetType``/``props`` to place an existing one. - - ``{"op": "set_props", "id": "", "props": {...}}`` replaces the - props of a placement's widget, on every canvas that places it. - - ``{"op": "place", "id": "", "layout": {"col", "row", "colSpan", - "rowSpan"}}`` moves or resizes within the parent's grid. + The ``ops`` schema defines each operation; it does not express these + rules: - - ``{"op": "move", "id": "", "parent": "", "index": n}`` - reparents or reorders. - - ``{"op": "remove", "id": ""}`` removes a placement and its children. - - ``{"op": "set_scope", "kind": "filter" | "crossFilter" | - "customization", "id": "", "scope": {"mode": "auto" | "global" | - "custom", "targets": [...], "exclude": [...]}}``; ``scope: null`` - restores auto (the filter's nearest scoping container). - - ``{"op": "set_settings", "key": "refresh" | "colors" | "display" | - "crossFilters", "value": {...}}``. + - ``add`` takes either ``widgetType`` with ``props`` for a new widget + (get its props schema with ``get_widget_control_schema``) or + ``widgetId`` to place an existing one, never both. A new chart, table + or filter becomes a saved widget when the draft is published. + - ``set_props`` replaces all props, on every canvas that places the + widget after commit. Read its effective props from + ``get_canvas_draft``'s ``widgets`` map and send the complete + replacement to preserve unrelated fields. + - ``scope: null`` on ``set_scope`` restores auto (the filter's nearest + scoping container) and ``value: null`` on ``set_settings`` resets that + section. Both are required, so omitting one is an error, not a reset. - Grid layouts are 1-based: ``col`` runs from 1 to the grid's columns (24 - on the root; ``grid_columns`` has each container's) and ``row`` starts - at 1. Leave ``col``/``row`` out to auto-place, and spans out for the - widget's default size. ``place`` takes the whole layout. + A ``layout`` is ``{"col", "row", "colSpan", "rowSpan"}`` on a 1-based + grid: ``col`` runs from 1 to the grid's columns (24 on the root; + ``grid_columns`` has each container's) and ``row`` starts at 1. Leave + ``col``/``row`` out to auto-place, and spans out for the widget's default + size. ``place`` takes the whole layout. Overlaps push the later placement (in ``children`` order) down; a ``place`` first moves its node ahead of whatever it lands on, so it wins diff --git a/superset/mcp_service/canvas/tool/canvas_draft_common.py b/superset/mcp_service/canvas/tool/canvas_draft_common.py index 2d66b8cde5f5..b0873b279076 100644 --- a/superset/mcp_service/canvas/tool/canvas_draft_common.py +++ b/superset/mcp_service/canvas/tool/canvas_draft_common.py @@ -29,6 +29,7 @@ DraftConflictError, ) from superset.daos.canvas import CanvasDAO +from superset.mcp_service.canvas.tool.canvas_ops_common import payload_result NOT_FOUND = "Draft not found: it expired, was committed or discarded, or isn't yours" @@ -46,6 +47,7 @@ def describe(draft: CanvasDraft, include_definition: bool) -> dict[str, Any]: "canvas_id": payload["canvasId"], "base_revision": payload["baseRevision"], "revision": payload["revision"], + "draft_revision": payload["revision"], "url": draft_url(draft), } if include_definition: @@ -57,20 +59,11 @@ def describe(draft: CanvasDraft, include_definition: bool) -> dict[str, Any]: def error_result(ex: Exception) -> dict[str, Any] | None: """The tool result for a draft command error, or None to re-raise.""" if isinstance(ex, CanvasDraftNotFoundError): - return {"error": NOT_FOUND} + return {"error": NOT_FOUND, "error_type": "DraftUnavailable"} if isinstance(ex, CanvasNotFoundError): return {"error": "Canvas not found, or no access to it"} if isinstance(ex, CanvasForbiddenError): return {"error": "Only the canvas' editors can draft changes to it"} - if isinstance(ex, DraftConflictError): - payload = ex.to_payload() - return { - "error": payload["message"], - "revision": payload["revision"], - "canvas_changed": payload["canvasChanged"], - "changed_widgets": payload.get("widgets"), - } - if isinstance(ex, DefinitionInvalidError): - payload = ex.to_payload() - return {"error": payload.pop("message"), **payload} + if isinstance(ex, (DraftConflictError, DefinitionInvalidError)): + return payload_result(ex) return None diff --git a/superset/mcp_service/canvas/tool/canvas_ops_common.py b/superset/mcp_service/canvas/tool/canvas_ops_common.py index 9ab4fcb59be4..d45f2ab8df3b 100644 --- a/superset/mcp_service/canvas/tool/canvas_ops_common.py +++ b/superset/mcp_service/canvas/tool/canvas_ops_common.py @@ -32,8 +32,27 @@ CanvasNotFoundError, DefinitionConflictError, DefinitionInvalidError, + DraftConflictError, ) +# How MCP results, which are snake_case, name the keys of a REST error body. +# The body's ``widgets`` lists changed widgets, while a draft result's +# ``widgets`` holds the saved widgets its placements use. +_RESULT_KEYS = { + "message": "error", + "canvasChanged": "canvas_changed", + "widgets": "changed_widgets", + "draftRevision": "draft_revision", + "currentCanvasRevision": "current_canvas_revision", +} + + +def payload_result( + ex: DefinitionConflictError | DefinitionInvalidError | DraftConflictError, +) -> dict[str, Any]: + """The tool result for a command error that has a REST error body.""" + return {_RESULT_KEYS.get(key, key): value for key, value in ex.to_payload().items()} + def _apply_canvas_ops_impl( canvas_id: int | str, base_revision: int, ops: list[dict[str, Any]] @@ -45,6 +64,7 @@ def _apply_canvas_ops_impl( except ValidationError as ex: return { "error": "Invalid operations", + "error_type": "ValidationError", "errors": [ {"path": request_pointer(error["loc"]), "message": error["msg"]} for error in ex.errors() @@ -61,6 +81,5 @@ def _apply_canvas_ops_impl( except DefinitionVersionError as ex: return {"error": str(ex)} except (DefinitionConflictError, DefinitionInvalidError) as ex: - payload = ex.to_payload() - return {"error": payload.pop("message"), **payload} + return payload_result(ex) return {"revision": result.revision, "ops": result.ops} diff --git a/superset/mcp_service/canvas/tool/commit_canvas_draft.py b/superset/mcp_service/canvas/tool/commit_canvas_draft.py index a28c4b052046..ddef02ce6a4a 100644 --- a/superset/mcp_service/canvas/tool/commit_canvas_draft.py +++ b/superset/mcp_service/canvas/tool/commit_canvas_draft.py @@ -40,7 +40,7 @@ def _commit_canvas_draft_impl(token: str, revision: int) -> dict[str, Any]: return { "canvas_id": draft.canvas_id, "url": canvas.url if canvas else None, - "canvas_revision": result.revision if result else draft.base_revision, + "canvas_revision": result.revision, } @@ -57,11 +57,19 @@ def _commit_canvas_draft_impl(token: str, revision: int) -> dict[str, Any]: ), ) def commit_canvas_draft(token: str, revision: int) -> CanvasDraftResponse: - """Publish a canvas draft: its changes land on the canvas as one revision - and the draft is deleted. Returns the canvas ``url`` and its new - ``canvas_revision``. Rejected (``canvas_changed``) if the canvas - changed since the draft started; start a new draft then. Only commit - when the person asks you to publish. + """Publish a canvas draft and delete it. Call this only when the person + asks to publish. On ``CanvasRevisionConflict`` or ``WidgetRevisionConflict``, + start a fresh draft; on ``DraftRevisionConflict``, reload the draft. + + Returns the canvas ``url`` and ``canvas_revision`` observed by the command. + An empty draft (one with no accepted operations) closes without a published + write or revision increment, even if the canvas changed. + + A mutating commit returns the ``error_type`` ``CanvasRevisionConflict`` if + the canvas changed, or ``WidgetRevisionConflict`` if a shared widget + changed. Widget conflicts can have ``canvas_changed=false``. + ``DraftUnavailable`` means the token cannot be used. No conflict publishes + partial changes. """ return CanvasDraftResponse.model_validate( _commit_canvas_draft_impl(token, revision) diff --git a/superset/mcp_service/canvas/tool/create_canvas.py b/superset/mcp_service/canvas/tool/create_canvas.py index 7e641b632bb1..0da07cb3e0c9 100644 --- a/superset/mcp_service/canvas/tool/create_canvas.py +++ b/superset/mcp_service/canvas/tool/create_canvas.py @@ -28,7 +28,7 @@ from superset.commands.canvas.create import CreateCanvasCommand from superset.commands.canvas.delete import DeleteCanvasCommand from superset.commands.canvas.exceptions import CanvasInvalidError -from superset.mcp_service.canvas.schemas import CanvasWriteResponse +from superset.mcp_service.canvas.schemas import CanvasWriteResponse, InitialOpsInput from superset.mcp_service.canvas.tool.canvas_ops_common import _apply_canvas_ops_impl @@ -50,9 +50,17 @@ def _create_canvas_impl( try: canvas = CreateCanvasCommand(CanvasPostSchema().load(data)).run() except ValidationError as ex: - return {"error": "Invalid canvas", "errors": ex.messages} + return { + "error": "Invalid canvas", + "error_type": "ValidationError", + "errors": ex.messages, + } except CanvasInvalidError as ex: - return {"error": "Invalid canvas", "errors": ex.normalized_messages()} + return { + "error": "Invalid canvas", + "error_type": "ValidationError", + "errors": ex.normalized_messages(), + } result: dict[str, Any] = { "id": canvas.id, "url": canvas.url, @@ -84,13 +92,13 @@ def create_canvas( title: str, slug: str | None = None, description: str | None = None, - ops: list[dict[str, Any]] | None = None, + ops: InitialOpsInput | None = None, ) -> CanvasWriteResponse: """Create a canvas, with you as its editor, and return its id, URL and revision. ``slug`` is the readable URL (``/canvas//``); without one the URL - uses the id. ``ops`` optionally builds the canvas in the same call, with + uses the id. ``ops`` optionally builds published content in the same call, with the operations ``apply_canvas_draft_ops`` takes (grid layouts are 1-based; omit ``col``/``row`` to auto-place): if any fails, nothing is created and the error names the operation at fault. diff --git a/superset/mcp_service/canvas/tool/get_canvas_draft.py b/superset/mcp_service/canvas/tool/get_canvas_draft.py index ebeb60430b82..df7c2f97fb34 100644 --- a/superset/mcp_service/canvas/tool/get_canvas_draft.py +++ b/superset/mcp_service/canvas/tool/get_canvas_draft.py @@ -59,9 +59,11 @@ def _get_canvas_draft_impl(token: str, include_resolved: bool) -> dict[str, Any] ), ) def get_canvas_draft(token: str, include_resolved: bool = False) -> CanvasDraftResponse: - """Read a canvas draft: its definition, its revision and the canvas - revision it started from. ``include_resolved`` adds resolved grid - positions and filter scopes, as on ``get_canvas``. + """Read a canvas draft: its definition, effective ``widgets`` props, + ``draft_revision`` and the canvas revision it started from. Send + ``draft_revision`` as ``revision`` with the next draft write. + ``include_resolved`` adds resolved grid positions and filter scopes, + as on ``get_canvas``. """ return CanvasDraftResponse.model_validate( _get_canvas_draft_impl(token, include_resolved) diff --git a/superset/mcp_service/canvas/tool/update_canvas.py b/superset/mcp_service/canvas/tool/update_canvas.py index 2c59995c94b6..114688ab3d87 100644 --- a/superset/mcp_service/canvas/tool/update_canvas.py +++ b/superset/mcp_service/canvas/tool/update_canvas.py @@ -46,9 +46,17 @@ def _update_canvas_impl( try: updated = UpdateCanvasCommand(canvas.id, CanvasPutSchema().load(changes)).run() except ValidationError as ex: - return {"error": "Invalid canvas", "errors": ex.messages} + return { + "error": "Invalid canvas", + "error_type": "ValidationError", + "errors": ex.messages, + } except CanvasInvalidError as ex: - return {"error": "Invalid canvas", "errors": ex.normalized_messages()} + return { + "error": "Invalid canvas", + "error_type": "ValidationError", + "errors": ex.normalized_messages(), + } except CanvasNotFoundError: return {"error": f"Canvas {canvas_id} not found, or no access to it"} except CanvasForbiddenError: diff --git a/tests/unit_tests/canvas/api_test.py b/tests/unit_tests/canvas/api_test.py index eeff755cefdf..5d443ff80088 100644 --- a/tests/unit_tests/canvas/api_test.py +++ b/tests/unit_tests/canvas/api_test.py @@ -218,6 +218,8 @@ def test_overlapping_stale_write_is_rejected( "revision": 3, "conflicts": [node_id], "stale": False, + "error_type": "CanvasRevisionConflict", + "currentCanvasRevision": 3, } @@ -417,3 +419,91 @@ def test_schema_is_published( assert by_id["filterbar"]["boundsFilterScope"] is False assert by_id["chart"]["filterable"] is True assert by_id["chart"]["schemaVersion"] == 1 + + +@pytest.mark.parametrize( + ( + "error_type", + "revision", + "canvas_changed", + "widgets", + "draft_revision", + "canvas_revision", + ), + [ + ("DraftRevisionConflict", 2, False, [], 2, None), + ("CanvasRevisionConflict", 8, True, [], 2, 8), + ("WidgetRevisionConflict", 7, False, ["changed-widget"], 2, 7), + ], +) +def test_draft_conflict_response_preserves_legacy_fields_and_revision_provenance( + client: Any, + full_api_access: None, + mocker: MockerFixture, + error_type: str, + revision: int, + canvas_changed: bool, + widgets: list[str], + draft_revision: int, + canvas_revision: int | None, +) -> None: + """REST keeps its status/envelope while exposing unambiguous conflicts, + with camelCase keys like the rest of the Canvas API.""" + from superset.commands.canvas.exceptions import DraftConflictError + + error = DraftConflictError( + revision, + canvas_changed=canvas_changed, + widgets=widgets, + draft_revision=draft_revision, + current_canvas_revision=canvas_revision, + ) + mocker.patch( + "superset.canvas.api.load_draft", + return_value=MagicMock(token="token"), # noqa: S106 + ) + command = mocker.patch("superset.canvas.api.CommitCanvasDraftCommand") + command.return_value.run.side_effect = error + + response = client.post( + f"{BASE}/draft/commit", + json={"revision": 1}, + headers={"X-Canvas-Draft": "token"}, + ) + + expected: dict[str, Any] = { + "message": str(error.message), + "revision": revision, + "canvasChanged": canvas_changed, + "error_type": error_type, + "draftRevision": draft_revision, + } + if widgets: + expected["widgets"] = widgets + if canvas_revision is not None: + expected["currentCanvasRevision"] = canvas_revision + assert response.status_code == 409 + assert response.json == expected + + +@pytest.mark.parametrize("unavailable", [True, False]) +def test_draft_and_canvas_not_found_responses_remain_distinct( + client: Any, full_api_access: None, mocker: MockerFixture, unavailable: bool +) -> None: + """Only unavailable draft tokens receive the draft recovery category.""" + from superset.commands.canvas.exceptions import ( + CanvasDraftNotFoundError, + CanvasNotFoundError, + ) + + error = CanvasDraftNotFoundError() if unavailable else CanvasNotFoundError() + mocker.patch("superset.canvas.api.load_draft", side_effect=error) + + response = client.get(f"{BASE}/draft", headers={"X-Canvas-Draft": "token"}) + + assert response.status_code == 404 + assert response.json == ( + {"message": "Not found", "error_type": "DraftUnavailable"} + if unavailable + else {"message": "Not found"} + ) diff --git a/tests/unit_tests/canvas/draft_test.py b/tests/unit_tests/canvas/draft_test.py index 59b7bb199082..7dce4e36fb4b 100644 --- a/tests/unit_tests/canvas/draft_test.py +++ b/tests/unit_tests/canvas/draft_test.py @@ -18,6 +18,7 @@ from __future__ import annotations +from collections.abc import Callable from typing import Any from unittest.mock import MagicMock @@ -28,6 +29,7 @@ from superset.commands.canvas.exceptions import ( CanvasDraftNotFoundError, CanvasForbiddenError, + CanvasNotFoundError, DefinitionInvalidError, DraftConflictError, ) @@ -116,6 +118,9 @@ def test_a_write_must_be_based_on_the_draft_revision(store: Store) -> None: with pytest.raises(DraftConflictError) as excinfo: ApplyCanvasDraftOperationsCommand(draft.token, 0, ops(ADD)).run() assert excinfo.value.to_payload()["revision"] == 1 + assert excinfo.value.to_payload()["draftRevision"] == 1 + assert excinfo.value.to_payload()["error_type"] == "DraftRevisionConflict" + assert "currentCanvasRevision" not in excinfo.value.to_payload() def test_draft_writes_are_validated_like_canvas_writes(store: Store) -> None: @@ -171,6 +176,40 @@ def test_losing_editorship_closes_the_draft(store: Store) -> None: with pytest.raises(CanvasForbiddenError): drafts.load_draft(draft.token) + assert store.entries + + +def test_a_missing_canvas_keeps_its_existing_not_found_error(store: Store) -> None: + """An owned token is not reclassified when its canvas disappears.""" + from superset.commands.canvas import draft as drafts + + draft = drafts.CreateCanvasDraftCommand(7).run() + dao: Any = drafts.CanvasDAO + dao.find_by_id_or_uuid.return_value = None + + with pytest.raises(CanvasNotFoundError): + drafts.load_draft(draft.token) + assert store.entries + + +@pytest.mark.parametrize("action", ["get", "apply", "commit"]) +def test_discarded_drafts_cannot_be_read_changed_or_committed( + store: Store, action: str +) -> None: + """A discarded token never reconstructs private or published state.""" + from superset.commands.canvas import draft as drafts + + draft = drafts.CreateCanvasDraftCommand(7).run() + drafts.DeleteCanvasDraftCommand(draft.token).run() + calls: dict[str, Callable[[], object]] = { + "get": lambda: drafts.load_draft(draft.token), + "apply": drafts.ApplyCanvasDraftOperationsCommand(draft.token, 0, ops(ADD)).run, + "commit": drafts.CommitCanvasDraftCommand(draft.token, 0).run, + } + + with pytest.raises(CanvasDraftNotFoundError): + calls[action]() + assert store.entries == {} def test_commit_replays_the_ops_strictly_and_deletes_the_draft( @@ -221,8 +260,34 @@ def test_committing_an_empty_draft_changes_nothing( command = mocker.patch(f"{DRAFT}.ApplyCanvasOperationsCommand") draft = CreateCanvasDraftCommand(7).run() - assert CommitCanvasDraftCommand(draft.token, 0).run() is None + assert CommitCanvasDraftCommand(draft.token, 0).run().revision == 3 command.assert_not_called() + assert store.entries == {} + + +def test_mutating_commit_keeps_its_revision_after_a_later_published_change( + store: Store, mocker: MockerFixture +) -> None: + """A response describes this commit, not a later published write.""" + from superset.commands.canvas import draft as drafts + from superset.commands.canvas.apply_ops import ApplyResult + + dao: Any = drafts.CanvasDAO + + def publish_then_advance() -> ApplyResult: + """Return this commit after another writer advances the canvas.""" + dao.find_by_id_or_uuid.return_value.revision = 5 + return ApplyResult(4, [], {}, {}) + + command = mocker.patch(f"{DRAFT}.ApplyCanvasOperationsCommand") + command.return_value.run.side_effect = publish_then_advance + draft = drafts.CreateCanvasDraftCommand(7).run() + drafts.ApplyCanvasDraftOperationsCommand(draft.token, 0, ops(ADD)).run() + + result = drafts.CommitCanvasDraftCommand(draft.token, 1).run() + + assert result.revision == 4 + assert store.entries == {} def test_drafts_are_stored_as_json_strings(store: Store) -> None: diff --git a/tests/unit_tests/canvas/placement_data_test.py b/tests/unit_tests/canvas/placement_data_test.py index f22d1037d41f..530288a8b416 100644 --- a/tests/unit_tests/canvas/placement_data_test.py +++ b/tests/unit_tests/canvas/placement_data_test.py @@ -16,17 +16,26 @@ # under the License. from __future__ import annotations +import sqlite3 from collections.abc import Iterator +from copy import deepcopy +from pathlib import Path +from types import SimpleNamespace from typing import Any from unittest.mock import MagicMock import pytest from pytest_mock import MockerFixture +from sqlalchemy import text from sqlalchemy.orm.session import Session from superset.canvas.data import PlacementData, query_context_payload, RuntimeFilter +from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn from superset.exceptions import SupersetSecurityException +from superset.models.canvas import Canvas +from superset.models.core import Database from superset.utils import json +from superset.widgets.models import Widget from tests.unit_tests.canvas.fixtures import FakeResolver pytestmark = pytest.mark.parametrize( @@ -191,3 +200,335 @@ def test_runtime_filters_narrow_only_the_bindings_dataset() -> None: assert payload["queries"][0]["filters"] == [ {"col": "country", "op": "IN", "val": ["FR"]} ] + + +ADHOC_SUM = { + "expressionType": "SIMPLE", + "aggregate": "SUM", + "column": {"column_name": "amount"}, + "label": "total_amount", +} +AGGREGATE_ROWS = [ + ("E", 1), + ("A", 7), + ("G", 18), + ("C", 3), + ("B", 35), + ("D", 27), + ("F", 14), + ("B", 29), + ("C", 8), + ("E", 4), + ("F", 34), + ("G", 23), + ("A", 11), + ("D", 12), + ("G", 19), + ("F", 42), + ("D", 31), + ("E", 5), + ("B", 36), + ("A", 22), + ("C", 9), +] + + +@pytest.fixture +def aggregate_table(session: Session, tmp_path: Path) -> SqlaTable: + """Persist shuffled input rows whose aggregate rank differs from row rank.""" + path = tmp_path / "canvas_aggregate_data.db" + with sqlite3.connect(path) as connection: + connection.execute( + "CREATE TABLE amounts (category TEXT, alternate_category TEXT, amount INT)" + ) + connection.executemany( + "INSERT INTO amounts VALUES (?, ?, ?)", + [ + (category, f"group_{category}", amount) + for category, amount in AGGREGATE_ROWS + ], + ) + + SqlaTable.metadata.create_all(session.get_bind()) # pylint: disable=no-member + table = SqlaTable( + table_name="amounts", + database=Database( + database_name="canvas_aggregates", sqlalchemy_uri=f"sqlite:///{path}" + ), + columns=[ + TableColumn(column_name="category", type="TEXT", groupby=True), + TableColumn(column_name="alternate_category", type="TEXT", groupby=True), + TableColumn(column_name="amount", type="INTEGER"), + ], + metrics=[SqlMetric(metric_name="total_amount", expression="SUM(amount)")], + cache_timeout=-1, + ) + session.add(table) + session.commit() + return table + + +@pytest.fixture +def aggregate_canvas( + aggregate_table: SqlaTable, session: Session, mocker: MockerFixture +) -> Canvas: + """A persisted canvas and widget that use the real database widget resolver.""" + from superset.canvas.definition.schemas import empty_definition + + widget = Widget( + widget_type="ag-grid-table", + name="Aggregated amounts", + props={ + "dataBinding": { + "datasetId": aggregate_table.id, + "metrics": ["total_amount"], + "dimensions": ["category"], + "orderBy": [{"field": "total_amount", "descending": True}], + "rowLimit": 5, + }, + "columnDefs": [{"field": "total_amount", "headerName": "Total amount"}], + }, + ) + session.add(widget) + session.flush() + definition = empty_definition() + definition["root"]["children"] = ["amounts"] + definition["nodes"] = {"amounts": {"widgetId": str(widget.uuid), "layout": {}}} + canvas = Canvas( + title="Aggregate queries", + slug="aggregate-queries", + definition=json.dumps(definition), + definition_version=1, + revision=1, + ) + session.add(canvas) + session.commit() + mocker.patch("superset.canvas.filters.security_manager.is_admin", return_value=True) + mocker.patch("superset.commands.canvas.apply_ops.publish_realtime") + return canvas + + +def published_state(session: Session) -> dict[str, Any]: + """Snapshot persisted Canvas/widget/dataset state around read or rejected calls.""" + return { + "canvases": [ + (row.id, row.title, row.definition, row.revision) + for row in session.query(Canvas).order_by(Canvas.id) + ], + "widgets": [ + (row.id, str(row.uuid), deepcopy(row.props), row.revision) + for row in session.query(Widget).order_by(Widget.id) + ], + "datasets": [ + (row.id, row.table_name, row.sql, tuple(row.column_names)) + for row in session.query(SqlaTable).order_by(SqlaTable.id) + ], + "metrics": [ + (row.id, row.metric_name, row.expression) + for row in session.query(SqlMetric).order_by(SqlMetric.id) + ], + } + + +@pytest.mark.parametrize("metric", ["total_amount", ADHOC_SUM], ids=["stored", "adhoc"]) +@pytest.mark.parametrize( + "descending, expected", + [ + (True, [("B", 100), ("F", 90), ("D", 70), ("G", 60), ("A", 40)]), + (False, [("E", 10), ("C", 20), ("A", 40), ("G", 60), ("D", 70)]), + (None, None), + ], + ids=["top-five", "bottom-five", "limit-without-order"], +) +@pytest.mark.parametrize( + "view_query", [True, False], ids=["query-visible", "query-hidden"] +) +def test_preview_and_page_execute_the_same_aggregate_query( + client: Any, + full_api_access: None, + aggregate_canvas: Canvas, + aggregate_table: SqlaTable, + session: Session, + mocker: MockerFixture, + can_view_query: MagicMock, + metric: str | dict[str, Any], + descending: bool | None, + expected: list[tuple[str, int]] | None, + view_query: bool, +) -> None: + """Execute both callers through real SQL without publishing changes.""" + from superset.commands.chart.data.get_data_command import ChartDataCommand + from superset.mcp_service.canvas.tool.preview_widget import _preview_widget_impl + + widget = session.query(Widget).one() + props = deepcopy(widget.props) + binding = props["dataBinding"] + binding["metrics"] = [deepcopy(metric)] + if descending is None: + binding.pop("orderBy") + else: + binding["orderBy"] = [{"field": "total_amount", "descending": descending}] + widget.props = props + session.commit() + before = published_state(session) + can_view_query.return_value = view_query + run = mocker.spy(ChartDataCommand, "run") + query = query_context_payload(binding)["queries"][0] + + assert query["metrics"] == [metric] + assert query["columns"] == ["category"] + assert query["row_limit"] == 5 + assert query["orderby"] == ( + [] if descending is None else [[metric, not descending]] + ) + + response = client.post( + f"/api/v1/canvas/{aggregate_canvas.id}/placement/amounts/data", json={} + ) + assert response.status_code == 200 + page = response.json["result"]["datasets"][0] + preview, image = _preview_widget_impl(aggregate_canvas.id, "amounts", False) + + assert "error" not in preview + assert image is None + assert run.call_count == 2 + assert preview["sample_rows"] == page["inline_data"] + assert preview["row_count"] == page["row_count"] <= 5 + assert preview["columns"] == ["category", "total_amount"] + if expected is not None: + assert page["inline_data"] == [ + {"category": category, "total_amount": total} + for category, total in expected + ] + if view_query: + assert preview["query"] == page["query"] + assert preview["query_language"] == page["language"] == "sql" + sql = page["query"].upper() + assert "SUM(AMOUNT)" in sql + if descending is None: + assert "ORDER BY" not in sql + else: + direction = "DESC" if descending else "ASC" + assert f"ORDER BY TOTAL_AMOUNT {direction}" in sql + else: + assert "query" not in preview + assert "query" not in page + assert published_state(session) == before + with aggregate_table.database.get_sqla_engine() as engine: + with engine.connect() as connection: + rows = connection.execute(text("SELECT category, amount FROM amounts")) + assert list(rows) == AGGREGATE_ROWS + + +def test_replaced_draft_dimension_queries_effective_widget_props( + client: Any, + full_api_access: None, + aggregate_canvas: Canvas, + session: Session, + mocker: MockerFixture, + can_view_query: MagicMock, +) -> None: + """A complete props replacement uses the draft binding and preserves other props.""" + from cachelib import SimpleCache + + from superset.commands.canvas.draft import CreateCanvasDraftCommand + from superset.mcp_service.canvas.tool.apply_canvas_draft_ops import ( + _apply_canvas_draft_ops_impl, + ) + from superset.mcp_service.canvas.tool.canvas_draft_common import describe + from superset.mcp_service.canvas.tool.get_canvas_draft import _get_canvas_draft_impl + from superset.mcp_service.canvas.tool.preview_widget import _preview_widget_impl + + mocker.patch( + "superset.commands.canvas.draft.cache_manager", + SimpleNamespace(explore_form_data_cache=SimpleCache()), + ) + mocker.patch("superset.commands.canvas.draft.get_user_id", return_value=5) + before = published_state(session) + draft = CreateCanvasDraftCommand(aggregate_canvas.id).run() + widget_id = draft.definition["nodes"]["amounts"]["widgetId"] + props = deepcopy( + describe(draft, include_definition=True)["widgets"][widget_id]["props"] + ) + props["dataBinding"]["dimensions"] = ["alternate_category"] + changed = _apply_canvas_draft_ops_impl( + draft.token, + draft.revision, + [{"op": "set_props", "id": "amounts", "props": props}], + ) + + assert "error" not in changed + assert changed["revision"] == draft.revision + 1 + effective = _get_canvas_draft_impl(draft.token, include_resolved=False) + assert effective["widgets"][widget_id]["props"]["columnDefs"] == props["columnDefs"] + response = client.post( + f"/api/v1/canvas/{aggregate_canvas.id}/placement/amounts/data", + json={}, + headers={"X-Canvas-Draft": draft.token}, + ) + assert response.status_code == 200 + page = response.json["result"]["datasets"][0] + preview, _ = _preview_widget_impl( + aggregate_canvas.id, "amounts", False, draft_token=draft.token + ) + + expected = [ + {"alternate_category": f"group_{category}", "total_amount": amount} + for category, amount in [("B", 100), ("F", 90), ("D", 70), ("G", 60), ("A", 40)] + ] + assert page["inline_data"] == preview["sample_rows"] == expected + assert page["query"] == preview["query"] + assert published_state(session) == before + + +@pytest.mark.parametrize( + "title, ops, error_path", + [ + ("", None, None), + ( + "Invalid canonical operation", + [ + {"op": "add", "id": "first", "widgetType": "ag-grid-table"}, + {"op": "remove"}, + {"op": "add", "id": "last", "widgetType": "markdown"}, + ], + "/ops/1/id", + ), + ( + "Invalid middle operation", + [ + {"op": "add", "id": "first", "widgetType": "ag-grid-table"}, + {"op": "remove", "id": "missing"}, + {"op": "add", "id": "last", "widgetType": "markdown"}, + ], + "/ops/1", + ), + ], + ids=["metadata", "canonical-operations", "definition-operations"], +) +def test_initial_create_failure_leaves_no_persisted_objects( + aggregate_canvas: Canvas, + aggregate_table: SqlaTable, + session: Session, + title: str, + ops: list[dict[str, Any]] | None, + error_path: str | None, +) -> None: + """Failed initial operations retain neither the new canvas nor staged widgets.""" + from superset.mcp_service.canvas.tool.create_canvas import _create_canvas_impl + + before = published_state(session) + operations = deepcopy(ops) + if operations: + operations[0]["props"] = { + "dataBinding": { + "datasetId": aggregate_table.id, + "metrics": ["total_amount"], + } + } + result = _create_canvas_impl(title, None, None, operations) + + assert "error" in result + if error_path is not None: + assert any(error["path"] == error_path for error in result["errors"]) + assert published_state(session) == before diff --git a/tests/unit_tests/canvas/widget_entities_test.py b/tests/unit_tests/canvas/widget_entities_test.py index 5e82ddfd3ff2..8f92fd17232f 100644 --- a/tests/unit_tests/canvas/widget_entities_test.py +++ b/tests/unit_tests/canvas/widget_entities_test.py @@ -19,16 +19,18 @@ from __future__ import annotations from collections.abc import Iterator +from types import SimpleNamespace from typing import Any from uuid import UUID import pytest +from cachelib import SimpleCache from pytest_mock import MockerFixture from sqlalchemy.orm.session import Session from superset_core.canvas import StoredWidget from superset.utils import json -from tests.unit_tests.canvas.fixtures import FakeResolver +from tests.unit_tests.canvas.fixtures import FakeResolver, ops pytestmark = pytest.mark.parametrize( "app", [{"FEATURE_FLAGS": {"CANVAS": True}}], indirect=True @@ -73,6 +75,53 @@ def saved_widget(session: Session, widget_id: str) -> Any: return session.query(Widget).filter(Widget.uuid == UUID(widget_id)).one() +@pytest.fixture +def draft_cache(mocker: MockerFixture) -> SimpleCache: + """Keep draft state isolated while using the real widget database.""" + cache = SimpleCache() + mocker.patch( + "superset.commands.canvas.draft.cache_manager", + SimpleNamespace(explore_form_data_cache=cache), + ) + mocker.patch("superset.commands.canvas.draft.get_user_id", return_value=5) + return cache + + +def publish_elsewhere(canvas: Any) -> None: + """Publish a change to ``canvas`` without a draft, as another writer would.""" + from superset.commands.canvas.apply_ops import ApplyCanvasOperationsCommand + + ApplyCanvasOperationsCommand( + canvas.id, canvas.revision, ops({"op": "add", "widgetType": "markdown"}) + ).run() + + +def draft_canceling_a_new_widget(canvas: Any, calls: str) -> Any: + """A draft of ``canvas`` that adds a new chart and then removes it.""" + from superset.commands.canvas.draft import ( + ApplyCanvasDraftOperationsCommand, + CreateCanvasDraftCommand, + ) + + add = {"op": "add", "id": "canceled", "widgetType": "chart"} + remove = {"op": "remove", "id": "canceled"} + draft = CreateCanvasDraftCommand(canvas.id).run() + for batch in [[add, remove]] if calls == "batch" else [[add], [remove]]: + draft, _ = ApplyCanvasDraftOperationsCommand( + draft.token, draft.revision, ops(*batch) + ).run() + return draft + + +def commit_draft(client: Any, draft: Any) -> Any: + """Commit ``draft`` at its current revision through the REST API.""" + return client.post( + "/api/v1/canvas/draft/commit", + json={"revision": draft.revision}, + headers={"X-Canvas-Draft": draft.token}, + ) + + def test_adding_a_chart_creates_a_widget_the_canvas_editors_edit( client: Any, full_api_access: None, canvas: Any, session: Session ) -> None: @@ -177,3 +226,400 @@ def test_a_draft_commit_fails_if_a_widget_it_changed_was_saved_since( assert excinfo.value.widgets == [widget_id] assert saved_widget(session, widget_id).props == {} + + +@pytest.mark.parametrize( + "scenario", + ["separate", "batch", "parent", "sibling", "shared_placement", "readded"], +) +def test_commit_after_removing_a_new_widget( + canvas: Any, session: Session, draft_cache: SimpleCache, scenario: str +) -> None: + """Canceled draft widgets never prevent commit or leave orphan rows.""" + from superset.commands.canvas.draft import ( + ApplyCanvasDraftOperationsCommand, + CommitCanvasDraftCommand, + CreateCanvasDraftCommand, + draft_payload, + load_draft, + ) + from superset.commands.canvas.exceptions import CanvasDraftNotFoundError + from superset.widgets.models import Widget + + draft = CreateCanvasDraftCommand(canvas.id).run() + initial: list[dict[str, Any]] = [] + if scenario == "parent": + initial.append({"op": "add", "id": "group", "widgetType": "group"}) + initial.append( + { + "op": "add", + "id": "canceled", + "widgetType": "chart", + "parent": "group" if scenario == "parent" else "root", + } + ) + if scenario == "sibling": + initial.append({"op": "add", "id": "kept", "widgetType": "chart"}) + removal = {"op": "remove", "id": "group" if scenario == "parent" else "canceled"} + if scenario == "batch": + initial.append(removal) + draft, _ = ApplyCanvasDraftOperationsCommand(draft.token, 0, ops(*initial)).run() + if scenario in {"shared_placement", "readded"}: + widget_id = draft.definition["nodes"]["canceled"]["widgetId"] + placements = [{"op": "add", "id": "kept", "widgetId": widget_id}] + if scenario == "readded": + placements.insert(0, removal) + draft, _ = ApplyCanvasDraftOperationsCommand( + draft.token, + draft.revision, + ops(*placements), + ).run() + if scenario not in {"batch", "readded"}: + draft, _ = ApplyCanvasDraftOperationsCommand( + draft.token, draft.revision, ops(removal) + ).run() + expected = draft.definition + visible_widget_ids = { + node["widgetId"] for node in expected["nodes"].values() if "widgetId" in node + } + + assert session.query(Widget).count() == 0 + assert set(draft_payload(draft)["widgets"]) == visible_widget_ids + CommitCanvasDraftCommand(draft.token, draft.revision).run() + + assert json.loads(canvas.definition) == expected + widgets = session.query(Widget).all() + assert len(widgets) == len(visible_widget_ids) + if widgets: + assert str(widgets[0].uuid) == expected["nodes"]["kept"]["widgetId"] + with pytest.raises(CanvasDraftNotFoundError): + load_draft(draft.token) + + +@pytest.mark.parametrize("calls", ["batch", "separate"]) +def test_committing_a_canceled_new_widget_publishes_a_revision( + client: Any, + full_api_access: None, + canvas: Any, + session: Session, + draft_cache: SimpleCache, + mocker: MockerFixture, + calls: str, +) -> None: + """ + Changes that cancel out do not make an empty draft: it keeps both + operations, and the canceled widget so they replay. Committing writes them + as publishing the same batch without a draft does, leaving the definition + as it was: the revision increments, viewers are notified, and the + operation log records the ``add`` of a widget that is never saved, then + its ``remove``. Closing such a draft without a write, as an empty draft + closes, would be a change of behavior. + """ + from superset.commands.canvas.draft import load_draft + from superset.commands.canvas.exceptions import CanvasDraftNotFoundError + from superset.widgets.models import Widget + + definition = json.loads(canvas.definition) + draft = draft_canceling_a_new_widget(canvas, calls) + widget_id = draft.ops[0]["widgetId"] + # Nothing is left to publish, but the draft is not empty. + assert draft.definition == definition + assert [op["op"] for op in draft.ops] == ["add", "remove"] + assert set(draft.widgets) == {widget_id} + publish = mocker.patch("superset.commands.canvas.apply_ops.publish_realtime") + + response = commit_draft(client, draft) + + assert response.status_code == 200 + assert response.json["result"] == {"canvasId": canvas.id, "revision": 2} + assert canvas.revision == 2 + assert json.loads(canvas.definition) == definition + assert session.query(Widget).count() == 0 + with pytest.raises(CanvasDraftNotFoundError): + load_draft(draft.token) + assert [call.kwargs["payload"] for call in publish.call_args_list] == [ + {"entity_type": "canvas", "id": canvas.id} + ] + assert client.get(f"{URL}/changes?since=1").json["result"] == { + "revision": 2, + "changes": [{"revision": 2, "ops": draft.ops}], + } + + +@pytest.mark.parametrize("calls", ["batch", "separate"]) +def test_committing_a_canceled_new_widget_conflicts_where_an_empty_draft_closes( + client: Any, + full_api_access: None, + canvas: Any, + session: Session, + draft_cache: SimpleCache, + mocker: MockerFixture, + calls: str, +) -> None: + """ + Once the canvas changes after the fork, committing changes that cancel out + is rejected as a canvas conflict and publishes nothing, while an empty + draft forked at the same revision closes successfully. + """ + from superset.commands.canvas.draft import CreateCanvasDraftCommand, load_draft + from superset.commands.canvas.exceptions import CanvasDraftNotFoundError + from superset.widgets.models import Widget + + canceled = draft_canceling_a_new_widget(canvas, calls) + empty = CreateCanvasDraftCommand(canvas.id).run() + publish_elsewhere(canvas) + assert (canceled.base_revision, empty.base_revision, canvas.revision) == (1, 1, 2) + published = (canvas.revision, canvas.definition) + publish = mocker.patch("superset.commands.canvas.apply_ops.publish_realtime") + + rejected = commit_draft(client, canceled) + closed = commit_draft(client, empty) + + assert rejected.status_code == 409 + assert rejected.json["error_type"] == "CanvasRevisionConflict" + assert rejected.json["canvasChanged"] is True + assert rejected.json["revision"] == 2 + assert closed.status_code == 200 + assert closed.json["result"] == {"canvasId": canvas.id, "revision": 2} + assert (canvas.revision, canvas.definition) == published + assert session.query(Widget).count() == 0 + publish.assert_not_called() + assert load_draft(canceled.token).to_value() == canceled.to_value() + with pytest.raises(CanvasDraftNotFoundError): + load_draft(empty.token) + + +@pytest.mark.parametrize( + ("external_publishes", "published_revision"), [(0, 1), (1, 2), (3, 4)] +) +def test_empty_draft_commit_reports_the_observed_published_revision( + client: Any, + full_api_access: None, + canvas: Any, + session: Session, + draft_cache: SimpleCache, + mocker: MockerFixture, + external_publishes: int, + published_revision: int, +) -> None: + """ + Closing an empty draft reports published state without changing it. + + The draft forks at revision 1, with draft revision 0. Writes published + after the fork make the observed revision differ from ``base_revision``, + and three of them from ``base_revision + 1`` as well; they also change + the definition, which the close must keep as published. + """ + from superset.commands.canvas.draft import CreateCanvasDraftCommand, load_draft + from superset.commands.canvas.exceptions import CanvasDraftNotFoundError + from superset.widgets.models import Widget + + draft = CreateCanvasDraftCommand(canvas.id).run() + for _ in range(external_publishes): + publish_elsewhere(canvas) + definition = canvas.definition + assert (draft.base_revision, draft.revision) == (1, 0) + assert canvas.revision == published_revision + publish = mocker.patch("superset.commands.canvas.apply_ops.publish_realtime") + + response = commit_draft(client, draft) + + assert response.status_code == 200 + assert response.json["result"] == { + "canvasId": canvas.id, + "revision": published_revision, + } + assert canvas.revision == published_revision + assert canvas.definition == definition + assert session.query(Widget).count() == 0 + publish.assert_not_called() + with pytest.raises(CanvasDraftNotFoundError): + load_draft(draft.token) + + +@pytest.mark.parametrize("reset", ["scope", "settings"]) +def test_nullable_resets_survive_draft_replay( + canvas: Any, session: Session, draft_cache: SimpleCache, reset: str +) -> None: + """Explicit reset values remain valid when the draft is committed.""" + from superset.commands.canvas.draft import ( + ApplyCanvasDraftOperationsCommand, + CommitCanvasDraftCommand, + CreateCanvasDraftCommand, + ) + + draft = CreateCanvasDraftCommand(canvas.id).run() + operations: list[dict[str, Any]] + if reset == "scope": + operations = [ + {"op": "add", "id": "filter", "widgetType": "filter"}, + { + "op": "set_scope", + "id": "filter", + "kind": "filter", + "scope": {"mode": "global"}, + }, + {"op": "set_scope", "id": "filter", "kind": "filter", "scope": None}, + ] + else: + operations = [ + {"op": "set_settings", "key": "display", "value": {"showTimestamps": True}}, + {"op": "set_settings", "key": "display", "value": None}, + ] + draft, _ = ApplyCanvasDraftOperationsCommand(draft.token, 0, ops(*operations)).run() + expected = draft.definition + + CommitCanvasDraftCommand(draft.token, draft.revision).run() + + assert json.loads(canvas.definition) == expected + + +@pytest.mark.parametrize("conflict", ["widget", "canvas", "draft"]) +def test_commit_conflicts_preserve_the_draft_and_all_published_widgets( + canvas: Any, session: Session, draft_cache: SimpleCache, conflict: str +) -> None: + """Conflicts retain their provenance and roll back earlier widget writes.""" + from superset.commands.canvas.apply_ops import ApplyCanvasOperationsCommand + from superset.commands.canvas.draft import ( + ApplyCanvasDraftOperationsCommand, + CommitCanvasDraftCommand, + CreateCanvasDraftCommand, + load_draft, + ) + from superset.commands.canvas.exceptions import DraftConflictError + from superset.widgets.models import Widget + + ApplyCanvasOperationsCommand( + canvas.id, + canvas.revision, + ops( + {"op": "add", "id": "first", "widgetType": "chart"}, + {"op": "add", "id": "conflicted", "widgetType": "chart"}, + ), + ).run() + draft = CreateCanvasDraftCommand(canvas.id).run() + draft, _ = ApplyCanvasDraftOperationsCommand( + draft.token, + draft.revision, + ops( + {"op": "add", "id": "new", "widgetType": "chart"}, + {"op": "set_props", "id": "first", "props": {"metric": "avg"}}, + {"op": "set_props", "id": "conflicted", "props": {"metric": "sum"}}, + ), + ).run() + conflicted_id = draft.definition["nodes"]["conflicted"]["widgetId"] + edited = saved_widget(session, conflicted_id) + edited.revision += 1 + edited.props = {"metric": "external"} + if conflict != "widget": + canvas.revision += 1 + session.commit() + published_revision = canvas.revision + published_definition = canvas.definition + published_widgets = { + str(widget.uuid): (widget.props, widget.revision) + for widget in session.query(Widget).all() + } + submitted_revision = 0 if conflict == "draft" else draft.revision + + with pytest.raises(DraftConflictError) as excinfo: + CommitCanvasDraftCommand(draft.token, submitted_revision).run() + + error = excinfo.value + assert error.draft_revision == draft.revision + if conflict == "draft": + assert error.recovery_type == "DraftRevisionConflict" + assert error.revision == draft.revision + assert error.current_canvas_revision is None + else: + assert error.recovery_type == ( + "CanvasRevisionConflict" + if conflict == "canvas" + else "WidgetRevisionConflict" + ) + assert error.revision == published_revision + assert error.current_canvas_revision == published_revision + assert error.canvas_changed is (conflict == "canvas") + assert error.widgets == ([conflicted_id] if conflict == "widget" else []) + session.expire_all() + assert canvas.revision == published_revision + assert canvas.definition == published_definition + assert { + str(widget.uuid): (widget.props, widget.revision) + for widget in session.query(Widget).all() + } == published_widgets + assert load_draft(draft.token).to_value() == draft.to_value() + + +@pytest.mark.parametrize("draft_write", [True, False]) +def test_invalid_middle_operation_leaves_no_draft_or_published_changes( + canvas: Any, session: Session, draft_cache: SimpleCache, draft_write: bool +) -> None: + """A valid creation before a bad operation leaves no partial changes.""" + from superset.commands.canvas.apply_ops import ApplyCanvasOperationsCommand + from superset.commands.canvas.draft import ( + ApplyCanvasDraftOperationsCommand, + CreateCanvasDraftCommand, + load_draft, + ) + from superset.commands.canvas.exceptions import DefinitionInvalidError + from superset.widgets.models import Widget + + draft = CreateCanvasDraftCommand(canvas.id).run() + definition, revision = canvas.definition, canvas.revision + operations = ops( + {"op": "add", "id": "first", "widgetType": "chart"}, + {"op": "remove", "id": "missing"}, + {"op": "add", "id": "last", "widgetType": "chart"}, + ) + command = ( + ApplyCanvasDraftOperationsCommand(draft.token, 0, operations) + if draft_write + else ApplyCanvasOperationsCommand(canvas.id, revision, operations) + ) + + with pytest.raises(DefinitionInvalidError) as excinfo: + command.run() + + assert excinfo.value.operation_index == 1 + assert excinfo.value.issues[0].path == "/ops/1" + assert session.query(Widget).count() == 0 + assert (canvas.definition, canvas.revision) == (definition, revision) + assert load_draft(draft.token).to_value() == draft.to_value() + + +def test_removing_a_saved_widget_placement_preserves_its_deliberate_edit( + canvas: Any, session: Session, draft_cache: SimpleCache +) -> None: + """Pruning canceled creations never deletes or drops edits to saved widgets.""" + from superset.commands.canvas.apply_ops import ApplyCanvasOperationsCommand + from superset.commands.canvas.draft import ( + ApplyCanvasDraftOperationsCommand, + CommitCanvasDraftCommand, + CreateCanvasDraftCommand, + ) + from superset.widgets.models import Widget + + ApplyCanvasOperationsCommand( + canvas.id, + canvas.revision, + ops({"op": "add", "id": "shared", "widgetType": "chart"}), + ).run() + widget_id = json.loads(canvas.definition)["nodes"]["shared"]["widgetId"] + draft = CreateCanvasDraftCommand(canvas.id).run() + draft, _ = ApplyCanvasDraftOperationsCommand( + draft.token, + draft.revision, + ops( + {"op": "set_props", "id": "shared", "props": {"metric": "avg"}}, + {"op": "remove", "id": "shared"}, + ), + ).run() + + CommitCanvasDraftCommand(draft.token, draft.revision).run() + + assert json.loads(canvas.definition)["nodes"] == {} + assert session.query(Widget).count() == 1 + widget = saved_widget(session, widget_id) + assert widget.props == {"metric": "avg"} + assert widget.revision == 2 diff --git a/tests/unit_tests/mcp_service/canvas/tool/test_canvas_contract.py b/tests/unit_tests/mcp_service/canvas/tool/test_canvas_contract.py new file mode 100644 index 000000000000..b7e7e60cf86a --- /dev/null +++ b/tests/unit_tests/mcp_service/canvas/tool/test_canvas_contract.py @@ -0,0 +1,979 @@ +# 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. + +"""Canvas contracts through protected registration and the MCP protocol.""" + +from __future__ import annotations + +import importlib +from collections.abc import Iterator +from copy import deepcopy +from types import SimpleNamespace +from typing import Any +from unittest.mock import MagicMock, patch + +import pytest +from cachelib import SimpleCache +from fastmcp import Client, FastMCP +from fastmcp.utilities.json_schema import dereference_refs +from flask_appbuilder.security.sqla.models import Role, User +from jsonschema import Draft202012Validator +from mcp.types import CallToolResult, TextContent +from pytest_mock import MockerFixture +from sqlalchemy import create_engine +from sqlalchemy.engine import Engine +from sqlalchemy.orm import Session +from sqlalchemy.pool import StaticPool + +from superset import security_manager +from superset.app import SupersetApp +from superset.canvas.data import PlacementData +from superset.canvas.definition import registry +from superset.canvas.definition.validation import Issue +from superset.commands.canvas.exceptions import ( + CanvasDraftNotFoundError, + CanvasForbiddenError, + CanvasNotFoundError, + DefinitionConflictError, + DefinitionInvalidError, + DraftConflictError, +) +from superset.connectors.sqla.models import SqlaTable +from superset.mcp_service.mcp_config import ( + MCP_NATIVE_TOOL_LIST_CONFIG, + MCP_TOOL_SEARCH_CONFIG, +) +from superset.mcp_service.server import ( + _apply_compact_tool_list_transform, + _apply_tool_search_transform, + build_middleware_list, +) +from superset.models.canvas import Canvas +from superset.subjects.models import Subject +from superset.subjects.types import SubjectType +from superset.utils import json +from superset.widgets.models import Widget +from superset.widgets.registry import registry as widget_registry +from tests.unit_tests.canvas import placement_data_test +from tests.unit_tests.canvas.fixtures import canvas_widget_types +from tests.unit_tests.canvas.placement_data_test import published_state + +pytestmark = pytest.mark.parametrize( + "app", [{"FEATURE_FLAGS": {"CANVAS": True}}], indirect=True +) +aggregate_table = placement_data_test.aggregate_table + +MODULE = "superset.mcp_service.canvas.tool" +TOOLS = ( + "apply_canvas_draft_ops", + "create_canvas", + "preview_widget", + "get_canvas", + "create_canvas_draft", + "get_canvas_draft", + "commit_canvas_draft", + "delete_canvas_draft", + "update_canvas", +) + + +@pytest.fixture(autouse=True) +def principal( + app: SupersetApp, disable_mcp_rbac: None, mocker: MockerFixture +) -> Iterator[User]: + """Inject a FAB principal with operator-defined roles and real RBAC checks.""" + user = User( + id=701, + username="canvas-contract-editor", + first_name="Canvas", + last_name="Editor", + email="canvas@example.com", + active=True, + roles=[Role(name="Canvas contract editor")], + ) + mocker.patch.dict( + security_manager.builtin_roles, + { + "Canvas contract editor": [["^Canvas$", "^can_(read|write)$"]], + "Canvas contract reader": [["^Canvas$", "^can_read$"]], + "Canvas query viewer": [["^Dashboard$", "^can_view_query$"]], + }, + ) + app.config["MCP_RBAC_ENABLED"] = True + mocker.patch("superset.mcp_service.auth.get_user_from_request", return_value=user) + mocker.patch( + "superset.mcp_service.middleware.get_user_from_request", return_value=user + ) + # Principal resolution is injected; permission checks use the real manager. + # The transient principal has no audit-log row. + mocker.patch("superset.mcp_service.middleware.event_logger") + yield user + app.config["MCP_RBAC_ENABLED"] = False + + +@pytest.fixture +def session_engine() -> Iterator[Engine]: + """Share only this test's in-memory metadata across sequential tool workers.""" + engine = create_engine( + "sqlite://", connect_args={"check_same_thread": False}, poolclass=StaticPool + ) + yield engine + engine.dispose() + + +@pytest.fixture +def metadata_session( + session: Session, principal: User, mocker: MockerFixture +) -> Iterator[Session]: + """Persist the injected principal and use isolated metadata and draft cache.""" + Canvas.metadata.create_all(session.get_bind()) # pylint: disable=no-member + session.add(principal) + session.flush() + if not session.query(Subject).filter_by(user_id=principal.id).first(): + session.add( + Subject( + user_id=principal.id, label=principal.username, type=SubjectType.USER + ) + ) + session.commit() + mocker.patch( + "superset.commands.canvas.draft.cache_manager", + SimpleNamespace(explore_form_data_cache=SimpleCache()), + ) + mocker.patch("superset.commands.canvas.apply_ops.publish_realtime") + registry.set_widget_resolver(None) + yield session + session.rollback() + registry.set_widget_resolver(None) + + +@pytest.fixture +def draft_state(metadata_session: Session, mocker: MockerFixture) -> Session: + """Use small widget schemas with actual persisted widgets and editorship.""" + mocker.patch.dict(widget_registry, canvas_widget_types(), clear=True) + return metadata_session + + +async def _server( + *, + structured: bool, + proxy: bool = False, + compact: bool = False, + summary: bool = False, +) -> FastMCP: + """Register actual tools with production decorators and middleware.""" + server = FastMCP( + "canvas-contract", + middleware=build_middleware_list(structured_output_enabled=structured), + ) + with patch("superset.mcp_service.app.mcp", server): + for name in TOOLS: + importlib.reload(importlib.import_module(f"{MODULE}.{name}")) + for name in TOOLS: + tool = await server.get_tool(name) + assert tool is not None + assert getattr(tool.fn, "_mcp_auth_protected", False) + if proxy: + _apply_tool_search_transform( + server, + { + **MCP_TOOL_SEARCH_CONFIG, + "always_visible": [], + "include_schemas": not summary, + "max_results": len(TOOLS), + }, + ) + elif compact: + _apply_compact_tool_list_transform( + server, {**MCP_NATIVE_TOOL_LIST_CONFIG, "compact": True} + ) + return server + + +async def _call( + client: Client[Any], name: str, arguments: dict[str, Any], *, proxy: bool +) -> CallToolResult: + """Invoke the same named tool directly or through the search transform.""" + return await client.call_tool_mcp( + "call_tool" if proxy else name, + {"name": name, "arguments": arguments} if proxy else arguments, + ) + + +def _data(result: CallToolResult, *, structured: bool) -> dict[str, Any]: + """Check both result envelopes and decode the Canvas result data.""" + assert result.isError is False + block = result.content[0] + assert isinstance(block, TextContent) + data = json.loads(block.text) + assert result.structuredContent == (data if structured else None) + return data + + +def _assert_operation_schema( + schema: dict[str, Any], *, create: bool, discriminator: bool = False +) -> None: + """Check the advertised operation grammar after resolving local references.""" + Draft202012Validator.check_schema(schema) + if discriminator: + raw_ops = schema["properties"]["ops"] + if create: + raw_ops = next( + option for option in raw_ops["anyOf"] if option.get("type") == "array" + ) + assert raw_ops["items"]["discriminator"]["propertyName"] == "op" + resolved = dereference_refs(schema) + assert set(resolved["properties"]) == ( + {"title", "slug", "description", "ops"} + if create + else {"token", "revision", "ops"} + ) + ops = resolved["properties"]["ops"] + if create: + assert "ops" not in resolved["required"] + assert {"type": "null"} in ops["anyOf"] + ops = next(option for option in ops["anyOf"] if option.get("type") == "array") + assert "minItems" not in ops + else: + assert ops["minItems"] == 1 + variants = { + item["properties"]["op"]["const"]: item for item in ops["items"]["oneOf"] + } + assert set(variants) == { + "add", + "remove", + "move", + "place", + "set_props", + "set_scope", + "set_settings", + } + assert all("op" in variant["required"] for variant in variants.values()) + assert "widgetType" in variants["add"]["properties"] + assert "widget_type" not in variants["add"]["properties"] + assert variants["move"]["properties"]["index"]["anyOf"][0]["minimum"] == 0 + assert "layout" not in variants["move"]["required"] + for op, field in (("set_scope", "scope"), ("set_settings", "value")): + assert field in variants[op]["required"] + assert {"type": "null"} in variants[op]["properties"][field]["anyOf"] + validator = Draft202012Validator(ops["items"]) + assert validator.is_valid({"op": "remove", "id": "chart"}) + assert not validator.is_valid({"id": "chart"}) + assert not validator.is_valid({"op": "unknown", "id": "chart"}) + assert not validator.is_valid({"op": "move", "id": "chart", "index": -1}) + + +# What the draft tools must advertise even within the shipped prose budgets of +# tool search and compact native listings. +DRAFT_RECOVERY_GUIDANCE = { + "apply_canvas_draft_ops": ( + "in order and atomically", + "Only the draft changes; ``commit_canvas_draft`` publishes it", + "Send the last ``draft_revision`` as ``revision``", + "``DraftRevisionConflict``, reload the draft", + ), + "commit_canvas_draft": ( + "Publish a canvas draft and delete it", + "only when the person asks to publish", + "On ``CanvasRevisionConflict`` or ``WidgetRevisionConflict``, " + "start a fresh draft", + "``DraftRevisionConflict``, reload the draft", + ), +} + + +def _assert_draft_recovery_guidance( + descriptions: dict[str, str], *, budget: int | None +) -> None: + """Publish intent and conflict recovery survive the advertised prose budget.""" + for name, phrases in DRAFT_RECOVERY_GUIDANCE.items(): + description = descriptions[name] + if budget: + assert len(description) <= budget, name + prose = " ".join(description.split()) + assert [phrase for phrase in phrases if phrase not in prose] == [], name + + +@pytest.mark.asyncio +@pytest.mark.parametrize("structured", [False, True]) +@pytest.mark.parametrize("compact", [False, True]) +async def test_native_canvas_schemas_expose_canonical_operations( + structured: bool, compact: bool +) -> None: + """Native and compact listings preserve operation constraints in both modes.""" + server = await _server(structured=structured, compact=compact) + for name in TOOLS[:2]: + registered = await server.get_tool(name) + assert registered is not None + _assert_operation_schema( + registered.parameters, create=name == "create_canvas", discriminator=True + ) + async with Client(server) as client: + tools = {tool.name: tool for tool in await client.list_tools()} + budget = MCP_NATIVE_TOOL_LIST_CONFIG["max_description_length"] + _assert_draft_recovery_guidance( + {name: tools[name].description or "" for name in DRAFT_RECOVERY_GUIDANCE}, + budget=budget if compact else None, + ) + for name in TOOLS[:2]: + tool = tools[name] + _assert_operation_schema(tool.inputSchema, create=name == "create_canvas") + assert (tool.outputSchema is not None) == structured + if compact: + assert len(tool.description or "") <= budget + + +@pytest.mark.asyncio +@pytest.mark.parametrize("structured", [False, True]) +@pytest.mark.parametrize("summary", [False, True]) +async def test_canvas_search_preserves_full_schema_or_explicit_summary( + structured: bool, summary: bool +) -> None: + """Full search retains the grammar; summary mode exposes only flat hints.""" + server = await _server(structured=structured, proxy=True, summary=summary) + async with Client(server) as client: + result = await client.call_tool_mcp("search_tools", {}) + assert result.isError is False + block = result.content[0] + assert isinstance(block, TextContent) + entries = {tool["name"]: tool for tool in json.loads(block.text)} + # Search shortens descriptions with or without schemas. + _assert_draft_recovery_guidance( + {name: entries[name]["description"] for name in DRAFT_RECOVERY_GUIDANCE}, + budget=MCP_TOOL_SEARCH_CONFIG["max_description_length"], + ) + for name in TOOLS[:2]: + tool = entries[name] + if summary: + assert "inputSchema" not in tool + assert tool["parameters_hint"] == ( + "title, slug, description, ops" + if name == "create_canvas" + else "token, revision, ops" + ) + else: + _assert_operation_schema( + tool["inputSchema"], create=name == "create_canvas" + ) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("structured", [False, True]) +@pytest.mark.parametrize("proxy", [False, True]) +async def test_canvas_transport_preserves_omitted_and_explicit_null_operations( + structured: bool, proxy: bool, mocker: MockerFixture +) -> None: + """Schema enrichment leaves raw dictionaries and nullable reset keys intact.""" + server = await _server(structured=structured, proxy=proxy) + ops = [ + {"op": "add", "widgetType": "markdown"}, + {"op": "move", "id": "chart", "parent": "root"}, + {"op": "set_scope", "kind": "filter", "id": "filter", "scope": None}, + {"op": "set_settings", "key": "colors", "value": None}, + ] + apply = mocker.patch( + f"{MODULE}.apply_canvas_draft_ops._apply_canvas_draft_ops_impl", + return_value={"revision": 1}, + ) + async with Client(server) as client: + result = await _call( + client, + "apply_canvas_draft_ops", + {"token": "tok", "revision": 0, "ops": ops}, + proxy=proxy, + ) + assert _data(result, structured=structured) == {"revision": 1} + apply.assert_called_once_with("tok", 0, ops) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("structured", [False, True]) +@pytest.mark.parametrize("proxy", [False, True]) +@pytest.mark.parametrize( + ("ops", "path"), + [ + ([], "/ops"), + ([{"op": "set_settings", "key": "colors"}], "/ops/0/value"), + ([{"op": "set_scope", "kind": "filter", "id": "filter"}], "/ops/0/scope"), + ([{"op": "add", "widgetId": "saved", "widgetType": "markdown"}], "/ops/0"), + ( + [ + {"op": "remove", "id": "a"}, + {"op": "move", "id": "b", "parent": "root", "index": -1}, + ], + "/ops/1/index", + ), + ], +) +async def test_invalid_operations_keep_canvas_paths_before_draft_loading( + structured: bool, + proxy: bool, + ops: list[dict[str, Any]], + path: str, + mocker: MockerFixture, +) -> None: + """Canonical helper validation still produces data with precise field paths.""" + server = await _server(structured=structured, proxy=proxy) + load = mocker.patch(f"{MODULE}.apply_canvas_draft_ops.load_draft") + command = mocker.patch( + f"{MODULE}.apply_canvas_draft_ops.ApplyCanvasDraftOperationsCommand" + ) + async with Client(server) as client: + result = await _call( + client, + "apply_canvas_draft_ops", + {"token": "unknown", "revision": 0, "ops": ops}, + proxy=proxy, + ) + data = _data(result, structured=structured) + assert data["error"] == "Invalid operations" + assert data["error_type"] == "ValidationError" + assert data["errors"][0]["path"] == path + load.assert_not_called() + command.assert_not_called() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("structured", [False, True]) +@pytest.mark.parametrize("proxy", [False, True]) +@pytest.mark.parametrize("extra", [{}, {"ops": None}, {"ops": []}]) +async def test_empty_creation_keeps_all_optional_operations_forms( + structured: bool, proxy: bool, extra: dict[str, Any], mocker: MockerFixture +) -> None: + """Omitted, null, and empty initial operations all create an empty canvas.""" + server = await _server(structured=structured, proxy=proxy) + create = mocker.patch(f"{MODULE}.create_canvas.CreateCanvasCommand") + create.return_value.run.return_value = MagicMock(id=5, url="/canvas/5/", revision=1) + apply = mocker.patch(f"{MODULE}.create_canvas._apply_canvas_ops_impl") + async with Client(server) as client: + result = await _call( + client, "create_canvas", {"title": "Empty", **extra}, proxy=proxy + ) + assert _data(result, structured=structured) == { + "id": 5, + "url": "/canvas/5/", + "revision": 1, + } + create.assert_called_once_with({"title": "Empty"}) + apply.assert_not_called() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("structured", [False, True]) +@pytest.mark.parametrize("proxy", [False, True]) +async def test_denied_principal_cannot_execute_canvas_writes( + structured: bool, proxy: bool, principal: User, mocker: MockerFixture +) -> None: + """An authenticated custom role without can_write cannot reach a write helper.""" + principal.roles = [Role(name="Canvas contract reader")] + server = await _server(structured=structured, proxy=proxy) + create = mocker.patch(f"{MODULE}.create_canvas._create_canvas_impl") + async with Client(server) as client: + result = await _call(client, "create_canvas", {"title": "Denied"}, proxy=proxy) + assert result.isError is True + assert any( + isinstance(block, TextContent) and "Permission denied" in block.text + for block in result.content + ) + create.assert_not_called() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("structured", [False, True]) +@pytest.mark.parametrize("proxy", [False, True]) +@pytest.mark.parametrize("view_query", [False, True]) +async def test_preview_query_visibility_uses_actual_role_grants( + structured: bool, + proxy: bool, + view_query: bool, + principal: User, + mocker: MockerFixture, +) -> None: + """Preview query text requires its Dashboard grant on both invocation paths.""" + if view_query: + principal.roles.append(Role(name="Canvas query viewer")) + server = await _server(structured=structured, proxy=proxy) + dao = mocker.patch(f"{MODULE}.preview_widget.CanvasDAO") + dao.load.return_value = { + "nodes": { + "chart": { + "widgetType": "metric-tile", + "props": {"dataBinding": {"datasetId": 13}}, + } + } + } + query = "SELECT SUM(sales) FROM sales" + mocker.patch( + f"{MODULE}.preview_widget.run_binding", + return_value=PlacementData([{"sales": 40}], query, "sql"), + ) + async with Client(server) as client: + result = await _call( + client, + "preview_widget", + {"canvas_id": 1, "placement_id": "chart"}, + proxy=proxy, + ) + data = _data(result, structured=structured) + assert data.get("query") == (query if view_query else None) + assert data["sample_rows"] == [{"sales": 40}] + + +@pytest.mark.asyncio +@pytest.mark.parametrize("structured", [False, True]) +@pytest.mark.parametrize("proxy", [False, True]) +@pytest.mark.parametrize( + ("exception", "expected"), + [ + (CanvasDraftNotFoundError(), {"error_type": "DraftUnavailable"}), + ( + DraftConflictError(4, draft_revision=4), + { + "error_type": "DraftRevisionConflict", + "revision": 4, + "draft_revision": 4, + "canvas_changed": False, + }, + ), + ( + DraftConflictError( + 9, canvas_changed=True, draft_revision=4, current_canvas_revision=9 + ), + { + "error_type": "CanvasRevisionConflict", + "revision": 9, + "draft_revision": 4, + "current_canvas_revision": 9, + "canvas_changed": True, + }, + ), + ( + DraftConflictError( + 9, widgets=["widget-uuid"], draft_revision=4, current_canvas_revision=9 + ), + { + "error_type": "WidgetRevisionConflict", + "revision": 9, + "draft_revision": 4, + "current_canvas_revision": 9, + "canvas_changed": False, + "changed_widgets": ["widget-uuid"], + }, + ), + ( + DefinitionInvalidError( + "Invalid placement", + [Issue("/nodes/chart", "Unknown placement")], + operation_index=1, + ), + { + "error_type": "ValidationError", + "operation": 1, + "errors": [{"path": "/nodes/chart", "message": "Unknown placement"}], + }, + ), + (CanvasNotFoundError(), {"error": "Canvas not found, or no access to it"}), + ( + CanvasForbiddenError(), + {"error": "Only the canvas' editors can draft changes to it"}, + ), + ], +) +async def test_recovery_results_preserve_categories_and_revision_provenance( + structured: bool, + proxy: bool, + exception: Exception, + expected: dict[str, Any], + mocker: MockerFixture, +) -> None: + """Domain failures survive response models and both MCP envelope modes.""" + server = await _server(structured=structured, proxy=proxy) + mocker.patch(f"{MODULE}.commit_canvas_draft.load_draft") + command = mocker.patch(f"{MODULE}.commit_canvas_draft.CommitCanvasDraftCommand") + command.return_value.run.side_effect = exception + async with Client(server) as client: + result = await _call( + client, + "commit_canvas_draft", + {"token": "private-token", "revision": 3}, + proxy=proxy, + ) + data = _data(result, structured=structured) + assert "error" in data + assert {key: value for key, value in data.items() if key != "error"} == { + key: value for key, value in expected.items() if key != "error" + } + if "error" in expected: + assert data["error"] == expected["error"] + assert "private-token" not in json.dumps(data) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("structured", [False, True]) +@pytest.mark.parametrize("proxy", [False, True]) +async def test_initial_operation_conflict_keeps_its_canvas_revision( + structured: bool, proxy: bool, mocker: MockerFixture +) -> None: + """The write response model ignores unknown keys, so the REST body's + camelCase revision must reach it in snake_case.""" + server = await _server(structured=structured, proxy=proxy) + create = mocker.patch(f"{MODULE}.create_canvas.CreateCanvasCommand") + create.return_value.run.return_value = MagicMock(id=5, url="/canvas/5/", revision=1) + delete = mocker.patch(f"{MODULE}.create_canvas.DeleteCanvasCommand") + command = mocker.patch(f"{MODULE}.canvas_ops_common.ApplyCanvasOperationsCommand") + command.return_value.run.side_effect = DefinitionConflictError( + 2, ["chart"], stale=False + ) + async with Client(server) as client: + result = await _call( + client, + "create_canvas", + {"title": "Raced", "ops": [{"op": "remove", "id": "chart"}]}, + proxy=proxy, + ) + data = _data(result, structured=structured) + assert data["error_type"] == "CanvasRevisionConflict" + assert data["revision"] == data["current_canvas_revision"] == 2 + delete.assert_called_once_with([5]) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("structured", [False, True]) +@pytest.mark.parametrize("proxy", [False, True]) +async def test_real_draft_lifecycle_keeps_published_state_until_commit( + structured: bool, + proxy: bool, + draft_state: Session, +) -> None: + """Create, edit, read, commit and discard use real commands and role checks.""" + server = await _server(structured=structured, proxy=proxy) + async with Client(server) as client: + + async def invoke(name: str, **arguments: Any) -> dict[str, Any]: + """Decode a protected call without replacing any domain command.""" + return _data( + await _call(client, name, arguments, proxy=proxy), structured=structured + ) + + created = await invoke("create_canvas", title="Private working copy") + canvas_id = created["id"] + published = await invoke("get_canvas", canvas_id=canvas_id) + before = published_state(draft_state) + draft = await invoke("create_canvas_draft", canvas_id=canvas_id) + token = draft["token"] + assert draft["revision"] == draft["draft_revision"] == 0 + added = await invoke( + "apply_canvas_draft_ops", + token=token, + revision=0, + ops=[ + { + "op": "add", + "id": "sales", + "widgetType": "chart", + "props": {"title": "Keep this", "metric": "count"}, + } + ], + ) + assert added["revision"] == added["draft_revision"] == 1 + assert published_state(draft_state) == before + assert await invoke("get_canvas", canvas_id=canvas_id) == published + effective = await invoke("get_canvas_draft", token=token) + widget_id = effective["definition"]["nodes"]["sales"]["widgetId"] + assert draft_state.query(Widget).count() == 0 + + rejected = await invoke( + "apply_canvas_draft_ops", + token=token, + revision=1, + ops=[ + {"op": "add", "id": "temporary", "widgetType": "chart"}, + {"op": "remove", "id": "missing"}, + {"op": "add", "id": "later", "widgetType": "markdown"}, + ], + ) + assert rejected["error_type"] == "ValidationError" + assert rejected["operation"] == 1 + assert await invoke("get_canvas_draft", token=token) == effective + assert published_state(draft_state) == before + + stale = await invoke( + "apply_canvas_draft_ops", + token=token, + revision=0, + ops=[{"op": "remove", "id": "sales"}], + ) + assert stale["error_type"] == "DraftRevisionConflict" + assert stale["revision"] == stale["draft_revision"] == 1 + assert "current_canvas_revision" not in stale + props = deepcopy(effective["widgets"][widget_id]["props"]) + props["metric"] = "sum" + changed = await invoke( + "apply_canvas_draft_ops", + token=token, + revision=1, + ops=[{"op": "set_props", "id": "sales", "props": props}], + ) + assert changed["draft_revision"] == 2 + committed = await invoke("commit_canvas_draft", token=token, revision=2) + assert committed["canvas_revision"] == created["revision"] + 1 + unavailable = await invoke("get_canvas_draft", token=token) + assert unavailable["error_type"] == "DraftUnavailable" + published = await invoke("get_canvas", canvas_id=canvas_id) + assert published["widgets"][widget_id]["props"] == props + assert props["title"] == "Keep this" + assert draft_state.query(Widget).count() == 1 + + draft = await invoke("create_canvas_draft", canvas_id=canvas_id) + await invoke( + "apply_canvas_draft_ops", + token=draft["token"], + revision=0, + ops=[{"op": "remove", "id": "sales"}], + ) + assert await invoke("delete_canvas_draft", token=draft["token"]) == {} + assert await invoke("get_canvas_draft", token=draft["token"]) == unavailable + assert await invoke("get_canvas", canvas_id=canvas_id) == published + + +@pytest.mark.asyncio +@pytest.mark.parametrize("structured", [False, True]) +@pytest.mark.parametrize("proxy", [False, True]) +async def test_empty_draft_close_reports_the_revision_published_since_the_fork( + structured: bool, + proxy: bool, + draft_state: Session, +) -> None: + """An empty close returns the observed published revision, not the fork's.""" + server = await _server(structured=structured, proxy=proxy) + async with Client(server) as client: + + async def invoke(name: str, **arguments: Any) -> dict[str, Any]: + """Decode a protected call without replacing any domain command.""" + return _data( + await _call(client, name, arguments, proxy=proxy), structured=structured + ) + + created = await invoke("create_canvas", title="Empty close") + empty = await invoke("create_canvas_draft", canvas_id=created["id"]) + assert (empty["base_revision"], empty["draft_revision"]) == (1, 0) + for note in range(3): + other = await invoke("create_canvas_draft", canvas_id=created["id"]) + await invoke( + "apply_canvas_draft_ops", + token=other["token"], + revision=0, + ops=[{"op": "add", "id": f"note-{note}", "widgetType": "markdown"}], + ) + published = await invoke( + "commit_canvas_draft", token=other["token"], revision=1 + ) + assert published["canvas_revision"] == 4 + before = published_state(draft_state) + + closed = await invoke("commit_canvas_draft", token=empty["token"], revision=0) + + assert closed["canvas_revision"] == 4 + assert published_state(draft_state) == before + unavailable = await invoke("get_canvas_draft", token=empty["token"]) + assert unavailable["error_type"] == "DraftUnavailable" + + +@pytest.mark.asyncio +@pytest.mark.parametrize("structured", [False, True]) +@pytest.mark.parametrize("proxy", [False, True]) +@pytest.mark.parametrize( + "arguments", + [ + {"title": ""}, + {"title": "Invalid schema", "ops": [{"op": "unknown"}]}, + { + "title": "Invalid middle operation", + "ops": [ + {"op": "add", "widgetType": "chart", "id": "new-chart"}, + {"op": "remove", "id": "missing"}, + ], + }, + ], +) +async def test_invalid_create_has_no_persisted_canvas_or_widget( + structured: bool, + proxy: bool, + arguments: dict[str, Any], + draft_state: Session, +) -> None: + """Rejected metadata and initial operations leave metadata row counts intact.""" + server = await _server(structured=structured, proxy=proxy) + before = published_state(draft_state) + async with Client(server) as client: + result = await _call(client, "create_canvas", arguments, proxy=proxy) + assert _data(result, structured=structured)["error_type"] == "ValidationError" + assert published_state(draft_state) == before + + +@pytest.mark.asyncio +@pytest.mark.parametrize("structured", [False, True]) +@pytest.mark.parametrize("proxy", [False, True]) +async def test_unavailable_tokens_do_not_disclose_draft_ownership( + structured: bool, + proxy: bool, + draft_state: Session, +) -> None: + """Unknown, discarded and another editor's tokens return identical data.""" + server = await _server(structured=structured, proxy=proxy) + other = User( + id=702, + username="another-editor", + first_name="Other", + last_name="Editor", + email="other@example.com", + active=True, + roles=[draft_state.query(Role).filter_by(name="Canvas contract editor").one()], + ) + draft_state.add(other) + draft_state.commit() + async with Client(server) as client: + created = _data( + await _call( + client, "create_canvas", {"title": "Private draft"}, proxy=proxy + ), + structured=structured, + ) + draft = _data( + await _call( + client, "create_canvas_draft", {"canvas_id": created["id"]}, proxy=proxy + ), + structured=structured, + ) + with ( + patch( + "superset.mcp_service.auth.get_user_from_request", return_value=other + ), + patch( + "superset.mcp_service.middleware.get_user_from_request", + return_value=other, + ), + ): + foreign = _data( + await _call( + client, "get_canvas_draft", {"token": draft["token"]}, proxy=proxy + ), + structured=structured, + ) + unknown = _data( + await _call( + client, + "get_canvas_draft", + {"token": "00000000-0000-0000-0000-000000000000"}, + proxy=proxy, + ), + structured=structured, + ) + assert ( + _data( + await _call( + client, + "delete_canvas_draft", + {"token": draft["token"]}, + proxy=proxy, + ), + structured=structured, + ) + == {} + ) + discarded = _data( + await _call( + client, "get_canvas_draft", {"token": draft["token"]}, proxy=proxy + ), + structured=structured, + ) + assert foreign == unknown == discarded + assert foreign["error_type"] == "DraftUnavailable" + assert set(foreign) == {"error", "error_type"} + assert draft["token"] not in json.dumps(foreign) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("structured", [False, True]) +@pytest.mark.parametrize("proxy", [False, True]) +async def test_real_aggregate_preview_through_protected_transport( + structured: bool, + proxy: bool, + aggregate_table: SqlaTable, + metadata_session: Session, + principal: User, + mocker: MockerFixture, +) -> None: + """The protected native and proxy paths execute aggregate SQL before limiting.""" + from superset.commands.chart.data.get_data_command import ChartDataCommand + + principal.roles.append(Role(name="Canvas query viewer")) + editor = metadata_session.query(Subject).filter_by(user_id=principal.id).one() + aggregate_table.editors = [editor] + metadata_session.commit() + server = await _server(structured=structured, proxy=proxy) + run = mocker.spy(ChartDataCommand, "run") + async with Client(server) as client: + created = _data( + await _call( + client, + "create_canvas", + { + "title": "Real aggregate preview", + "ops": [ + { + "op": "add", + "id": "amounts", + "widgetType": "ag-grid-table", + "props": { + "dataBinding": { + "datasetId": aggregate_table.id, + "metrics": ["total_amount"], + "dimensions": ["category"], + "orderBy": [ + {"field": "total_amount", "descending": True} + ], + "rowLimit": 5, + } + }, + } + ], + }, + proxy=proxy, + ), + structured=structured, + ) + before = published_state(metadata_session) + result = await _call( + client, + "preview_widget", + { + "canvas_id": created["id"], + "placement_id": "amounts", + }, + proxy=proxy, + ) + data = _data(result, structured=structured) + assert "error" not in data + assert data["sample_rows"] == [ + {"category": category, "total_amount": total} + for category, total in [("B", 100), ("F", 90), ("D", 70), ("G", 60), ("A", 40)] + ] + assert data["row_count"] == 5 + assert "ORDER BY TOTAL_AMOUNT DESC" in data["query"].upper() + assert run.call_count == 1 + assert published_state(metadata_session) == before diff --git a/tests/unit_tests/mcp_service/canvas/tool/test_canvas_tools.py b/tests/unit_tests/mcp_service/canvas/tool/test_canvas_tools.py index 02390251575f..096c826b814b 100644 --- a/tests/unit_tests/mcp_service/canvas/tool/test_canvas_tools.py +++ b/tests/unit_tests/mcp_service/canvas/tool/test_canvas_tools.py @@ -67,7 +67,11 @@ def test_apply_returns_conflicts_as_data() -> None: result = _apply_canvas_ops_impl(1, 3, [{"op": "remove", "id": "revenue-trend"}]) assert result["revision"] == 7 + assert result["current_canvas_revision"] == 7 + assert result["error_type"] == "CanvasRevisionConflict" assert "error" in result + # The response models ignore unknown keys, so a camelCase one would vanish. + assert all(key.islower() for key in result) def test_get_canvas_is_lean_unless_resolution_is_asked_for() -> None: @@ -389,12 +393,14 @@ def test_draft_errors_come_back_as_data() -> None: patch(f"{module}.CommitCanvasDraftCommand") as command, ): command.return_value.run.side_effect = DraftConflictError( - 5, canvas_changed=True + 5, canvas_changed=True, draft_revision=1, current_canvas_revision=5 ) result = _commit_canvas_draft_impl("tok", 1) assert result["canvas_changed"] is True - assert result["revision"] == 5 + assert result["revision"] == result["current_canvas_revision"] == 5 + assert result["draft_revision"] == 1 + assert all(key.islower() for key in result) def test_preview_returns_the_query_to_users_who_can_view_queries() -> None: @@ -534,8 +540,10 @@ def test_commit_returns_the_canvas_url_and_revision() -> None: patch(f"{module}.CanvasDAO") as dao, ): load.return_value.canvas_id = 7 + load.return_value.base_revision = 3 command.return_value.run.return_value = MagicMock(revision=9) - dao.find_by_id.return_value = MagicMock(url="/canvas/sales/") + # A later published write must not replace the commit's own revision. + dao.find_by_id.return_value = MagicMock(url="/canvas/sales/", revision=10) result = _commit_canvas_draft_impl("tok", 1) assert result == {"canvas_id": 7, "url": "/canvas/sales/", "canvas_revision": 9} diff --git a/tests/unit_tests/mcp_service/native_tool_inventory.json b/tests/unit_tests/mcp_service/native_tool_inventory.json index 8e71e8db65e2..b97d91c51535 100644 --- a/tests/unit_tests/mcp_service/native_tool_inventory.json +++ b/tests/unit_tests/mcp_service/native_tool_inventory.json @@ -23,14 +23,14 @@ "openWorldHint": false, "readOnlyHint": false }, - "compact_description_chars": 292, - "description_chars": 2326, + "compact_description_chars": 276, + "description_chars": 1970, "entry_bytes": { - "structured": 5699, - "text_only": 2999 + "structured": 9918, + "text_only": 6362 }, - "input_schema_bytes": 232, - "output_schema_bytes": 2684 + "input_schema_bytes": 4075, + "output_schema_bytes": 3540 }, "apply_dashboard_filters": { "annotations": { @@ -55,14 +55,14 @@ "openWorldHint": false, "readOnlyHint": false }, - "compact_description_chars": 257, - "description_chars": 327, + "compact_description_chars": 292, + "description_chars": 768, "entry_bytes": { - "structured": 3454, - "text_only": 754 + "structured": 4759, + "text_only": 1203 }, "input_schema_bytes": 149, - "output_schema_bytes": 2684 + "output_schema_bytes": 3540 }, "create_canvas": { "annotations": { @@ -72,13 +72,13 @@ "readOnlyHint": false }, "compact_description_chars": 163, - "description_chars": 441, + "description_chars": 448, "entry_bytes": { - "structured": 1832, - "text_only": 1074 + "structured": 6021, + "text_only": 4911 }, - "input_schema_bytes": 371, - "output_schema_bytes": 742 + "input_schema_bytes": 4201, + "output_schema_bytes": 1094 }, "create_canvas_draft": { "annotations": { @@ -90,11 +90,11 @@ "compact_description_chars": 291, "description_chars": 362, "entry_bytes": { - "structured": 3485, + "structured": 4341, "text_only": 785 }, "input_schema_bytes": 147, - "output_schema_bytes": 2684 + "output_schema_bytes": 3540 }, "create_dataset": { "annotations": { @@ -170,11 +170,11 @@ "compact_description_chars": 51, "description_chars": 51, "entry_bytes": { - "structured": 3132, + "structured": 3988, "text_only": 432 }, "input_schema_bytes": 108, - "output_schema_bytes": 2684 + "output_schema_bytes": 3540 }, "delete_chart": { "annotations": { @@ -391,14 +391,14 @@ "openWorldHint": false, "readOnlyHint": true }, - "compact_description_chars": 181, - "description_chars": 194, + "compact_description_chars": 283, + "description_chars": 304, "entry_bytes": { - "structured": 3301, - "text_only": 601 + "structured": 4269, + "text_only": 713 }, "input_schema_bytes": 162, - "output_schema_bytes": 2684 + "output_schema_bytes": 3540 }, "get_catalog": { "annotations": { @@ -1402,11 +1402,11 @@ "compact_description_chars": 128, "description_chars": 137, "entry_bytes": { - "structured": 1528, + "structured": 1880, "text_only": 770 }, "input_schema_bytes": 359, - "output_schema_bytes": 742 + "output_schema_bytes": 1094 }, "update_chart": { "annotations": { @@ -1491,12 +1491,12 @@ }, "totals": { "catalog_bytes": { - "structured": 573294, - "text_only": 234467 + "structured": 586039, + "text_only": 242228 }, "compact_catalog_bytes": { - "structured": 512834, - "text_only": 174007 + "structured": 525614, + "text_only": 181803 }, "largest_entry": { "structured": { diff --git a/tests/unit_tests/mcp_service/test_tool_inventory.py b/tests/unit_tests/mcp_service/test_tool_inventory.py index 834f82596e17..0d37a8fd5b24 100644 --- a/tests/unit_tests/mcp_service/test_tool_inventory.py +++ b/tests/unit_tests/mcp_service/test_tool_inventory.py @@ -54,12 +54,16 @@ def budgeted_bytes(text: str) -> int: TOOL_BUDGETS = { "add_chart_to_existing_dashboard": 1_500, - "apply_canvas_draft_ops": 900, + # Includes the canonical Canvas operation grammar inlined in the input + # schema: 5,271 bytes, rounded up plus the standard 100-byte headroom. + "apply_canvas_draft_ops": 5_400, "commit_canvas_draft": 800, "create_canvas_draft": 800, "delete_canvas_draft": 600, "get_canvas_draft": 700, - "create_canvas": 900, + # Includes the same operation grammar for the optional initial operations: + # 5,264 bytes, rounded up plus the standard 100-byte headroom. + "create_canvas": 5_400, "get_canvas": 700, "get_widget_control_schema": 1_100, "list_canvases": 800,