codeant-ai-for-open-source[bot] commented on code in PR #40361: URL: https://github.com/apache/superset/pull/40361#discussion_r3326681951
########## superset/mcp_service/saved_query/tool/create_saved_query.py: ########## @@ -0,0 +1,126 @@ +# 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 typing import Any + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.saved_query.schemas import ( + CreateSavedQueryRequest, + CreateSavedQueryResponse, +) + +logger = logging.getLogger(__name__) + + +@tool( + tags=["mutate"], + class_permission_name="SavedQuery", + method_permission_name="write", + annotations=ToolAnnotations( + title="Create saved query", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def create_saved_query( + request: CreateSavedQueryRequest, ctx: Context +) -> CreateSavedQueryResponse: + """Save a SQL query to the Saved Queries list so it can be reloaded and shared. + + Creates a persistent SavedQuery that appears in the Saved Queries page + and can be opened in SQL Lab via the returned URL. + + Workflow: + 1. Call execute_sql to verify the query returns expected results + 2. Call this tool with a label and the SQL to persist it + 3. Use the returned ``url`` to open the saved query in SQL Lab + """ + await ctx.info( + "Creating saved query: db_id=%s, label=%r" % (request.db_id, request.label) + ) + + try: + from superset.commands.query.create import CreateSavedQueryCommand + from superset.commands.query.exceptions import ( + SavedQueryCreateFailedError, + SavedQueryInvalidError, + ) + from superset.mcp_service.utils.url_utils import get_superset_base_url + + properties: dict[str, Any] = { + "db_id": request.db_id, + "label": request.label, + "sql": request.sql, + } Review Comment: **Suggestion:** The new create tool never persists `catalog`, even though SavedQuery supports it and the existing SQL Lab save tool writes it. For engines that require a catalog, saved queries created here will reopen with incomplete connection context and can fail or target the wrong namespace. Add `catalog` to the request/response schema and include it in the properties passed to `CreateSavedQueryCommand`. [incomplete implementation] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ create_saved_query MCP tool drops catalog on SavedQuery creation. - ⚠️ SQL Lab reopens saved queries without correct catalog context. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start the MCP server using `init_fastmcp_server` in `superset/mcp_service/app.py:104-131`, which imports and registers `create_saved_query` at `superset/mcp_service/app.py:63-66`. 2. From an MCP client, call the `create_saved_query` tool, which is implemented by `create_saved_query()` in `superset/mcp_service/saved_query/tool/create_saved_query.py:43-55` and takes a `CreateSavedQueryRequest` defined in `superset/mcp_service/saved_query/schemas.py:23-58` (this schema has `label`, `sql`, `db_id`, `schema`, `description`, `template_parameters` but no `catalog` field). 3. Inside `create_saved_query`, the properties dict is built at `superset/mcp_service/saved_query/tool/create_saved_query.py:68-78` with keys `db_id`, `label`, `sql`, and optional `schema`, `description`, `template_parameters` only; no `catalog` key is ever added, so the catalog context from the original query (if any) is dropped. 4. `CreateSavedQueryCommand` in `superset/commands/query/create.py:36-45` passes this properties dict to `SavedQueryDAO.create`, which constructs a `SavedQuery` ORM object (`superset/models/sql_lab.py:3-21`) whose `catalog` column (`superset/models/sql_lab.py:17`, and exported in `export_fields` at lines 44-51) remains `NULL`, whereas the existing `save_sql_query` MCP tool in `superset/mcp_service/sql_lab/tool/save_sql_query.py:100-109` explicitly sets `"catalog": request.catalog`; as a result, saved queries created via `create_saved_query` for multi-catalog databases lose their catalog context and may reopen in SQL Lab with incomplete or wrong connection settings. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=90cc22149a8a48f1a947496957672f99&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=90cc22149a8a48f1a947496957672f99&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/saved_query/tool/create_saved_query.py **Line:** 68:72 **Comment:** *Incomplete Implementation: The new create tool never persists `catalog`, even though SavedQuery supports it and the existing SQL Lab save tool writes it. For engines that require a catalog, saved queries created here will reopen with incomplete connection context and can fail or target the wrong namespace. Add `catalog` to the request/response schema and include it in the properties passed to `CreateSavedQueryCommand`. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40361&comment_hash=dd03a673222732294ae68d1fb1179331c4abb7aeb277a4d29c28f1c12f60722b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40361&comment_hash=dd03a673222732294ae68d1fb1179331c4abb7aeb277a4d29c28f1c12f60722b&reaction=dislike'>👎</a> ########## superset/mcp_service/saved_query/tool/update_saved_query.py: ########## @@ -0,0 +1,142 @@ +# 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 typing import Any + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.saved_query.schemas import ( + UpdateSavedQueryRequest, + UpdateSavedQueryResponse, +) + +logger = logging.getLogger(__name__) + +_LOGGABLE_FIELDS = ( + "label", + "sql", + "db_id", + "schema", + "description", + "template_parameters", +) + + +def _build_update_properties(request: "UpdateSavedQueryRequest") -> dict[str, Any]: + """Return only the fields the caller explicitly provided.""" + fields = { + "label": request.label, + "sql": request.sql, + "db_id": request.db_id, + "schema": request.schema, + "description": request.description, + "template_parameters": request.template_parameters, + } + return {k: v for k, v in fields.items() if v is not None} + + +@tool( + tags=["mutate"], + class_permission_name="SavedQuery", + method_permission_name="write", + annotations=ToolAnnotations( + title="Update saved query", + readOnlyHint=False, + destructiveHint=True, + ), +) +async def update_saved_query( + request: UpdateSavedQueryRequest, ctx: Context +) -> UpdateSavedQueryResponse: + """Update an existing saved query's label, SQL, database, schema, or description. + + All fields except ``id`` are optional — only provided fields are changed. + The query must already exist and the caller must have write access to + the SavedQuery resource. + + Example: rename only + ```json + {"id": 42, "label": "Monthly Revenue"} + ``` + + Example: update SQL and description + ```json + {"id": 42, "sql": "SELECT * FROM orders LIMIT 100", "description": "All orders"} + ``` + """ + changed = [f for f in _LOGGABLE_FIELDS if getattr(request, f, None) is not None] + await ctx.info("Updating saved query: id=%s, fields=%s" % (request.id, changed)) + + try: + from superset.commands.query.exceptions import ( + SavedQueryInvalidError, + SavedQueryNotFoundError, + SavedQueryUpdateFailedError, + ) + from superset.commands.query.update import UpdateSavedQueryCommand + from superset.mcp_service.utils.url_utils import get_superset_base_url + + properties = _build_update_properties(request) + + with event_logger.log_context(action="mcp.update_saved_query.update"): + saved_query = UpdateSavedQueryCommand(request.id, properties).run() Review Comment: **Suggestion:** The tool accepts requests with only `id` and treats them as successful updates, but this performs no mutation and can silently hide client-side request bugs. Validate that at least one mutable field is provided and return a structured validation error when the update payload is empty. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ update_saved_query silently accepts id-only no-op requests. - ⚠️ Client request bugs go undetected by MCP saved-query tool. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The MCP server registers `update_saved_query` in `superset/mcp_service/app.py:63-66`, and clients send requests conforming to `UpdateSavedQueryRequest` defined in `superset/mcp_service/saved_query/schemas.py:74-103`, where only `id` is required and all other fields (`label`, `sql`, `db_id`, `schema`, `description`, `template_parameters`) are optional and default to `None`. 2. A client issues an `update_saved_query` call with only `id` set (all mutable fields omitted), so inside `update_saved_query` at `superset/mcp_service/saved_query/tool/update_saved_query.py:84-85` the `changed` list is computed from `_LOGGABLE_FIELDS` but becomes empty, and the function still proceeds after logging. 3. `_build_update_properties(request)` at `superset/mcp_service/saved_query/tool/update_saved_query.py:42-52` constructs the `fields` dict and filters out any keys whose values are `None`; with an id-only request this returns an empty dict, so `properties` is `{}` at line 96 and is passed unchanged to `UpdateSavedQueryCommand(request.id, properties)` at lines 98-99. 4. `UpdateSavedQueryCommand.run` in `superset/commands/query/update.py:42-47` calls `validate()` (which only ensures the saved query exists and validates `db_id` if present) and then invokes `SavedQueryDAO.update(self._model, attributes=self._properties)` with `attributes={}`, resulting in a no-op update; nonetheless, `update_saved_query` returns an `UpdateSavedQueryResponse` with `id` set and no error (`superset/mcp_service/saved_query/schemas.py:119-135`), meaning id-only requests that perform no mutation are treated as successful updates and can silently hide client-side bugs where callers forget to include any mutable fields. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=af913a04c368475baa1c2f6234785081&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=af913a04c368475baa1c2f6234785081&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/saved_query/tool/update_saved_query.py **Line:** 96:99 **Comment:** *Logic Error: The tool accepts requests with only `id` and treats them as successful updates, but this performs no mutation and can silently hide client-side request bugs. Validate that at least one mutable field is provided and return a structured validation error when the update payload is empty. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40361&comment_hash=41344e4f7e5911ccc72107142ed70d169c30fd2051eca9744dfe8335922ab5ae&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40361&comment_hash=41344e4f7e5911ccc72107142ed70d169c30fd2051eca9744dfe8335922ab5ae&reaction=dislike'>👎</a> ########## superset/mcp_service/saved_query/tool/update_saved_query.py: ########## @@ -0,0 +1,142 @@ +# 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 typing import Any + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.saved_query.schemas import ( + UpdateSavedQueryRequest, + UpdateSavedQueryResponse, +) + +logger = logging.getLogger(__name__) + +_LOGGABLE_FIELDS = ( + "label", + "sql", + "db_id", + "schema", + "description", + "template_parameters", +) + + +def _build_update_properties(request: "UpdateSavedQueryRequest") -> dict[str, Any]: + """Return only the fields the caller explicitly provided.""" + fields = { + "label": request.label, + "sql": request.sql, + "db_id": request.db_id, + "schema": request.schema, + "description": request.description, + "template_parameters": request.template_parameters, + } Review Comment: **Suggestion:** Update payload construction omits `catalog`, so callers cannot update it, and changing `db_id` can leave an old catalog value attached to the query. That creates inconsistent saved-query metadata across database changes. Include `catalog` in request schema and update properties (or explicitly clear catalog when `db_id` changes). [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ update_saved_query can't modify catalog for existing SavedQuery records. - ⚠️ Changing db_id leaves old catalog, causing inconsistent metadata. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start the MCP server via `init_fastmcp_server` in `superset/mcp_service/app.py:104-131`, which imports and registers `update_saved_query` at `superset/mcp_service/app.py:63-66`. 2. An MCP client sends an `UpdateSavedQueryRequest` (schema in `superset/mcp_service/saved_query/schemas.py:74-103`) to `update_saved_query`; this schema exposes optional `label`, `sql`, `db_id`, `schema`, `description`, `template_parameters` fields but no `catalog` field, even though the `SavedQuery` model in `superset/models/sql_lab.py:3-21` defines a `catalog` column at line 17 and the REST API `SavedQueryRestApi` includes `catalog` in `add_columns` and `edit_columns` at `superset/queries/saved_queries/api.py:134-144`. 3. Inside `update_saved_query`, `_build_update_properties()` at `superset/mcp_service/saved_query/tool/update_saved_query.py:42-52` builds a `fields` dict containing only `label`, `sql`, `db_id`, `schema`, `description`, and `template_parameters`, filters out `None` values, and never includes `catalog`; thus, even if a saved query was created with a `catalog` (e.g., via `save_sql_query` which sets `"catalog": request.catalog` in `superset/mcp_service/sql_lab/tool/save_sql_query.py:100-109`), callers of `update_saved_query` have no way to change or clear that catalog. 4. When a client changes only `db_id` via `update_saved_query`, `_build_update_properties` returns a dict containing the new `db_id` but no `catalog`, and `UpdateSavedQueryCommand` at `superset/commands/query/update.py:36-47` calls `SavedQueryDAO.update(self._model, attributes=self._properties)` with this partial payload; this updates the `db_id` column but leaves the old `catalog` column (`superset/models/sql_lab.py:17`, exported in `export_fields` at 44-51) unchanged, resulting in inconsistent saved-query metadata where the catalog no longer matches the database for engines that rely on catalog to select the correct namespace. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b6e0ced0f1bf42e49ae8c69bcf6933a2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b6e0ced0f1bf42e49ae8c69bcf6933a2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/saved_query/tool/update_saved_query.py **Line:** 44:51 **Comment:** *Api Mismatch: Update payload construction omits `catalog`, so callers cannot update it, and changing `db_id` can leave an old catalog value attached to the query. That creates inconsistent saved-query metadata across database changes. Include `catalog` in request schema and update properties (or explicitly clear catalog when `db_id` changes). Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40361&comment_hash=c3bacd2576b9b32a6fb02e50e61d4ed28cb012737bd02532966bf3afa224b42a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40361&comment_hash=c3bacd2576b9b32a6fb02e50e61d4ed28cb012737bd02532966bf3afa224b42a&reaction=dislike'>👎</a> -- 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]
