EnxDev commented on code in PR #42472:
URL: https://github.com/apache/superset/pull/42472#discussion_r3658303364
##########
superset/commands/chart/importers/v1/utils.py:
##########
@@ -201,12 +203,29 @@ def import_chart(
if chart.id is None:
db.session.flush()
- if user:
- from superset.subjects.utils import get_user_subject
+ # Only newly created charts inherit the creator's editor/viewer defaults;
+ # re-importing over an existing chart (overwrite or soft-delete restore)
+ # must not silently grant the importer's groups access. Mirrors the
+ # dashboard importer's ``not existing`` guard.
+ if not existing and user:
+ from superset.subjects.utils import (
+ get_default_viewers_for_new_asset,
+ get_user_subject,
+ )
subj = get_user_subject(user.id)
if subj and subj not in chart.editors:
chart.editors.append(subj)
+ # Resolved once by bulk importers and passed in; recomputed here only
+ # for direct callers that omit it (one membership query per asset).
+ viewers = (
+ default_viewers
+ if default_viewers is not None
+ else get_default_viewers_for_new_asset(user.id)
+ )
+ for viewer in viewers:
+ if viewer not in chart.viewers:
+ chart.viewers.append(viewer)
Review Comment:
This doesn't apply here. viewers isn't in Slice.export_fields (nor
extra_import_fields), so Slice.import_from_dict never reads viewers from a v1
payload — a newly-created chart's viewers is always empty at this point, so
appending the creator's group defaults can't "expand" or override an explicit
list. A v1 chart bundle has no way to specify viewers.
##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -449,10 +451,23 @@ def import_dashboard( # noqa: C901
db.session.flush()
if not existing and user:
- from superset.subjects.utils import get_user_subject
+ from superset.subjects.utils import (
+ get_default_viewers_for_new_asset,
+ get_user_subject,
+ )
subj = get_user_subject(user.id)
if subj and subj not in dashboard.editors:
dashboard.editors.append(subj)
+ # Resolved once by bulk importers and passed in; recomputed here only
+ # for direct callers that omit it (one membership query per asset).
+ viewers = (
+ default_viewers
+ if default_viewers is not None
+ else get_default_viewers_for_new_asset(user.id)
+ )
+ for viewer in viewers:
+ if viewer not in dashboard.viewers:
+ dashboard.viewers.append(viewer)
Review Comment:
Same as the chart-importer thread: viewers isn't in Dashboard.export_fields,
so a v1 dashboard payload can't carry viewers and dashboard.viewers is empty
for a new import. This is also the exact case the saved review instruction
([3656973976]) describes — "viewers are not read from v1 payloads and are empty
for new dashboards" — so it shouldn't be flagged.
--
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]
