You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@superset.apache.org by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org> on 2026/10/14 18:42:54 UTC

[PR] fix(dashboard): resolve legacy chart id refs instead of failing the import [superset]

SEPURI-SAI-KRISHNA opened a new pull request, #45301:
URL: https://github.com/apache/superset/pull/45301

   ### SUMMARY
   
   `update_id_refs` remaps old chart IDs to new ones when a dashboard is imported. Four metadata fields carry chart IDs. `filter_scopes` and `default_filters` skip an ID that is not in the map, because #17741 added that check for stale references. `timed_refresh_immune_slices` and `expanded_slices` index the map directly, so the import fails with "Import dashboard failed for an unknown reason" in three cases:
   
   | metadata | result on master |
   | --- | --- |
   | `timed_refresh_immune_slices: [999]`, chart 999 not in the bundle | `KeyError: 999` |
   | `timed_refresh_immune_slices: ["101"]`, chart 101 **in** the bundle | `KeyError: '101'` |
   | `expanded_slices: {"abc": true}` | `ValueError: invalid literal for int()` |
   
   The second row is a live reference, not a stale one. These fields can hold string-typed IDs: `superset/commands/dashboard/export.py` coerces them with `int()` on the way out, and the MCP `remove_chart_from_dashboard` tool has a test named "timed_refresh_immune_slices entries may be string-typed IDs". The import side did no coercion, so a string entry missed the integer-keyed map.
   
   Unresolvable IDs are routine. Nothing prunes these two fields when a chart is deleted or leaves a dashboard: `DeleteChartCommand` does not touch dashboard metadata, and the only pruning code in the tree is the MCP tool above. An incomplete map is routine too: a chart in the layout is skipped at import when its dataset is not resolvable in the bundle, `/api/v1/assets/import/` lets a dashboard arrive without every chart yaml it references, and bundles exported before #40588 were never pruned on the way out.
   
   This adds `_remap_legacy_chart_id`, which coerces like the exporter does and returns `None` when the ID cannot be resolved, so a live string reference remaps and an unresolvable one is dropped instead of failing the import. `filter_scopes` and `default_filters` keep their existing inline checks; they already behave correctly for these inputs.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   
   Not applicable.
   
   ### TESTING INSTRUCTIONS
   
   Three tests next to #17741's own test in `tests/unit_tests/dashboards/commands/importers/v1/utils_test.py`, one per row of the table above. All three fail on master.
   
   ```
   pytest tests/unit_tests/dashboards/commands/importers/v1/utils_test.py
   ```
   
   Manually: export a dashboard that has a refresh-immune or expanded chart, delete that chart's dataset yaml from the bundle, and import it. On master the import fails with "Import dashboard failed for an unknown reason". With this change the dashboard imports and the dangling reference is gone.
   
   ### ADDITIONAL INFORMATION
   
   - [x] Has associated issue: #45300
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dashboard): resolve legacy chart id refs instead of failing the import [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.

codeant-ai-for-open-source[bot] commented on code in PR #45301:
URL: https://github.com/apache/superset/pull/45301#discussion_r4278411285


##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -183,6 +186,23 @@ def update_id_refs(  # pylint: disable=too-many-locals  # noqa: C901
     return fixed
 
 
+def _remap_legacy_chart_id(old_id: Any, id_map: dict[int, int]) -> int | None:
+    """Map one legacy integer chart ID to its new ID, or ``None``.
+
+    The integer-keyed metadata fields can hold a string rather than an int, and
+    can name a chart that is not in the bundle: nothing prunes them when a
+    chart is deleted or leaves a dashboard, and a chart in the layout is
+    skipped at import when its dataset is unresolvable. Returning ``None`` lets
+    the caller drop an unresolvable reference rather than fail the whole
+    import. The exporter coerces the same fields the same way; see
+    ``superset.commands.dashboard.export._stabilize_chart_ids``.
+    """
+    try:
+        return id_map.get(int(old_id))

Review Comment:
   ✅ **Customized review instruction saved!**
   
   **Instruction:**
   > Keep chart-ID coercion consistent between dashboard import and export; do not flag importer coercion in isolation when it mirrors the exporter.
   
   **Applied to:**
     - `superset/commands/dashboard/importers/v1/utils.py`
   
   ---
   💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dashboard): resolve legacy chart id refs instead of failing the import [superset]

Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.

SEPURI-SAI-KRISHNA commented on code in PR #45301:
URL: https://github.com/apache/superset/pull/45301#discussion_r4278387343


