bito-code-review[bot] commented on code in PR #40399:
URL: https://github.com/apache/superset/pull/40399#discussion_r3351891895
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1063,8 +1270,20 @@ def _sanitize_dashboard_info_for_llm_context(
return DashboardInfo.model_validate(payload)
+def _safe_user_label(value: Any) -> str | None:
+ """Coerce a `*_by_name` model attribute to a display string or None.
+
+ The Dashboard model exposes ``created_by_name`` / ``changed_by_name``
+ as plain strings, but some serializer call sites pass through
+ objects (User instances, Mocks in tests) — defensive coercion keeps
+ the response a valid string and avoids leaking ``repr(user)``.
+ """
+ if isinstance(value, str) and value:
+ return value
+ return None
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for _safe_user_label</b></div>
<div id="fix">
Add unit tests for the new `_safe_user_label` function in
`test_dashboard_schemas.py`. Cover cases for non-empty strings, empty strings,
`None`, and non-string objects to verify the defensive coercion.
</div>
</div>
<small><i>Code Review Run #fd006d</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><div>
<div id="suggestion">
<div id="issue"><b>Missing slug uniqueness validation</b></div>
<div id="fix">
Before assigning `request.slug` to `dashboard.slug`, call
`DashboardDAO.validate_slug_uniqueness(request.slug)`. If it returns `False`,
call `db.session.rollback()` and return a `GenerateDashboardResponse` with
`error = f"A dashboard with slug '{request.slug}' already exists. Choose a
different slug."`.
</div>
</div>
<small><i>Code Review Run #fd006d</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]
