codeant-ai-for-open-source[bot] commented on code in PR #40007:
URL: https://github.com/apache/superset/pull/40007#discussion_r3216296751
##########
superset/commands/dashboard/export.py:
##########
@@ -230,3 +241,14 @@ def _export(
dataset = DatasetDAO.find_by_id(dataset_id)
if dataset:
yield from
ExportDatasetsCommand([dataset_id]).run()
+
+ # Export datasets referenced by display controls
+ for customization in payload.get("metadata", {}).get(
+ "chart_customization_config", []
+ ):
+ for target in customization.get("targets", []):
+ dataset_id = target.pop("datasetId", None)
+ if dataset_id is not None:
+ dataset = DatasetDAO.find_by_id(dataset_id)
+ if dataset:
+ yield from
ExportDatasetsCommand([dataset_id]).run()
Review Comment:
**Suggestion:** Dataset export is executed once per display-control target,
so multiple targets pointing to the same dataset repeatedly run
`DatasetDAO.find_by_id` and `ExportDatasetsCommand`, causing avoidable repeated
DB work and export processing. Collect unique dataset IDs first, then export
each dataset once. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard export slower with many display controls sharing datasets.
- ⚠️ Additional database lookups during each dashboard export request.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger a dashboard export via the REST API endpoint `GET
/api/v1/dashboard/export/`
implemented in `superset/dashboards/api.py:1207-63`, which calls
`ExportDashboardsCommand(requested_ids).run()` at
`superset/dashboards/api.py:1245-1248`.
2. `ExportDashboardsCommand.run()` (inherited from `ExportModelsCommand.run`
at
`superset/commands/export/models.py:58-73`) invokes
`ExportDashboardsCommand._export(model, export_related=True)` for each
requested
`Dashboard` (`superset/commands/dashboard/export.py:188-196`).
3. Inside `_export`, after building `payload = model.export_to_dict(...)` at
`superset/commands/dashboard/export.py:216-221`, the code enters the `if
export_related:`
block at `superset/commands/dashboard/export.py:233-234` and iterates
`payload["metadata"]["chart_customization_config"]` at lines `245-249`, then
for each
`target` executes:
- `dataset_id = target.pop("datasetId", None)` (`export.py:250`),
- `dataset = DatasetDAO.find_by_id(dataset_id)` (`export.py:252`),
- `yield from ExportDatasetsCommand([dataset_id]).run()`
(`export.py:254`).
4. When multiple display-control targets in `chart_customization_config`
reference the
same `datasetId` (a common case when several controls are bound to the same
dataset), this
loop performs `DatasetDAO.find_by_id` and instantiates and runs
`ExportDatasetsCommand([dataset_id])` once per target, duplicating database
lookups and
dataset-export work for the same dataset ID even though the outer
`ExportModelsCommand.run` will ultimately de-duplicate by file name only.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fcommands%2Fdashboard%2Fexport.py%0A%2A%2ALine%3A%2A%2A%20245%3A254%0A%2A%2AComment%3A%2A%2A%0A%09%2APerformance%3A%20Dataset%20export%20is%20executed%20once%20per%20display-control%20target%2C%20so%20multiple%20targets%20pointing%20to%20the%20same%20dataset%20repeatedly%20run%20%60DatasetDAO.find_by_id%60%20and%20%60ExportDatasetsCommand%60%2C%20causing%20avoidable%20repeated%20DB%20work%20and%20export%20processing.%20Collect%20unique%20dataset%20IDs%20first%2C%20then%20export%20each%20dataset%20once.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user
%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fcommands%2Fdashboard%2Fexport.py%0A%2A%2ALine%3A%2A%2A%20245%3A254%0A%2A%2AComment%3A%2A%2A%0A%09%2APerformance%3A%20Dataset%20export%20is%20executed%20once%20per%20display-control%20target%2C%20so%20multiple%20targets%20pointing%20to%20the%20same%20dataset%20repeatedly%20run%20%60DatasetDAO.find_by_id%60%20and%20%60ExportDatasetsCommand%60%2C%20causing%20avoidable%20repeated%20DB%20work%20and%20export%20processing.%20Collect%20unique%20dataset%20IDs%20first%2C%20then%20export%20each%20dataset%20once.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%
20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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/commands/dashboard/export.py
**Line:** 245:254
**Comment:**
*Performance: Dataset export is executed once per display-control
target, so multiple targets pointing to the same dataset repeatedly run
`DatasetDAO.find_by_id` and `ExportDatasetsCommand`, causing avoidable repeated
DB work and export processing. Collect unique dataset IDs first, then export
each dataset once.
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%2F40007&comment_hash=44eb0cec8c12c2a83bd546d54e714c5850eacb8c20c08dbedaec4b7546f2924e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40007&comment_hash=44eb0cec8c12c2a83bd546d54e714c5850eacb8c20c08dbedaec4b7546f2924e&reaction=dislike'>👎</a>
##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -139,6 +145,18 @@ def update_id_refs( # pylint: disable=too-many-locals #
noqa: C901
native_filter["scope"]["excluded"] = [
id_map[old_id] for old_id in scope_excluded if old_id in id_map
]
+
+ # fix display control dataset references
+ chart_customization_config = fixed.get("metadata", {}).get(
+ "chart_customization_config", []
+ )
+ for customization in chart_customization_config:
+ targets = customization.get("targets", [])
+ for target in targets:
+ dataset_uuid = target.pop("datasetUuid", None)
+ if dataset_uuid:
+ target["datasetId"] =
dataset_info[dataset_uuid]["datasource_id"]
Review Comment:
**Suggestion:** This new display-control remapping can raise a `KeyError`
when a `datasetUuid` is present in dashboard metadata but missing from
`dataset_info` (for example, partial bundles or unresolved dataset imports),
which aborts the whole dashboard import. Guard the lookup before indexing and
skip or gracefully handle unknown dataset UUIDs instead of hard-failing.
[possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard import can crash on incomplete dashboard+dataset bundles.
- ⚠️ Users receive HTTP 500 instead of clear import error.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the dashboard import endpoint `POST /api/v1/dashboard/import/`
implemented in
`superset/dashboards/api.py:1852-1938` with a ZIP bundle whose
`dashboards/*.yaml`
metadata includes `chart_customization_config` targets containing a
`datasetUuid` that
does not have a corresponding `datasets/*.yaml` entry in the same bundle.
2. The API builds `contents` from the upload
(`dashboards/api.py:73-81,1930-3`) and
instantiates `ImportDashboardsCommand(contents, ...)` then calls
`command.run()` at
`dashboards/api.py:1929-37`, which in turn executes
`ImportDashboardsCommand._import()`
(`superset/commands/dashboard/importers/v1/__init__.py:74-78`).
3. Inside `_import`, `dataset_info` is constructed only from dataset files
present in the
bundle at `superset/commands/dashboard/importers/v1/__init__.py:121-133`,
mapping
`str(dataset.uuid)` to a dict with `"datasource_id"` and related fields. Any
`datasetUuid`
that appears only in dashboard metadata (e.g., in display controls) but has
no
corresponding `datasets/*.yaml` is absent from `dataset_info`.
4. Later in `_import`, each dashboard config is passed to
`update_id_refs(config,
chart_ids, dataset_info)`
(`superset/commands/dashboard/importers/v1/__init__.py:172-175`). In
`update_id_refs`
(`superset/commands/dashboard/importers/v1/utils.py:66-161`), after handling
native
filters, the new block at lines `149-158` iterates
`chart_customization_config` targets
and for each with a `datasetUuid` executes
`dataset_info[dataset_uuid]["datasource_id"]`
without checking membership. If `dataset_uuid` is missing from
`dataset_info`, this line
raises a `KeyError`, causing the entire import command to fail and the
`/api/v1/dashboard/import/` request to return an internal server error
instead of
gracefully handling unknown or missing datasets.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fcommands%2Fdashboard%2Fimporters%2Fv1%2Futils.py%0A%2A%2ALine%3A%2A%2A%20149%3A158%0A%2A%2AComment%3A%2A%2A%0A%09%2APossible%20Bug%3A%20This%20new%20display-control%20remapping%20can%20raise%20a%20%60KeyError%60%20when%20a%20%60datasetUuid%60%20is%20present%20in%20dashboard%20metadata%20but%20missing%20from%20%60dataset_info%60%20%28for%20example%2C%20partial%20bundles%20or%20unresolved%20dataset%20imports%29%2C%20which%20aborts%20the%20whole%20dashboard%20import.%20Guard%20the%20lookup%20before%20indexing%20and%20skip%20or%20gracefully%20handle%20unknown%20dataset%20UUIDs%20instead%20of%20hard-failing.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20
fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fcommands%2Fdashboard%2Fimporters%2Fv1%2Futils.py%0A%2A%2ALine%3A%2A%2A%20149%3A158%0A%2A%2AComment%3A%2A%2A%0A%09%2APossible%20Bug%3A%20This%20new%20display-control%20remapping%20can%20raise%20a%20%60KeyError%60%20when%20a%20%60datasetUuid%60%20is%20present%20in%20dashboard%20metadata%20but%20missing%20from%20%60dataset_info%60%20%28for%20example%2C%20partial%20bundles%20or%20unresolved%20dataset%20imports%29%2C%20which%20aborts%20the%20whole%20dashboard%20import.%20Guard%20the%20lookup
%20before%20indexing%20and%20skip%20or%20gracefully%20handle%20unknown%20dataset%20UUIDs%20instead%20of%20hard-failing.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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/commands/dashboard/importers/v1/utils.py
**Line:** 149:158
**Comment:**
*Possible Bug: This new display-control remapping can raise a
`KeyError` when a `datasetUuid` is present in dashboard metadata but missing
from `dataset_info` (for example, partial bundles or unresolved dataset
imports), which aborts the whole dashboard import. Guard the lookup before
indexing and skip or gracefully handle unknown dataset UUIDs instead of
hard-failing.
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%2F40007&comment_hash=3b655a40eac00d5cb8cbff14726b4608636910268b4272582bcd7871af7226a1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40007&comment_hash=3b655a40eac00d5cb8cbff14726b4608636910268b4272582bcd7871af7226a1&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]
