aminghadersohi commented on code in PR #46048: URL: https://github.com/apache/superset/pull/46048#discussion_r4497556900
########## superset/mcp_service/dataset/tool/delete_dataset_column.py: ########## @@ -0,0 +1,228 @@ +# 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. + +"""Delete a calculated column on a dataset (FastMCP tool).""" + +from typing import Any + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset import security_manager +from superset.extensions import event_logger +from superset.mcp_service.dataset.schemas import ( + DeleteDatasetColumnRequest, + DeleteDatasetColumnResponse, +) +from superset.mcp_service.dataset.tool.delete_dataset_metric import ( + _find_affected_charts, +) +from superset.mcp_service.dataset.tool.update_dataset_column import ( + _column_not_found_message, + _find_column, + _is_calculated, + _serialize_column, +) + +# Chart config keys whose value is a column name or a list of column names. +_COLUMN_KEYS = frozenset( + { + "all_columns", + "all_columns_x", + "all_columns_y", + "columns", + "entity", + "granularity_sqla", + "groupby", + "groupbyColumns", + "groupbyRows", + "order_by_cols", + "series", + "series_columns", + "x", + "x_axis", + "y", + } +) Review Comment: MCP-saved charts have no `query_context`, so only these keys are checked: a column used as Mixed Timeseries `groupby_b`, a Gantt/histogram column or a Sankey `source`/`target` is missing from `affected_charts`. ```suggestion "y", "column", "groupby_b", "start_time", "end_time", "y_axis", "tooltip_columns", "source", "target", "dimension", } ) ``` ########## superset/mcp_service/dataset/schemas.py: ########## @@ -920,6 +925,254 @@ class DeleteDatasetMetricResponse(BaseModel): error: str | None = Field(None, description="Error message, or null on success") +UPDATABLE_COLUMN_FIELDS: frozenset[str] = frozenset( + { + "column_name", + "expression", + "verbose_name", + "description", + "type", + "is_dttm", + "groupby", + "filterable", + "python_date_format", + } +) + +# Properties tied to a physical column's identity in the source table. Changing +# them on a physical column breaks it rather than editing it. +PHYSICAL_COLUMN_LOCKED_FIELDS: frozenset[str] = frozenset({"column_name", "expression"}) + + +class DatasetColumnProperties(OmittedMeansUnchanged): + """Dataset identifier and writable column properties.""" + + model_config = ConfigDict(populate_by_name=True) + + dataset_id: int | str = Field( + ..., + description="Dataset identifier — numeric ID or UUID string. " + "Use list_datasets to find valid IDs.", + ) + + @field_validator("dataset_id", mode="before") + @classmethod + def reject_bool_identifiers(cls, value: object) -> object: + """bool is a subclass of int, so true would coerce to ID 1.""" + if isinstance(value, bool): + raise ValueError("identifier must be an integer ID or string") + return value + + column_name: str | None = Field( + None, + max_length=255, + description="New column name, unique within the dataset. Only pass this " + "to rename a calculated column; physical columns cannot be renamed.", + ) + expression: str | None = Field( + None, + description="SQL expression evaluated per row, e.g. " + "\"CASE WHEN amount > 100 THEN 'large' ELSE 'small' END\". " + "Only calculated columns have an expression.", + ) + verbose_name: str | None = Field( + None, max_length=1024, description="Human-friendly display label." + ) + description: str | None = Field(None, description="Column description.") + type: str | None = Field( + None, + description="SQL type of the expression's result (e.g. 'VARCHAR', " + "'DOUBLE', 'TIMESTAMP'). Helps Superset pick filters and formatting.", + ) + is_dttm: bool | None = Field( + None, description="Whether the column is temporal (usable as a time axis)." + ) + groupby: bool | None = Field( + None, description="Whether the column can be used as a dimension." + ) + filterable: bool | None = Field( + None, description="Whether the column can be filtered on." + ) + python_date_format: str | None = Field( Review Comment: Nit: An explicit `null` for `groupby`/`filterable` is stored as NULL, which hides the column as a dimension or filter until the next sync flips it back to true; the REST column schema rejects null for both. ```suggestion groupby: bool | None = Field( None, description="Whether the column can be used as a dimension." ) filterable: bool | None = Field( None, description="Whether the column can be filtered on." ) @field_validator("groupby", "filterable") @classmethod def reject_null_flags(cls, value: bool | None) -> bool | None: """Reject an explicit null, as the dataset editor does; omit to keep.""" if value is None: raise ValueError("pass true or false, or omit to leave unchanged") return value python_date_format: str | None = Field( ``` ########## tests/unit_tests/mcp_service/dataset/tool/test_dataset_column_mutations.py: ########## @@ -0,0 +1,606 @@ +# 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. + +"""Calculated column tools exercised through MCP and the dataset commands.""" + +from collections.abc import Iterator +from typing import Any +from unittest.mock import MagicMock, patch, PropertyMock + +import pytest +from fastmcp import Client +from pydantic import ValidationError +from sqlalchemy import create_engine +from sqlalchemy.engine import Engine +from sqlalchemy.orm import Session +from sqlalchemy.pool import StaticPool + +from superset.commands.dataset.columns.exceptions import ( + DatasetColumnDeleteFailedError, + DatasetColumnForbiddenError, + DatasetColumnNotFoundError, +) +from superset.commands.dataset.exceptions import ( + DatasetColumnsDuplicateValidationError, + DatasetForbiddenError, + DatasetInvalidError, + DatasetNotFoundError, + DatasetSoftDeletedTwinExistsError, + DatasetUpdateFailedError, +) +from superset.connectors.sqla.models import SqlaTable, TableColumn +from superset.exceptions import SupersetSecurityException +from superset.mcp_service.app import mcp +from superset.mcp_service.dataset.schemas import ( + CreateDatasetColumnRequest, + DeleteDatasetColumnRequest, + UpdateDatasetColumnRequest, +) +from superset.mcp_service.dataset.tool.delete_dataset_column import ( + _references_column, +) +from superset.models.core import Database +from superset.utils import json + +CREATE, UPDATE, DELETE = ( + "create_dataset_column", + "update_dataset_column", + "delete_dataset_column", +) +TOOLS = [CREATE, UPDATE, DELETE] +UUID = "a1b2c3d4-5678-90ab-cdef-1234567890ab" +UPDATE_COMMAND = "superset.commands.dataset.update.UpdateDatasetCommand" +DELETE_COMMAND = "superset.commands.dataset.columns.delete.DeleteDatasetColumnCommand" +REFERENCES = ( + "superset.mcp_service.dataset.tool.delete_dataset_column._find_affected_charts" +) + + [email protected] +def session_engine() -> Engine: + """Let the tool worker thread use the ``session`` built by this test.""" + return create_engine( + "sqlite://", connect_args={"check_same_thread": False}, poolclass=StaticPool + ) + + [email protected](autouse=True) +def mock_auth() -> Iterator[None]: + """Authenticate an MCP caller without testing authentication itself.""" + with patch("superset.mcp_service.auth.get_user_from_request") as get_user: + get_user.return_value = MagicMock(id=1, username="admin") + yield + + [email protected](autouse=True) +def editorship() -> Iterator[MagicMock]: + """Permit editorship except in tests explicitly denying it.""" + with patch( + "superset.security.SupersetSecurityManager.raise_for_editorship" + ) as check: + yield check + + [email protected] +def dataset() -> MagicMock: + """A dataset with one physical and one calculated column.""" + dataset = MagicMock(id=1, table_name="sales") + dataset.columns = [ + TableColumn(id=20, column_name="amount", type="DOUBLE"), + TableColumn( + id=21, + column_name="order_size", + expression="CASE WHEN amount > 100 THEN 'large' ELSE 'small' END", + type="VARCHAR", + uuid=UUID, + ), + ] + return dataset + + [email protected] +def references() -> Iterator[MagicMock]: + """Avoid database access in tool orchestration tests.""" + with patch(REFERENCES, return_value=[]) as lookup: + yield lookup + + +def request_for(tool: str) -> dict[str, Any]: + """Build a minimal valid request for each tool.""" + if tool == CREATE: + return { + "dataset_id": 1, + "column_name": "double_amount", + "expression": "amount * 2", + } + if tool == UPDATE: + return {"dataset_id": 1, "column": "order_size", "verbose_name": "Size"} + return {"dataset_id": 1, "column": "order_size"} + + +async def call(tool: str, request: dict[str, Any]) -> dict[str, Any]: + """Call the registered tool through MCP and decode its response.""" + async with Client(mcp) as client: + result = await client.call_tool(tool, {"request": request}) + return json.loads(result.content[0].text) + + [email protected]("field", ["column_name", "expression"]) [email protected]("value", [None, "", " "]) +def test_create_rejects_blank_required_properties(field: str, value: Any) -> None: + """Names and SQL expressions cannot be null, empty, or whitespace.""" + request = {**request_for(CREATE), field: value} + with pytest.raises(ValidationError): + CreateDatasetColumnRequest.model_validate(request) + + [email protected]("field", ["column_name", "expression"]) +def test_create_requires_properties(field: str) -> None: + """Both name and expression are mandatory.""" + request = request_for(CREATE) + del request[field] + with pytest.raises(ValidationError): + CreateDatasetColumnRequest.model_validate(request) + + +def test_rejects_invalid_python_date_format() -> None: + """Date formats are checked before the request reaches the command.""" + with pytest.raises(ValidationError, match="python_date_format"): + CreateDatasetColumnRequest.model_validate( + {**request_for(CREATE), "python_date_format": "not a format"} + ) + request = CreateDatasetColumnRequest.model_validate( + {**request_for(CREATE), "python_date_format": "epoch_ms"} + ) + assert request.python_date_format == "epoch_ms" + + +def test_update_requires_a_property() -> None: + """An update with nothing to change is rejected.""" + with pytest.raises(ValidationError, match="At least one column property"): + UpdateDatasetColumnRequest.model_validate({"dataset_id": 1, "column": 21}) + + +def test_update_rejects_clearing_expression() -> None: + """Clearing an expression is a delete, not an update.""" + with pytest.raises(ValidationError, match="delete_dataset_column"): + UpdateDatasetColumnRequest.model_validate( + {"dataset_id": 1, "column": 21, "expression": None} + ) + + [email protected] [email protected]("identifier", [1, "1", UUID]) +async def test_create_success(dataset: MagicMock, identifier: int | str) -> None: + """Optional properties pass through as one additive column write.""" + request = { + **request_for(CREATE), + "dataset_id": identifier, + "verbose_name": "Double amount", + "description": "Exact <UNTRUSTED-CONTENT> text", + "type": "DOUBLE", + "is_dttm": False, + "groupby": True, + "filterable": False, + } + properties = {key: value for key, value in request.items() if key != "dataset_id"} + created = TableColumn(id=22, uuid=UUID, **properties) + updated = MagicMock(id=1, table_name="sales", columns=[*dataset.columns, created]) + with ( + patch( + "superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset + ) as find, + patch(UPDATE_COMMAND) as command, + ): + command.return_value.run.return_value = updated + result = await call(CREATE, request) + assert find.call_args.args == (1 if identifier != UUID else UUID,) + command.assert_called_once_with( + 1, {"columns": [properties]}, preserve_existing_columns=True + ) + assert result["error"] is None + assert result["column"]["id"] == 22 + assert result["column"]["expression"] == "amount * 2" + assert result["column"]["is_calculated"] is True + assert result["column"]["filterable"] is False + assert "datasource_id=1" in result["url"] + + [email protected] +async def test_create_duplicate(dataset: MagicMock) -> None: + """Duplicate names produce actionable errors without invoking persistence.""" + with ( + patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset), + patch(UPDATE_COMMAND) as command, + ): + result = await call(CREATE, {**request_for(CREATE), "column_name": "amount"}) + command.assert_not_called() + assert result["column"] is None + assert "'amount' already exists" in result["error"] + + [email protected] [email protected]("identifier", [21, "21", UUID.upper(), "order_size"]) +async def test_update_success(dataset: MagicMock, identifier: int | str) -> None: + """Only the target column, with only the requested changes, is sent.""" + with ( + patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset), + patch(UPDATE_COMMAND) as command, + ): + command.return_value.run.return_value = dataset + result = await call( + UPDATE, + { + "dataset_id": 1, + "column": identifier, + "column_name": "size_bucket", + "expression": "CASE WHEN amount > 500 THEN 'large' END", + }, + ) + command.assert_called_once_with( + 1, + { + "columns": [ + { + "id": 21, + "column_name": "size_bucket", + "expression": "CASE WHEN amount > 500 THEN 'large' END", + } + ] + }, + preserve_existing_columns=True, + ) + assert result["error"] is None + assert result["updated_properties"] == ["column_name", "expression"] + assert result["column"]["id"] == 21 + + [email protected] +async def test_update_physical_column_display_properties(dataset: MagicMock) -> None: + """Physical columns accept label and behaviour changes.""" + with ( + patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset), + patch(UPDATE_COMMAND) as command, + ): + command.return_value.run.return_value = dataset + result = await call( + UPDATE, + {"dataset_id": 1, "column": "amount", "verbose_name": "Amount ($)"}, + ) + command.assert_called_once() + assert result["error"] is None + assert result["column"]["is_calculated"] is False + + [email protected] [email protected]( + "change", [{"column_name": "amt"}, {"expression": "amount / 100"}] +) +async def test_update_rejects_physical_identity_changes( + dataset: MagicMock, change: dict[str, str] +) -> None: + """Renaming a physical column or giving it an expression would break it.""" + with ( + patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset), + patch(UPDATE_COMMAND) as command, + ): + result = await call(UPDATE, {"dataset_id": 1, "column": "amount", **change}) + command.assert_not_called() + assert "physical column" in result["error"] + + [email protected] +async def test_update_rename_collision(dataset: MagicMock) -> None: + """Renaming onto another column's name is refused up front.""" + with ( + patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset), + patch(UPDATE_COMMAND) as command, + ): + result = await call( + UPDATE, + {"dataset_id": 1, "column": "order_size", "column_name": "amount"}, + ) + command.assert_not_called() + assert "'amount' already exists" in result["error"] + + [email protected] [email protected]("tool", [UPDATE, DELETE]) [email protected]("empty", [False, True]) +async def test_unknown_column(tool: str, dataset: MagicMock, empty: bool) -> None: + """Reuse did-you-mean guidance, including the empty dataset case.""" + if empty: + dataset.columns = [] + with ( + patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset), + patch(UPDATE_COMMAND) as update, + patch(DELETE_COMMAND) as delete, + ): + result = await call(tool, {**request_for(tool), "column": "order_siz"}) + update.assert_not_called() + delete.assert_not_called() + assert result["column"] is None + assert ("no columns" if empty else "Did you mean: order_size?") in result["error"] + + [email protected] [email protected]("identifier", [21, UUID, "order_size"]) +async def test_delete_success( + dataset: MagicMock, references: MagicMock, identifier: int | str +) -> None: + """Delete only the target column and report referencing charts.""" + references.return_value = [{"id": 5, "uuid": UUID, "slice_name": "Sizes"}] + with ( + patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset), + patch(DELETE_COMMAND) as command, + ): + result = await call(DELETE, {"dataset_id": 1, "column": identifier}) + command.assert_called_once_with(1, 21) + command.return_value.run.assert_called_once_with() + assert references.call_args.args == (1, "order_size") + assert references.call_args.kwargs["is_reference"] is _references_column + assert result["error"] is None + assert result["column"]["column_name"] == "order_size" + assert result["affected_charts"] == references.return_value + + [email protected] +async def test_delete_rejects_physical_column( + dataset: MagicMock, references: MagicMock +) -> None: + """Physical columns are managed by syncing the dataset, not deleted here.""" + with ( + patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset), + patch(DELETE_COMMAND) as command, + ): + result = await call(DELETE, {"dataset_id": 1, "column": "amount"}) + command.assert_not_called() + references.assert_not_called() + assert "physical column" in result["error"] + + [email protected] [email protected]("tool", TOOLS) +async def test_unknown_dataset(tool: str) -> None: + """An unknown dataset yields a clear error and no writes.""" + with ( + patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=None), + patch(UPDATE_COMMAND) as update, + patch(DELETE_COMMAND) as delete, + ): + result = await call(tool, request_for(tool)) + update.assert_not_called() + delete.assert_not_called() + assert "No dataset found" in result["error"] + assert result["column"] is None + + [email protected] [email protected]("tool", TOOLS) +async def test_non_editor_cannot_inspect_columns( + tool: str, dataset: MagicMock, editorship: MagicMock +) -> None: + """Deny before duplicate checks, name suggestions, or chart inspection.""" + editorship.side_effect = SupersetSecurityException(MagicMock(message="denied")) + type(dataset).columns = PropertyMock(side_effect=AssertionError("columns read")) + with ( + patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset), + patch(UPDATE_COMMAND) as update, + patch(DELETE_COMMAND) as delete, + ): + result = await call(tool, request_for(tool)) + update.assert_not_called() + delete.assert_not_called() + assert "owner" in result["error"] + assert "order_size" not in result["error"] + + [email protected] [email protected]("tool", [CREATE, UPDATE]) [email protected]( + "exception, message", + [ + (DatasetNotFoundError(), "No dataset found"), + (DatasetForbiddenError(), "owner"), + (DatasetUpdateFailedError(), "Failed to"), + (DatasetSoftDeletedTwinExistsError(UUID), f"/api/v1/dataset/{UUID}/restore"), + ( + DatasetInvalidError(exceptions=[DatasetColumnsDuplicateValidationError()]), + "duplicate", + ), + ], +) +async def test_update_command_errors( + tool: str, exception: Exception, message: str, dataset: MagicMock +) -> None: + """Surface the dataset update command's validation and failures.""" + with ( + patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset), + patch(UPDATE_COMMAND) as command, + ): + command.return_value.run.side_effect = exception + result = await call(tool, request_for(tool)) + assert result["column"] is None + assert message in result["error"] + + [email protected] [email protected]( + "exception, message", + [ + (DatasetColumnNotFoundError(), "not found"), + (DatasetColumnForbiddenError(), "owner"), + (DatasetColumnDeleteFailedError(), "Failed to delete column"), + ], +) +async def test_delete_command_errors( + exception: Exception, message: str, dataset: MagicMock, references: MagicMock +) -> None: + """Surface the column delete command's failures.""" + with ( + patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset), + patch(DELETE_COMMAND) as command, + ): + command.return_value.run.side_effect = exception + result = await call(DELETE, request_for(DELETE)) + assert result["column"] is None + assert message in result["error"] + assert not result.get("affected_charts") + + [email protected]( + "config, expected", + [ + ({"groupby": ["region", "order_size"]}, True), + ({"columns": ["order_size"]}, True), + ({"x_axis": "order_size"}, True), + ({"granularity_sqla": "order_size"}, True), + ({"all_columns": ["order_size"]}, True), + ({"series": "order_size"}, True), + ( + {"adhoc_filters": [{"expressionType": "SIMPLE", "subject": "order_size"}]}, + True, + ), + ({"queries": [{"filters": [{"col": "order_size", "op": "=="}]}]}, True), + ({"queries": [{"columns": ["order_size"]}]}, True), + ({"metrics": [{"column": {"column_name": "order_size"}}]}, True), + ( + {"adhoc_filters": [{"expressionType": "SQL", "subject": "order_size"}]}, + False, + ), + ({"metrics": ["order_size"], "slice_name": "order_size"}, False), + ({"groupby": ["order_size_v2"]}, False), + ({}, False), + ], +) +def test_column_references(config: dict[str, Any], expected: bool) -> None: + """Detect references by column name, not names elsewhere in the config.""" + assert _references_column(config, "order_size") is expected + + +def _make_dataset(session: Session, name: str) -> tuple[SqlaTable, TableColumn]: + """Persist a dataset with one physical column.""" + Database.metadata.create_all(session.bind) + database = Database(database_name=name, sqlalchemy_uri="sqlite://") + physical = TableColumn(column_name="amount", type="DOUBLE") + dataset = SqlaTable(database=database, table_name=name, columns=[physical]) + session.add(dataset) + session.commit() + return dataset, physical + + [email protected] +async def test_real_commands_create_update_delete(session: Session) -> None: + """Persist a calculated column, edit it, and delete it via the real commands.""" + from superset.daos.dataset import DatasetDAO + + dataset, physical = _make_dataset(session, "column_mutations") + dataset_id, physical_id = dataset.id, physical.id + with ( + patch.object(DatasetDAO, "base_filter", None), + patch("superset.security.SupersetSecurityManager.is_admin", return_value=True), + patch(REFERENCES, return_value=[]), + ): + created = await call(CREATE, {**request_for(CREATE), "dataset_id": dataset_id}) + assert created["error"] is None + column_id = created["column"]["id"] + assert created["column"]["uuid"] + session.expire_all() + assert session.get(TableColumn, column_id).expression == "amount * 2" + + updated = await call( + UPDATE, + { + "dataset_id": dataset_id, + "column": column_id, + "expression": "amount * 3", + "verbose_name": "Triple", + }, + ) + assert updated["error"] is None + session.expire_all() + stored = session.get(TableColumn, column_id) + assert (stored.expression, stored.verbose_name) == ("amount * 3", "Triple") + assert stored.column_name == "double_amount" + + deleted = await call(DELETE, {"dataset_id": dataset_id, "column": column_id}) + assert deleted["error"] is None + session.expire_all() + assert session.get(TableColumn, column_id) is None + assert session.get(TableColumn, physical_id) is not None + + [email protected] [email protected]("tool", [CREATE, UPDATE]) +async def test_writes_preserve_column_changes_after_lookup( + session: Session, tool: str +) -> None: + """A concurrent addition and rename must survive the column write.""" + from superset.daos.dataset import DatasetDAO + + dataset, physical = _make_dataset(session, f"concurrent_{tool}") + target = TableColumn(column_name="order_size", expression="amount > 100") + dataset.columns.append(target) + session.commit() + dataset_id, physical_id = dataset.id, physical.id + find_by_id = DatasetDAO.find_by_id + lookup_count = 0 + added = TableColumn(column_name="concurrent", expression="amount + 1") + + def find_with_concurrent_change(*args: Any, **kwargs: Any) -> SqlaTable | None: + """Commit another writer's changes between tool lookup and validation.""" + nonlocal lookup_count + lookup_count += 1 + if lookup_count == 2: + physical.verbose_name = "changed elsewhere" + dataset.columns.append(added) + session.commit() + return find_by_id(*args, **kwargs) + + request = ( + {**request_for(CREATE), "dataset_id": dataset_id} + if tool == CREATE + else {"dataset_id": dataset_id, "column": "order_size", "verbose_name": "Size"} + ) + with ( + patch.object(DatasetDAO, "base_filter", None), + patch.object(DatasetDAO, "find_by_id", side_effect=find_with_concurrent_change), + patch("superset.security.SupersetSecurityManager.is_admin", return_value=True), + ): + result = await call(tool, request) + assert result["error"] is None + session.expire_all() + assert session.get(TableColumn, physical_id).verbose_name == "changed elsewhere" + names = { + column.column_name for column in session.get(SqlaTable, dataset_id).columns + } + expected = {"amount", "order_size", "concurrent"} + assert names == (expected | {"double_amount"} if tool == CREATE else expected) + + [email protected]( + "model, payload", + [ + (UpdateDatasetColumnRequest, {"dataset_id": True, "column": 1}), + (UpdateDatasetColumnRequest, {"dataset_id": 1, "column": True}), Review Comment: Nit: Both Update payloads have no updatable property, so `validate_updates` raises `ValidationError` even with the boolean guards removed (verified by mutation); add a property so these cases pin the guards. ```suggestion (UpdateDatasetColumnRequest, {"dataset_id": True, "column": 1, "type": "INT"}), (UpdateDatasetColumnRequest, {"dataset_id": 1, "column": True, "type": "INT"}), ``` ########## superset/mcp_service/dataset/tool/update_dataset_column.py: ########## @@ -0,0 +1,294 @@ +# 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. + +"""Update a column on a dataset (FastMCP tool).""" + +import difflib +from typing import Any + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset import security_manager +from superset.extensions import event_logger +from superset.mcp_service.dataset.schemas import ( + _calculated_expression, + DatasetColumnDetail, + PHYSICAL_COLUMN_LOCKED_FIELDS, + UpdateDatasetColumnRequest, + UpdateDatasetColumnResponse, +) + + +def _is_calculated(column: Any) -> bool: + """A calculated column is one defined by a non-blank SQL expression.""" + return _calculated_expression(column) is not None Review Comment: Expanded Trino/BigQuery nested fields carry an expression equal to their quoted path but are physical (`_is_calculated_column` in models.py). This check calls them calculated, so `update_dataset_column` renames and `delete_dataset_column` deletes them. ```suggestion def _is_calculated(column: Any) -> bool: """A user-defined calculated column, not an expanded nested source field.""" from superset.connectors.sqla.models import _is_calculated_column return _calculated_expression(column) is not None and _is_calculated_column(column) ``` -- 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]
