codeant-ai-for-open-source[bot] commented on code in PR #40140:
URL: https://github.com/apache/superset/pull/40140#discussion_r3245301186
##########
tests/unit_tests/dashboards/commands/importers/v1/utils_test.py:
##########
@@ -123,6 +123,99 @@ def test_update_native_filter_config_scope_excluded():
}
+def test_update_id_refs_remaps_charts_in_scope():
+ """
+ Regression for #26338: ``chartsInScope`` on a native filter holds chart
+ IDs and must be remapped from source-env IDs to destination-env IDs
+ during import.
+
+ The export side already converts ``chartsInScope`` IDs to UUIDs (see
+ ``export_example.py:325``). The import side must symmetrically convert
+ them back to the destination environment's chart IDs. Without that
+ remap, the field carries stale source IDs into the imported dashboard
+ and breaks ``filtersInScope`` / ``filtersOutScope`` computation —
+ filters end up applied to the wrong charts (or none at all).
+ """
+ from superset.commands.dashboard.importers.v1.utils import update_id_refs
+
+ config: dict[str, Any] = {
+ "position": {
+ "CHART1": {
+ "id": "CHART1",
+ "meta": {"chartId": 101, "uuid": "uuid1"},
+ "type": "CHART",
+ },
+ "CHART2": {
+ "id": "CHART2",
+ "meta": {"chartId": 102, "uuid": "uuid2"},
+ "type": "CHART",
+ },
+ },
+ "metadata": {
+ "native_filter_configuration": [
+ {
+ "id": "NATIVE_FILTER-region",
+ "scope": {"rootPath": ["ROOT_ID"], "excluded": []},
+ # chartsInScope contains source-env chart IDs.
+ "chartsInScope": [101, 102, 103],
Review Comment:
**Suggestion:** This regression test uses integer IDs in `chartsInScope`,
but the exporter writes `chartsInScope` as chart UUIDs. Because of that
contract mismatch, the test can pass even if UUID-based imports are still
broken. Build the fixture with UUID values (for example `["uuid1", "uuid2",
...]`) so it validates the real import path. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Native filter chartsInScope remapping bug remains undetected.
- ⚠️ Dashboard import tests misrepresent real native filter payload.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the export path for native filters in
`superset/commands/dashboard/export_example.py`, where
`remap_native_filters` is defined
at line 315 (from `grep` output) and called at lines 426–428 to build export
metadata;
inside that function (lines 26–30 in the snippet we read) `chartsInScope` is
rewritten
from numeric chart IDs to chart UUID strings via `chart_id_to_uuid.get(cid,
cid)`.
2. Inspect the import path in
`superset/commands/dashboard/importers/v1/__init__.py`,
lines 169–183, where `ImportDashboardsCommand._import` iterates dashboard
configs and
calls `update_id_refs(config, chart_ids, dataset_info)` before
`import_dashboard`; this
proves that `update_id_refs` operates directly on the exported dashboard
payload produced
by `export_example.py`.
3. Inspect `update_id_refs` in
`superset/commands/dashboard/importers/v1/utils.py`, lines
132–147, where native filter handling only remaps `scope["excluded"]` using
an `id_map` of
old integer IDs to new IDs, and does not touch `chartsInScope` at all—so any
UUID-valued
`chartsInScope` from export would pass through unchanged.
4. Inspect the regression test `test_update_id_refs_remaps_charts_in_scope`
in
`tests/unit_tests/dashboards/commands/importers/v1/utils_test.py`, lines
27–75
(corresponding to new hunk lines 126–175), where the fixture sets
`"chartsInScope": [101,
102, 103]` (line 160) as integer IDs; because the real export format uses
UUID strings in
`chartsInScope`, a future implementation of `update_id_refs` that
incorrectly assumes
integer IDs (e.g., remapping via `id_map` instead of `chart_ids` keyed by
UUID) would
still satisfy this test while failing to remap actual UUID-based payloads,
demonstrating
that the test currently uses the wrong data shape for the field it is meant
to guard.
```
</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%20tests%2Funit_tests%2Fdashboards%2Fcommands%2Fimporters%2Fv1%2Futils_test.py%0A%2A%2ALine%3A%2A%2A%20160%3A160%0A%2A%2AComment%3A%2A%2A%0A%09%2AApi%20Mismatch%3A%20This%20regression%20test%20uses%20integer%20IDs%20in%20%60chartsInScope%60%2C%20but%20the%20exporter%20writes%20%60chartsInScope%60%20as%20chart%20UUIDs.%20Because%20of%20that%20contract%20mismatch%2C%20the%20test%20can%20pass%20even%20if%20UUID-based%20imports%20are%20still%20broken.%20Build%20the%20fixture%20with%20UUID%20values%20%28for%20example%20%60%5B%22uuid1%22%2C%20%22uuid2%22%2C%20...%5D%60%29%20so%20it%20validates%20the%20real%20import%20path.%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%20tests%2Funit_tests%2Fdashboards%2Fcommands%2Fimporters%2Fv1%2Futils_test.py%0A%2A%2ALine%3A%2A%2A%20160%3A160%0A%2A%2AComment%3A%2A%2A%0A%09%2AApi%20Mismatch%3A%20This%20regression%20test%20uses%20integer%20IDs%20in%20%60chartsInScope%60%2C%20but%20the%20exporter%20writes%20%60chartsInScope%60%20as%20chart%20UUIDs.%20Because%20of%20that%20contract%20mismatch%2C%20the%20test%20can%20pass%20even%20if%20UUID-based%20imports%20are%20still%20broken.%20Build%20the%20fixture%20with%20UUID%20values%20%28f
or%20example%20%60%5B%22uuid1%22%2C%20%22uuid2%22%2C%20...%5D%60%29%20so%20it%20validates%20the%20real%20import%20path.%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:** tests/unit_tests/dashboards/commands/importers/v1/utils_test.py
**Line:** 160:160
**Comment:**
*Api Mismatch: This regression test uses integer IDs in
`chartsInScope`, but the exporter writes `chartsInScope` as chart UUIDs.
Because of that contract mismatch, the test can pass even if UUID-based imports
are still broken. Build the fixture with UUID values (for example `["uuid1",
"uuid2", ...]`) so it validates the real import path.
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%2F40140&comment_hash=d1704f0790b26efc98944b68ae3cf1880fee4e2d745ecb2069d9574283a825f2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40140&comment_hash=d1704f0790b26efc98944b68ae3cf1880fee4e2d745ecb2069d9574283a825f2&reaction=dislike'>👎</a>
##########
tests/unit_tests/dashboards/commands/importers/v1/utils_test.py:
##########
@@ -123,6 +123,99 @@ def test_update_native_filter_config_scope_excluded():
}
+def test_update_id_refs_remaps_charts_in_scope():
+ """
+ Regression for #26338: ``chartsInScope`` on a native filter holds chart
+ IDs and must be remapped from source-env IDs to destination-env IDs
+ during import.
+
+ The export side already converts ``chartsInScope`` IDs to UUIDs (see
+ ``export_example.py:325``). The import side must symmetrically convert
+ them back to the destination environment's chart IDs. Without that
+ remap, the field carries stale source IDs into the imported dashboard
+ and breaks ``filtersInScope`` / ``filtersOutScope`` computation —
+ filters end up applied to the wrong charts (or none at all).
+ """
+ from superset.commands.dashboard.importers.v1.utils import update_id_refs
+
+ config: dict[str, Any] = {
+ "position": {
+ "CHART1": {
+ "id": "CHART1",
+ "meta": {"chartId": 101, "uuid": "uuid1"},
+ "type": "CHART",
+ },
+ "CHART2": {
+ "id": "CHART2",
+ "meta": {"chartId": 102, "uuid": "uuid2"},
+ "type": "CHART",
+ },
+ },
+ "metadata": {
+ "native_filter_configuration": [
+ {
+ "id": "NATIVE_FILTER-region",
+ "scope": {"rootPath": ["ROOT_ID"], "excluded": []},
+ # chartsInScope contains source-env chart IDs.
+ "chartsInScope": [101, 102, 103],
+ }
+ ],
+ },
+ }
+ chart_ids = {"uuid1": 1, "uuid2": 2}
+ dataset_info: dict[str, dict[str, Any]] = {}
+
+ fixed = update_id_refs(config, chart_ids, dataset_info)
+ filter_config = fixed["metadata"]["native_filter_configuration"][0]
+
+ # Resolved IDs are remapped; unknown IDs (103) are dropped rather than
+ # left to silently bind to whatever chart owns that integer in the
+ # destination environment.
+ assert filter_config["chartsInScope"] == [1, 2]
+
+
+def test_update_id_refs_remaps_cross_filter_charts_in_scope():
+ """
+ Companion to test_update_id_refs_remaps_charts_in_scope. Cross-filter
+ config also stores ``chartsInScope`` (under ``crossFilters`` per chart)
+ and must be remapped on import for the same reason.
+ """
+ from superset.commands.dashboard.importers.v1.utils import update_id_refs
+
+ config: dict[str, Any] = {
+ "position": {
+ "CHART1": {
+ "id": "CHART1",
+ "meta": {"chartId": 101, "uuid": "uuid1"},
+ "type": "CHART",
+ },
+ "CHART2": {
+ "id": "CHART2",
+ "meta": {"chartId": 102, "uuid": "uuid2"},
+ "type": "CHART",
+ },
+ },
+ "metadata": {
+ "chart_configuration": {
+ "101": {
+ "id": 101,
+ "crossFilters": {
+ "scope": {"rootPath": ["ROOT_ID"], "excluded": []},
+ "chartsInScope": [101, 102, 103],
+ },
+ },
+ },
Review Comment:
**Suggestion:** The cross-filter regression test models
`chart_configuration` with numeric string keys and numeric `id`, but exported
dashboards use chart UUIDs for both. This means the test does not cover the
actual import contract and can give false confidence while UUID-keyed configs
still fail remapping. Use UUID keys/IDs in this fixture to match exported
payload shape. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Cross-filter chart_configuration remapping issues escape unit tests.
- ⚠️ Dashboard import may mis-handle UUID-keyed cross-filter config.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the export path for cross-filters in
`superset/commands/dashboard/export_example.py`, where
`remap_chart_configuration` is
defined at line 347 and called at lines 432–434; in that function (snippet
lines 52–75)
the exporter rewrites `chart_configuration` to use chart UUIDs as keys
(`remapped[chart_uuid] = new_config`) and sets `new_config["id"] =
chart_uuid`, and also
rewrites `crossFilters["chartsInScope"]` entries from integer IDs to UUID
strings.
2. Inspect the import path in
`superset/commands/dashboard/importers/v1/__init__.py`,
lines 169–183, where `ImportDashboardsCommand._import` calls
`update_id_refs(config,
chart_ids, dataset_info)` on each dashboard config produced by the v1 import
bundle,
establishing that `update_id_refs` is the importer's only place to normalize
`chart_configuration` back to integer IDs.
3. Inspect `update_cross_filter_scoping` in
`superset/commands/dashboard/importers/v1/utils.py`, lines 188–217, where it
iterates
`metadata["chart_configuration"].items()` expecting keys that can be parsed
as integers
(`old_id_int = int(old_id_str)`), uses `id_map[old_id_int]` to produce a new
integer ID,
sets `chart_config["id"] = new_id`, and writes the result under
`new_chart_configuration[str(new_id)]`; this shows the import logic is
written for numeric
string keys and numeric `id` fields, not UUID keys / IDs.
4. Inspect the regression test
`test_update_id_refs_remaps_cross_filter_charts_in_scope`
in `tests/unit_tests/dashboards/commands/importers/v1/utils_test.py`, lines
78–117
(corresponding to new hunk lines 177–216), where the fixture models
`"chart_configuration": { "101": { "id": 101, "crossFilters": { ...,
"chartsInScope":
[101, 102, 103] } } }` (lines around 199–205) and asserts that after
`update_id_refs` the
result lives under key `"1"` with `chartsInScope == [1, 2]`; because the
actual exported
`chart_configuration` uses UUID keys and sets `id` to a UUID string, this
test exercises
an outdated integer-keyed schema and can still pass even if `update_id_refs`
fails to
remap or handle UUID-keyed `chart_configuration`, leaving real cross-filter
import bugs
untested.
```
</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%20tests%2Funit_tests%2Fdashboards%2Fcommands%2Fimporters%2Fv1%2Futils_test.py%0A%2A%2ALine%3A%2A%2A%20199%3A207%0A%2A%2AComment%3A%2A%2A%0A%09%2AApi%20Mismatch%3A%20The%20cross-filter%20regression%20test%20models%20%60chart_configuration%60%20with%20numeric%20string%20keys%20and%20numeric%20%60id%60%2C%20but%20exported%20dashboards%20use%20chart%20UUIDs%20for%20both.%20This%20means%20the%20test%20does%20not%20cover%20the%20actual%20import%20contract%20and%20can%20give%20false%20confidence%20while%20UUID-keyed%20configs%20still%20fail%20remapping.%20Use%20UUID%20keys%2FIDs%20in%20this%20fixture%20to%20match%20exported%20payload%20shape.%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%20tests%2Funit_tests%2Fdashboards%2Fcommands%2Fimporters%2Fv1%2Futils_test.py%0A%2A%2ALine%3A%2A%2A%20199%3A207%0A%2A%2AComment%3A%2A%2A%0A%09%2AApi%20Mismatch%3A%20The%20cross-filter%20regression%20test%20models%20%60chart_configuration%60%20with%20numeric%20string%20keys%20and%20numeric%20%60id%60%2C%20but%20exported%20dashboards%20use%20chart%20UUIDs%20for%20both.%20This%20means%20the%20test%20does%20not%20cover%20the%20actual%20import%20contract%20and%20can%20give%20false%20c
onfidence%20while%20UUID-keyed%20configs%20still%20fail%20remapping.%20Use%20UUID%20keys%2FIDs%20in%20this%20fixture%20to%20match%20exported%20payload%20shape.%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:** tests/unit_tests/dashboards/commands/importers/v1/utils_test.py
**Line:** 199:207
**Comment:**
*Api Mismatch: The cross-filter regression test models
`chart_configuration` with numeric string keys and numeric `id`, but exported
dashboards use chart UUIDs for both. This means the test does not cover the
actual import contract and can give false confidence while UUID-keyed configs
still fail remapping. Use UUID keys/IDs in this fixture to match exported
payload shape.
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%2F40140&comment_hash=fa8c414253dc88ac2a29c988c120bdccaeb578dd256e6ad0819ff9c1b07d2c9f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40140&comment_hash=fa8c414253dc88ac2a29c988c120bdccaeb578dd256e6ad0819ff9c1b07d2c9f&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]
