villebro opened a new pull request, #43625:
URL: https://github.com/apache/superset/pull/43625
### SUMMARY
Cleanup pass over the `gaq-to-gtf` feature branch, addressing findings from a
three-part review (backend core, frontend, websocket/infra). It targets the
usual agentic-development cleanup categories — duplicated logic, dead code,
redundant/stale comments, and missing test coverage — plus a handful of
latent
bugs the review surfaced along the way.
**Scope discipline:** every change is confined to code this feature branch
itself introduced, or to drift the branch caused (docs/util references to
config
keys the branch removed). An earlier draft rippled into ~14 unrelated
list-view
pages (via a `useListViewResource` signature refactor) and an unrelated tags
integration fixture (via a global feature-flag-manager change); both were
reverted so this PR touches no code outside the branch's own surface. The
only
files here that the branch didn't already touch are additive
(`scripts/oxfmt.sh`,
new helpers in `superset/tasks/utils.py`, a `Hooks` type export) or folded-in
tests.
Merges into `gaq-to-gtf`, not `master`.
#### Latent bugs fixed
- **Async chart-data re-issue drift.** `FilterValue`, `FiltersConfigForm`,
and
`DrillByModal` re-issued the post-completion request with async mode still
on,
so a repeat-`202` body could be cast straight to a result array. They now
share
one `requestChartDataResolved` helper that always re-issues
**synchronously**
and rejects a non-`200` re-issue — matching `exploreJSON`'s already-correct
behavior. `ChartVersionPreview` (a 7th copy) adopts it too.
- **Silent websocket limit disabling.** `superset-websocket` parsed some
numeric
env overrides with bare `Number`, so a typo in `MAX_TOTAL_CONNECTIONS` /
`SOCKET_RESPONSE_TIMEOUT_MS` produced `NaN` and silently disabled the
limit.
All numeric overrides now go through validating parsing with a default
fallback.
- **Dev-compose cookie mismatch.**
`docker/superset-websocket/config.example.json`
still carried the removed `redisStreamPrefix` and the old `async-token`
cookie
name; dev-compose mounts it, so the dev server looked for a cookie
Superset no
longer mints. Now mirrors the current `superset-ws-token` config.
- **`_should_run_async`** received `int | None` where it declared `int`.
#### Backend (`superset/`)
- **DRY:** shared Redis-command mixin across the two coordination backends;
collapsed the four-name contribution-totals indirection; unified
guest-subscriber-key derivation, `factory.create` calls, `naive_utcnow`,
and
the app-context wrapper; used the existing `generate_random_task_key()` /
`parse_properties()` helpers instead of re-implementations.
- **Dead code:** removed `coordination/utils.py`,
`REALTIME_NOTIFICATION_CLAIM`,
`get_channel_id`, and the write-only `username` websocket JWT claim
(unread after the websocket side dropped it).
- **Comments:** rewrote legacy/time-specific comments to state the current
contract; fixed the `task_dependencies` migration's stale `Revises:`
docstring.
- **Tests (folded into existing files):** `get_statuses_changed_since`
polling
primitive (baseline / `>=` cursor bound / advancing cursor), the guest
task-visibility filter, and the `status_changes` REST endpoint.
#### Frontend (`superset-frontend/`)
- The shared re-issue helper above; `handleChartDataResponse.refetch` made
required; `ChartDataRequestResponse['json']` widened so the three
`as unknown as AsyncJob` casts are gone; `async_mode_override` now honored
in
drill-by; `resolveAsyncMode` typed on `Hooks` (dropped a `hooks?: any`);
`Task.status` typed as `TaskStatus`.
- Trimmed speculative surface: dead `realtimeIdField` param removed;
`AsyncModeSection`'s redundant internal feature-flag guard removed; a
shared
`selectAsyncModeOverride` selector replaces three copies.
- Fixtures: nine stale `{ job_id, channel_id }` 202-body fixtures updated to
`{ task_ids }`.
- **Tests:** poll backoff growth/reset, `LiveDuration` re-anchor, the
repeat-202
path, and the `resolveAsyncMode` default; also fixed a vacuous
realtime-debounce
test (it waited 600ms against a 1000ms debounce).
#### Websocket + infra
- One `principalChannel` helper shared by fanout and upgrade validation (they
had diverged on the guest prefix); JWT verified once per connection
instead of
twice; `SocketInstance.identity` made required;
`isSocketActive`/`isPrincipalType`
reused.
- **Tests:** `subscribeToChannels` psubscribe + retry + exactly-once
handler, a
parametrized JWT-rejection matrix (expiry/aud/iss/principal_type/channel
binding), and malformed-guest-subscriber drop.
- Docs: `cache.mdx` / `securing_superset.mdx` / `README.md` updated for
removed
`GLOBAL_ASYNC_QUERIES_*` keys and current websocket requirements;
`superset_core`
`__all__` lists restored (+ `TaskDependency`); shared `scripts/oxfmt.sh`
for the
duplicated pre-commit hook.
#### Discovered and fixed while here (out of original brief)
- The `docker-compose-non-dev.yml` websocket service could not have started —
`x-common-build target: dev` lacked the Node runtime + esbuild bundle
(only the
`lean` stage had them) and CI only smoke-tested `lean`. Moved the copies
into
the shared stage and widened the smoke test to cover `dev`.
- `superset-websocket/utils/loadtest.js` still referenced the removed
`redisStreamPrefix`; rewritten to publish on the current Pub/Sub channels.
#### Items reviewed and intentionally skipped
- `useListViewResource` options-object refactor — reverted (unrelated
ripple).
- `feature_flag_manager` dict-copy — reverted (changed global behavior,
broke an
unrelated fixture).
- `StatefulChart` adopting the re-issue helper — it posts via `ChartClient`
across
a package boundary and doesn't have the bug.
- `docker-compose-non-dev.yml` `*superset-user` anchor — doesn't exist in
that
file; removed the explicit `user: root` instead.
- `UPDATING.md` note for the websocket previous-secret var — the whole
websocket
compose service is new on this branch, so it's not a released breaking
change.
- `task_subscribers` `CheckConstraint` — would require a migration; left as a
follow-up.
### TESTING INSTRUCTIONS
- Backend unit tests (all green locally): `pytest
tests/unit_tests/daos/test_tasks.py tests/unit_tests/tasks/
tests/unit_tests/common/ tests/unit_tests/charts/test_chart_data_api.py
tests/unit_tests/websocket/test_channel.py` (178 passed). The new
`status_changes` coverage in `tests/integration_tests/tasks/api_tests.py` needs
a test DB.
- Frontend (affected suites green locally, 183 tests): `npm run test --
src/components/Chart/chartActions.test.ts src/middleware/asyncEvent.test.ts
src/views/CRUD/hooks.test.tsx src/features/tasks/LiveDuration.test.tsx` plus
the StatefulChart / AsyncModeSection / asyncMode / FilterValue /
ChartVersionPreview / DrillByModal suites.
- Websocket (73/73 green locally): `npm --prefix superset-websocket run test
&& npm --prefix superset-websocket run type`.
- `ruff check` / `ruff format --check` clean on the backend; `oxlint` /
`oxfmt` / custom-rules / `pylint` / `zizmor` clean.
> Note: the frontend `Type-Checking` pre-commit hook reports 4 errors in
`getTotalsMetrics.test.ts` and two `buildQuery.ts` files. These are
**pre-existing** and unrelated — `toTotalsAggregate` exists in `src` but is
missing from stale local `lib/*.d.ts` build output; rebuilding the package
declarations (or CI's fresh build) clears them. None of those files are touched
by this PR.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [x] Required feature flags/config: none new (cleanup of existing
`GLOBAL_ASYNC_QUERIES` / websocket surface)
- [x] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [x] Removes existing feature or API <!-- dead code: coordination/utils.py,
unused JWT username claim, REALTIME_NOTIFICATION_CLAIM, get_channel_id -->
--
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]
