sadpandajoe commented on code in PR #44992:
URL: https://github.com/apache/superset/pull/44992#discussion_r4211902344
##########
tests/integration_tests/reports/api_tests.py:
##########
@@ -66,6 +68,249 @@
class TestReportSchedulesApi(SupersetTestCase):
+ @pytest.fixture
+ def sip_schedule_cleanup(self) -> Iterator[None]:
+ """Remove schedules created by dynamic-executor API scenarios."""
+ yield
+ for schedule in db.session.query(ReportSchedule).filter(
+ ReportSchedule.name.like("sip209_api_%")
+ ):
+ db.session.delete(schedule)
+ db.session.commit()
+
+ def _sip_report_payload(self, name: str) -> dict[str, Any]:
+ """Build a chart report with the ordinary API-required fields."""
+ chart = db.session.query(Slice).first()
+ assert chart is not None
+ return {
+ "type": ReportScheduleType.REPORT,
+ "name": name,
+ "description": "Dynamic executor integration test",
+ "creation_method": ReportCreationMethod.ALERTS_REPORTS,
+ "crontab": "0 9 * * *",
+ "chart": chart.id,
+ }
+
+ @parameterized.expand([(False,), (True,)])
+ @with_feature_flags(ALERT_REPORT_DYNAMIC_EXECUTOR=True)
+ @pytest.mark.usefixtures(
+ "load_birth_names_dashboard_with_slices", "sip_schedule_cleanup"
+ )
+ def test_admin_create_report_executor(self, application_default: bool) ->
None:
+ """An admin may choose their own account or the application default."""
+ self.login(ADMIN_USERNAME)
+ admin = self.get_user(ADMIN_USERNAME)
+ payload =
self._sip_report_payload(f"sip209_api_admin_{application_default}")
+ payload.update(
+ {
+ "run_as": None if application_default else admin.id,
+ "run_as_type": None if application_default else
ExecutorType.FIXED_USER,
+ }
+ )
+
+ response = self.client.post("/api/v1/report/", json=payload)
+ assert response.status_code == 201, response.json
+ schedule = db.session.get(ReportSchedule, response.json["id"])
+ assert schedule.run_as_fk == (None if application_default else
admin.id)
+ assert schedule.run_as_type == (
+ None if application_default else ExecutorType.FIXED_USER
+ )
+
+ @with_feature_flags(ALERT_REPORT_DYNAMIC_EXECUTOR=True)
+ @pytest.mark.usefixtures(
+ "load_birth_names_dashboard_with_slices", "sip_schedule_cleanup"
+ )
+ def test_report_list_includes_executor_identity(self) -> None:
+ """The subscription editor can distinguish self from application
default."""
+ self.login(ADMIN_USERNAME)
+ admin = self.get_user(ADMIN_USERNAME)
+ payload = self._sip_report_payload("sip209_api_subscription_executor")
+ payload.update({"run_as": admin.id, "run_as_type": "fixed_user"})
+ created = self.client.post("/api/v1/report/", json=payload)
+ assert created.status_code == 201, created.json
+
+ query = rison.dumps(
+ {"filters": [{"col": "id", "opr": "eq", "value":
created.json["id"]}]}
+ )
+ response = self.client.get(f"/api/v1/report/?q={query}")
+ assert response.status_code == 200, response.json
+ assert response.json["result"][0]["run_as_type"] == "fixed_user"
+ assert response.json["result"][0]["run_as"]["id"] == admin.id
+
+ @parameterized.expand([("self",), ("default",), ("other",)])
+ @with_feature_flags(ALERT_REPORT_DYNAMIC_EXECUTOR=True)
+ @pytest.mark.usefixtures(
+ "load_birth_names_dashboard_with_slices", "sip_schedule_cleanup"
+ )
+ def test_non_admin_create_report_executor(self, executor: str) -> None:
+ """A non-admin can select only their own account."""
+ alpha = self.get_user("alpha")
+ admin = self.get_user(ADMIN_USERNAME)
+ self.login("alpha")
+ payload = self._sip_report_payload(f"sip209_api_alpha_{executor}")
+ if executor == "self":
+ payload.update({"run_as": alpha.id, "run_as_type": "fixed_user"})
+ elif executor == "default":
+ payload.update({"run_as": None, "run_as_type": None})
+ else:
+ payload.update({"run_as": admin.id, "run_as_type": "fixed_user"})
+
+ response = self.client.post("/api/v1/report/", json=payload)
+ if executor == "self":
+ assert response.status_code == 201, response.json
+ schedule = db.session.get(ReportSchedule, response.json["id"])
+ assert schedule.run_as_fk == alpha.id
+ else:
+ assert response.status_code == 422, response.json
+ assert "run_as" in response.json["message"]
+
+ @with_feature_flags(ALERT_REPORT_DYNAMIC_EXECUTOR=True)
+ @pytest.mark.usefixtures(
+ "load_birth_names_dashboard_with_slices", "sip_schedule_cleanup"
+ )
+ def test_non_admin_edits_own_executor_report(self) -> None:
+ """An owner may change content when the schedule runs as that owner."""
+ alpha = self.get_user("alpha")
+ self.login("alpha")
+ payload = self._sip_report_payload("sip209_api_owned")
+ payload.update({"run_as": alpha.id, "run_as_type": "fixed_user"})
+ created = self.client.post("/api/v1/report/", json=payload)
+ assert created.status_code == 201, created.json
+
+ response = self.client.put(
+ f"/api/v1/report/{created.json['id']}",
+ json={
+ "recipients": [
+ {
+ "type": ReportRecipientType.EMAIL,
+ "recipient_config_json": {"target": alpha.email},
+ }
+ ]
+ },
+ )
+ assert response.status_code == 200, response.json
+ schedule = db.session.get(ReportSchedule, created.json["id"])
+ assert schedule.run_as_fk == alpha.id
+ assert len(schedule.recipients) == 1
+
+ @with_feature_flags(ALERT_REPORT_DYNAMIC_EXECUTOR=True)
+ @pytest.mark.usefixtures(
+ "load_birth_names_dashboard_with_slices", "sip_schedule_cleanup"
+ )
+ def test_non_admin_edits_other_executor_report(self) -> None:
+ """An editor may change metadata, but content requires switching to
self."""
+ admin = self.get_user(ADMIN_USERNAME)
+ alpha = self.get_user("alpha")
+ self.login(ADMIN_USERNAME)
+ payload = self._sip_report_payload("sip209_api_shared")
+ payload.update({"run_as": admin.id, "run_as_type": "fixed_user"})
+ created = self.client.post("/api/v1/report/", json=payload)
+ assert created.status_code == 201, created.json
+ schedule = db.session.get(ReportSchedule, created.json["id"])
+ schedule.editors = _subjects_for_users([admin, alpha])
+ db.session.commit()
+ uri = f"/api/v1/report/{schedule.id}"
+ recipients = [
+ {
+ "type": ReportRecipientType.EMAIL,
+ "recipient_config_json": {"target": alpha.email},
+ }
+ ]
+
+ self.logout()
+ self.login("alpha")
+ metadata = self.client.put(uri, json={"name":
"sip209_api_shared_renamed"})
+ assert metadata.status_code == 200, metadata.json
+ db.session.refresh(schedule)
+ changed_format = (
+ ReportDataFormat.PDF
+ if schedule.report_format != ReportDataFormat.PDF
+ else ReportDataFormat.PNG
+ )
+ format_only = self.client.put(uri, json={"report_format":
changed_format})
Review Comment:
This PUT asserts 200 for a format-only change by an editor whose report runs
as Admin, but `report_format` is now in `CONTENT_FIELDS`, so
`_validate_executors` returns 422 (CI shows `assert 422 == 200` here on every
backend). Should this assert the rejection and unchanged format, then cover the
format change after switching execution to the editor?
##########
superset/commands/report/execute.py:
##########
@@ -672,6 +722,12 @@ def _get_url(
# _get_embedded_data, _get_notification_content) funnels through this
# method, so this is the single choke point.
if chart is None and dashboard is None:
+ if (
+ user_friendly
+ and self._report_schedule.type == ReportScheduleType.ALERT
+ and not self._attachments_enabled()
+ ):
+ return ""
Review Comment:
An attachment-free alert with no chart/dashboard now returns an empty URL
here, but `send_error` (and the retry/final-failure builders) still pass
`include_cta=self._report_schedule.include_cta is not False` without the
`bool(url)` guard used in `_get_notification_content`. When the alert query
fails, the editor email renders an "Explore in Superset" link with `href=""`.
Should those sibling paths drop the CTA when the URL is empty?
##########
tests/unit_tests/migrations/test_add_run_as_and_report_config.py:
##########
@@ -0,0 +1,181 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Tests for migration ``e4a7c2b9d1f3_add_run_as_and_report_config`` (SIP-209).
+
+Runs ``upgrade()`` and ``downgrade()`` against an in-memory SQLite engine
+seeded with minimal ``ab_user`` and ``report_schedule`` tables and asserts the
+new nullable columns and the ``report_config`` table are created and removed
+without touching existing rows.
+"""
+
+from __future__ import annotations
+
+from importlib import import_module
+from pathlib import Path
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from alembic.script import ScriptDirectory
+from sqlalchemy import (
+ Column,
+ create_engine,
+ inspect,
+ Integer,
+ MetaData,
+ select,
+ String,
+ Table,
+)
+from sqlalchemy.engine import Engine
+
+migration = import_module(
+ "superset.migrations.versions."
+ "2026-10-05_12-00_e4a7c2b9d1f3_add_run_as_and_report_config"
+)
+
+
[email protected]
+def engine() -> Engine:
+ engine = create_engine("sqlite:///:memory:")
+ md = MetaData()
+ Table(
+ "ab_user",
+ md,
+ Column("id", Integer, primary_key=True),
+ Column("username", String(64)),
+ )
+ report_schedule = Table(
+ migration.REPORT_SCHEDULE_TABLE,
+ md,
+ Column("id", Integer, primary_key=True),
+ Column("name", String(150), nullable=False),
+ Column("type", String(50), nullable=False),
+ )
+ md.create_all(engine)
+ with engine.begin() as conn:
+ conn.execute(
+ report_schedule.insert(),
+ [{"id": 1, "name": "daily", "type": "Report"}],
+ )
+ return engine
+
+
+def _columns(engine: Engine, table: str) -> set[str]:
+ return {col["name"] for col in inspect(engine).get_columns(table)}
+
+
+def _run(engine: Engine, fn) -> None:
+ with engine.begin() as conn:
+ ctx = MigrationContext.configure(conn)
+ with Operations.context(ctx):
+ fn()
+
+
+def test_upgrade_adds_columns_and_config_table(engine: Engine) -> None:
+ _run(engine, migration.upgrade)
+
+ columns = _columns(engine, migration.REPORT_SCHEDULE_TABLE)
+ assert {
+ "run_as_fk",
+ "run_alert_query_as_fk",
+ "run_as_type",
+ "run_alert_query_as_type",
+ } <= columns
+ assert inspect(engine).has_table(migration.REPORT_CONFIG_TABLE)
+ assert {"id", "key", "value", "created_on", "changed_on"} <= _columns(
+ engine, migration.REPORT_CONFIG_TABLE
+ )
+
+ # Existing schedules are untouched and keep NULL executors (legacy path).
+ with engine.begin() as conn:
+ table = Table(migration.REPORT_SCHEDULE_TABLE, MetaData(),
autoload_with=conn)
+ rows = conn.execute(select(table)).fetchall()
+ assert len(rows) == 1
+ assert rows[0].run_as_fk is None
+ assert rows[0].run_alert_query_as_fk is None
+
+ # Idempotent: a second run is a no-op.
+ _run(engine, migration.upgrade)
+
+
+def test_downgrade_reverts(engine: Engine) -> None:
+ _run(engine, migration.upgrade)
+ _run(engine, migration.downgrade)
+
+ assert not inspect(engine).has_table(migration.REPORT_CONFIG_TABLE)
+ columns = _columns(engine, migration.REPORT_SCHEDULE_TABLE)
+ assert "run_as_fk" not in columns
+ assert "run_alert_query_as_fk" not in columns
+ with engine.begin() as conn:
+ table = Table(migration.REPORT_SCHEDULE_TABLE, MetaData(),
autoload_with=conn)
+ assert len(conn.execute(select(table)).fetchall()) == 1
+
+
+def test_sip_migration_is_the_single_head() -> None:
+ directory = Path(__file__).resolve().parents[3] / "superset" / "migrations"
+ script = ScriptDirectory(str(directory))
+ assert script.get_heads() == [migration.revision]
Review Comment:
`script.get_heads() == [migration.revision]` pins this revision as the
repository head, so the next unrelated migration that lands on top of it fails
this test even though the chain is correct. `test_single_migration_head.py`
already asserts a single head; should this check that this revision is an
ancestor of the head instead?
##########
superset-frontend/src/features/reports/ReportModal/index.tsx:
##########
@@ -182,13 +190,26 @@ function ReportModal({
const isEditMode = report && Object.keys(report).length;
useEffect(() => {
+ setExecuteAsSelf(false);
if (isEditMode) {
setCurrentReport(report);
} else {
setCurrentReport('reset');
}
}, [isEditMode, report]);
+ const formatChanged =
+ isChart &&
+ Boolean(isEditMode) &&
+ (currentReport.report_format || defaultNotificationFormat) !==
+ report.report_format;
+ const formatRequiresTakeover =
+ isFeatureEnabled(FeatureFlag.AlertReportDynamicExecutor) &&
+ formatChanged &&
+ !isUserAdmin(bootstrapUser) &&
+ (report.run_as_type !== 'fixed_user' ||
Review Comment:
`formatRequiresTakeover` reads `report.run_as?.id` and `run_as_type` from
the store, but after `ADD_REPORT`/`SUBSCRIBE_REPORT`/`EDIT_REPORT` the reducer
stores the request payload echoed by the API (`run_as` as an integer, or absent
on a plain edit or a server-defaulted create). A non-admin who saves their own
subscription and changes the format in the same session then gets
`formatRequiresTakeover === true` and must tick "execute using my permissions"
for a schedule that already runs as them. Should the comparison handle the
integer/absent shape, or the store be refetched after mutations?
##########
superset/commands/report/execute.py:
##########
@@ -2571,22 +2678,10 @@ def run(self) -> None: # noqa: C901
deadline.remaining_seconds,
)
- # Resolve the executor at the run() boundary, tolerating a missing
- # user (find_user -> None) so the state machine still runs and its
- # error envelope writes the ERROR execution-log row and sends the
- # editor notification. The dedicated
ReportScheduleExecutorNotFoundError
- # guard lives at the content sites (_get_screenshots / _get_data /
- # _get_embedded_data), which raise inside that envelope. Guarding
here
- # instead would surface the executor error above the state machine,
- # suppressing both the log row and the editor notification. The
- # alert-query path (AlertCommand) is intentionally left unchanged
— a
- # missing executor there surfaces as a query error, not the
dedicated
- # executor error; tightening it is out of scope here.
- _, username = get_executor(
- executors=app.config["ALERT_REPORTS_EXECUTORS"],
- model=self._model,
- )
- user = security_manager.find_user(username)
+ # Resolve content identity without rejecting an attachment-free
alert.
+ # Content fetches and AlertCommand validate their respective
identities
+ # inside the state machine, where failures are logged and
terminalized.
+ user, username = get_executor_user(self._model)
Review Comment:
`get_executor_user` now intentionally returns `None` for an attachment-free
alert whose content user is inactive, but the dashboard-permalink pre-commit
just below still runs under that identity. With a saved dashboard tab
(`ALERT_REPORT_TABS`), `get_dashboard_urls()` creates a tab permalink through
the dashboard access check, which fails for the anonymous user outside the
state machine's error envelope, so the alert query never runs even though its
own executor is valid. Should the permalink step be skipped (or moved inside
the envelope) when no attachment or link is needed?
##########
superset/commands/report/update.py:
##########
@@ -42,17 +48,97 @@
from superset.exceptions import SupersetSecurityException
from superset.reports.models import (
ReportCreationMethod,
+ ReportDataFormat,
ReportRecipientType,
ReportSchedule,
ReportScheduleType,
ReportState,
)
+from superset.tasks.exceptions import ExecutorNotFoundError
+from superset.tasks.types import ExecutorType
+from superset.tasks.utils import get_executor
from superset.utils import json
-from superset.utils.core import get_user_email
+from superset.utils.core import get_user, get_user_email, get_user_id
from superset.utils.decorators import on_error, transaction
logger = logging.getLogger(__name__)
+# Payload fields that change the delivered asset, its rendering, or its
recipients.
+# When a schedule executes as another user, only admins may change these.
+CONTENT_FIELDS: frozenset[str] = frozenset(
Review Comment:
`type` isn't in `CONTENT_FIELDS`, so an editor of a schedule that runs as
another user can send `{"type": "Report"}` on an alert whose stored
`report_format` is PNG and whose attachments are globally disabled. Nothing in
the stored content changes, so the guard doesn't fire, but reports always
generate the attachment, so the next run delivers the chart rendered as the
other user to the existing recipients. Should an Alert→Report switch be treated
as a content change here?
--
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]