##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -110,8 +112,9 @@ def update_id_refs(  # pylint: disable=too-many-locals  # noqa: C901
 
     if "expanded_slices" in metadata:
         metadata["expanded_slices"] = {
-            str(id_map[int(old_id)]): value
+            str(new_id): value
             for old_id, value in metadata["expanded_slices"].items()
+            if (new_id := _remap_legacy_chart_id(old_id, id_map)) is not None
         }
 
     if "default_filters" in metadata:

Review Comment:
   Done. Both keys go through `_remap_legacy_chart_id` now, and so do the `immune` lists inside `filter_scopes`.
   
   The `immune` half was not crashing, but it was the last bare subscript left in the function, and it dropped a resolvable string ID rather than remapping it:
   
   ```
   immune: ["102", 999]    before -> []     both dropped
                            after -> [2]    "102" resolved, 999 dropped
   ```
   
   The exporter already routes all three of these through its own `remap_id`, so this is the import side catching up rather than new behaviour. For what it is worth the frontend types `immune` three different ways, `string[]` in `serializeFilterScopes.ts`, `number[]` in `FilterScopeSelector.tsx` and `number | string` in `getFilterScopeFromNodesTree.ts`, so a string is permitted there, but I could not point at a producer so I would not claim it as a live shape.
   
   `test_update_id_refs_filter_scopes_and_default_filters_unresolvable` covers the three key kinds and the `immune` remap. It fails on the previous head with `ValueError: invalid literal for int() with base 10: 'abc'`.
   



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dashboard): resolve legacy chart id refs instead of failing the import [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.

codeant-ai-for-open-source[bot] commented on code in PR #45301:
URL: https://github.com/apache/superset/pull/45301#discussion_r4274918442


##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -183,6 +186,23 @@ def update_id_refs(  # pylint: disable=too-many-locals  # noqa: C901
     return fixed
 
 
+def _remap_legacy_chart_id(old_id: Any, id_map: dict[int, int]) -> int | None:
+    """Map one legacy integer chart ID to its new ID, or ``None``.
+
+    The integer-keyed metadata fields can hold a string rather than an int, and
+    can name a chart that is not in the bundle: nothing prunes them when a
+    chart is deleted or leaves a dashboard, and a chart in the layout is
+    skipped at import when its dataset is unresolvable. Returning ``None`` lets
+    the caller drop an unresolvable reference rather than fail the whole
+    import. The exporter coerces the same fields the same way; see
+    ``superset.commands.dashboard.export._stabilize_chart_ids``.
+    """
+    try:
+        return id_map.get(int(old_id))

Review Comment:
   **Suggestion:** Import metadata is only shallowly validated, so `int(old_id)` truncates `101.7` or maps `True` to `1`, silently applying state to a different chart.
   
   **Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Type error`
   
   [![Use CodeAnt Skill](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/use-codeant-skill-flat-v2.svg)](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [![Fix in Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=50e52b020dab4571988fae8aee359c66&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [![Fix in VSCode Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=50e52b020dab4571988fae8aee359c66&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   <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:** 201:201
   **Comment:**
   	*Type Error: Import metadata is only shallowly validated, so `int(old_id)` truncates `101.7` or maps `True` to `1`, silently applying state to a different chart.
   
   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%2F45301&comment_hash=dad9c615b87fb0f7a4149d666beb023946adab80d6325b8853c82ba8c5e90be5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45301&comment_hash=dad9c615b87fb0f7a4149d666beb023946adab80d6325b8853c82ba8c5e90be5&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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dashboard): resolve legacy chart id refs instead of failing the import [superset]

Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.

SEPURI-SAI-KRISHNA commented on code in PR #45301:
URL: https://github.com/apache/superset/pull/45301#discussion_r4278409013


##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -183,6 +186,23 @@ def update_id_refs(  # pylint: disable=too-many-locals  # noqa: C901
     return fixed
 
 
+def _remap_legacy_chart_id(old_id: Any, id_map: dict[int, int]) -> int | None:
+    """Map one legacy integer chart ID to its new ID, or ``None``.
+
+    The integer-keyed metadata fields can hold a string rather than an int, and
+    can name a chart that is not in the bundle: nothing prunes them when a
+    chart is deleted or leaves a dashboard, and a chart in the layout is
+    skipped at import when its dataset is unresolvable. Returning ``None`` lets
+    the caller drop an unresolvable reference rather than fail the whole
+    import. The exporter coerces the same fields the same way; see
+    ``superset.commands.dashboard.export._stabilize_chart_ids``.
+    """
+    try:
+        return id_map.get(int(old_id))

Review Comment:
   Leaving this one as it is, deliberately.
   
   `True` is unchanged by this PR. `True == 1` as a dict key, so master's `id_map[old_id]` already resolves it against chart 1 wherever the map holds 1, and `int(True)` does the same thing.
   
   `101.7` is the only real difference: master raises and fails the whole import, this maps it to 101. Neither answer is right for a chart ID that is already nonsense, and the export side does the identical bare `int()` in `_stabilize_chart_ids.remap_id`, which is what keeps the two directions agreeing on these fields. Tightening only the importer would put them out of step on the same metadata. If float chart IDs are worth rejecting outright, both sides should reject them, and that is a separate change from stopping the opaque import failure.
   



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dashboard): resolve legacy chart id refs instead of failing the import [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.

codecov[bot] commented on PR #45301:
URL: https://github.com/apache/superset/pull/45301#issuecomment-6176128601

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45301?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :x: Patch coverage is `60.00000%` with `2 lines` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 64.95%. Comparing base ([`b411454`](https://app.codecov.io/gh/apache/superset/commit/b411454508c49ba4780047582ab61ccababa856c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f737a1d`](https://app.codecov.io/gh/apache/superset/commit/f737a1d6b8f0e66f63aaf9d4b27c467a6820ea02?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/45301?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [superset/commands/dashboard/importers/v1/utils.py](https://app.codecov.io/gh/apache/superset/pull/45301?src=pr&el=tree&filepath=superset%2Fcommands%2Fdashboard%2Fimporters%2Fv1%2Futils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGFzaGJvYXJkL2ltcG9ydGVycy92MS91dGlscy5weQ==) | 60.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/45301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #45301       +/-   ##
   ===========================================
   - Coverage   83.33%   64.95%   -18.39%     
   ===========================================
     Files        3040     3040               
     Lines      198611   198616        +5     
     Branches    46346    46346               
   ===========================================
   - Hits       165511   129007    -36504     
   - Misses      29876    66700    +36824     
   + Partials     3224     2909      -315     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/45301/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/45301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.98% <20.00%> (-0.01%)` | :arrow_down: |
   | [mysql](https://app.codecov.io/gh/apache/superset/pull/45301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [postgres](https://app.codecov.io/gh/apache/superset/pull/45301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.00% <60.00%> (+<0.01%)` | :arrow_up: |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/45301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.74% <20.00%> (-0.01%)` | :arrow_down: |
   | [python](https://app.codecov.io/gh/apache/superset/pull/45301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.08% <60.00%> (-33.85%)` | :arrow_down: |
   | [sqlite](https://app.codecov.io/gh/apache/superset/pull/45301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [unit](https://app.codecov.io/gh/apache/superset/pull/45301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45301?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dashboard): resolve legacy chart id refs instead of failing the import [superset]

Posted by "EnxDev (via GitHub)" <gi...@apache.org>.

EnxDev merged PR #45301:
URL: https://github.com/apache/superset/pull/45301


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dashboard): resolve legacy chart id refs instead of failing the import [superset]

Posted by "SEPURI-SAI-KRISHNA (via GitHub)" <gi...@apache.org>.

SEPURI-SAI-KRISHNA commented on code in PR #45301:
URL: https://github.com/apache/superset/pull/45301#discussion_r4278392396


##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -183,6 +186,23 @@ def update_id_refs(  # pylint: disable=too-many-locals  # noqa: C901
     return fixed
 
 
+def _remap_legacy_chart_id(old_id: Any, id_map: dict[int, int]) -> int | None:
+    """Map one legacy integer chart ID to its new ID, or ``None``.
+
+    The integer-keyed metadata fields can hold a string rather than an int, and
+    can name a chart that is not in the bundle: nothing prunes them when a
+    chart is deleted or leaves a dashboard, and a chart in the layout is
+    skipped at import when its dataset is unresolvable. Returning ``None`` lets
+    the caller drop an unresolvable reference rather than fail the whole
+    import. The exporter coerces the same fields the same way; see
+    ``superset.commands.dashboard.export._stabilize_chart_ids``.
+    """
+    try:
+        return id_map.get(int(old_id))

Review Comment:
   Added, and you read the coverage right. `"abc": True` is in the `expanded_slices` input now. The two lines codecov flagged are the `except` arm and its `return None`; traced them to check they are actually reached rather than assuming:
   
   ```
   result: {'1': True}
   line 203: hits=1  |    except (TypeError, ValueError):
   line 204: hits=1  |        return None
   ```
   



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dashboard): resolve legacy chart id refs instead of failing the import [superset]

Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.

