codeant-ai-for-open-source[bot] commented on code in PR #40975:
URL: https://github.com/apache/superset/pull/40975#discussion_r3437472831
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -554,6 +554,138 @@ class CreateVirtualDatasetResponse(BaseModel):
)
+UPDATABLE_METRIC_FIELDS: frozenset[str] = frozenset(
+ {
+ "metric_name",
+ "expression",
+ "verbose_name",
+ "description",
+ "d3format",
+ "metric_type",
+ "currency",
+ "warning_text",
+ "extra",
+ }
+)
+
+
+class MetricCurrency(BaseModel):
+ """Currency formatting configuration for a metric."""
+
+ symbol: str | None = Field(
+ None, description="ISO 4217 currency code (e.g. 'USD', 'EUR')."
+ )
+ symbolPosition: Literal["prefix", "suffix"] | None = Field( # noqa: N815
+ None, description="Where to render the symbol relative to the value."
+ )
+
+
+class UpdateDatasetMetricRequest(BaseModel):
+ """Request schema for update_dataset_metric."""
+
+ 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.",
+ )
+ metric: int | str = Field(
+ ...,
+ description="Metric to update — numeric metric ID, metric UUID, or "
+ "metric_name (e.g. 'sum_revenue'). Numeric strings are treated as IDs.
"
+ "Use get_dataset_info to discover a dataset's saved metrics.",
+ )
+ metric_name: str | None = Field(
+ None,
+ max_length=255,
+ description="New metric name, unique within the dataset. "
+ "Only pass this to rename the metric.",
+ )
+ expression: str | None = Field(
+ None,
+ description="New SQL aggregation expression (e.g. 'SUM(revenue)').",
+ )
+ verbose_name: str | None = Field(
+ None, max_length=1024, description="Human-friendly display label."
+ )
+ description: str | None = Field(None, description="Metric description.")
+ d3format: str | None = Field(
+ None,
+ max_length=128,
+ description="D3 number format string (e.g. ',.2f', '.1%').",
+ )
+ metric_type: str | None = Field(
+ None, max_length=32, description="Metric type (e.g. 'count', 'sum')."
+ )
+ currency: MetricCurrency | None = Field(
+ None, description="Currency formatting configuration."
+ )
+ warning_text: str | None = Field(
+ None, description="Warning shown to users of this metric."
+ )
+ extra: str | None = Field(
+ None, description="JSON-encoded string with extra metric metadata."
+ )
+
+ def updates(self) -> Dict[str, Any]:
+ """Return only the metric properties explicitly provided by the caller.
+
+ ``exclude_unset`` distinguishes "not provided" (leave the stored value
+ alone) from an explicit ``null`` (clear the stored value).
+ """
+ return self.model_dump(
+ exclude_unset=True,
+ include=set(UPDATABLE_METRIC_FIELDS),
+ )
+
+ @model_validator(mode="after")
+ def validate_updates(self) -> "UpdateDatasetMetricRequest":
Review Comment:
**Suggestion:** Add an inline docstring to this newly introduced validator
method so all new functions are documented consistently. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new validator method has no docstring in the final file state, and the
rule requires newly added Python functions and classes to be documented inline.
This is a real rule violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a8d2c2053e424148831c16eb4e22a259&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a8d2c2053e424148831c16eb4e22a259&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/dataset/schemas.py
**Line:** 643:643
**Comment:**
*Custom Rule: Add an inline docstring to this newly introduced
validator method so all new functions are documented consistently.
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%2F40975&comment_hash=83c7eec47e3b8caeb1f285757f855215fd30a38675d0c92c04f1adbfd416444f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40975&comment_hash=83c7eec47e3b8caeb1f285757f855215fd30a38675d0c92c04f1adbfd416444f&reaction=dislike'>👎</a>
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -554,6 +554,138 @@ class CreateVirtualDatasetResponse(BaseModel):
)
+UPDATABLE_METRIC_FIELDS: frozenset[str] = frozenset(
+ {
+ "metric_name",
+ "expression",
+ "verbose_name",
+ "description",
+ "d3format",
+ "metric_type",
+ "currency",
+ "warning_text",
+ "extra",
+ }
+)
+
+
+class MetricCurrency(BaseModel):
+ """Currency formatting configuration for a metric."""
+
+ symbol: str | None = Field(
+ None, description="ISO 4217 currency code (e.g. 'USD', 'EUR')."
+ )
+ symbolPosition: Literal["prefix", "suffix"] | None = Field( # noqa: N815
+ None, description="Where to render the symbol relative to the value."
+ )
+
+
+class UpdateDatasetMetricRequest(BaseModel):
+ """Request schema for update_dataset_metric."""
+
+ 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.",
+ )
+ metric: int | str = Field(
+ ...,
+ description="Metric to update — numeric metric ID, metric UUID, or "
+ "metric_name (e.g. 'sum_revenue'). Numeric strings are treated as IDs.
"
+ "Use get_dataset_info to discover a dataset's saved metrics.",
+ )
+ metric_name: str | None = Field(
+ None,
+ max_length=255,
+ description="New metric name, unique within the dataset. "
+ "Only pass this to rename the metric.",
+ )
+ expression: str | None = Field(
+ None,
+ description="New SQL aggregation expression (e.g. 'SUM(revenue)').",
+ )
+ verbose_name: str | None = Field(
+ None, max_length=1024, description="Human-friendly display label."
+ )
+ description: str | None = Field(None, description="Metric description.")
+ d3format: str | None = Field(
+ None,
+ max_length=128,
+ description="D3 number format string (e.g. ',.2f', '.1%').",
+ )
+ metric_type: str | None = Field(
+ None, max_length=32, description="Metric type (e.g. 'count', 'sum')."
+ )
+ currency: MetricCurrency | None = Field(
+ None, description="Currency formatting configuration."
+ )
+ warning_text: str | None = Field(
+ None, description="Warning shown to users of this metric."
+ )
+ extra: str | None = Field(
+ None, description="JSON-encoded string with extra metric metadata."
+ )
+
+ def updates(self) -> Dict[str, Any]:
+ """Return only the metric properties explicitly provided by the caller.
+
+ ``exclude_unset`` distinguishes "not provided" (leave the stored value
+ alone) from an explicit ``null`` (clear the stored value).
+ """
+ return self.model_dump(
+ exclude_unset=True,
+ include=set(UPDATABLE_METRIC_FIELDS),
+ )
+
+ @model_validator(mode="after")
+ def validate_updates(self) -> "UpdateDatasetMetricRequest":
+ provided = self.model_fields_set & UPDATABLE_METRIC_FIELDS
+ if not provided:
+ raise ValueError(
+ "At least one metric property must be provided to update. "
+ f"Updatable properties: {sorted(UPDATABLE_METRIC_FIELDS)}."
+ )
+ if "metric_name" in provided and not (self.metric_name or "").strip():
+ raise ValueError("metric_name cannot be empty or null")
+ if "expression" in provided and not (self.expression or "").strip():
+ raise ValueError("expression cannot be empty or null")
+ return self
+
+
+class DatasetMetricDetail(SqlMetricInfo):
+ """Full saved-metric details, including identifiers."""
+
+ id: int | None = Field(None, description="Metric ID")
Review Comment:
**Suggestion:** Do not expose the internal integer metric ID in this new
response model when a UUID field is already provided; return only the UUID
public identifier. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new public response model `DatasetMetricDetail` exposes an internal
integer `id` alongside a `uuid` field. This matches the rule about not exposing
internal integer IDs when a UUID public identifier is being added or changed.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=375c3b53ee07455bacaac9119676a275&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=375c3b53ee07455bacaac9119676a275&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/dataset/schemas.py
**Line:** 660:660
**Comment:**
*Custom Rule: Do not expose the internal integer metric ID in this new
response model when a UUID field is already provided; return only the UUID
public identifier.
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%2F40975&comment_hash=a2419970975cbfacde03fc4256bf516f86e75ad05f7d72f19a9f1385e1bcd014&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40975&comment_hash=a2419970975cbfacde03fc4256bf516f86e75ad05f7d72f19a9f1385e1bcd014&reaction=dislike'>👎</a>
##########
superset/mcp_service/dataset/tool/update_dataset_metric.py:
##########
@@ -0,0 +1,258 @@
+# 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 saved metric on a dataset (FastMCP tool)."""
+
+import difflib
+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.dataset.schemas import (
+ DatasetMetricDetail,
+ MetricCurrency,
+ UpdateDatasetMetricRequest,
+ UpdateDatasetMetricResponse,
+)
+from superset.mcp_service.utils import (
+ escape_llm_context_delimiters,
+ sanitize_for_llm_context,
+)
+
+logger = logging.getLogger(__name__)
+
+
+def _find_metric(metrics: list[Any], identifier: int | str) -> Any | None:
+ """Find a saved metric by ID, UUID, or metric_name."""
+ if isinstance(identifier, str):
+ try:
+ identifier = int(identifier)
+ except ValueError:
+ pass
+
+ if isinstance(identifier, int):
+ return next((m for m in metrics if m.id == identifier), None)
+
+ lowered = identifier.lower()
+ by_uuid = next(
+ (m for m in metrics if m.uuid and str(m.uuid).lower() == lowered), None
+ )
+ if by_uuid is not None:
+ return by_uuid
+ return next((m for m in metrics if m.metric_name == identifier), None)
+
+
+def _metric_not_found_message(metrics: list[Any], identifier: int | str) ->
str:
+ names = [m.metric_name for m in metrics]
+ msg = f"Metric '{identifier}' not found on this dataset."
+ if not names:
+ return f"{msg} This dataset has no saved metrics."
+ suggestions = difflib.get_close_matches(str(identifier), names, n=3,
cutoff=0.6)
+ if suggestions:
+ return f"{msg} Did you mean: {', '.join(suggestions)}?"
+ return f"{msg} Available metrics: {', '.join(sorted(names))}."
+
+
+def _serialize_metric(metric: Any) -> DatasetMetricDetail:
+ currency = getattr(metric, "currency", None)
Review Comment:
**Suggestion:** Add an inline docstring to this new helper function
describing what fields are serialized and the expected output object.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is another newly added Python helper function without a docstring.
Since the rule requires new functions to be documented inline, this is a real
violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=aef484f8302944e5912413552ef87644&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=aef484f8302944e5912413552ef87644&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/dataset/tool/update_dataset_metric.py
**Line:** 73:74
**Comment:**
*Custom Rule: Add an inline docstring to this new helper function
describing what fields are serialized and the expected output object.
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%2F40975&comment_hash=763af189e3396f236777da79184b7e9e3fd0290e261e7279e75f406b9d054e60&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40975&comment_hash=763af189e3396f236777da79184b7e9e3fd0290e261e7279e75f406b9d054e60&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py:
##########
@@ -0,0 +1,378 @@
+# 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.
+
+from types import SimpleNamespace
+from unittest.mock import MagicMock, patch
+
+import pytest
+from fastmcp import Client
+from pydantic import ValidationError
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.dataset.schemas import UpdateDatasetMetricRequest
+from superset.mcp_service.utils.sanitization import (
+ LLM_CONTEXT_CLOSE_DELIMITER,
+ LLM_CONTEXT_OPEN_DELIMITER,
+)
+from superset.utils import json
+
+
+def _wrapped(value: str) -> str:
+ return
f"{LLM_CONTEXT_OPEN_DELIMITER}\n{value}\n{LLM_CONTEXT_CLOSE_DELIMITER}"
+
+
[email protected]
+def mcp_server():
+ return mcp
Review Comment:
**Suggestion:** Add a short docstring to the `mcp_server` fixture describing
what object it provides to tests. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The `mcp_server` fixture is a newly added function and it does not include a
docstring. The custom rule requires newly added Python functions and classes to
be documented inline, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=abe3cb60fa2c4fd1be4ba0eebf70e809&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=abe3cb60fa2c4fd1be4ba0eebf70e809&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:**
tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py
**Line:** 38:40
**Comment:**
*Custom Rule: Add a short docstring to the `mcp_server` fixture
describing what object it provides to tests.
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%2F40975&comment_hash=36c920cca450268afe771ceb0a3be6d7e0313fca6ff21376e47865b0f6cb993b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40975&comment_hash=36c920cca450268afe771ceb0a3be6d7e0313fca6ff21376e47865b0f6cb993b&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py:
##########
@@ -0,0 +1,378 @@
+# 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.
+
+from types import SimpleNamespace
+from unittest.mock import MagicMock, patch
+
+import pytest
+from fastmcp import Client
+from pydantic import ValidationError
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.dataset.schemas import UpdateDatasetMetricRequest
+from superset.mcp_service.utils.sanitization import (
+ LLM_CONTEXT_CLOSE_DELIMITER,
+ LLM_CONTEXT_OPEN_DELIMITER,
+)
+from superset.utils import json
+
+
+def _wrapped(value: str) -> str:
+ return
f"{LLM_CONTEXT_OPEN_DELIMITER}\n{value}\n{LLM_CONTEXT_CLOSE_DELIMITER}"
+
+
[email protected]
+def mcp_server():
+ return mcp
+
+
[email protected](autouse=True)
+def mock_auth():
Review Comment:
**Suggestion:** Add explicit type hints to the `mock_auth` fixture
signature, including an appropriate return type for the yielded mock object.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python fixture in a new source file, so the custom
rule requires it to be fully typed. The fixture signature omits a return type
annotation, and the yielded mock object is not typed either, so the suggestion
identifies a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5a8ccc742be643fbbc5998e1513297ff&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5a8ccc742be643fbbc5998e1513297ff&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:**
tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py
**Line:** 43:44
**Comment:**
*Custom Rule: Add explicit type hints to the `mock_auth` fixture
signature, including an appropriate return type for the yielded mock object.
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%2F40975&comment_hash=4c9de39fb98d3fa3f8aac7ca05f53842b2718764720522218f706c73ae454bc0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40975&comment_hash=4c9de39fb98d3fa3f8aac7ca05f53842b2718764720522218f706c73ae454bc0&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]
