feat(mcp): add update_dataset_metric tool for editing saved dataset metrics - #40975
Conversation
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| def resolve_dataset( | ||
| identifier: int | str, eager_options: list[Any] | None = None | ||
| ) -> Any | None: | ||
| """Resolve a dataset by int ID or UUID string. | ||
|
|
||
| Replicates the identifier resolution logic from ModelGetInfoCore._find_object(). | ||
| """ | ||
| from superset.daos.dataset import DatasetDAO | ||
|
|
||
| opts = eager_options or None | ||
|
|
||
| if isinstance(identifier, int): | ||
| return DatasetDAO.find_by_id(identifier, query_options=opts) | ||
|
|
||
| # Try parsing as int | ||
| try: | ||
| id_val = int(identifier) | ||
| return DatasetDAO.find_by_id(id_val, query_options=opts) | ||
| except (ValueError, TypeError): | ||
| pass |
There was a problem hiding this comment.
Suggestion: Restrict dataset lookup inputs to UUID-only identifiers and remove integer-ID resolution paths so this new helper does not expose internal numeric IDs through a public MCP-facing flow. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This new helper explicitly accepts and resolves integer IDs before UUIDs, which exposes internal numeric identifiers through a public MCP-facing lookup path.
That matches the custom rule about not introducing internal integer IDs when UUID-based identifiers are being used.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/dataset_utils.py
**Line:** 25:44
**Comment:**
*Custom Rule: Restrict dataset lookup inputs to UUID-only identifiers and remove integer-ID resolution paths so this new helper does not expose internal numeric IDs through a public MCP-facing flow.
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| 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))}." |
There was a problem hiding this comment.
Suggestion: Add a concise docstring to this newly added helper function so it is documented consistently with the other new functions in the file. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added Python helper function and it does not include a docstring. The custom rule requires new functions and classes to be documented inline, so the suggestion accurately identifies a real violation.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/tool/update_dataset_metric.py
**Line:** 62:70
**Comment:**
*Custom Rule: Add a concise docstring to this newly added helper function so it is documented consistently with the other new functions in the file.
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| def mcp_server(): | ||
| return mcp |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation to this fixture function so new Python code is fully typed. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a new Python function in the PR and it has no return type annotation.
The rule requires new Python code to be fully typed, so this is a real violation.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py
**Line:** 39:40
**Comment:**
*Custom Rule: Add an explicit return type annotation to this fixture function so new Python code is fully typed.
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|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def mock_auth(): |
There was a problem hiding this comment.
Suggestion: Annotate the return type of this yielding fixture (for example as a generator type) to satisfy the full type-hint requirement for new functions. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This new fixture function lacks a return type annotation. Under the typing rule for new Python code, that omission is a genuine violation.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py
**Line:** 44:44
**Comment:**
*Custom Rule: Annotate the return type of this yielding fixture (for example as a generator type) to satisfy the full type-hint requirement for new functions.
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| def make_metric( | ||
| metric_id=10, | ||
| metric_name="count", | ||
| uuid="a1b2c3d4-5678-90ab-cdef-1234567890ab", | ||
| **overrides, | ||
| ): |
There was a problem hiding this comment.
Suggestion: Add type hints for all parameters and the return value of this helper to comply with the fully typed new-code rule. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The helper is newly added and its parameters and return value are untyped.
That directly violates the requirement that new Python code be fully typed.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py
**Line:** 56:61
**Comment:**
*Custom Rule: Add type hints for all parameters and the return value of this helper to comply with the fully typed new-code rule.
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| return metric | ||
|
|
||
|
|
||
| def make_dataset(dataset_id=1, metrics=None): |
There was a problem hiding this comment.
Suggestion: Add parameter and return type annotations to this helper function so it meets the typing requirement for newly added functions. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This new helper function has untyped parameters and no return annotation, so it does violate the new-code typing rule.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py
**Line:** 79:79
**Comment:**
*Custom Rule: Add parameter and return type annotations to this helper function so it meets the typing requirement for newly added functions.
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
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| dataset_id: int | str = Field( | ||
| ..., | ||
| description="Dataset identifier — numeric ID or UUID string. " | ||
| "Use list_datasets to find valid IDs.", | ||
| ) |
There was a problem hiding this comment.
Suggestion: Restrict this request identifier to UUID-only input (or a dedicated UUID field) so the new public API does not continue accepting internal integer IDs. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a new public API request field that explicitly accepts numeric internal IDs in addition to UUIDs. The rule says new public API identifiers should not expose or depend on internal integer IDs when UUID-based identifiers are being added or changed. This is a real violation.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/schemas.py
**Line:** 520:524
**Comment:**
*Custom Rule: Restrict this request identifier to UUID-only input (or a dedicated UUID field) so the new public API does not continue accepting internal integer IDs.
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| 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.", |
There was a problem hiding this comment.
Suggestion: Remove numeric metric ID support from this API identifier and require UUID (or non-internal name-only lookup) to avoid exposing or depending on internal integer IDs in the new interface. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This request schema explicitly allows a numeric metric ID and even treats numeric strings as IDs, which exposes and depends on internal integer identifiers in a new public interface. That matches the rule violation.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/schemas.py
**Line:** 525:529
**Comment:**
*Custom Rule: Remove numeric metric ID support from this API identifier and require UUID (or non-internal name-only lookup) to avoid exposing or depending on internal integer IDs in the new interface.
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| id: int | None = Field(None, description="Metric ID") | ||
| uuid: str | None = Field(None, description="Metric UUID") |
There was a problem hiding this comment.
Suggestion: Drop the integer metric ID from the response model and expose only the UUID-based identifier for this newly added metric detail payload. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The response model includes an integer metric ID alongside a UUID field. Since this is a newly added public API payload and a UUID identifier is being introduced, exposing the internal integer ID violates the rule.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/schemas.py
**Line:** 592:593
**Comment:**
*Custom Rule: Drop the integer metric ID from the response model and expose only the UUID-based identifier for this newly added metric detail payload.
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| class UpdateDatasetMetricResponse(BaseModel): | ||
| """Response schema for update_dataset_metric.""" | ||
|
|
||
| dataset_id: int | None = Field(None, description="Dataset ID") |
There was a problem hiding this comment.
Suggestion: Replace this integer dataset identifier field with a UUID-based public identifier in the new response schema to avoid exposing internal numeric IDs. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This response schema exposes an internal integer dataset ID in a new public API response. That is exactly the kind of identifier the rule says to avoid when UUID-based identifiers are being added or changed.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/schemas.py
**Line:** 604:604
**Comment:**
*Custom Rule: Replace this integer dataset identifier field with a UUID-based public identifier in the new response schema to avoid exposing internal numeric IDs.
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40975 +/- ##
==========================================
- Coverage 65.02% 65.01% -0.01%
==========================================
Files 2742 2744 +2
Lines 153422 153567 +145
Branches 35198 35211 +13
==========================================
+ Hits 99761 99840 +79
- Misses 51752 51820 +68
+ Partials 1909 1907 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| symbolPosition: Literal["prefix", "suffix"] | None = Field( # noqa: N815 | ||
| None, description="Where to render the symbol relative to the value." |
There was a problem hiding this comment.
Suggestion: Restricting currency.symbolPosition to only "prefix"/"suffix" is incompatible with the existing dataset update contract, which accepts arbitrary non-empty strings (and legacy data may contain other values). This can cause valid update requests or serialization of existing metric currency payloads to fail with validation errors. Align this field type with the broader Superset schema behavior instead of enforcing a strict enum here. [api mismatch]
Severity Level: Major ⚠️
- ❌ update_dataset_metric fails for metrics with custom symbolPosition
- ⚠️ MCP clients see unexpected validation or internal errorsSteps of Reproduction ✅
1. Observe the existing REST dataset update contract in
`superset/datasets/schemas.py:17-20`, where
`DatasetMetricCurrencyPutSchema.symbolPosition` is defined as
`fields.String(validate=Length(1, 128))` with no enum restriction, and in the generated
OpenAPI docs (`docs/static/resources/openapi.json:5205-5208`) describing `symbolPosition`
as a generic string field. This confirms existing APIs accept arbitrary non-empty strings
(not just `"prefix"`/`"suffix"`).
2. Note that metric currency values are stored in the database via `CurrencyType` in
`superset/models/sql_types/base.py:67-86`, which parses legacy string values into dicts
using `parse_currency_string` without constraining `symbolPosition`. As a result, existing
datasets can legitimately contain arbitrary `currency={"symbol": "USD", "symbolPosition":
"before"}` or other non-enum values.
3. Call the MCP tool `update_dataset_metric` (entrypoint
`superset/mcp_service/dataset/tool/update_dataset_metric.py:113-229`) against any dataset
whose metric has such a currency dict (e.g. created previously via the REST API or UI), by
sending a minimal request that does not touch currency: `{"request": {"dataset_id": 1,
"metric": "count", "expression": "COUNT(1)"}}`. The tool loads the dataset metrics (lines
167-179) and locates the target metric (lines 179-183).
4. After `UpdateDatasetCommand.run()` returns the updated dataset (lines 207-213), the
tool serializes the metric via `_serialize_metric` at `update_dataset_metric.py:73-100`.
That calls `MetricCurrency.model_validate(currency)` when `currency` is a dict (lines
93-95); however `MetricCurrency.symbolPosition` is declared as `Literal["prefix",
"suffix"]` in `superset/mcp_service/dataset/schemas.py:35-43`, so any existing metric with
`symbolPosition` not equal to `"prefix"` or `"suffix"` causes a
`pydantic.ValidationError`. This bubbles into the broad `except Exception` block at lines
253-258, which logs `"Unexpected error updating dataset metric"` and re-raises, causing
the MCP tool call to fail with an internal error even though the underlying dataset update
may have succeeded.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/schemas.py
**Line:** 510:511
**Comment:**
*Api Mismatch: Restricting `currency.symbolPosition` to only `"prefix"`/`"suffix"` is incompatible with the existing dataset update contract, which accepts arbitrary non-empty strings (and legacy data may contain other values). This can cause valid update requests or serialization of existing metric currency payloads to fail with validation errors. Align this field type with the broader Superset schema behavior instead of enforcing a strict enum here.
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| 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") |
There was a problem hiding this comment.
Suggestion: The response model for updated metrics omits the extra property even though the request allows updating it and the tool advertises returning the metric after update. This makes the response incomplete for one of the supported update fields and breaks callers that expect to read back the updated extra value. Add extra to the metric detail response schema so response content matches supported updates. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ update_dataset_metric response omits updated extra metadata
- ⚠️ Callers cannot confirm saved metric extra changesSteps of Reproduction ✅
1. Inspect the request schema for the MCP tool in
`superset/mcp_service/dataset/schemas.py:46-92`. `UpdateDatasetMetricRequest` includes
`extra: str | None = Field(None, description="JSON-encoded string with extra metric
metadata.")` (lines 88-92) and `UPDATABLE_METRIC_FIELDS` (lines 20-32) includes `"extra"`,
so callers are explicitly allowed to update `extra`.
2. Inspect the response-side metric model `DatasetMetricDetail` in the same file at
`superset/mcp_service/dataset/schemas.py:120-129`. It subclasses `SqlMetricInfo` (which
defines `metric_name`, `verbose_name`, `expression`, `description`, `d3format` at lines
41-50) and adds `id`, `uuid`, `metric_type`, `currency`, and `warning_text` (lines
123-129), but does not define an `extra` field.
3. Examine the tool implementation in
`superset/mcp_service/dataset/tool/update_dataset_metric.py:73-100`. `_serialize_metric()`
builds a `DatasetMetricDetail` from the ORM metric, setting `id`, `uuid`, `metric_name`,
`verbose_name`, `expression`, `description`, `d3format`, `metric_type`, `currency`, and
`warning_text`, but never includes `metric.extra` when constructing the response object.
4. From an MCP client, call the tool `update_dataset_metric` (entrypoint
`update_dataset_metric.py:113-229`) with a payload that only updates `extra`, for example:
`{"request": {"dataset_id": 1, "metric": "count", "extra": "{\"foo\": \"bar\"}"}}`. The
request validates and `UpdateDatasetMetricRequest.updates()` (lines 94-103) includes
`"extra"` in the metrics payload sent to `UpdateDatasetCommand`. After the update, the
tool returns `UpdateDatasetMetricResponse.metric` as a `DatasetMetricDetail` (lines
220-223), but because `DatasetMetricDetail` has no `extra` field and `_serialize_metric()`
never passes it through, the JSON response lacks any `metric["extra"]` field. Thus callers
that just updated `extra` cannot read back the new value via this tool, despite the schema
advertising that the metric is returned "after the update".Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/schemas.py
**Line:** 589:598
**Comment:**
*Incomplete Implementation: The response model for updated metrics omits the `extra` property even though the request allows updating it and the tool advertises returning the metric after update. This makes the response incomplete for one of the supported update fields and breaks callers that expect to read back the updated `extra` value. Add `extra` to the metric detail response schema so response content matches 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![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #513598
Actionable Suggestions - 1
-
superset/mcp_service/dataset/dataset_utils.py - 1
- Missing unit tests for new utility · Line 25-50
Additional Suggestions - 2
-
tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py - 1
-
Missing error path test coverage · Line 253-378The test suite covers DatasetNotFoundError, DatasetForbiddenError, and DatasetInvalidError, but does not test DatasetUpdateFailedError which is handled at update_dataset_metric.py lines 248-252. Adding coverage ensures complete error path validation.
-
-
superset/mcp_service/app.py - 1
-
Documentation field name mismatch · Line 166-166The documentation says 'label' but the actual updatable field is named 'verbose_name' per the schema definition. Update the description to say 'verbose_name' for consistency.
Code suggestion
--- a/superset/mcp_service/app.py +++ b/superset/mcp_service/app.py @@ -163,7 +163,7 @@ Dataset Management: - list_datasets: List datasets with advanced filters (1-based pagination) - get_dataset_info: Get detailed dataset information by ID (includes columns/metrics) - create_virtual_dataset: Save a SQL query as a virtual dataset for charting (requires write access) -- update_dataset_metric: Update a saved metric on a dataset — expression, name, label, format (requires dataset ownership) +- update_dataset_metric: Update a saved metric on a dataset — expression, name, verbose_name, format (requires dataset ownership) - query_dataset: Query a dataset using its semantic layer (saved metrics, dimensions, filters) without needing a saved chart
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py - 1
- Missing pytest-asyncio decorator · Line 340-340
-
superset/mcp_service/dataset/schemas.py - 1
- Missing validation_alias for symbolPosition · Line 510-512
Review Details
-
Files reviewed - 8 · Commit Range:
2311482..2311482- superset/mcp_service/app.py
- superset/mcp_service/dataset/dataset_utils.py
- superset/mcp_service/dataset/schemas.py
- superset/mcp_service/dataset/tool/__init__.py
- superset/mcp_service/dataset/tool/query_dataset.py
- superset/mcp_service/dataset/tool/update_dataset_metric.py
- tests/unit_tests/mcp_service/dataset/tool/test_query_dataset.py
- tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| def resolve_dataset( | ||
| identifier: int | str, eager_options: list[Any] | None = None | ||
| ) -> Any | None: | ||
| """Resolve a dataset by int ID or UUID string. | ||
|
|
||
| Replicates the identifier resolution logic from ModelGetInfoCore._find_object(). | ||
| """ | ||
| from superset.daos.dataset import DatasetDAO | ||
|
|
||
| opts = eager_options or None | ||
|
|
||
| if isinstance(identifier, int): | ||
| return DatasetDAO.find_by_id(identifier, query_options=opts) | ||
|
|
||
| # Try parsing as int | ||
| try: | ||
| id_val = int(identifier) | ||
| return DatasetDAO.find_by_id(id_val, query_options=opts) | ||
| except (ValueError, TypeError): | ||
| pass | ||
|
|
||
| # Try UUID | ||
| if _is_uuid(str(identifier)): | ||
| return DatasetDAO.find_by_id(identifier, id_column="uuid", query_options=opts) | ||
|
|
||
| return None |
There was a problem hiding this comment.
The resolve_dataset function is new but lacks dedicated unit tests. Per BITO.md rule [11730], new tools/functions require comprehensive unit tests covering success paths, error scenarios, and edge cases. Tests should be placed in tests/unit_tests/mcp_service/dataset/ following existing MCP test patterns.
Code Review Run #513598
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
2311482 to
4077b01
Compare
| ) | ||
|
|
||
| @model_validator(mode="after") | ||
| def validate_updates(self) -> "UpdateDatasetMetricRequest": |
There was a problem hiding this comment.
Suggestion: Add an inline docstring to this newly introduced validator method so all new functions are documented consistently. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| class DatasetMetricDetail(SqlMetricInfo): | ||
| """Full saved-metric details, including identifiers.""" | ||
|
|
||
| id: int | None = Field(None, description="Metric ID") |
There was a problem hiding this 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
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| def _serialize_metric(metric: Any) -> DatasetMetricDetail: | ||
| currency = getattr(metric, "currency", None) |
There was a problem hiding this 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
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
| @pytest.fixture | ||
| def mcp_server(): | ||
| return mcp |
There was a problem hiding this comment.
Suggestion: Add a short docstring to the mcp_server fixture describing what object it provides to tests. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| @pytest.fixture(autouse=True) | ||
| def mock_auth(): |
There was a problem hiding this 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
Why it matters? 🤔
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
| 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") | ||
|
|
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ 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.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
| 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))}." |
There was a problem hiding this comment.
Suggestion: The not-found error message interpolates raw user-controlled identifier and stored metric names directly into the response text without LLM-context sanitization, which can inject delimiter/control content into MCP outputs. Sanitize or escape both identifier and suggested/available metric names before constructing error. [security]
Severity Level: Major ⚠️
- ⚠️ MCP error text can inject delimiters into LLM context.
- ⚠️ Enables prompt-injection via crafted metric identifiers or names.Steps of Reproduction ✅
1. The helper `_metric_not_found_message` at
`superset/mcp_service/dataset/tool/update_dataset_metric.py:62-70` builds an error string
directly from the user-supplied `identifier` and the stored `metric_name` values: `msg =
f"Metric '{identifier}' not found..."`, then either suggests close matches (`',
'.join(suggestions)`) or lists all available metric names (`', '.join(sorted(names))`)
with no sanitization or escaping.
2. When `update_dataset_metric` cannot find the requested metric (`target is None` at
`update_dataset_metric.py:181`), it calls `_metric_not_found_message(metrics,
request.metric)` and returns the resulting string as the `error` field of
`UpdateDatasetMetricResponse` at `update_dataset_metric.py:182-188`.
`UpdateDatasetMetricResponse`'s schema in
`superset/mcp_service/dataset/schemas.py:150-167` defines `error: str | None` without any
field validator, so this text is passed through unchanged to the MCP client/LLM.
3. Other fields intended for LLM exposure are explicitly sanitized: `_serialize_metric`
applies `escape_llm_context_delimiters` and `sanitize_for_llm_context` to metric_name,
verbose_name, expression, description, and warning_text at
`update_dataset_metric.py:78-99`, and `DatasetError.error` wraps error text with
`sanitize_for_llm_context` via a `field_validator` at
`superset/mcp_service/dataset/schemas.py:60-64`. In contrast, `_metric_not_found_message`
bypasses these utilities.
4. If a caller supplies a crafted `metric` identifier containing LLM control content (for
example ``"```json\n{\"attack\": true}\n```"``) or if stored `metric_name` values include
such delimiters, `_metric_not_found_message` will interpolate them directly into `msg`
(lines 64-70), and the MCP tool will return this string in
`UpdateDatasetMetricResponse.error` (lines 184-188). This unsanitized error text is then
embedded into the LLM tool response, creating a prompt-injection vector that can break
tool-output framing or influence downstream agent behavior.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/tool/update_dataset_metric.py
**Line:** 62:70
**Comment:**
*Security: The not-found error message interpolates raw user-controlled identifier and stored metric names directly into the response text without LLM-context sanitization, which can inject delimiter/control content into MCP outputs. Sanitize or escape both identifier and suggested/available metric names before constructing `error`.
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 fixThere was a problem hiding this comment.
What do you think of this?
There was a problem hiding this comment.
Yes — this is a valid security issue.
Why it matters
_metric_not_found_message() currently builds the error string from:
- the caller-controlled
identifier - stored
metric_namevalues
and returns it directly in UpdateDatasetMetricResponse.error. Since this is MCP output, raw delimiter/control content can leak into the LLM context and create prompt-injection risk.
Minimal fix
Sanitize/escape the values before interpolating them into the error message, consistent with _serialize_metric().
For example:
from superset.mcp_service.utils import (
escape_llm_context_delimiters,
sanitize_for_llm_context,
)
def _metric_not_found_message(metrics: list[Any], identifier: int | str) -> str:
safe_identifier = sanitize_for_llm_context(str(identifier), field_path=("metric", "identifier"))
names = [
escape_llm_context_delimiters(m.metric_name) or ""
for m in metrics
]
msg = f"Metric '{safe_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), [m.metric_name for m in metrics], n=3, cutoff=0.6)
if suggestions:
safe_suggestions = [escape_llm_context_delimiters(name) or "" for name in suggestions]
return f"{msg} Did you mean: {', '.join(safe_suggestions)}?"
return f"{msg} Available metrics: {', '.join(sorted(names))}."Recommendation
I’d keep the fix focused on this helper rather than changing the response schema, since the issue is specifically in how the message is composed.
If you want, I can also check the rest of the PR comments and help validate/fix those too.
| metrics = list(dataset.metrics) | ||
| target = _find_metric(metrics, request.metric) | ||
| if target is None: | ||
| message = _metric_not_found_message(metrics, request.metric) | ||
| await ctx.warning("Metric not found: %s" % (request.metric,)) | ||
| return UpdateDatasetMetricResponse( | ||
| dataset_id=dataset.id, | ||
| dataset_name=dataset.table_name, | ||
| error=message, | ||
| ) |
There was a problem hiding this comment.
Suggestion: This code performs metric existence checks before ownership is enforced, so a caller who can invoke the tool but does not own the dataset can distinguish "metric exists" vs "metric not found" and enumerate metric names via error messages/suggestions. Enforce ownership immediately after dataset resolution (before _find_metric / _metric_not_found_message) so unauthorized callers always get the same forbidden response. [security]
Severity Level: Major ⚠️
- ⚠️ MCP `update_dataset_metric` leaks metric existence to non-owners.
- ⚠️ Enables metric name enumeration via distinct error responses.Steps of Reproduction ✅
1. In `superset/mcp_service/dataset/tool/update_dataset_metric.py:103-113`, the
`update_dataset_metric` FastMCP tool is exposed with `class_permission_name="Dataset"` and
`method_permission_name="write"`, meaning any role with the Dataset write class permission
can invoke this tool from an MCP client.
2. When the tool runs, it first resolves the dataset via
`resolve_dataset(request.dataset_id, eager_options)` at
`update_dataset_metric.py:167-168`, which calls `DatasetDAO.find_by_id` through
`resolve_dataset` (`superset/mcp_service/dataset/dataset_utils.py:25-48`). This applies
the `DatasourceFilter` base filter (`superset/daos/dataset.py:52-61`) for data-access
permissions but does not enforce ownership.
3. After resolution, before any ownership check, the tool materializes all metrics and
attempts to locate the target metric (`metrics = list(dataset.metrics)` and `target =
_find_metric(...)` at `update_dataset_metric.py:179-180`). If the metric is not found,
`_metric_not_found_message` is called and a detailed error is returned at
`update_dataset_metric.py:181-188`, including the dataset ID/name (from `dataset.id` /
`dataset.table_name`) and suggested/available metric names.
4. Only when the metric *is* found does execution continue to the update path, where
`UpdateDatasetCommand(...).run()` is invoked at `update_dataset_metric.py:207-209`. Inside
`UpdateDatasetCommand.validate` (`superset/commands/dataset/update.py:98-112`), ownership
is enforced via `security_manager.raise_for_ownership`, which can raise
`DatasetForbiddenError` and is caught at `update_dataset_metric.py:236-243` to return the
generic `"You must be an owner..."` error. A caller who can invoke the tool but is not an
owner can therefore distinguish (a) invalid dataset ID (`dataset is None` at lines
170-177), (b) non-existent metric (`target is None` at lines 179-188 with a
metric-not-found message listing metrics), and (c) existing metric but forbidden update
(DatasetForbiddenError at lines 236-243), allowing probing and enumeration of metric
existence and names on datasets they cannot edit.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/tool/update_dataset_metric.py
**Line:** 179:188
**Comment:**
*Security: This code performs metric existence checks before ownership is enforced, so a caller who can invoke the tool but does not own the dataset can distinguish "metric exists" vs "metric not found" and enumerate metric names via error messages/suggestions. Enforce ownership immediately after dataset resolution (before `_find_metric` / `_metric_not_found_message`) so unauthorized callers always get the same forbidden response.
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
Code Review Agent Run #b96f16Actionable Suggestions - 0Additional Suggestions - 4
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
There was a problem hiding this comment.
EnxDev's Review Agent — #40975
Overall
Well-scoped addition. I verified the two key assumptions:
DatasetDAO.update_metrics() merges metric stubs over the stored metrics, so rebuilding the full list preserves untouched metrics without creating or deleting entries.
UpdateDatasetCommand enforces ownership and validates metric expressions before persisting changes.
🟡 Should fix
-
update_dataset_metric.py:62-70 – _metric_not_found_message returns metric names and suggestions without applying the same sanitization used in _serialize_metric. It would be safer to escape these values as well to keep the error path consistent with the success path. A regression test would help prevent regressions.
-
update_dataset_metric.py:108-110 – Consider destructiveHint=True. Updating a metric changes all dependent charts and overwrites the previous definition, which seems closer to a destructive operation than a purely additive one.
🔵 Nits
Add min_length=1 for d3format and metric_type to match the existing Marshmallow schema.
Mention in the docs that purely numeric metric names must be referenced by ID/UUID.
Optionally validate that extra contains valid JSON instead of silently accepting invalid values.
🙌 Nice work
Routing updates through UpdateDatasetCommand correctly reuses existing authorization and validation logic.
Rebuilding the metric list as stubs is a clean way to avoid accidental deletions.
exclude_unset provides a nice implementation for partial updates.
Summary
Looks solid overall. My only substantive request is to sanitize the error path consistently with the success path. Everything else is minor.
LGTM once the sanitization issue is addressed.
Reviewed by EnxDev's Review Agent — @EnxDev.
alexandrusoare
left a comment
alexandrusoare
left a comment
There was a problem hiding this comment.
Overall lgtm, a few nits, conflict issues solving and we are good
- Escape metric names/identifier in the not-found error path so it matches the sanitization applied on the success path (EnxDev review) - Mark the tool destructiveHint=True, consistent with update_chart, since it overwrites an existing metric and affects dependent charts - Add docstring to _metric_not_found_message - Validate that 'extra' is a valid JSON-encoded string; reject empty d3format/metric_type via min_length=1 (matches Marshmallow schema) - Fix DEFAULT_INSTRUCTIONS to say 'verbose_name' instead of 'label' - Add type hints to test helpers/fixtures; cover DatasetUpdateFailedError, error-path escaping, and extra JSON validation
4077b01 to
c5cf803
Compare
| sanitize_for_llm_context, | ||
| ) | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for the module logger variable to satisfy the type-hint requirement for annotatable variables. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The new module-level variable logger is unannotated even though it can be given a concrete type such as logging.Logger. This matches the type-hint rule for annotatable variables in new Python code.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/tool/update_dataset_metric.py
**Line:** 40:40
**Comment:**
*Custom Rule: Add an explicit type annotation for the module logger variable to satisfy the type-hint requirement for annotatable variables.
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| } | ||
| ``` | ||
| """ | ||
| updates = request.updates() |
There was a problem hiding this comment.
Suggestion: Add a concrete type annotation for the local updates container returned from the request so this new variable is explicitly typed. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The local variable updates is introduced without an explicit type annotation, and it is a value whose type can reasonably be annotated (for example as a mapping of updated field names to values). This is a real omission under the Python type-hint rule.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/tool/update_dataset_metric.py
**Line:** 161:161
**Comment:**
*Custom Rule: Add a concrete type annotation for the local updates container returned from the request so this new variable is explicitly typed.
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|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_update_dataset_metric_success(mcp_server) -> None: |
There was a problem hiding this comment.
Suggestion: Add a concrete type annotation for the test fixture argument in this async test signature. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
This new test function takes the fixture argument mcp_server without a type annotation, so it matches the custom rule requiring Python type hints on annotatable function parameters.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py
**Line:** 173:173
**Comment:**
*Custom Rule: Add a concrete type annotation for the test fixture argument in this async test signature.
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|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_update_dataset_metric_returns_extra(mcp_server) -> None: |
There was a problem hiding this comment.
Suggestion: Add an explicit type hint to the fixture parameter in this test function. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The test signature includes the fixture parameter mcp_server without any type hint, which is a real omission under the stated Python type-hint rule.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py
**Line:** 245:245
**Comment:**
*Custom Rule: Add an explicit type hint to the fixture parameter in this test function.
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| async def test_update_dataset_metric_forbidden_non_owner( | ||
| mcp_server, allow_ownership | ||
| ) -> None: |
There was a problem hiding this comment.
Suggestion: Provide explicit type annotations for all fixture parameters in this async test definition. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
Both fixture parameters in this new test function are unannotated, so the code does violate the type-hint requirement.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py
**Line:** 415:417
**Comment:**
*Custom Rule: Provide explicit type annotations for all fixture parameters in this async test definition.
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|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_update_dataset_metric_invalid_error(mcp_server) -> None: |
There was a problem hiding this comment.
Suggestion: Add a type annotation to the fixture argument in this test function signature. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
This test function has an untyped fixture parameter mcp_server, so the suggestion identifies a real missing type hint.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py
**Line:** 463:463
**Comment:**
*Custom Rule: Add a type annotation to the fixture argument in this test function signature.
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|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_update_dataset_metric_update_failed(mcp_server) -> None: |
There was a problem hiding this comment.
Suggestion: Specify the expected type of the fixture parameter in this async test declaration. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The mcp_server parameter is not type-annotated in this new test function, so it fits the custom type-hint rule violation.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dataset/tool/test_update_dataset_metric.py
**Line:** 505:505
**Comment:**
*Custom Rule: Specify the expected type of the fixture parameter in this async test declaration.
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
|
|
||
| with event_logger.log_context(action="mcp.query_dataset.lookup"): | ||
| dataset = _resolve_dataset(request.dataset_id, eager_options) | ||
| dataset = resolve_dataset(request.dataset_id, eager_options) |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for the newly introduced local variable so the dataset object type is clear and compliant with the type-hinting rule. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The new local variable dataset is introduced without a type annotation, and its type is inferable/annotatable from the call site. This matches the custom rule requiring type hints on relevant Python variables that can be annotated.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dataset/tool/query_dataset.py
**Line:** 147:147
**Comment:**
*Custom Rule: Add an explicit type annotation for the newly introduced local variable so the dataset object type is clear and compliant with the type-hinting rule.
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…a field Address review feedback: - Enforce ownership immediately after dataset resolution so unauthorized callers cannot enumerate metric names via the not-found error path. - Add 'extra' to the metric detail response so all updatable properties are echoed back. - Relax currency.symbolPosition to a free string to tolerate legacy stored values on the read-back path. - Add docstrings to new helpers/validators and cover the DatasetUpdateFailedError and non-owner enumeration paths in tests.
c5cf803 to
02323a9
Compare
Code Review Agent Run #837f32Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
3 participants
SUMMARY
MCP exposes saved dataset metrics read-only via
get_dataset_info, but there is no write path — editing a metric's SQL expression, name, label, or format requires the UI. This adds anupdate_dataset_metrictool: given a dataset (ID/UUID) and a metric (ID/UUID/name), it partially updates the metric's properties (expression,metric_name,verbose_name,description,d3format,metric_type,currency,warning_text,extra).Design notes:
DatasetDAO.update_metricsis full-replacement (metrics omitted from the array are deleted), which is an easy foot-gun for LLM callers. The tool rebuilds the full metrics list internally — untouched metrics as{id, metric_name}stubs, the target with only the explicitly-passed properties merged on top — so it can never create or delete metrics.UpdateDatasetCommand, which enforces dataset ownership and validates name uniqueness and the SQL expression (validate_stored_expression).exclude_unsetsemantics distinguish "not provided" (keep stored value) from explicitnull(clear it).query_datasetinto a shareddataset_utils.resolve_dataset.Example call:
{"dataset_id": 123, "metric": "sum_revenue", "expression": "SUM(net_revenue)", "d3format": "$,.2f"}BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — MCP tool, no UI.
TESTING INSTRUCTIONS
pytest tests/unit_tests/mcp_service/dataset/(97 tests)update_dataset_metricwith a dataset ID and metric name, confirm the change in the dataset editor and that other metrics are untouched.ADDITIONAL INFORMATION
update_dataset_metric