netlify[bot] commented on PR #45301:
URL: https://github.com/apache/superset/pull/45301#issuecomment-6182991595

   ### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
   
   
   |  Name | Link |
   |:-:|------------------------|
   |<span aria-hidden="true">🔨</span> Latest commit | 557836d95c271cb28707d48c0eff980b45ed0690 |
   |<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ad0391d976bf1ad50336fa5 |
   |<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-45301--superset-docs-preview.netlify.app](https://deploy-preview-45301--superset-docs-preview.netlify.app) |
   |<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br />![QR Code](https://app.netlify.com/qr-code/eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTQ1MzAxLS1zdXBlcnNldC1kb2NzLXByZXZpZXcubmV0bGlmeS5hcHAifQ.Ht3EHcHfcqTujiMh0oxqsd0LQa2di8rkUIuBoY11RP8)<br /><br />_Use your smartphone camera to open QR code link._</details> |
   |<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctBCoAgEEDRu8w6yRANu0HHGB1FQVPURRHdPXdv8f8LtZVcBxwAC5iGlw3TPt4rYQ-mYCMWcy1tsD4wOWYDTkfqsydXU3lOmodC4kJvpHdl_IYkuRDKo4TvBw) |
   ---
   <!-- [superset-docs-preview Preview](https://deploy-preview-45301--superset-docs-preview.netlify.app) -->
   _To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dashboard): resolve legacy chart id refs instead of failing the import [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.

codeant-ai-for-open-source[bot] commented on PR #45301:
URL: https://github.com/apache/superset/pull/45301#issuecomment-6175851728

   ## CodeAnt PR Risk: Medium Risk
   
   - The PR needs attention before merging because malformed metadata IDs can be coerced to a different chart ID.
   - In superset/commands/dashboard/importers/v1/utils.py, int(101.7) becomes 101 and int(True) becomes 1, potentially applying dashboard state to the wrong chart.
   
   **Assessed commit:** `f737a1d6b8f0`


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dashboard): resolve legacy chart id refs instead of failing the import [superset]

Posted by "EnxDev (via GitHub)" <gi...@apache.org>.

EnxDev commented on code in PR #45301:
URL: https://github.com/apache/superset/pull/45301#discussion_r4276221291


##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -110,8 +112,9 @@ def update_id_refs(  # pylint: disable=too-many-locals  # noqa: C901
 
     if "expanded_slices" in metadata:
         metadata["expanded_slices"] = {
-            str(id_map[int(old_id)]): value
+            str(new_id): value
             for old_id, value in metadata["expanded_slices"].items()
+            if (new_id := _remap_legacy_chart_id(old_id, id_map)) is not None
         }
 
     if "default_filters" in metadata:

Review Comment:
   Not blocking, but a non-numeric key like `{"abc": ...}` still throws `ValueError` here and in `filter_scopes` above, since both do a bare `int(old_id)` before the membership check. That's the same failure as row 3 in your table.
   
   Since the helper exists now, would you be up for routing these two through `_remap_legacy_chart_id` too? Fine to leave for a follow-up if you'd rather keep this one narrow.



##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -183,6 +186,23 @@ def update_id_refs(  # pylint: disable=too-many-locals  # noqa: C901
     return fixed
 
 
+def _remap_legacy_chart_id(old_id: Any, id_map: dict[int, int]) -> int | None:
+    """Map one legacy integer chart ID to its new ID, or ``None``.
+
+    The integer-keyed metadata fields can hold a string rather than an int, and
+    can name a chart that is not in the bundle: nothing prunes them when a
+    chart is deleted or leaves a dashboard, and a chart in the layout is
+    skipped at import when its dataset is unresolvable. Returning ``None`` lets
+    the caller drop an unresolvable reference rather than fail the whole
+    import. The exporter coerces the same fields the same way; see
+    ``superset.commands.dashboard.export._stabilize_chart_ids``.
+    """
+    try:
+        return id_map.get(int(old_id))

Review Comment:
   Nothing in the new tests reaches this branch, which lines up with the 2 uncovered lines codecov reports. The description says there's one test per table row, but the `{"abc": true}` row isn't covered.
   
   Adding an `"abc": True` entry to the input in `test_update_id_refs_expanded_slices_missing` would cover it.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org