rusackas commented on code in PR #40140:
URL: https://github.com/apache/superset/pull/40140#discussion_r3245498333
##########
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:
The standard `ExportDashboardsCommand`
(`superset/commands/dashboard/export.py`) leaves `chartsInScope` as integer
chart IDs — it does not remap to UUIDs. The UUID conversion you may be thinking
of lives only in `export_example.py` (the example-dashboard exporter), which is
a separate code path used by the seed-examples script, not the production
export. The bug filed in #26338 is precisely that the imported dashboard ends
up with stale source-env integer IDs in `chartsInScope` because no remap exists
in `update_id_refs`. Integer IDs in this fixture match the real exported
payload shape; using UUIDs here would test the wrong code path.
##########
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:
Same reasoning as the sibling thread on `chartsInScope`: the standard
`chart_configuration` payload uses string-of-int keys and integer IDs (see
existing tests in this file at line 126:
`test_update_id_refs_cross_filter_chart_configuration_key_and_excluded_mapping`
uses the same shape). UUIDs would be testing the example-exporter path, which
is not what #26338 reports.
##########
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],
+ },
+ },
+ },
+ },
+ }
+ chart_ids = {"uuid1": 1, "uuid2": 2}
+ dataset_info: dict[str, dict[str, Any]] = {}
+
+ fixed = update_id_refs(config, chart_ids, dataset_info)
+ cross_filters =
fixed["metadata"]["chart_configuration"]["1"]["crossFilters"]
+
+ assert cross_filters["chartsInScope"] == [1, 2]
Review Comment:
Correct — that's exactly what this test is designed to surface. The PR is a
TDD-style validation: failing CI here means the bug is real and a fix PR is
needed (likely a remap pass for `chartsInScope` in `update_id_refs`, mirroring
the existing `scope.excluded` handling). No code change needed in this
test-only PR.
--
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]
