fix(dashboard-import): dashboard import fails with keyerror when referencing deleted charts in expanded_slices metadata#37042
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #0be174Actionable Suggestions - 0Additional Suggestions - 1
Review 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 |
Nitpicks 🔍
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
CodeAnt AI finished reviewing your PR. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #37042 +/- ##
===========================================
+ Coverage 0 66.52% +66.52%
===========================================
Files 0 643 +643
Lines 0 49107 +49107
Branches 0 5520 +5520
===========================================
+ Hits 0 32669 +32669
- Misses 0 15142 +15142
- Partials 0 1296 +1296
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
EnxDev
added
the
🎪 ⚡ showtime-trigger-start
github-actions
Bot
added
🎪 efa5b45 🚦 building
🎪 ⌛ 48h
|
🎪 Showtime deployed environment on GHA for efa5b45 • Environment: http://35.167.230.94:8080 (admin/admin) |
ramiroaquinoromero
force-pushed
the
ramiro.aquino.romero/sc-93629/dashboard-import-fails-with-keyerror-when
branch
from
efa5b45 to
4aaf14b
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI Incremental review completed. |
Code Review Agent Run #381a5eActionable Suggestions - 0Additional Suggestions - 1
Review 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 |
|
@ramiroaquinoromero would it be possible to add some before-and-after photos or videos? |
EnxDev
added
the
🎪 ⚡ showtime-trigger-start
|
@EnxDev Good morning. Sure. I will add a video with this behavior. Thank you. |
There was a problem hiding this comment.
Nice test coverage!!!!
I have some concerns, that would be good to address.
Also probably the commit msg and title of the PR are not following the conventions.
Something like:
fix(api): import fails with keyerror when referencing deleted charts in expanded_slices metadata
might work.
| dashboards = ( | ||
| db.session.query(Dashboard) | ||
| .filter(Dashboard.slices.any(id=chart_id)) # type: ignore[attr-defined] | ||
| .all() | ||
| ) |
There was a problem hiding this comment.
[Personal concern, since I don't have context]
How many records are we expecting to load here? I think we should investigate that, since it could break the server, also not sure if we should be safe by adding a limit.
Make sure we don't have a status column that marks records as deleted, because that could affect the number of records returned here.
There was a problem hiding this comment.
Good morning @reynoldmorel, you're right. I've optimized this:
Performance: Changed from loading full Dashboard objects to just (id, json_metadata) tuples. Added a safety limit of 1000 dashboards with logging if exceeded.
Soft-delete: Confirmed there's no soft-delete column on Dashboard - only published boolean, so the query won't return deleted records.
The limit of 1000 is somewhat arbitrary - happy to adjust based on what makes sense for your production usage. The error log will make it clear if we ever hit that edge case.
| if "expanded_slices" in metadata: | ||
| chart_id_str = str(chart_id) | ||
| if chart_id_str in metadata["expanded_slices"]: | ||
| del metadata["expanded_slices"][chart_id_str] | ||
| modified = True | ||
| logger.info( | ||
| "Removed chart %s from expanded_slices in dashboard %s", | ||
| chart_id, | ||
| dashboard.id, | ||
| ) |
There was a problem hiding this comment.
Looks like this pattern is repeated a lot, can't we just put it behind a function and do something like:
(modified, updated_expanded_slices_metadata) = delete_metadata_by_chart("expanded_slices", metadata, chart_id) or modified
(modified, updated_timed_refresh_immune_slices_metadata) = delete_metadata_by_chart("timed_refresh_immune_slices", updated_expanded_slices_metadata, chart_id) or modified
For different patterns you can write a condition to check whether the property needs a unique pattern or not to get its value where chart_id might be located
Also I don't think you should be updating metadata and dashboard properties by object reference, this could lead to unpredictable behavior if this function keeps growing and could become harder to maintain.
I would return a new updated metadata reference in general from delete_metadata_by_chart, you could use copy.deepcopy or just build a new reference of metadata once, before start calling delete_metadata_by_chart and then you could modify the props directly to the reference instead of touching the existing one
| # Clean up dashboard metadata before deleting charts | ||
| for chart in self._models: | ||
| self._cleanup_dashboard_metadata(chart.id) | ||
| ChartDAO.delete(self._models) |
There was a problem hiding this comment.
[Personal concern, since I don't have context]
It doesn't feel really safe to do this since the I don't know how many models we could get here and also _cleanup_dashboard_metadata could be time consuming. I would suggest to investigate if this won't cause any performance issues, and if not, I would write a comment denoting why this solution works and what scenario will cover, so if at some point in the future it fails, devs would know what to consider to fix it.
| config = { | ||
| "position": { | ||
| "CHART1": { | ||
| "id": "CHART1", | ||
| "meta": {"chartId": 101, "uuid": "uuid1"}, | ||
| "type": "CHART", | ||
| }, | ||
| "CHART2": { | ||
| "id": "CHART2", | ||
| "meta": {"chartId": 102, "uuid": "uuid2"}, | ||
| "type": "CHART", | ||
| }, | ||
| }, | ||
| "metadata": { | ||
| "timed_refresh_immune_slices": [ | ||
| 101, # This chart exists | ||
| 102, # This chart exists | ||
| 103, # This chart was deleted and doesn't exist | ||
| 104, # Another deleted chart | ||
| ], | ||
| }, | ||
| } |
There was a problem hiding this comment.
You could add a general function that builds a new config for you, so you can modify as you wish for each test
| chart_id, | ||
| dashboard.id, | ||
| ) | ||
|
|
There was a problem hiding this comment.
I see a lot of repeating logic for the if statements I think a util function will clean this up quite a bit
| scope_excluded = native_filter.get("scope", {}).get("excluded", []) | ||
| if chart_id in scope_excluded: | ||
| scope_excluded.remove(chart_id) |
There was a problem hiding this comment.
Suggestion: The code compares and removes chart_id as an int from lists that may store IDs as strings (and vice versa), so removals can silently fail (logic bug). Normalize comparisons by checking and removing both the integer and string representations to ensure the chart ID is removed regardless of stored type. [logic error]
Severity Level: Critical 🚨
- ❌ Dashboard export/importers/v1 may fail on orphaned references.
- ❌ Native filter exclusion lists remain stale.
- ⚠️ Chart deletion metadata cleanup incomplete.| scope_excluded = native_filter.get("scope", {}).get("excluded", []) | |
| if chart_id in scope_excluded: | |
| scope_excluded.remove(chart_id) | |
| scope = native_filter.get("scope", {}) | |
| scope_excluded = scope.get("excluded", []) | |
| removed = False | |
| # remove both int and string representations if present | |
| if chart_id in scope_excluded: | |
| scope_excluded.remove(chart_id) | |
| removed = True | |
| chart_id_str = str(chart_id) | |
| if chart_id_str in scope_excluded: | |
| scope_excluded.remove(chart_id_str) | |
| removed = True | |
| if removed: |
Steps of Reproduction ✅
1. Create a dashboard whose json_metadata["native_filter_configuration"] contains an entry
where scope.excluded is a list of string IDs (e.g., ["123"]) rather than ints. The cleanup
loop is at superset/commands/chart/delete.py:177-183 in
DeleteChartCommand._cleanup_dashboard_metadata (function start at
superset/commands/chart/delete.py:78).
2. Invoke DeleteChartCommand.run with the chart id 123 present on that dashboard
(superset/commands/chart/delete.py:49). The run method calls _cleanup_dashboard_metadata
at superset/commands/chart/delete.py:52-54.
3. The current code checks `if chart_id in scope_excluded` at
superset/commands/chart/delete.py:180. Because scope_excluded contains string "123" and
chart_id is the integer 123, the test is False; no removal occurs and modified remains
False for that dashboard.
4. The string-ID exclusion remains in the dashboard metadata and is not cleaned. This
leaves an orphaned reference that can later trigger import-time KeyError in the importer
(see PR description for superset/commands/dashboard/importers/v1/utils.py). The behavior
is concrete and reproducible using string-stored IDs.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/chart/delete.py
**Line:** 180:182
**Comment:**
*Logic Error: The code compares and removes `chart_id` as an int from lists that may store IDs as strings (and vice versa), so removals can silently fail (logic bug). Normalize comparisons by checking and removing both the integer and string representations to ensure the chart ID is removed regardless of stored type.
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.
@ramiroaquinoromero If you have some time, would you be able to take care of this? |
Yup I am pushing some changes I will add some videos about this one. |
Code Review Agent Run #0fe9a7Actionable 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 |
ramiroaquinoromero
force-pushed
the
ramiro.aquino.romero/sc-93629/dashboard-import-fails-with-keyerror-when
branch
from
2b988e1 to
7ba0378
Compare
|
I cant reproduce this behavior again It was fixed on this PR #36551 . I am closing this ticket. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #eacd8a
Actionable Suggestions - 1
-
superset/commands/chart/delete.py - 1
- Error Handling Gap · Line 168-175
Review Details
-
Files reviewed - 3 · Commit Range:
1a098fb..7ba0378- superset/commands/chart/delete.py
- superset/commands/dashboard/importers/v1/utils.py
- tests/unit_tests/dashboards/commands/importers/v1/utils_test.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| # Clean up default_filters | ||
| if "default_filters" in metadata: | ||
| default_filters = json.loads(metadata["default_filters"]) | ||
| chart_id_str = str(chart_id) | ||
| if chart_id_str in default_filters: | ||
| del default_filters[chart_id_str] | ||
| metadata["default_filters"] = json.dumps(default_filters) | ||
| modified = True |
There was a problem hiding this comment.
The json.loads call for metadata["default_filters"] lacks error handling, which could raise an exception if the value is malformed. It looks like this should be wrapped in a try-except block similar to the main metadata parsing, to log a warning and continue instead of failing the entire chart deletion.
Code suggestion
Check the AI-generated fix before applying
| # Clean up default_filters | |
| if "default_filters" in metadata: | |
| default_filters = json.loads(metadata["default_filters"]) | |
| chart_id_str = str(chart_id) | |
| if chart_id_str in default_filters: | |
| del default_filters[chart_id_str] | |
| metadata["default_filters"] = json.dumps(default_filters) | |
| modified = True | |
| # Clean up default_filters | |
| if "default_filters" in metadata: | |
| try: | |
| default_filters = json.loads(metadata["default_filters"]) | |
| chart_id_str = str(chart_id) | |
| if chart_id_str in default_filters: | |
| del default_filters[chart_id_str] | |
| metadata["default_filters"] = json.dumps(default_filters) | |
| modified = True | |
| except (json.JSONDecodeError, TypeError): | |
| logger.warning("Could not parse default_filters for dashboard %s", dashboard_id) |
Code Review Run #eacd8a
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
5 participants
SUMMARY
This PR fixes a critical bug where dashboard imports fail with a
KeyErrorwhen the exported dashboard contains references to deleted charts in its metadata.Problem:
When a chart is deleted from Superset, the dashboard metadata (specifically
expanded_slices,timed_refresh_immune_slices, and other metadata fields) retains references to the deleted chart IDs. When these dashboards are exported and then imported into another workspace, the import process crashes with aKeyErrorand displays a generic error message: "Import dashboard failed for an unknown reason."Root Cause:
The
update_id_refs()function insuperset/commands/dashboard/importers/v1/utils.pyattempted to map all chart IDs from the metadata without checking if the charts exist in the import, causing aKeyErrorwhen encountering deleted chart references.Solution:
Modified
update_id_refs()to gracefully skip missing chart references inexpanded_slicesandtimed_refresh_immune_slicesmetadata fields, consistent with how other metadata fields (filter_scopes,default_filters,native_filter_configuration) already handle missing charts.Added
_cleanup_dashboard_metadata()method toDeleteChartCommandthat automatically cleans up all dashboard metadata when a chart is deleted, preventing orphaned references from being created in the first place.Changes:
Modified
superset/commands/dashboard/importers/v1/utils.py:if int(old_id) in id_mapcheck toexpanded_slicesprocessing (line 105)if old_id in id_mapcheck totimed_refresh_immune_slicesprocessing (line 80)Modified
superset/commands/chart/delete.py:_cleanup_dashboard_metadata()method to remove chart references from dashboard metadata on deletionexpanded_slices,timed_refresh_immune_slices,filter_scopes,default_filters,native_filter_configuration,chart_configuration, andglobal_chart_configurationAdded comprehensive unit tests in
tests/unit_tests/dashboards/commands/importers/v1/utils_test.py:test_update_id_refs_expanded_slices_with_missing_chart()test_update_id_refs_timed_refresh_immune_slices_with_missing_chart()test_update_id_refs_multiple_missing_chart_references()BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE:
AFTER:
ADDITIONAL INFORMATION