bito-code-review[bot] commented on code in PR #40399: URL: https://github.com/apache/superset/pull/40399#discussion_r3293926240
########## superset/mcp_service/dashboard/tool/update_dashboard.py: ########## @@ -0,0 +1,205 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Update dashboard FastMCP tool + +This module contains the FastMCP tool for updating an existing dashboard's +layout, theme, and styling. Companion to ``generate_dashboard`` for +incremental edits without re-creating the dashboard. +""" + +import logging + +from fastmcp import Context +from sqlalchemy.exc import SQLAlchemyError +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import db, event_logger +from superset.mcp_service.dashboard.schemas import ( + DashboardError, + GenerateDashboardResponse, + UpdateDashboardRequest, + dashboard_serializer, +) +from superset.mcp_service.utils.url_utils import get_superset_base_url +from superset.utils import json + +logger = logging.getLogger(__name__) + + +def _resolve_dashboard(identifier): + """Look up a dashboard by id, uuid, or slug. Returns the model or None.""" + # Deferred import — DashboardDAO transitively pulls models whose + # Column definitions need the Flask app to be initialized first. + # Importing inside the function lets tool registration succeed at + # module-load time without triggering "App not initialized yet". + from superset.daos.dashboard import DashboardDAO + + try: + return DashboardDAO.get_by_id_or_slug(identifier) + except Exception: # pylint: disable=broad-except Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Blind exception catch too broad</b></div> <div id="fix"> Catching bare `Exception` is too broad (BLE001). Catch specific exceptions like `DashboardNotFoundError` or other expected exceptions instead. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion try: return DashboardDAO.get_by_id_or_slug(identifier) except (DashboardNotFoundError, Exception): # pylint: disable=broad-except ```` </div> </details> </div> <small><i>Code Review Run #f8211d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py: ########## @@ -625,3 +626,92 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None: assert len(req.sanitization_warnings) == 1 assert "dashboard_title" in req.sanitization_warnings[0] assert "injected" not in req.sanitization_warnings[0] + + +class TestGenerateDashboardRequestLayoutTheme: + """generate_dashboard accepts optional position_json, theme overrides, CSS.""" + + def test_layout_theme_css_fields_default_to_none(self) -> None: + req = GenerateDashboardRequest(chart_ids=[1]) + assert req.position_json is None + assert req.json_metadata_overrides is None + assert req.css is None + assert req.slug is None + + def test_position_json_accepted(self) -> None: + position = { + "ROOT_ID": {"children": ["GRID_ID"], "type": "ROOT"}, + "GRID_ID": {"children": ["ROW-1"], "type": "GRID"}, + } + req = GenerateDashboardRequest(chart_ids=[1], position_json=position) + assert req.position_json == position + + def test_json_metadata_overrides_accepted(self) -> None: + overrides = { + "label_colors": {"Electronics": "#4C78A8"}, + "cross_filters_enabled": False, + } + req = GenerateDashboardRequest( + chart_ids=[1], json_metadata_overrides=overrides + ) + assert req.json_metadata_overrides == overrides + + def test_css_accepted(self) -> None: + req = GenerateDashboardRequest( + chart_ids=[1], css=".header-controls{display:none}" + ) + assert req.css == ".header-controls{display:none}" + + def test_slug_accepted(self) -> None: + req = GenerateDashboardRequest(chart_ids=[1], slug="my-dashboard") + assert req.slug == "my-dashboard" + + def test_css_max_length_enforced(self) -> None: + with pytest.raises(ValidationError, match="at most 50000"): + GenerateDashboardRequest(chart_ids=[1], css="x" * 50001) + + +class TestUpdateDashboardRequest: + """Schema validation for update_dashboard's request.""" + + def test_identifier_required(self) -> None: + with pytest.raises(ValidationError, match="Field required"): + UpdateDashboardRequest() # type: ignore[call-arg] + + def test_int_identifier_accepted(self) -> None: + req = UpdateDashboardRequest(identifier=42) + assert req.identifier == 42 + + def test_string_identifier_accepted(self) -> None: + req = UpdateDashboardRequest(identifier="my-slug") + assert req.identifier == "my-slug" + + def test_all_optional_fields_default_to_none(self) -> None: + req = UpdateDashboardRequest(identifier=1) + assert req.dashboard_title is None + assert req.description is None + assert req.slug is None + assert req.published is None + assert req.position_json is None + assert req.json_metadata_overrides is None + assert req.css is None Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing max_length constraint tests</b></div> <div id="fix"> The `test_all_optional_fields_default_to_none` test verifies default values but does not validate `max_length` constraints defined in the schema (slug=255 at line 682, dashboard_title=500 at line 672). Add `test_slug_max_length_enforced` and `test_dashboard_title_max_length_enforced` to `TestUpdateDashboardRequest`. </div> </div> <small><i>Code Review Run #f8211d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/mcp_service/dashboard/tool/generate_dashboard.py: ########## @@ -308,6 +318,12 @@ def generate_dashboard( # noqa: C901 if request.description: dashboard.description = request.description + if request.slug: + dashboard.slug = request.slug Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing slug-uniqueness error handling</b></div> <div id="fix"> Setting `dashboard.slug = request.slug` at line 322 can raise `IntegrityError` when the slug is already taken, since the `Dashboard.slug` column is `unique=True` (dashboard.py:144). The existing broad `except SQLAlchemyError` block at line 363 catches this, but the error message at line 379 is generic ("Failed to create dashboard due to a database error") and does not tell the caller that the slug is the problem. Compare with `update_dashboard.py:126-129` which handles slug assignment without an explicit guard. Add `IntegrityError` handling to produce a specific "slug already in use" error. </div> </div> <small><i>Code Review Run #f8211d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
