codeant-ai-for-open-source[bot] commented on code in PR #40975:
URL: https://github.com/apache/superset/pull/40975#discussion_r3437500316
##########
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")
+ uuid: str | None = Field(None, description="Metric UUID")
+ metric_type: str | None = Field(None, description="Metric type")
+ currency: MetricCurrency | None = Field(
+ None, description="Currency formatting configuration"
+ )
+ warning_text: str | None = Field(None, description="Warning text")
+
Review Comment:
**Suggestion:** `extra` is declared as an updatable metric property, but the
response model for the updated metric omits that field. This creates an API
contract gap where updates to `extra` succeed but are not returned in `metric`,
so callers cannot verify the applied value and may treat the update as failed
or stale. Add `extra` to `DatasetMetricDetail` (and keep serialization aligned)
so response payloads reflect all supported updates. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP update_dataset_metric hides updated metric extra metadata.
- ⚠️ Callers cannot verify extra value via MCP response.
- ⚠️ Request/response contract inconsistent for extra field updates.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the MCP server (`mcp` fixture from `superset.mcp_service.app`,
imported in
`tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py:25`)
and connect
a FastMCP client as done in `test_update_dataset_metric_success` at lines
139–184 (the
client calls the `"update_dataset_metric"` tool via
`Client(mcp_server).call_tool(...)`).
2. From the client, invoke the `update_dataset_metric` tool (entry point
`superset/mcp_service/dataset/tool/update_dataset_metric.py:113`) with a
request body that
includes `extra`, for example:
`{"request": {"dataset_id": 1, "metric": "count", "extra": "{\"foo\":
\"bar\"}"}}`.
This request is validated by `UpdateDatasetMetricRequest` in
`superset/mcp_service/dataset/schemas.py:64-110`, where `extra` is an
allowed,
updatable field (`UPDATABLE_METRIC_FIELDS` includes `"extra"` at lines
38–50 in the
same file, corresponding to diff lines 557–569).
3. In `update_dataset_metric`, the tool computes `updates =
request.updates()` at
`update_dataset_metric.py:142`, which includes `"extra": "<value>"` because
`UPDATABLE_METRIC_FIELDS` contains `"extra"` and `updates()` explicitly
includes all
provided updatable fields (lines 112–121 of `schemas.py`). The tool then
persists these
updates via `UpdateDatasetCommand(...).run()` at
`update_dataset_metric.py:207–209`, so
the underlying metric's `extra` is updated successfully.
4. After the update, the tool builds the response metric via
`_serialize_metric(updated_metric)` at `update_dataset_metric.py:220–223`.
`_serialize_metric` (lines 73–100) constructs a `DatasetMetricDetail`
instance, but only
passes `id`, `uuid`, `metric_name`, `verbose_name`, `expression`,
`description`,
`d3format`, `metric_type`, `currency`, and `warning_text`; it never includes
`extra`.
`DatasetMetricDetail` itself (defined in
`superset/mcp_service/dataset/schemas.py:138–147`, diff lines 657–667)
extends
`SqlMetricInfo` (lines 120–129) and adds `metric_type`, `currency`, and
`warning_text`,
but does not declare an `extra` field. As a result, when the MCP client
deserializes the
tool result exactly as in `test_update_dataset_metric_success` (lines
173–186, using
`json.loads(result.content[0].text)`), the top-level response contains
`updated_properties` including `"extra"` (because `updates` included it), but
`data["metric"]` has no `extra` key or value. Callers using only MCP APIs
can see that
`extra` was updated but cannot see the applied `extra` content, creating an
inconsistent
API contract between what can be written and what is returned.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2b1cf7bed145411a9243190c654a7674&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=2b1cf7bed145411a9243190c654a7674&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:** 557:667
**Comment:**
*Incomplete Implementation: `extra` is declared as an updatable metric
property, but the response model for the updated metric omits that field. This
creates an API contract gap where updates to `extra` succeed but are not
returned in `metric`, so callers cannot verify the applied value and may treat
the update as failed or stale. Add `extra` to `DatasetMetricDetail` (and keep
serialization aligned) so response payloads reflect all supported updates.
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=2e752d4ff30981844a21bf959d0df5ce06de83b0b3a23b7171aa5930fc5dccf7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40975&comment_hash=2e752d4ff30981844a21bf959d0df5ce06de83b0b3a23b7171aa5930fc5dccf7&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]
