bito-code-review[bot] commented on code in PR #37859:
URL: https://github.com/apache/superset/pull/37859#discussion_r2789925839
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1807,66 +1808,72 @@ async def _get_chart_preview_internal( # noqa: C901
from superset.daos.chart import ChartDAO
# Find the chart
- chart: Any = None
- if isinstance(request.identifier, int) or (
- isinstance(request.identifier, str) and
request.identifier.isdigit()
- ):
- chart_id = (
- int(request.identifier)
- if isinstance(request.identifier, str)
- else request.identifier
- )
- await ctx.debug(
- "Performing ID-based chart lookup: chart_id=%s" % (chart_id,)
- )
- chart = ChartDAO.find_by_id(chart_id)
- else:
- await ctx.debug(
- "Performing UUID-based chart lookup: uuid=%s" %
(request.identifier,)
- )
- # Try UUID lookup using DAO flexible method
- chart = ChartDAO.find_by_id(request.identifier, id_column="uuid")
-
- # If not found and looks like a form_data_key, try to create
transient chart
- if (
- not chart
- and isinstance(request.identifier, str)
- and len(request.identifier) > 8
+ with
event_logger.log_context(action="mcp.get_chart_preview.chart_lookup"):
+ chart: Any = None
+ if isinstance(request.identifier, int) or (
+ isinstance(request.identifier, str) and
request.identifier.isdigit()
):
- # This might be a form_data_key, try to get form data from
cache
- from superset.commands.explore.form_data.get import
GetFormDataCommand
- from superset.commands.explore.form_data.parameters import (
- CommandParameters,
+ chart_id = (
+ int(request.identifier)
+ if isinstance(request.identifier, str)
+ else request.identifier
)
-
- try:
- cmd_params = CommandParameters(key=request.identifier)
- cmd = GetFormDataCommand(cmd_params)
- form_data_json = cmd.run()
- if form_data_json:
- from superset.utils import json as utils_json
-
- form_data = utils_json.loads(form_data_json)
-
- # Create a transient chart object from form data
- class TransientChart:
- def __init__(self, form_data: Dict[str, Any]):
- self.id = None
- self.slice_name = "Unsaved Chart Preview"
- self.viz_type = form_data.get("viz_type",
"table")
- self.datasource_id = None
- self.datasource_type = "table"
- self.params = utils_json.dumps(form_data)
- self.form_data = form_data
- self.uuid = None
-
- chart = TransientChart(form_data)
- except Exception as e:
- # Form data key not found or invalid
- logger.debug(
- "Failed to get form data for key %s: %s",
request.identifier, e
+ await ctx.debug(
+ "Performing ID-based chart lookup: chart_id=%s" %
(chart_id,)
+ )
+ chart = ChartDAO.find_by_id(chart_id)
+ else:
+ await ctx.debug(
+ "Performing UUID-based chart lookup: uuid=%s"
+ % (request.identifier,)
+ )
+ # Try UUID lookup using DAO flexible method
+ chart = ChartDAO.find_by_id(request.identifier,
id_column="uuid")
+
+ # If not found and looks like a form_data_key, try transient
+ if (
+ not chart
+ and isinstance(request.identifier, str)
+ and len(request.identifier) > 8
+ ):
+ # This might be a form_data_key
+ from superset.commands.explore.form_data.get import (
+ GetFormDataCommand,
+ )
+ from superset.commands.explore.form_data.parameters import
(
+ CommandParameters,
)
+ try:
+ cmd_params = CommandParameters(key=request.identifier)
+ cmd = GetFormDataCommand(cmd_params)
+ form_data_json = cmd.run()
+ if form_data_json:
+ from superset.utils import json as utils_json
+
+ form_data = utils_json.loads(form_data_json)
+
+ # Create a transient chart object from form data
+ class TransientChart:
+ def __init__(self, form_data: Dict[str, Any]):
+ self.id = None
+ self.slice_name = "Unsaved Chart Preview"
+ self.viz_type = form_data.get("viz_type",
"table")
+ self.datasource_id = None
+ self.datasource_type = "table"
+ self.params = utils_json.dumps(form_data)
+ self.form_data = form_data
+ self.uuid = None
+
+ chart = TransientChart(form_data)
+ except Exception as e:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Avoid catching blind Exception</b></div>
<div id="fix">
Replace bare `except Exception` with specific exception types (e.g., `except
(ValueError, KeyError, AttributeError)`) to catch only expected errors.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
except (ValueError, KeyError, AttributeError, TypeError)
as e:
````
</div>
</details>
</div>
<small><i>Code Review Run #141cfd</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/middleware.py:
##########
@@ -115,16 +117,76 @@ async def on_message(
user_id = get_user_id()
except Exception:
user_id = None
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Blind exception catch without specificity</b></div>
<div id="fix">
Replace the bare `Exception` catch on line 118 with a more specific
exception type (e.g., `RuntimeError`, `ValueError`) to avoid catching
unexpected errors. Multiple similar issues exist on lines 263, 541, 738, 756,
and 777.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
except (RuntimeError, ValueError):
user_id = None
````
</div>
</details>
</div>
<small><i>Code Review Run #141cfd</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]
