bito-code-review[bot] commented on code in PR #36271:
URL: https://github.com/apache/superset/pull/36271#discussion_r2560991482
##########
superset/mcp_service/chart/tool/get_chart_info.py:
##########
@@ -41,7 +41,7 @@
@mcp_auth_hook
@parse_request(GetChartInfoRequest)
async def get_chart_info(
- request: GetChartInfoRequest, ctx: Context
+ request: str | GetChartInfoRequest, ctx: Context
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Type annotation inconsistency with parse_request
decorator</b></div>
<div id="fix">
The type annotation was changed to allow `str | GetChartInfoRequest`, but
the `@parse_request(GetChartInfoRequest)` decorator ensures the parameter is
always parsed into a `GetChartInfoRequest` object. The code accesses
`request.identifier`, which would fail if a plain string is passed without
parsing. This inconsistency can cause runtime AttributeErrors when the
decorator fails to parse invalid strings. Revert to `GetChartInfoRequest` to
match the decorator's behavior and prevent type mismatches that could break
functionality in `get_chart_info` and similar tools like `list_charts` or
`get_dataset_info`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
request: GetChartInfoRequest, ctx: Context
````
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/36271#issuecomment-3576906529>#5f89eb</a></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/tool/update_chart_preview.py:
##########
@@ -50,7 +50,7 @@
@mcp_auth_hook
@parse_request(UpdateChartPreviewRequest)
def update_chart_preview(
- request: UpdateChartPreviewRequest, ctx: Context
+ request: str | UpdateChartPreviewRequest, ctx: Context
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect parameter type hint union</b></div>
<div id="fix">
The type hint for the `request` parameter was changed to `str |
UpdateChartPreviewRequest`, but the function body immediately accesses
attributes like `request.config`, `request.dataset_id`, and
`request.form_data_key` which only exist on `UpdateChartPreviewRequest`. Since
the `@parse_request(UpdateChartPreviewRequest)` decorator ensures the input is
parsed to that type, allowing `str` in the union will cause an `AttributeError`
at runtime if a string is somehow passed. Revert to the correct type hint to
match the decorator's behavior and prevent runtime failures.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
request: UpdateChartPreviewRequest, ctx: Context
````
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/36271#issuecomment-3576906529>#5f89eb</a></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/sql_lab/tool/execute_sql.py:
##########
@@ -41,7 +41,7 @@
@mcp.tool
@mcp_auth_hook
@parse_request(ExecuteSqlRequest)
-async def execute_sql(request: ExecuteSqlRequest, ctx: Context) ->
ExecuteSqlResponse:
+async def execute_sql(request: str | ExecuteSqlRequest, ctx: Context) ->
ExecuteSqlResponse:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect function parameter type</b></div>
<div id="fix">
The function parameter type was changed from `ExecuteSqlRequest` to `str |
ExecuteSqlRequest`, but the function body accesses attributes like
`database_id`, `sql`, `timeout`, etc., which only exist on `ExecuteSqlRequest`.
Passing a `str` would cause an `AttributeError` at runtime, breaking SQL
execution. The `@parse_request(ExecuteSqlRequest)` decorator ensures the input
is parsed into `ExecuteSqlRequest`, so the union type is unnecessary and
dangerous.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
async def execute_sql(request: ExecuteSqlRequest, ctx: Context) ->
ExecuteSqlResponse:
````
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/36271#issuecomment-3576906529>#5f89eb</a></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:
##########
@@ -122,7 +122,7 @@ def _create_dashboard_layout(chart_objects: List[Any]) ->
Dict[str, Any]:
@mcp_auth_hook
@parse_request(GenerateDashboardRequest)
def generate_dashboard(
- request: GenerateDashboardRequest, ctx: Context
+ request: str | GenerateDashboardRequest, ctx: Context
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect type hint for request parameter</b></div>
<div id="fix">
The type hint for the 'request' parameter was incorrectly changed to 'str |
GenerateDashboardRequest', but the @parse_request decorator ensures the
function always receives a 'GenerateDashboardRequest' object. This violates
type safety guidelines and MyPy compliance requirements. Revert the type hint
to 'GenerateDashboardRequest' to accurately reflect the parameter type and
maintain proper typing.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
request: GenerateDashboardRequest, ctx: Context
````
</div>
</details>
</div>
<small><i>Code Review Run <a
href=https://github.com/apache/superset/pull/36271#issuecomment-3576906529>#5f89eb</a></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]
