Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[dashboard] use filter_scopes metadata when import old dashboard #9145

Merged
merged 2 commits into from
Feb 19, 2020
Merged

[dashboard] use filter_scopes metadata when import old dashboard #9145

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ccc8dc
[dashboard] update filter_scopes metadata when import old dashboard
Feb 14, 2020
d428247
fix review comments
Feb 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 30 additions & 22 deletions superset/models/dashboard.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
from superset.models.tags import DashboardUpdater
from superset.models.user_attributes import UserAttribute
from superset.utils import core as utils
from superset.utils.dashboard_filter_scopes_converter import convert_filter_scopes

if TYPE_CHECKING:
# pylint: disable=unused-import
Expand Down Expand Up @@ -286,11 +287,11 @@ def alter_positions(dashboard, old_to_new_slc_id_dict):
# copy slices object as Slice.import_slice will mutate the slice
# and will remove the existing dashboard - slice association
slices = copy(dashboard_to_import.slices)
old_json_metadata = json.loads(dashboard_to_import.json_metadata or "{}")
old_to_new_slc_id_dict = {}
new_filter_immune_slices = []
new_filter_immune_slice_fields = {}
new_timed_refresh_immune_slices = []
new_expanded_slices = {}
new_filter_scopes = {}
i_params_dict = dashboard_to_import.params_dict
remote_id_slice_map = {
slc.params_dict["remote_id"]: slc
Expand All @@ -309,18 +310,6 @@ def alter_positions(dashboard, old_to_new_slc_id_dict):
# update json metadata that deals with slice ids
new_slc_id_str = "{}".format(new_slc_id)
old_slc_id_str = "{}".format(slc.id)
if (
"filter_immune_slices" in i_params_dict
and old_slc_id_str in i_params_dict["filter_immune_slices"]
):
new_filter_immune_slices.append(new_slc_id_str)
if (
"filter_immune_slice_fields" in i_params_dict
and old_slc_id_str in i_params_dict["filter_immune_slice_fields"]
):
new_filter_immune_slice_fields[new_slc_id_str] = i_params_dict[
"filter_immune_slice_fields"
][old_slc_id_str]
if (
"timed_refresh_immune_slices" in i_params_dict
and old_slc_id_str in i_params_dict["timed_refresh_immune_slices"]
Expand All @@ -334,6 +323,29 @@ def alter_positions(dashboard, old_to_new_slc_id_dict):
old_slc_id_str
]

# since PR #9109, filter_immune_slices and filter_immune_slice_fields
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way we can avoid having this special logic? I'm worried about code in Superset getting (even more) bloated. When will create_from_import have the extra fields? Will it always? Is it ok to just not support this case and warn that your change is not backwards compatible?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export dashboard and import dashboard is a widely used feature in Superset. it helps ppl to move dashboard from different data system (and both of them use Superset). If someone wants to copy a dashboard from an older system which didn't upgrade recently, but the other system used updated version, this PR can make their export - import routine keep working.

if the backward compatible only introduce a couple of lines, I would like to keep it. But if it will introduce a lot of overhead, i will not maintain it.

# are converted to filter_scopes
# but dashboard create from import may still have old dashboard filter metadata
# here we convert them to new filter_scopes metadata first
filter_scopes: Dict = {}
if (
"filter_immune_slices" in i_params_dict
or "filter_immune_slice_fields" in i_params_dict
):
filter_scopes = convert_filter_scopes(old_json_metadata, slices)

if "filter_scopes" in i_params_dict:
filter_scopes = old_json_metadata.get("filter_scopes")

# then replace old slice id to new slice id:
for (slice_id, scopes) in filter_scopes.items():
new_filter_key = old_to_new_slc_id_dict[int(slice_id)]
new_filter_scopes[str(new_filter_key)] = scopes
for scope in scopes.values():
scope["immune"] = [
old_to_new_slc_id_dict[slice_id] for slice_id in scope.get("immune")
]

# override the dashboard
existing_dashboard = None
for dash in session.query(Dashboard).all():
Expand All @@ -351,16 +363,12 @@ def alter_positions(dashboard, old_to_new_slc_id_dict):
if dashboard_to_import.position_json:
alter_positions(dashboard_to_import, old_to_new_slc_id_dict)
dashboard_to_import.alter_params(import_time=import_time)
dashboard_to_import.remove_params("filter_immune_slices")
dashboard_to_import.remove_params("filter_immune_slice_fields")
if new_filter_scopes:
dashboard_to_import.alter_params(filter_scopes=new_filter_scopes)
if new_expanded_slices:
dashboard_to_import.alter_params(expanded_slices=new_expanded_slices)
if new_filter_immune_slices:
dashboard_to_import.alter_params(
filter_immune_slices=new_filter_immune_slices
)
if new_filter_immune_slice_fields:
dashboard_to_import.alter_params(
filter_immune_slice_fields=new_filter_immune_slice_fields
)
if new_timed_refresh_immune_slices:
dashboard_to_import.alter_params(
timed_refresh_immune_slices=new_timed_refresh_immune_slices
Expand Down
6 changes: 6 additions & 0 deletions superset/models/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,12 @@ def alter_params(self, **kwargs):
d.update(kwargs)
self.params = json.dumps(d)

def remove_params(self, param, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we pass in **kwargs to this function anywhere? If not, can remove the kwargs parts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, typing plz ❤️

d = self.params_dict
d.pop(param, None)
d.update(kwargs)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used kwargs here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, what I meant was - is this function ever called with keyword arguments? It looks like you only called it with the positional argument param_to_remove, so I don't thiink we need the kwargs logic.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. Thanks!

self.params = json.dumps(d)

def reset_ownership(self):
""" object will belong to the user the current user """
# make sure the object doesn't have relations to a user
Expand Down
3 changes: 2 additions & 1 deletion superset/views/dashboard/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,10 @@ class DashboardJSONMetadataSchema(Schema):
expanded_slices = fields.Dict()
refresh_frequency = fields.Integer()
default_filters = fields.Str()
filter_immune_slice_fields = fields.Dict()
stagger_refresh = fields.Boolean()
stagger_time = fields.Integer()
color_scheme = fields.Str()
label_colors = fields.Dict()


def validate_json(value):
Expand Down
13 changes: 11 additions & 2 deletions tests/import_export_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,11 +400,16 @@ def test_import_dashboard_2_slices(self):
dash_with_2_slices.json_metadata = json.dumps(
{
"remote_id": 10003,
"filter_immune_slices": ["{}".format(e_slc.id)],
"expanded_slices": {
"{}".format(e_slc.id): True,
"{}".format(b_slc.id): False,
},
# mocked filter_scope metadata
"filter_scopes": {
str(e_slc.id): {
"region": {"scope": ["ROOT_ID"], "immune": [b_slc.id]}
}
},
}
)

Expand All @@ -421,7 +426,11 @@ def test_import_dashboard_2_slices(self):
expected_json_metadata = {
"remote_id": 10003,
"import_time": 1991,
"filter_immune_slices": ["{}".format(i_e_slc.id)],
"filter_scopes": {
str(i_e_slc.id): {
"region": {"scope": ["ROOT_ID"], "immune": [i_b_slc.id]}
}
},
"expanded_slices": {
"{}".format(i_e_slc.id): True,
"{}".format(i_b_slc.id): False,
Expand Down