aminghadersohi commented on code in PR #45485:
URL: https://github.com/apache/superset/pull/45485#discussion_r4337488710
##########
superset/mcp_service/utils/schema_utils.py:
##########
@@ -34,6 +36,22 @@
T = TypeVar("T")
+@contextmanager
+def field_validation_error(field_name: str) -> Iterator[None]:
+ """Attach a declared field location to checks in a model-level validator.
+
+ Keep the original ValueError for Pydantic consumers; the MCP diagnostic
+ formatter exposes only the schema-backed location and a static reason.
+ """
+ try:
+ yield
+ except ValueError as ex:
+ raise ValidationError.from_exception_data(
+ "Field validation",
+ [{"type": "value_error", "loc": (field_name,), "ctx": {"error":
ex}}],
+ ) from ex
Review Comment:
Follow-up fixed in 6632cff2c8b12ff4660d1a28fa864e29646f77e3: the shared
sanitizers emit stable error types, so sibling text fields get an actionable,
input-free reason instead of “Invalid value”. Bullet comma labels and
empty-after-sanitization certification text have their own reasons. Compact
chart-config diagnostics resolve declared nested fields server-side; unknown
tags/fields and dictionary keys remain masked.
Regression test:
`tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py::test_sibling_sanitization_wire_error_names_field_with_actionable_reason`
— all 28 cases fail at e8c421c3c2f319f6ec2d86102fa6fb925bddb972 and pass at
6632cff2c8b12ff4660d1a28fa864e29646f77e3. I copied the new tests into a
detached worktree at that prior head and ran pytest with `-m 'not manual and
not llm_compliance'`.
Additional regressions:
`test_sibling_alias_sanitization_wire_error_names_canonical_field` (5 failures
before), `test_nested_chart_sanitization_wire_error_names_field` (12 failures
before, covering
generate_chart/update_chart/generate_explore_link/update_chart_preview), and
`test_sanitizer_guard_wire_error_is_actionable` (6 failures before; 2
pre-existing native length guards already pass). Combined before result: 51
failed, 2 passed; the seven sanitizer/schema/wire modules pass after: 1057
passed. The full MCP run with the same marker exclusions and `-n 4` reports
11225 passed, 12 skipped. Changed-branch-file pre-commit passes, including
mypy, ruff and pylint. No real-provider or live-deployment check was run.
Siblings checked:
- superset/mcp_service/dashboard/schemas.py
(GenerateDashboardRequest.dashboard_title) — fixed in
6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/dashboard/schemas.py
(UpdateDashboardRequest.dashboard_title) — fixed in
6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/dashboard/schemas.py
(DuplicateDashboardRequest.dashboard_title) — fixed in
6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/dashboard/schemas.py
(ManageDashboardCertificationRequest.certified_by) — fixed in
6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/dashboard/schemas.py
(ManageDashboardCertificationRequest.certification_details) — fixed in
6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py (GenerateChartRequest.chart_name) —
fixed in 6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py (UpdateChartRequest.chart_name) —
fixed in 6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py (BaseChartConfig.temporal_column) —
fixed in 6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py (ColumnRef.name; including
column_name/column aliases) — fixed in 6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py (ColumnRef.label) — fixed in
6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py (ColumnRef.sql_expression;
SQL-specific rejections) — fixed in 6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py (FilterConfig.column; including col
alias) — fixed in 6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py (FilterConfig.value; scalar/list,
including val alias) — fixed in 6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py (GanttSortByConfig.column; including
col alias) — fixed in 6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py (SunburstChartConfig.time_range) —
fixed in 6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py
(InteractivePivotChartConfig.comparison_period) — fixed in
6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py (BulletChartConfig.time_range) —
fixed in 6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py (BulletChartConfig.range_labels) —
fixed in 6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py (BulletChartConfig.marker_labels) —
fixed in 6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/chart/schemas.py
(BulletChartConfig.marker_line_labels) — fixed in
6632cff2c8b12ff4660d1a28fa864e29646f77e3
- superset/mcp_service/dashboard/schemas.py (_sanitize_header_text) — not
affected: plain React header text only strips invisible characters and checks
length; it does not use the shared URL/content sanitizer.
- superset/mcp_service/utils/error_builder.py (_sanitize_user_input) — not
affected: output escaping, not a Pydantic input-rejection path.
SQL expressions retain their separate policy: prose containing `data:` is
not itself rejected there; SQL-specific content rejections receive the safe
reason. No sanitizer acceptance/rejection policy changed in this follow-up.
--
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]
