rebenitez1802 opened a new pull request, #45826:
URL: https://github.com/apache/superset/pull/45826
### SUMMARY
Functional fixes for the Canvas draft lifecycle and its MCP/REST contract,
targeting `msyavuz/feat/canvas`.
**Draft lifecycle (3 bugs, each reproduced before the fix)**
- **Canceled new widgets.** If a draft added a new widget and then removed
it, the staged entity was pruned mid-batch. So the commit failed with 422 "add
widgets that cannot be placed" (separate calls, one batch, parent removal,
surviving sibling). A re-added widget also lost its stored content. Drafts now
keep new widget entities until the final replay (`keep_unplaced`). The commit
then drops new widgets that end up unplaced, so no orphan widget rows are
written. Edits to existing widgets are never pruned.
- **Explicit-null resets.** `set_scope.scope: null` and `set_settings.value:
null` were removed from the replay log (`exclude_none`), so replay failed with
`Field required`. The nulls are now logged explicitly. An omitted field is
still a validation error, never a reset.
- **Empty close revision.** Closing an empty draft returned the fork's
`base_revision`, even after another publish. `CommitCanvasDraftCommand` now
returns a typed `CommitResult` with the canvas revision actually written or
observed. Both callers (REST and MCP) are updated.
**MCP contract**
- `apply_canvas_draft_ops.ops` and `create_canvas.ops` advertise the
canonical `Operation` union in their input schemas
(`BeforeValidator(json_schema_input_type=...)`). There is no second,
handwritten grammar.
- The arguments are unchanged: still flat, camelCase.
- Omitted, null and default values behave as before. `create_canvas` still
accepts `ops` omitted, `null` or `[]`.
- Indexed field-path errors (`/ops/1/index`) are preserved.
- Results add `error_type` (`ValidationError`, `DraftUnavailable`,
`DraftRevisionConflict`, `CanvasRevisionConflict`, `WidgetRevisionConflict`),
`draft_revision` and `current_canvas_revision`. Legacy keys are unchanged.
- An unavailable or foreign token discloses no revisions.
- Tool descriptions for `commit_canvas_draft` and `apply_canvas_draft_ops`
now lead with publish intent and conflict recovery. That guidance survives the
300-character tool-search truncation.
**REST**
- 409 bodies add `error_type`, `draftRevision` and `currentCanvasRevision`
next to the existing `revision` and `canvasChanged`.
- 404 for an unavailable draft adds `error_type: DraftUnavailable`.
- Status codes and existing keys are unchanged.
**Behavior worth knowing (pinned by tests, documented in
`mcp-server.mdx`):** a draft whose operations cancel out, e.g. add a new widget
and then remove it, commits as a real write. That is the same result as
publishing those operations without a draft. If the canvas changed meanwhile,
the commit returns 409 `CanvasRevisionConflict`. Treating such a draft as an
empty close would be a new behavior and is left for a follow-up.
The query translator and chart-data path were already correct. The new
real-SQL tests confirm this, and no query code changed.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend and MCP only).
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/canvas tests/unit_tests/mcp_service/canvas \
tests/unit_tests/mcp_service/test_tool_search_transform.py \
tests/unit_tests/mcp_service/test_mcp_server.py \
tests/unit_tests/mcp_service/test_native_tool_surface.py \
tests/unit_tests/mcp_service/test_tool_inventory.py
```
- **Focused suite:** 750 pass.
- **Run against the old code:** the new lifecycle tests fail 8 times (5
canceled-widget cases, 1 empty-close revision, 2 null resets).
- **Mutation testing:** reverting any single production fix turns tests red.
- **Wider sweep:** `tests/unit_tests/mcp_service` plus
`tests/unit_tests/canvas` gives 13,665 passed and 0 failed (base: 13,502
passed).
- **Checks:** OpenAPI regenerated with marshmallow 4.3.1 shows no drift.
Pre-commit, MyPy and Pylint (10.00/10) pass on the changed files.
Test additions:
- `tests/unit_tests/canvas/widget_entities_test.py`, `draft_test.py`,
`api_test.py`: lifecycle regressions, conflict precedence and provenance, REST
payloads.
- `tests/unit_tests/canvas/placement_data_test.py`: real SQLite aggregate
queries (top and bottom 5, unsorted limit, query-text visibility) through an
unmocked `ChartDataCommand`.
- `tests/unit_tests/mcp_service/canvas/tool/test_canvas_contract.py` (new):
schema exposure, and native vs. `search_tools`/`call_tool` proxy parity with
protected wrappers and RBAC enabled.
`native_tool_inventory.json` and the
`apply_canvas_draft_ops`/`create_canvas` budgets in `test_tool_inventory.py`
were regenerated or raised. The inlined operation grammar grows each entry to
about 5.3 KB. Only Canvas entries and catalog totals changed.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [x] Required feature flags: `CANVAS`
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
