Vitor-Avila commented on code in PR #44992:
URL: https://github.com/apache/superset/pull/44992#discussion_r4202740133
##########
superset/commands/report/update.py:
##########
@@ -65,6 +114,153 @@ def run(self) -> Model:
self.validate()
return ReportScheduleDAO.update(self._model, self._properties)
+ def _changed_content_fields(self) -> set[str]:
+ """
+ Return the content fields whose payload value differs from the stored
+ one. Must run before other validators replace ids with model objects.
+ """
+ assert self._model is not None
+ model = self._model
+ current: dict[str, Any] = {
+ "chart": model.chart_id,
+ "dashboard": model.dashboard_id,
+ "extra": _normalize_json(model.extra_json),
+ "recipients": _normalize_recipients(model.recipients),
+ "report_format": model.report_format,
+ }
+ normalizers: dict[str, Callable[[Any], Any]] = {
+ "extra": _normalize_json,
+ "recipients": _normalize_recipients,
+ }
+ changed: set[str] = set()
+ for field in CONTENT_FIELDS & set(self._properties):
+ value = self._properties[field]
+ normalizer = normalizers.get(field)
+ if normalizer:
+ value = normalizer(value)
+ if value != current[field]:
+ changed.add(field)
+ return changed
+
+ def _validate_executors(
+ self,
+ report_type: str,
+ changed_content_fields: set[str],
+ exceptions: list[ValidationError],
+ ) -> None:
+ """
+ Resolve the "Run As" fields (SIP-209) on update.
+
+ Fields absent from the payload keep their stored value. Admins may set
+ any active user or explicit executor type. Clearing content restores
the
+ legacy application-configured executor;
+ clearing the query executor inherits the content identity.
+ Non-admins may only point the fields at themselves and, when the
schedule
+ executes as another user,
+ may not change its content or recipients: doing so would let an editor
+ read data with someone else's credentials.
+ """
+ assert self._model is not None
+ has_run_as = bool({"run_as", "run_as_type"} & self._properties.keys())
+ has_query_as = bool(
+ {"run_alert_query_as", "run_alert_query_as_type"} &
self._properties.keys()
+ )
+ run_as_id = self._properties.pop("run_as", None)
+ query_as_id = self._properties.pop("run_alert_query_as", None)
+ if not is_feature_enabled("ALERT_REPORT_DYNAMIC_EXECUTOR"):
Review Comment:
I think the latter, because once the FF is off, people won't really be able
to update `run_as` fields:
* The UI won't show them
* They're discarded from API payloads
So validating `changed_content_fields` would just block users from updating
the schedule.
I'll go ahead and push a new commit that would always clear the fields on
this scenario so that the schedule is configured for "application default".
This would match the behavior (with FF off, all schedules run with "application
default").
--
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]
