aminghadersohi commented on code in PR #40957:
URL: https://github.com/apache/superset/pull/40957#discussion_r3493544601
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -871,6 +902,28 @@ def sanitize_dashboard_title(cls, v: str | None) -> str |
None:
v, "Dashboard title", max_length=500, allow_empty=True
)
+ @field_validator("slug")
+ @classmethod
+ def normalize_slug(cls, v: str | None) -> str | None:
+ """Normalize the slug to match the REST DashboardPutSchema contract.
+
+ Mirrors ``BaseDashboardSchema.post_load``: strip, replace spaces with
+ hyphens, and drop characters outside ``[\\w-]`` so the tool cannot
+ persist slugs the REST update path would have cleaned.
+
+ Raises ``ValueError`` when a non-empty input normalizes to an empty
+ string (e.g. ``"!!!"``) to prevent silently clearing the slug.
+ """
+ if not v:
+ return v
+ normalized = re.sub(r"[^\w\-]+", "", v.strip().replace(" ", "-"))
+ if not normalized:
+ raise ValueError(
+ "slug contains only characters that are removed during "
+ "normalization; use letters, digits, underscores, or hyphens"
+ )
Review Comment:
Fixed in a2c740a336. Added a strip-before-check in `normalize_slug`:
whitespace-only inputs (e.g. `" "`) now return `""` (clear slug), matching the
REST `BaseDashboardSchema` behavior. Non-whitespace-only special-char slugs
(e.g. `"!!!"`) still raise a `ValueError` to prevent silent slot clearing. Test
updated with a new whitespace assertion and the corrected match regex.
##########
superset/mcp_service/dashboard/tool/update_dashboard.py:
##########
@@ -212,6 +308,10 @@ def update_dashboard(
if auth_error is not None:
return auth_error
+ validation_error = _validate_update_request(dashboard, request)
+ if validation_error is not None:
+ return validation_error
Review Comment:
Already handled: `validate_tags` runs inside its own `except
SQLAlchemyError` handler (lines 259–264 in the current code) that returns a
`DashboardError` rather than propagating. The comment appears to describe a
version of the code before that handler was added.
##########
superset/mcp_service/dashboard/tool/update_dashboard.py:
##########
@@ -113,22 +113,58 @@ def _merge_json_metadata(dashboard: Any, overrides:
dict[str, Any]) -> str:
existing: dict[str, Any] = {}
if dashboard.json_metadata:
try:
- parsed = json.loads(dashboard.json_metadata)
- if isinstance(parsed, dict):
+ if isinstance(parsed := json.loads(dashboard.json_metadata), dict):
existing = parsed
except (ValueError, TypeError):
pass
existing.update(overrides)
return json.dumps(existing)
+# Typed json_metadata convenience fields. Each maps 1:1 to a json_metadata
+# key but is exposed as a validated field so an LLM does not have to hand-build
+# the raw ``json_metadata_overrides`` dict for common toggles.
+_TYPED_METADATA_FIELDS = (
+ "cross_filters_enabled",
+ "refresh_frequency",
+ "filter_bar_orientation",
+)
Review Comment:
Declining type annotation nits (this thread and the ones for `typed`,
`metadata_overrides`, `validation_error`): the commit `d9ba43fe` already added
explicit type annotations where they meaningfully aid readability; annotating
every intermediate local in comprehensions and trivially-inferred assignments
would add noise. `validation_error` is already annotated as `DashboardError |
None` in the current code.
--
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]
