bito-code-review[bot] commented on code in PR #42655:
URL: https://github.com/apache/superset/pull/42655#discussion_r3717626752
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2347,6 +2347,14 @@ class UpdateChartRequest(ChartRequestNormalizerMixin,
QueryCacheControl):
None,
description="Chart configuration. Optional; omit to only update
chart_name.",
)
+ add_columns: List[ColumnRef] | None = Field(
+ None,
+ description=(
+ "Table columns or metrics to append while preserving every
existing "
+ "column and metric. Use this instead of config.columns when adding
"
+ "columns to an existing table chart."
+ ),
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test coverage for new field</b></div>
<div id="fix">
The new `add_columns` field (lines 2350-2357) has no corresponding unit
tests in `test_chart_schemas.py`. Per the project's testing guidelines, every
new MCP mutation tool schema field requires dedicated unit test coverage
including request validation and schema serialization.
</div>
</div>
<small><i>Code Review Run #b295fd</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/chart/schemas.py:
##########
@@ -2379,6 +2387,19 @@ class UpdateChartRequest(ChartRequestNormalizerMixin,
QueryCacheControl):
),
)
+ @model_validator(mode="after")
+ def validate_column_patch(self) -> "UpdateChartRequest":
+ """Keep full-config replacement and additive table updates
unambiguous."""
+ if self.config is not None and self.add_columns is not None:
+ raise ValueError(
+ "Use either 'config' for a full visualization replacement or "
+ "'add_columns' to append table columns while preserving the
existing "
+ "configuration, not both."
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test for validator error branches</b></div>
<div id="fix">
The `validate_column_patch` validator (lines 2393-2401) that enforces mutual
exclusivity of `config` and `add_columns` has no unit tests. Both its error
branches — blocking co-provision and blocking empty list — are untested.
</div>
</div>
<small><i>Code Review Run #b295fd</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]
