aminghadersohi commented on code in PR #45359:
URL: https://github.com/apache/superset/pull/45359#discussion_r4300351361
##########
superset/datasource/api.py:
##########
@@ -651,6 +652,18 @@ def compatible(self, datasource_type: str, datasource_id:
int) -> FlaskResponse:
selected_metrics = body.get("selected_metrics", [])
selected_dimensions = body.get("selected_dimensions", [])
+ selection_errors: list[str] = validate_names(
+ selected_metrics,
+ {metric.metric_name for metric in datasource.metrics},
+ "metric",
+ ) + validate_names(
+ selected_dimensions,
+ {dimension.column_name for dimension in datasource.columns},
+ "dimension",
+ )
Review Comment:
Confirmed fixed at 753b8499 — and yes, keep the normalization at this
boundary; my suggestion was the narrower one. Filtering only inside
`validate_names` still leaves `sorted(selected_metrics)` raising on a mixed
`["a", {...}]` list and `n in metric_map` raising on a dict in
`SemanticView.get_compatible_metrics`, neither of which `validate_names`
reaches.
Siblings checked:
- superset/datasource/api.py:660 — fixed in
80633a0b84297900f945b50dacdb24d2fca0ed59
- superset/mcp_service/semantic_layer/tool/get_compatible_metrics.py:161 —
not affected: `GetCompatibleMetricsRequest.selected_metrics` is a Pydantic
`list[str]`, so non-strings never reach the validator
- superset/mcp_service/semantic_layer/tool/get_compatible_dimensions.py:166
— not affected: same Pydantic boundary
- superset/mcp_service/utils/query_utils.py:36 — not affected: MCP-only
callers, Pydantic-validated upstream
- superset/common/tabular_query.py:240 — not affected:
`validate_query_names` already filters `isinstance(name, str)`
Regression test: `test_compatible_ignores_non_name_selections` covers
dict/None/int on both fields with `has_members` True and False, so the
400-vs-500 split on an empty member set is closed, and it asserts the real name
still reaches `get_compatible_metrics`. Red before: at d1041eda the dict hits
`name not in valid` unhashable and the `has_members=False` arm returns 400, not
200.
##########
superset/datasource/api.py:
##########
@@ -651,6 +652,18 @@ def compatible(self, datasource_type: str, datasource_id:
int) -> FlaskResponse:
selected_metrics = body.get("selected_metrics", [])
selected_dimensions = body.get("selected_dimensions", [])
+ selection_errors: list[str] = validate_names(
+ selected_metrics,
+ {metric.metric_name for metric in datasource.metrics},
Review Comment:
Confirmed fixed at 753b8499 — and yes, keep validation ahead of the cache
read. The cache key sets `metadata_version` only for `SemanticView` and `None`
otherwise, so renaming a `SqlaTable` metric leaves the key unchanged and a
post-cache check would keep serving the stale 200 this PR exists to prevent.
One relationship load on a named hit is the right price for that; the free half
is gone.
Siblings checked:
- superset/datasource/api.py:671 — fixed in
80633a0b84297900f945b50dacdb24d2fca0ed59
- superset/datasource/api.py:678 — fixed in
80633a0b84297900f945b50dacdb24d2fca0ed59
- superset/common/tabular_query.py:240 — not affected:
`validate_query_names` takes prepared name sets and reads no datasource
relationship
Regression test:
`test_compatible_empty_selection_skips_metadata_on_cache_hit` asserts both
PropertyMocks are untouched on an empty cached request. Red before: at d1041eda
both set comprehensions are evaluated as arguments ahead of the cache read, so
`metrics.assert_not_called()` fails.
--
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]
