korbit-ai[bot] commented on code in PR #36035:
URL: https://github.com/apache/superset/pull/36035#discussion_r2501008590
##########
superset/mcp_service/dashboard/tool/list_dashboards.py:
##########
@@ -73,10 +74,29 @@ async def list_dashboards(
from superset.daos.dashboard import DashboardDAO
+ def _serialize_dashboard(obj: Any, cols: Any) -> DashboardInfo | None:
+ """Serialize dashboard object with column selection."""
+ full_dashboard = serialize_dashboard_object(obj)
+ if not full_dashboard:
+ return None
+
+ # If columns were specified, exclude fields not in the selection
+ # Always include 'id' as it's the primary identifier
+ if cols and isinstance(cols, list):
+ all_fields = set(DashboardInfo.model_fields.keys())
+ requested_fields = set(cols) | {"id"} # Always include id
+ exclude_fields = all_fields - requested_fields
+
+ # Create new model instance with excluded fields removed
+ dumped = full_dashboard.model_dump(exclude=exclude_fields)
+ return DashboardInfo(**dumped)
Review Comment:
### Unsafe model reconstruction from dumped data <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code creates a new DashboardInfo instance from dumped data, but this may
fail if required fields are excluded or if the dumped data contains fields that
DashboardInfo doesn't accept.
###### Why this matters
This could cause runtime errors when creating the DashboardInfo instance,
especially if required fields are excluded from the model or if there are
validation issues with the reconstructed model.
###### Suggested change ∙ *Feature Preview*
Add error handling and validation when reconstructing the model:
```python
try:
dumped = full_dashboard.model_dump(exclude=exclude_fields)
return DashboardInfo(**dumped)
except Exception as e:
logger.warning(f"Failed to create filtered DashboardInfo: {e}")
return full_dashboard
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc075d21-e9b3-4cf2-964b-84045e0ee84a/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc075d21-e9b3-4cf2-964b-84045e0ee84a?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc075d21-e9b3-4cf2-964b-84045e0ee84a?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc075d21-e9b3-4cf2-964b-84045e0ee84a?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc075d21-e9b3-4cf2-964b-84045e0ee84a)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6958c7fc-2804-4f2c-bf26-1345af8bf17d -->
[](6958c7fc-2804-4f2c-bf26-1345af8bf17d)
##########
superset/mcp_service/dashboard/tool/list_dashboards.py:
##########
@@ -73,10 +74,29 @@ async def list_dashboards(
from superset.daos.dashboard import DashboardDAO
+ def _serialize_dashboard(obj: Any, cols: Any) -> DashboardInfo | None:
+ """Serialize dashboard object with column selection."""
+ full_dashboard = serialize_dashboard_object(obj)
+ if not full_dashboard:
+ return None
+
+ # If columns were specified, exclude fields not in the selection
+ # Always include 'id' as it's the primary identifier
+ if cols and isinstance(cols, list):
+ all_fields = set(DashboardInfo.model_fields.keys())
+ requested_fields = set(cols) | {"id"} # Always include id
Review Comment:
### Hardcoded field assumption without validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code assumes 'id' is always a valid field in DashboardInfo, but doesn't
verify this assumption, which could lead to issues if the model structure
changes.
###### Why this matters
If the DashboardInfo model doesn't have an 'id' field or if it's renamed,
this logic will still try to include it, potentially causing unexpected
behavior or errors.
###### Suggested change ∙ *Feature Preview*
Verify that 'id' exists in the model fields before including it:
```python
all_fields = set(DashboardInfo.model_fields.keys())
requested_fields = set(cols)
if "id" in all_fields:
requested_fields.add("id")
exclude_fields = all_fields - requested_fields
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b41aeddd-ea4d-49b6-bf24-017a1b49af7b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b41aeddd-ea4d-49b6-bf24-017a1b49af7b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b41aeddd-ea4d-49b6-bf24-017a1b49af7b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b41aeddd-ea4d-49b6-bf24-017a1b49af7b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b41aeddd-ea4d-49b6-bf24-017a1b49af7b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:cdcb77f6-1702-4e92-8134-82036db4ab8a -->
[](cdcb77f6-1702-4e92-8134-82036db4ab8a)
##########
superset/mcp_service/dataset/tool/list_datasets.py:
##########
@@ -102,11 +103,30 @@ async def list_datasets(request: ListDatasetsRequest,
ctx: Context) -> DatasetLi
try:
from superset.daos.dataset import DatasetDAO
+ def _serialize_dataset(obj: Any, cols: Any) -> DatasetInfo | None:
+ """Serialize dataset object with column selection."""
+ full_dataset = serialize_dataset_object(obj)
+ if not full_dataset:
+ return None
+
+ # If columns were specified, exclude fields not in the selection
+ # Always include 'id' as it's the primary identifier
+ if cols and isinstance(cols, list):
+ all_fields = set(DatasetInfo.model_fields.keys())
+ requested_fields = set(cols) | {"id"} # Always include id
+ exclude_fields = all_fields - requested_fields
+
+ # Create new model instance with excluded fields removed
+ dumped = full_dataset.model_dump(exclude=exclude_fields)
+ return DatasetInfo(**dumped)
Review Comment:
### Potential validation error when excluding required fields
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code creates a new DatasetInfo instance from a dumped dictionary, but
this may fail if the excluded fields are required fields in the DatasetInfo
model.
###### Why this matters
If any of the excluded fields are required (non-optional) in the DatasetInfo
Pydantic model, attempting to create a new instance without them will raise a
validation error, causing the function to crash.
###### Suggested change ∙ *Feature Preview*
Add validation to ensure required fields are never excluded, or handle the
potential ValidationError:
```python
from pydantic import ValidationError
def _serialize_dataset(obj: Any, cols: Any) -> DatasetInfo | None:
"""Serialize dataset object with column selection."""
full_dataset = serialize_dataset_object(obj)
if not full_dataset:
return None
# If columns were specified, exclude fields not in the selection
# Always include 'id' as it's the primary identifier
if cols and isinstance(cols, list):
all_fields = set(DatasetInfo.model_fields.keys())
requested_fields = set(cols) | {"id"} # Always include id
# Get required fields to ensure they're never excluded
required_fields = {
name for name, field in DatasetInfo.model_fields.items()
if field.is_required()
}
requested_fields |= required_fields # Always include required fields
exclude_fields = all_fields - requested_fields
try:
# Create new model instance with excluded fields removed
dumped = full_dataset.model_dump(exclude=exclude_fields)
return DatasetInfo(**dumped)
except ValidationError:
# Fallback to full dataset if validation fails
return full_dataset
return full_dataset
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ee4ca05-0797-4690-b2cd-5c8de4070680/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ee4ca05-0797-4690-b2cd-5c8de4070680?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ee4ca05-0797-4690-b2cd-5c8de4070680?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ee4ca05-0797-4690-b2cd-5c8de4070680?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8ee4ca05-0797-4690-b2cd-5c8de4070680)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a83ef902-b9be-4b34-a131-e2e304a38d49 -->
[](a83ef902-b9be-4b34-a131-e2e304a38d49)
##########
superset/mcp_service/chart/tool/list_charts.py:
##########
@@ -95,8 +95,23 @@ async def list_charts(request: ListChartsRequest, ctx:
Context) -> ChartList:
from superset.daos.chart import ChartDAO
def _serialize_chart(obj: "Slice | None", cols: Any) -> ChartInfo | None:
- """Serialize chart object with proper type casting."""
- return serialize_chart_object(cast(ChartLike | None, obj))
+ """Serialize chart object with proper type casting and column
selection."""
+ full_chart = serialize_chart_object(cast(ChartLike | None, obj))
+ if not full_chart:
+ return None
+
+ # If columns were specified, exclude fields not in the selection
+ # Always include 'id' as it's the primary identifier
+ if cols and isinstance(cols, list):
+ all_fields = set(ChartInfo.model_fields.keys())
+ requested_fields = set(cols) | {"id"} # Always include id
+ exclude_fields = all_fields - requested_fields
+
+ # Create new model instance with excluded fields removed
+ dumped = full_chart.model_dump(exclude=exclude_fields)
+ return ChartInfo(**dumped)
Review Comment:
### Function has multiple responsibilities <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The _serialize_chart function violates the Single Responsibility Principle
by handling both serialization and field filtering logic.
###### Why this matters
Mixing these responsibilities makes the code harder to maintain and test.
Changes to either serialization or filtering logic could affect the other,
increasing the risk of bugs.
###### Suggested change ∙ *Feature Preview*
Split the function into two separate functions:
```python
def _serialize_chart(obj: "Slice | None") -> ChartInfo | None:
return serialize_chart_object(cast(ChartLike | None, obj))
def _filter_chart_fields(chart: ChartInfo, cols: list) -> ChartInfo:
all_fields = set(ChartInfo.model_fields.keys())
requested_fields = set(cols) | {"id"}
exclude_fields = all_fields - requested_fields
return ChartInfo(**chart.model_dump(exclude=exclude_fields))
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c472d423-5ba3-4238-9169-f0426e0dcc4e/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c472d423-5ba3-4238-9169-f0426e0dcc4e?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c472d423-5ba3-4238-9169-f0426e0dcc4e?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c472d423-5ba3-4238-9169-f0426e0dcc4e?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c472d423-5ba3-4238-9169-f0426e0dcc4e)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4b605e0f-e877-405b-9da3-d5acc64b61ad -->
[](4b605e0f-e877-405b-9da3-d5acc64b61ad)
##########
superset/mcp_service/dashboard/tool/list_dashboards.py:
##########
@@ -73,10 +74,29 @@ async def list_dashboards(
from superset.daos.dashboard import DashboardDAO
+ def _serialize_dashboard(obj: Any, cols: Any) -> DashboardInfo | None:
Review Comment:
### Overly permissive type hints <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function uses 'Any' type hints for both parameters, which reduces type
safety and makes the code less maintainable.
###### Why this matters
Using 'Any' bypasses Python's type checking system, making it harder to
catch type-related bugs early and reducing code clarity for other developers.
###### Suggested change ∙ *Feature Preview*
```python
def _serialize_dashboard(
obj: DashboardDAO.Model,
cols: list[str] | None
) -> DashboardInfo | None:
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a1d0e18-6856-4e9d-885d-ef2bb559dca2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a1d0e18-6856-4e9d-885d-ef2bb559dca2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a1d0e18-6856-4e9d-885d-ef2bb559dca2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a1d0e18-6856-4e9d-885d-ef2bb559dca2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1a1d0e18-6856-4e9d-885d-ef2bb559dca2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d3a9f123-de67-42c0-bae2-dc6d2a9c99a7 -->
[](d3a9f123-de67-42c0-bae2-dc6d2a9c99a7)
--
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]
