bito-code-review[bot] commented on code in PR #44386:
URL: https://github.com/apache/superset/pull/44386#discussion_r4047314258
##########
superset/mcp_service/middleware.py:
##########
@@ -71,6 +76,40 @@
# tools) while rejecting StatsD metadata characters and unbounded lengths.
_METRIC_TOOL_NAME_RE = re.compile(r"[A-Za-z0-9_][A-Za-z0-9_.\-]{0,127}")
+# Character cap for the free-form string fields kept in a minimal
committed-write
+# confirmation (see ``_shrink_minimal_response``). Generous enough to keep a
+# chart name or a short error readable, small enough that the whole
confirmation
+# stays bounded no matter how large the fields were in the original payload.
+_MINIMAL_FIELD_CHARS = 200
+
+# Identifying fields kept when a structured ``error`` has to be reduced to fit
+# (see ``_clip_error``). Everything else on ``MCPBaseError`` and its subclasses
+# is an unbounded container -- ``validation_errors``, ``dataset_context``,
+# ``query_info``, ``suggestions`` -- any of which can dwarf the write
+# confirmation it is riding on. ``error`` mirrors ``message`` as a
+# backward-compatible alias, so both are kept.
+_MINIMAL_ERROR_FIELDS = ("error_type", "error", "message", "error_code",
"details")
+
+# Scalar fields kept when a committed write's identifying object (a nested
+# ``chart``/``dashboard``/``metric`` dict) has to be reduced to fit. These are
+# the names across the info models that answer "what was written" --
+# everything else on them is either unbounded or irrelevant to that question.
+# ``is_unsaved_state`` is here because update_chart defaults to
+# ``generate_preview=True`` and then persists nothing, so it is the caller's
+# only in-band way to tell a cached preview from a persisted write.
+_MINIMAL_IDENTITY_FIELDS = (
+ "id",
+ "uuid",
+ "url",
+ "slice_name",
+ "dashboard_title",
+ "metric_name",
+ "table_name",
+ "dataset_name",
+ "label",
+ "is_unsaved_state",
+)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Identity fields miss restore keys</b></div>
<div id="fix">
`_MINIMAL_IDENTITY_FIELDS` keeps only `id`/`uuid`/`url`/name-style keys, but
`RestoreChartResponse`/`RestoreDashboardResponse`/`DeleteChartResponse` carry
identity as `restored_id`, `restored_name`, `deleted_id`, `deleted_name`
(chart/schemas.py:4450, dashboard/schemas.py:2822). For those tools the dict
reduction at `_shrink_minimal_response` keeps nothing and only appends a note,
so the caller loses the only in-band record of what was written. Consider
adding those keys or deriving them from the spec.
</div>
</div>
<small><i>Code Review Run #4fe4d2</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]
