Copilot commented on code in PR #40358: URL: https://github.com/apache/superset/pull/40358#discussion_r3285753121
########## superset/mcp_service/tag/tool/create_tag.py: ########## @@ -0,0 +1,111 @@ +# 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. + +import logging + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.tag.schemas import CreateTagRequest, CreateTagResponse + +logger = logging.getLogger(__name__) + + +@tool( + tags=["mutate"], + class_permission_name="Tag", + method_permission_name="write", + annotations=ToolAnnotations( + title="Create a tag", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def create_tag(request: CreateTagRequest, ctx: Context) -> CreateTagResponse: + """Create a new custom tag in Superset, optionally applying it to objects. + + Use this tool to create tags for organizing charts, dashboards, datasets, + and queries. Tags can be applied to multiple objects at creation time. + + Workflow: + 1. Call this tool with a tag name and optional description + 2. Optionally provide objects_to_tag to apply the tag immediately + 3. Use the returned ``id`` to reference the tag in future operations + """ + await ctx.info("Creating tag: name=%r" % (request.name,)) + + try: + from superset.commands.tag.create import CreateCustomTagWithRelationshipsCommand + from superset.commands.tag.exceptions import ( + TagCreateFailedError, + TagInvalidError, + ) + from superset.daos.tag import TagDAO + + with event_logger.log_context(action="mcp.create_tag.create"): + properties = { + "name": request.name, + "description": request.description or "", + "objects_to_tag": request.objects_to_tag, + } + objects_tagged, objects_skipped = CreateCustomTagWithRelationshipsCommand( Review Comment: The tool forces `description` to `""` when sending properties (`request.description or ""`), but the response echoes `request.description` (possibly `None`). This can make the MCP response disagree with what was persisted. Consider normalizing `description` consistently (e.g., strip/None handling) and returning the saved value (`tag.description`) so clients get an accurate result. ########## superset/mcp_service/tag/tool/create_tag.py: ########## @@ -0,0 +1,111 @@ +# 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. + +import logging + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.tag.schemas import CreateTagRequest, CreateTagResponse + +logger = logging.getLogger(__name__) + + +@tool( + tags=["mutate"], + class_permission_name="Tag", + method_permission_name="write", + annotations=ToolAnnotations( + title="Create a tag", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def create_tag(request: CreateTagRequest, ctx: Context) -> CreateTagResponse: + """Create a new custom tag in Superset, optionally applying it to objects. + + Use this tool to create tags for organizing charts, dashboards, datasets, + and queries. Tags can be applied to multiple objects at creation time. + + Workflow: + 1. Call this tool with a tag name and optional description + 2. Optionally provide objects_to_tag to apply the tag immediately + 3. Use the returned ``id`` to reference the tag in future operations + """ + await ctx.info("Creating tag: name=%r" % (request.name,)) + + try: + from superset.commands.tag.create import CreateCustomTagWithRelationshipsCommand + from superset.commands.tag.exceptions import ( + TagCreateFailedError, + TagInvalidError, + ) + from superset.daos.tag import TagDAO + + with event_logger.log_context(action="mcp.create_tag.create"): + properties = { + "name": request.name, + "description": request.description or "", + "objects_to_tag": request.objects_to_tag, + } + objects_tagged, objects_skipped = CreateCustomTagWithRelationshipsCommand( + properties + ).run() + + tag = TagDAO.find_by_name(request.name) Review Comment: `CreateCustomTagWithRelationshipsCommand` strips the tag name before creating/looking it up (`tag_name.strip()`), but this tool re-queries with `TagDAO.find_by_name(request.name)` (no strip). If the request includes leading/trailing whitespace, `tag` can be `None` even though creation succeeded, and the response `id` will be null. Normalize `request.name` (strip + reject whitespace-only) and use the normalized name consistently for both command properties and DAO lookup/response. ########## superset/mcp_service/app.py: ########## @@ -670,6 +670,9 @@ def create_mcp_app( get_schema, health_check, ) +from superset.mcp_service.tag.tool import ( # noqa: F401, E402 + create_tag, +) Review Comment: The default MCP instructions in this file include a curated “Available tools” list used to guide LLM behavior, but `create_tag` isn’t listed there. Add an entry (e.g., under a Tag Management section) so agents know when/how to use the new tool. ########## superset/mcp_service/tag/schemas.py: ########## @@ -0,0 +1,68 @@ +# 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. + +"""Pydantic schemas for tag-related MCP tools.""" + +from pydantic import BaseModel, ConfigDict, Field + + +class CreateTagRequest(BaseModel): + model_config = ConfigDict(populate_by_name=True) + + name: str = Field( + ..., + min_length=1, + description=( + "Name for the new tag. Must be unique. " + "Used to identify and reference the tag across Superset." + ), + ) + description: str | None = Field( + None, + description="Optional human-readable description for the tag.", + ) + objects_to_tag: list[tuple[str, int]] = Field( + default_factory=list, + description=( + "Optional list of objects to apply this tag to immediately after creation. " + "Each item is a [object_type, object_id] pair where object_type is one of: " + "'chart', 'dashboard', 'dataset', 'query'." + ), + ) Review Comment: `CreateTagRequest.name` doesn’t enforce the Tag model’s max length (Tag.name is `String(250)`) and doesn’t strip/reject whitespace-only names. Also, `objects_to_tag` IDs aren’t constrained to `>=1` even though the REST schema validates `Range(min=1)`. Adding Pydantic constraints/validators here will prevent DB errors and match existing API behavior. ########## superset/mcp_service/tag/tool/create_tag.py: ########## @@ -0,0 +1,111 @@ +# 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. + +import logging + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.tag.schemas import CreateTagRequest, CreateTagResponse + +logger = logging.getLogger(__name__) + + +@tool( + tags=["mutate"], + class_permission_name="Tag", + method_permission_name="write", + annotations=ToolAnnotations( + title="Create a tag", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def create_tag(request: CreateTagRequest, ctx: Context) -> CreateTagResponse: + """Create a new custom tag in Superset, optionally applying it to objects. + + Use this tool to create tags for organizing charts, dashboards, datasets, + and queries. Tags can be applied to multiple objects at creation time. + + Workflow: + 1. Call this tool with a tag name and optional description + 2. Optionally provide objects_to_tag to apply the tag immediately + 3. Use the returned ``id`` to reference the tag in future operations + """ + await ctx.info("Creating tag: name=%r" % (request.name,)) + + try: + from superset.commands.tag.create import CreateCustomTagWithRelationshipsCommand + from superset.commands.tag.exceptions import ( + TagCreateFailedError, + TagInvalidError, + ) + from superset.daos.tag import TagDAO + + with event_logger.log_context(action="mcp.create_tag.create"): + properties = { + "name": request.name, + "description": request.description or "", + "objects_to_tag": request.objects_to_tag, + } + objects_tagged, objects_skipped = CreateCustomTagWithRelationshipsCommand( + properties + ).run() + + tag = TagDAO.find_by_name(request.name) + + await ctx.info( + "Tag created: id=%s, name=%r, objects_tagged=%d, objects_skipped=%d" + % ( + tag.id if tag else None, + request.name, + len(objects_tagged), + len(objects_skipped), + ) + ) + + return CreateTagResponse( + id=tag.id if tag else None, + name=request.name, + description=request.description, + objects_tagged=list(objects_tagged), + objects_skipped=list(objects_skipped), + ) Review Comment: `objects_tagged` / `objects_skipped` come back as sets from the command and are converted with `list(...)`. Set iteration order is non-deterministic, which can make responses (and future tests) flaky. Consider returning a stable ordering (e.g., sort by `(object_type, object_id)`). ########## superset/mcp_service/tag/tool/create_tag.py: ########## @@ -0,0 +1,111 @@ +# 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. + +import logging + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.tag.schemas import CreateTagRequest, CreateTagResponse + +logger = logging.getLogger(__name__) + + +@tool( + tags=["mutate"], + class_permission_name="Tag", + method_permission_name="write", + annotations=ToolAnnotations( + title="Create a tag", + readOnlyHint=False, + destructiveHint=False, + ), Review Comment: `create_tag` can be destructive when the tag name already exists: `CreateCustomTagWithRelationshipsCommand(...).run()` calls `TagDAO.create_tag_relationship(...)` which deletes existing tag/object relationships when `bulk_create=False` and `objects_to_tag` is empty (or doesn’t include previously tagged objects). Either enforce “create-only” semantics (fail if the tag already exists), or invoke the command with `bulk_create=True` / adjust the tool so it never removes existing relationships, and update `destructiveHint` accordingly. ########## superset/mcp_service/tag/tool/create_tag.py: ########## @@ -0,0 +1,111 @@ +# 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. + +import logging + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.tag.schemas import CreateTagRequest, CreateTagResponse + +logger = logging.getLogger(__name__) + + +@tool( + tags=["mutate"], + class_permission_name="Tag", + method_permission_name="write", + annotations=ToolAnnotations( + title="Create a tag", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def create_tag(request: CreateTagRequest, ctx: Context) -> CreateTagResponse: + """Create a new custom tag in Superset, optionally applying it to objects. + + Use this tool to create tags for organizing charts, dashboards, datasets, + and queries. Tags can be applied to multiple objects at creation time. + + Workflow: + 1. Call this tool with a tag name and optional description + 2. Optionally provide objects_to_tag to apply the tag immediately + 3. Use the returned ``id`` to reference the tag in future operations + """ + await ctx.info("Creating tag: name=%r" % (request.name,)) Review Comment: This PR introduces a new MCP mutation tool, but there are no unit tests added for its request schema and tool behavior. Existing MCP tools have dedicated unit tests (including happy path + permission/validation error cases); adding similar coverage for `create_tag` will help prevent regressions and ensure stable MCP responses. -- 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]
