Antonio-RiveroMartnez commented on PR #44992:
URL: https://github.com/apache/superset/pull/44992#issuecomment-6064496422
## Review — 1 blocker, 2 improvements
**Verdict:** The executor and permission model looks solid to me. The only
thing I'd want resolved before merge is a silent change to TEXT-alert content
that lands even with `ALERT_REPORT_DYNAMIC_EXECUTOR` off.
### Blockers
1. **TEXT alerts with a chart lose their embedded table when alert
attachments are off.** `superset/commands/report/execute.py:1599-1604`
On master (`execute.py:1544-1548`), a chart-backed `TEXT` schedule always
called `_get_embedded_data()`. This PR adds a `self._attachments_enabled()`
check in front of that. For alerts, the check (`execute.py:483-488`) requires
`alerts_attach_reports_enabled()`, which falls back to `ALERTS_ATTACH_REPORTS`.
So after upgrading, any deployment that runs with `ALERTS_ATTACH_REPORTS
= False`, or that saves "Enable attachments for alerts" as off, stops sending
the table in TEXT alerts. That happens with the new FF off and with no
configuration saved. The PR description says FF-off behavior is unchanged.
Neither `UPDATING.md` nor `alerts-reports.mdx:29` says the inline text table is
affected; the docs only mention PNG/PDF/CSV/XLSX. This matches @sadpandajoe's
unanswered comment on that line.
Either exempt `ReportDataFormat.TEXT` from the global gate, or, if this
is intended, say so explicitly in UPDATING.md and the docs.
### Improvements
1. **Config save is a read-merge-write with no lock, and it fails if the
stored row is missing.** `superset/daos/report.py:508-525`
I know this was deferred in
https://github.com/apache/superset/pull/44992#discussion_r4215527223. I'm
raising it once more because the fix is very small, and it would also fix a
second problem in the same code.
- **Lost updates.** `upsert()` reads the document without a lock, merges
the submitted keys, and overwrites the whole document. If two admins save at
the same time, one change is lost. If the lost change is
`allowed_email_domains` or `limit_recipients_to_users`, recipient delivery is
unrestricted again without anyone noticing. Unlike the single-value
`shared_entries.py` store, this is the first key_value entry that holds several
settings merged into one document. The lock is already available:
`KeyValueDAO.get_entry(..., for_update=True)` (`daos/key_value.py:42-54`), as
used in `commands/distributed_lock/release.py:112`.
- **Missing row.** The write goes through `KeyValueDAO.update_entry`,
which raises `KeyValueUpdateFailedError` when the row isn't there
(`daos/key_value.py:184`). Reads tolerate a missing row
(`daos/report.py:459-460`), so the config can be read but never saved on a
database without the migration seed (e.g. `create_all`, test fixtures, or a
deleted row). It also returns a **500**, not the intended 422: `on_error` only
translates `SQLAlchemyError` (`utils/decorators.py:236`), and
`KeyValueUpdateFailedError` is an `UpdateFailedError` with `status = 500`.
- **Suggested fix.** In one transaction: a locking read, then update the
row if it exists or create it if it doesn't.
2. **Factory reset can fail on the retained config row's audit FK.**
`superset/commands/security/reset.py:66-83`
Reset keeps the `ALERT_REPORT_CONFIG` key_value row but deletes every
non-Admin user. `KeyValueEntry.changed_by_fk` is a plain FK to `ab_user.id`
with no `ON DELETE SET NULL` (`key_value/models.py:40`).
This is a narrow case: someone saves the config while they're an Admin,
later loses the Admin role, and then a reset runs. On a database that enforces
FKs, deleting that user fails. Nulling `created_by_fk` / `changed_by_fk` on the
retained row before deleting users would avoid it.
### Nits
- `superset/reports/schemas.py:133`: `run_as_description` is missing a space
(`"...Only" "honored..."`), so the OpenAPI text reads "Onlyhonored".
### Considered and dropped
- **Migration conflict bot warning.** Dropped: `down_revision =
"00fab727cd0a"` matches master's current head, and nothing else on master
revises it.
- **`sharded-jest-tests (2)` failure.** Dropped as unrelated: the failing
test is in `src/dashboard/components/gridComponents/Chart/Chart.test.tsx`
("Export to full Excel"), and this PR touches nothing under `src/dashboard`.
- **Non-admin escalation through Run as.** I checked these paths and found
no gap:
- selecting another user (`commands/report/base.py:493-495`)
- clearing the query executor so it inherits another identity
(`update.py:302-323`)
- changing content or recipients while another identity is still in place
(`update.py:275-300`)
- subscription recipient rewriting, which happens before the
protected-field comparison (`update.py:389-419`)
- deleted or inactive executors, which fail closed rather than falling
back to defaults (`reports/utils.py:77-81`, `execute.py:147-180`)
--
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]
