fix: user UUIDs on export for Native Filter Configuration #18562
Conversation
hughhhh
force-pushed
the
hugh-save-import
branch
from
a22f3c7
to
eea0fc1
Compare
Codecov Report
@@ Coverage Diff @@
## master #18562 +/- ##
=======================================
Coverage 66.29% 66.29%
=======================================
Files 1594 1594
Lines 62623 62626 +3
Branches 6312 6312
=======================================
+ Hits 41518 41521 +3
Misses 19456 19456
Partials 1649 1649
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
hughhhh
force-pushed
the
hugh-save-import
branch
from
ce56c68
to
e50d6c5
Compare
There was a problem hiding this comment.
@hughhhh I think instead of changing the export, it's better to simply fix the import to update excluded.
if you change the export so that native_filter["scope"]["excluded"] contains UUIDs, not IDs, you need to add logic to the import so it can handle both cases (UUIDs vs. IDs in exluded), in order to maintain backward compatibility.
During dashboard import we already compute a map between the old IDs and the new ones:
superset/superset/dashboards/commands/importers/v1/utils.py
Lines 68 to 69 in 2633bcc
That function, update_id_refs, is already responsible for updating the dashboard metadata to use the new IDs after an import. I think the best solution here is to modify it to also update native_filter["scope"]["excluded"].
| @@ -17,7 +17,7 @@ | |||
|
|
|||
| import json | |||
| import logging | |||
| from typing import Any, Dict, Set | |||
| from typing import Any, Callable, Dict, Set, Union | |||
There was a problem hiding this comment.
These new imports are not needed, right?
| @@ -49,8 +49,10 @@ def test_update_id_refs_immune_missing( # pylint: disable=invalid-name | |||
| "101": {"filter_name": {"immune": [102, 103]}}, | |||
| "104": {"filter_name": {"immune": [102, 103]}}, | |||
| }, | |||
| "native_filter_configuration": [ | |||
| {"scope": {"excluded": ["uuid1", "uuid2"]}} | |||
There was a problem hiding this comment.
These should be IDs, not UUIDs.
Also, can we have a new test to check the logic, instead of piggybacking on test_update_id_refs_immune_missing?
There was a problem hiding this comment.
rookie mistake this is on import i will update accordingly 😅
tests/unit_tests/dashboards/commands/importers/v1/utils_test.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
mistercrunch
added
🏷️ bot
SUMMARY
Updating the current process of exporting dashboard native filter configurations to store uuids vs chartids. The problem is when users export charts from 1 superset instance (workspace) to another the chart id reference aren't the same. So fix that we are allowing the native filters to leverage uuids from the charts and then updating them once they are properly loaded into the workspace/instance.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION