fix(dashboard): handle missing chart metadata in dashboard export - #40575
fix(dashboard): handle missing chart metadata in dashboard export#40575mostafamos wants to merge 1 commit into
Conversation
Code Review Agent Run #cd74d9Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if uuid is not None and chart_id is not None: | ||
| result[uuid] = chart_id |
There was a problem hiding this comment.
Suggestion: The new guard still accepts any non-None uuid, including unhashable values like dict/list from corrupt payloads. Writing result[uuid] will then raise TypeError and break dashboard import. Validate that uuid is a string (or at least hashable) before using it as a dict key. [type error]
Severity Level: Major ⚠️
- ❌ Assets import API crashes on malformed dashboard UUID meta.
- ⚠️ Dashboard bundles with corrupt chart UUIDs cannot be imported.Steps of Reproduction ✅
1. Trigger the assets import API endpoint `POST /api/v1/assets/import/`, which is
implemented by `ImportExportRestApi.import_` in `superset/importexport/api.py:95-103`.
This endpoint reads the uploaded ZIP bundle and calls `ImportAssetsCommand(...)` at
`superset/importexport/api.py:228-237`.
2. Ensure the uploaded bundle contains a dashboard V1 YAML under `dashboards/` whose
`position` field includes a CHART node with `meta["uuid"]` set to a YAML mapping (parsed
as a Python `dict`) and `meta["chartId"]` set to a valid integer. The resulting config
dict is passed unchanged into `ImportAssetsCommand._import` in
`superset/commands/importers/v1/assets.py:122-165`.
3. In `ImportAssetsCommand._import`, for each `dashboards/...` file, the code executes
`config = update_id_refs(config, chart_ids, dataset_info)` at
`superset/commands/importers/v1/assets.py:161-165`. This calls `update_id_refs` in
`superset/commands/dashboard/importers/v1/utils.py:85-99` with the corrupted `position`
structure from the bundle.
4. Inside `update_id_refs`, `old_ids = build_uuid_to_id_map(fixed["position"])` at
`superset/commands/dashboard/importers/v1/utils.py:94` calls `build_uuid_to_id_map`. In
that function, the corrupted CHART node is processed, `uuid` is a `dict`, `chart_id` is an
`int`, the condition at line 65 (`if uuid is not None and chart_id is not None:`) passes,
and line 66 (`result[uuid] = chart_id`) raises `TypeError: unhashable type: 'dict'`,
causing the entire import request to fail with a server error instead of skipping the
malformed entry.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dashboard/importers/v1/utils.py
**Line:** 65:66
**Comment:**
*Type Error: The new guard still accepts any non-`None` `uuid`, including unhashable values like dict/list from corrupt payloads. Writing `result[uuid]` will then raise `TypeError` and break dashboard import. Validate that `uuid` is a string (or at least hashable) before using it as a dict key.
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| metadata["expanded_slices"] = { | ||
| str(id_map[int(old_id)]): value | ||
| for old_id, value in metadata["expanded_slices"].items() | ||
| if int(old_id) in id_map |
There was a problem hiding this comment.
Suggestion: Converting old_id with int(old_id) in the filter condition will throw ValueError for malformed keys (for example "foo"), so corrupt expanded_slices data still crashes import instead of being skipped. Parse IDs defensively (try/except) and drop non-numeric entries. [type error]
Severity Level: Major ⚠️
- ❌ Assets import API crashes on malformed expanded_slices metadata.
- ⚠️ Dashboards with corrupt expanded_slices JSON cannot be imported.Steps of Reproduction ✅
1. Use the assets import REST endpoint `POST /api/v1/assets/import/` implemented by
`ImportExportRestApi.import_` in `superset/importexport/api.py:95-103`. This endpoint
unwraps the uploaded ZIP and constructs an `ImportAssetsCommand` at
`superset/importexport/api.py:228-237` with the bundle contents.
2. Ensure the bundle includes a `dashboards/...` YAML whose `metadata` JSON (stored under
`json_metadata`) contains an `expanded_slices` object with at least one key that is not a
numeric string, for example `{"foo": true}`. When loaded, this becomes
`metadata["expanded_slices"]` in the `config` dict consumed by
`ImportAssetsCommand._import` in
`superset/commands/importers/v1/assets.py:134-143,161-165`.
3. In `ImportAssetsCommand._import`, for each dashboard file, the code calls `config =
update_id_refs(config, chart_ids, dataset_info)` at
`superset/commands/importers/v1/assets.py:161-165`, invoking `update_id_refs` in
`superset/commands/dashboard/importers/v1/utils.py:85-99` with the corrupted `metadata`
dict and the generated `id_map`.
4. Inside `update_id_refs`, the `"expanded_slices"` block at
`superset/commands/dashboard/importers/v1/utils.py:126-131` executes when that key is
present. During the dict comprehension, for the non-numeric key `old_id="foo"`, the filter
condition `if int(old_id) in id_map` at line 130 calls `int("foo")` and raises
`ValueError: invalid literal for int() with base 10: 'foo'`. This exception is uncaught,
causing the entire dashboard import to fail instead of simply dropping the malformed
`expanded_slices` entry.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dashboard/importers/v1/utils.py
**Line:** 130:130
**Comment:**
*Type Error: Converting `old_id` with `int(old_id)` in the filter condition will throw `ValueError` for malformed keys (for example `"foo"`), so corrupt `expanded_slices` data still crashes import instead of being skipped. Parse IDs defensively (try/except) and drop non-numeric entries.
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| if "uuid" in meta and meta["uuid"] in chart_ids: | ||
| meta["chartId"] = chart_ids[meta["uuid"]] |
There was a problem hiding this comment.
Suggestion: This membership check can raise TypeError when meta["uuid"] is an unhashable corrupt value (for example, a dict/list), so the "defensive" path still crashes on malformed exports. Add a type/hashability check before probing chart_ids. [type error]
Severity Level: Major ⚠️
- ❌ Assets import API crashes remapping chart IDs on import.
- ⚠️ Dashboard imports with corrupt UUID metadata fail unpredictably.Steps of Reproduction ✅
1. Call the assets import endpoint `POST /api/v1/assets/import/` handled by
`ImportExportRestApi.import_` in `superset/importexport/api.py:95-103`, which constructs
an `ImportAssetsCommand` at `superset/importexport/api.py:228-237` from the uploaded ZIP
bundle.
2. Include in the bundle a `dashboards/...` YAML whose `position` field has a CHART entry
with `meta["uuid"]` set to a YAML mapping (Python `dict`) and `meta["chartId"]` either
missing or explicitly `null`. This configuration is loaded into the `config` dict passed
to `ImportAssetsCommand._import` in `superset/commands/importers/v1/assets.py:122-165`.
3. In `ImportAssetsCommand._import`, when processing `dashboards/...` entries, the code
calls `config = update_id_refs(config, chart_ids, dataset_info)` at
`superset/commands/importers/v1/assets.py:161-165`, invoking `update_id_refs` in
`superset/commands/dashboard/importers/v1/utils.py:85-99` with the corrupted `position`.
4. Inside `update_id_refs`, after metadata fixes, the "fix position" loop at
`superset/commands/dashboard/importers/v1/utils.py:143-155` iterates over
`position.values()`. For the corrupted CHART child, `meta` is a dict and the condition `if
"uuid" in meta and meta["uuid"] in chart_ids:` at line 153 executes. Since `meta["uuid"]`
is a `dict`, evaluating `meta["uuid"] in chart_ids` raises `TypeError: unhashable type:
'dict'` before the body runs, crashing the import flow even though earlier guards were
intended to make the code more defensive.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/dashboard/importers/v1/utils.py
**Line:** 153:154
**Comment:**
*Type Error: This membership check can raise `TypeError` when `meta["uuid"]` is an unhashable corrupt value (for example, a dict/list), so the "defensive" path still crashes on malformed exports. Add a type/hashability check before probing `chart_ids`.
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40575 +/- ##
=======================================
Coverage 64.44% 64.44%
=======================================
Files 2655 2655
Lines 145473 145493 +20
Branches 33575 33583 +8
=======================================
+ Hits 93747 93761 +14
- Misses 50027 50029 +2
- Partials 1699 1703 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This needs a rebase on current Couple of things while you're in here: the title says "export" but everything's in Mostly trying to understand where these malformed |
Guard against KeyError when position entries have missing or corrupt 'type', 'meta', or 'chartId' keys in build_uuid_to_id_map() and the update_id_refs() position loop. Also add id_map membership checks in timed_refresh_immune_slices and expanded_slices to drop stale IDs. Closes #3 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
1fc969f to
101b04e
Compare
Code Review Agent Run #a085c6Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Would love updates on the various review comments (bot and human) if you want to move this forward. Thanks in advance :) |
Guard against KeyError when position entries have missing or corrupt 'type', 'meta', or 'chartId' keys in build_uuid_to_id_map() and the update_id_refs() position loop. Also add id_map membership checks in timed_refresh_immune_slices and expanded_slices to drop stale IDs.
Closes #3
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION