codeant-ai-for-open-source[bot] commented on code in PR #40448:
URL: https://github.com/apache/superset/pull/40448#discussion_r3311144279
##########
superset/mcp_service/chart/validation/schema_validator.py:
##########
@@ -392,22 +392,42 @@ def _pre_validate_big_number_config(
],
error_code="INVALID_BIG_NUMBER_METRIC_TYPE",
)
- if not metric.get("aggregate") and not metric.get("saved_metric"):
+ if (
+ not metric.get("aggregate")
+ and not metric.get("saved_metric")
+ and not metric.get("sql_expression")
+ ):
return False, ChartGenerationError(
error_type="missing_metric_aggregate",
- message="Big Number metric must include an aggregate function "
- "or reference a saved metric",
- details="The metric must have an 'aggregate' field "
- "or 'saved_metric': true",
+ message="Big Number metric must include an aggregate function,
"
+ "a saved metric reference, or a SQL expression",
+ details="The metric must have an 'aggregate' field, "
+ "'saved_metric': true, or 'sql_expression'",
suggestions=[
"Add 'aggregate' to your metric: "
"{'name': 'col', 'aggregate': 'SUM'}",
"Or use a saved metric: "
"{'name': 'total_sales', 'saved_metric': true}",
+ "Or a custom SQL metric: "
+ "{'sql_expression': 'SUM(a)/SUM(b)', 'label': 'Ratio'}",
"Valid aggregates: SUM, COUNT, AVG, MIN, MAX",
],
error_code="MISSING_BIG_NUMBER_AGGREGATE",
)
+ if metric.get("sql_expression") and not (metric.get("label") or
"").strip():
Review Comment:
**Suggestion:** The new SQL-metric label check assumes `label` is always a
string and calls `.strip()` directly. If a client sends a non-string label (for
example a number), this raises `AttributeError` during pre-validation and can
bubble into a 500 instead of returning a structured validation error. Guard the
type first (or coerce safely) before calling string methods. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ SchemaValidator crashes on non-string SQL metric labels.
- ⚠️ Big number configs miss targeted label-required validation error.
- ⚠️ Pipeline may return generic system error instead of guidance.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Follow the existing pattern in
`tests/unit_tests/mcp_service/chart/test_big_number_chart.py:4-19`, but
construct a raw
request dict with a non-string label, e.g.:
`request_data = {"dataset_id": 1, "config": {"chart_type": "big_number",
"metric":
{"sql_expression": "SUM(a)/SUM(b)", "label": 123}}}`.
2. Call `SchemaValidator.validate_request(request_data)` as done in
`TestSchemaValidatorBigNumber.test_valid_big_number_request` at
`tests/unit_tests/mcp_service/chart/test_big_number_chart.py:4-15`; this
invokes
`_pre_validate` in
`superset/mcp_service/chart/validation/schema_validator.py:28-31`.
3. Inside `_pre_validate`, `chart_type` is read from `config` and
`_pre_validate_chart_type` is called (lines 110-120 in
`schema_validator.py`), which
dispatches `"big_number"` configs to
`_pre_validate_big_number_config(config)` at
`schema_validator.py:39`.
4. In `_pre_validate_big_number_config` (lines 1-71 in
`schema_validator.py`),
`metric.get("sql_expression")` is truthy and `metric.get("label")` returns
the integer
`123`, so the condition at line 58 / diff line 417 (`if
metric.get("sql_expression") and
not (metric.get("label") or "").strip():`) evaluates `(123 or "")` and then
calls
`123.strip()`, raising `AttributeError` instead of returning a
`ChartGenerationError` with
error_code `"MISSING_SQL_METRIC_LABEL"`.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=384d4f95ba964c78852379654766f3f9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=384d4f95ba964c78852379654766f3f9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/validation/schema_validator.py
**Line:** 417:417
**Comment:**
*Type Error: The new SQL-metric label check assumes `label` is always a
string and calls `.strip()` directly. If a client sends a non-string label (for
example a number), this raises `AttributeError` during pre-validation and can
bubble into a 500 instead of returning a structured validation error. Guard the
type first (or coerce safely) before calling string methods.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40448&comment_hash=9ba0846d52bb83497ca29ff2581b694bc8790c11dbf5a73c5c932b225e62332c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40448&comment_hash=9ba0846d52bb83497ca29ff2581b694bc8790c11dbf5a73c5c932b225e62332c&reaction=dislike'>👎</a>
--
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]
