chore(viz): remove legacy explore_json + viz.py pipeline - #41714
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…ngrade Addresses @sadpandajoe's follow-up review comment on #41714: an original query_context with "queries": [] backs up as a falsy-but- present empty list, not None. downgrade_slice's truthiness check (`if queries_bak:`) treated that the same as "no context was ever stored" and set query_context to None, discarding the slice's original datasource and form_data instead of restoring it. Also fixes the same ambiguity at the source: QUERIES_BAK_FIELD_NAME defaulted to {} rather than None when absent from form_data, which could be misread as a real (if malformed) backup rather than "key not present". Changed to the natural None default so "no backup" and "an empty list backup" stay distinguishable, and dedented the query_context assignment that was incorrectly nested inside the "form_data" in query_context check (a context missing "form_data" would restore params/viz_type but silently leave query_context at its upgraded value). Also updates the stale UPDATING.md note for this PR: the percent- re-basing and deck_multi autozoom limitations it warned about were both resolved earlier in this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ipeline Routine rebase; the only conflict was package-lock.json, regenerated via npm install against the already-cleanly-merged package.json. Everything else (package.json, plugin-chart-partition's package.json, superset/views/base.py, superset/views/core.py, tests/integration_tests/security_tests.py) merged automatically.
…complete The lockfile committed while resolving the last rebase conflict against master was missing several real transitive dependencies (preact, @react-spring/*, polished, react-ace, @deck.gl/widgets), which failed npm ci in CI's frontend-build/docker-build with "Missing: X from lock file" errors, cascading into pre-commit/cypress/playwright/docker failures. Root-caused to two compounding issues on the machine that regenerated it: a corrupted local npm cache (npm cache verify found and cleaned up 643 bad entries) and the --legacy-peer-deps flag, which does not auto-install a peer dependency's own transitive deps the way a plain npm install does -- @deck.gl/widgets is itself a peer dependency, and preact is one of its own dependencies, so it was silently dropped. Regenerated cleanly with a plain npm install after clearing the cache; verified node_modules/preact, node_modules/@react-spring/core, and node_modules/@deck.gl/widgets are all present this time.
…ipeline A second, smaller conflict landed while pushing the previous rebase fix: master's Prettier-to-Oxfmt migration touched every plugin package.json, colliding with this branch's own renames/deletions of the legacy-prefixed packages. Resolved: - Two modify/delete conflicts (legacy-plugin-chart-rose, legacy-preset-chart-nvd3): kept deleted. Both packages are fully removed on this branch with no replacement package (Rose now lives inside plugin-chart-echarts/src/Rose/). - Five package.json field-reordering conflicts (chord, country-map, paired-t-test, parallel-coordinates, world-map): master's migration reordered/added fields (keywords, homepage) on the old legacy- prefixed paths while this branch had independently reordered the same renamed files. Reconstructed each to the canonical field order already used by sibling packages that merged cleanly (partition, echarts). - One content conflict in paired-t-test/src/TTestTable.tsx: discarded master's dead legacy reactable-based sortConfig block, already fully replaced by this branch's own COMPARATORS object earlier in the same file. package-lock.json regenerated via a clean npm install (no --legacy-peer-deps, after clearing corrupted npm cache entries) to correctly reflect both the dependency-tree changes from this rebase and the Oxfmt migration's own devDependency swap (prettier/eslint- plugin-prettier out, oxfmt in).
# Conflicts: # superset-frontend/package-lock.json
…merge revision master added a new migration head (f3a8c1d2e9b7) branching from the same revision already folded into 9d744c5dd981, producing two heads again.
|
I've been running an adaptation of #41730 (the Sharing them here with the fixes I ended up with, in case they're useful. Happy to open a PR against this branch for any or all of them. Confidence, so you can weigh them: 2, 3 and 4 I watched fail in production and then traced to the code below. 1 I derived from the code and fixed before it could bite, so I have not actually witnessed it; treat it as a code reading rather than a report. 1. Autozoom silently stops working
const features = props.payload?.data?.features || {};But the new Fix: collect the points from the features each layer's own 2. The container renders the "No results" empty stateWith const noResultQueries =
enableNoResults &&
(!queriesData ||
queriesData
.slice(0, getQueryCount())
.every(({ data }) => !data || (Array.isArray(data) && data.length === 0)));
const bypassNoResult = !(
currentFormDataExtended?.server_pagination &&
(hasSearchText || hasAgGridFilters)
);Fix: honor 3. Layer lookup 404s for any principal whose chart access is scoped
if (guest_dashboards := guest_embedded_dashboard_filter()) is not None:
return query.filter(self.model.dashboards.any(guest_dashboards))
if security_manager.can_access_all_datasources():
return query
return self._apply_viewers(query)An embedded guest gets denied by the first branch (the layers are not on the token's dashboards) and a role without an explicit grant on the layer gets denied by Confirming it was the filter and not a stale id or a missing session, for one of our layers:
Note this failure is deterministic rather than intermittent, so for embedded deployments it is the one that fully breaks the chart. The legacy pipeline never hit this because it resolved the layers server-side, under the container's access: slices = db.session.query(Slice).filter(Slice.id.in_(slice_ids)).all()Fix: a Worth noting this also collapses N requests into one. 4. Sub-layer requests still take the async handoffThis is the one that cost me the most time, so it may be the most valuable. use_async = (
is_feature_enabled("GLOBAL_ASYNC_QUERIES")
and query_context.result_format == ChartDataResultFormat.JSON
and query_context.result_type == ChartDataResultType.FULL
and cache_timeout != CACHE_DISABLED_TIMEOUT
)
if use_async:
return self._run_async(json_body, command, add_extra_log_payload)So with It presents as intermittent and "fixed by reloading", because by the time you reload the job has completed and warmed the cache. That made it quite hard to chase. Note this is the same failure the legacy path had on Fix: request If it helps in prioritizing: #2 and #3 break the chart completely and deterministically; #2 for everyone, #3 for embedded deployments; while #4 presents as intermittent and self-healing, which made it by far the most expensive to diagnose. |
Multi.tsx posts directly with SupersetClient.post and never registers a listener for the async job GLOBAL_ASYNC_QUERIES hands back for result_type 'full' with a cold cache, so a layer just never renders. 'results' returns the same data/colnames/coltypes each layer's transformProps reads, and skips the async handoff entirely. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@sancho11 this is exactly the kind of testing this migration needs before it lands, thanks for tracing all this. 1 and 2 were already fixed on the branch, autozoom buckets by slice_id and 3 is real too, and the trickiest of the four, a new endpoint scoped to the container's own access rather than the per-chart check. Want to open that one against this branch like you offered? I'd rather have your production traces backing it than guess at it myself. |
…n with extension storage table head CI on this branch was failing with "Multiple head revisions are present for given argument 'head'" because the branch's existing merge revision (0b99fc576740) didn't account for the extension storage table migration (e5f6a7b8c9d0), leaving two unmerged heads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ated endpoint Multi.tsx fetches each declared layer's metadata with GET /api/v1/chart/<layer_id>. Layer charts sit on no dashboard of their own, so the standard chart base filter 404s that request for any principal (e.g. an embedded guest, or a role without an explicit grant on the layer) who is only entitled to the deck_multi container -- even though the container itself is fully accessible to them. This breaks every layer on embedded dashboards deterministically. Add GET /api/v1/chart/<pk>/deck_layers/, which gates on the container chart (normal access rules apply there) and resolves the layers it declares in its own deck_slices config with skip_base_filter=True, mirroring how the removed explore_json/viz.py pipeline resolved layers server-side, under the container's access. Multi.tsx uses the new endpoint when the container is saved, and falls back to the old per-chart reads for an unsaved chart in Explore. Reported in-thread on this PR with a production repro against an embedded dashboard with GLOBAL_ASYNC_QUERIES enabled. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@sancho11 went ahead and built 3 myself rather than leave it on your plate: |
…tion, fix Explore layer preview lag - superset/charts/api.py: only skip the chart base filter for the deck_layers container-based lookup when the caller is an embedded guest; an ordinary logged-in user could otherwise read an arbitrary chart's params/datasource by naming it in a deck_multi container's deck_slices they can edit. - superset/migrations/shared/migrate_viz/base.py: back up a stored query_context with "queries": null, or a non-object query_context, through the same FULL_CONTEXT_BAK_KEY wholesale path used for a missing "queries" key, so downgrade can tell "no context" apart from "context had a null/odd queries" instead of discarding the slice's datasource/form_data, and so a non-dict context doesn't raise mid-migration and leave the row half-migrated. - Multi.tsx: fall back to per-chart reads for any deck_slices id missing from the container's persisted deck_layers response, so a layer just added in Explore (but not yet saved) still previews instead of waiting for a save. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
🎪 Showtime deployed environment on GHA for 1a2bee4 • Environment: http://34.222.17.87:8080 (admin/admin) |
|
@rusackas Thanks for picking 3 up; I looked at There is a fifth gap I ran into afterwards, and I'd rather ask than assume where it belongs. Composing layers loses the legend each of them shows standalone. I have it working locally against the migrated This is a parity gap rather than a migration regression, though, and I don't want to grow the scope of a 616-file PR without checking. Happy either way. I can open it against this branch now, or hold it as a follow-up once this lands. Your call. |
I might look into it to see if it's an easy fix, but this PR is big and fragile enough that I'm inclined to tackle that on another PR. Feel free to file an Issue if you'd like. @sancho11 It's a good catch, but if I go trying to solve all the pre-existing bugs with all the charts I've touched in this PR, I'll go crazy :) |
villebro
left a comment
There was a problem hiding this comment.
LGTM. Despite the massive and welcome changes here (and quite frankly somewhat difficult to review), I feel great care has been taken to address potential issues. And not to forget all the multiple improvements. The benefits this brings IMO far outweigh any regression risks, hence I'm going to approve this. Great work @rusackas and HUGE kudos for seeing this through! Also big thanks to reviewers/testers!
|
Kudos! 💪 |

SUMMARY
This branch removes the legacy viz pipeline — the deprecated
/superset/explore_json/endpoints andsuperset/viz.py(both@deprecated(eol_version="5.0.0")since 3.0) — by first migrating all 15 remaininguseLegacyApicharts to/api/v1/chart/data, then deleting the pipeline and dropping thelegacy-prefix from the surviving plugin packages.How the charts were migrated. Eleven charts were converted in place: same renderer, same
viz_type(so saved charts keep working with no DB migration), with a newbuildQuery.tsmirroring the oldviz.pyquery_obj()and theget_data()reshape ported totransformProps— each with Jest tests pinning parity against the legacy behavior (null padding, pandas sort orders, comma escaping,fill_valuesemantics,relativedelta/offset-rollback arithmetic, etc.). Where the legacy engine used pandas transforms with post-processing equivalents (rose), the newbuildQueryattaches the server-sidepost_processingpipeline (pivot → resample → rolling → compare → contribution → flatten) instead of re-implementing pandas in JS. Two charts were retargeted to existing ECharts charts viaMigrateVizprocessors + Alembic revisions with reversibleform_databackups:bubble→bubble_v2(the processor existed since the ECharts bubble shipped, but no revision ever ran it) andcompare→echarts_timeseries_line.deck_multibecame fully client-side, completing the deck.gl preset migration: the container issues an empty-queries context and each layer loads through its own registeredbuildQuery/transformProps, resolving its datasource from the chart API's authoritativedatasource_id/typerather than the (possibly stale)params.datasourcestring, and surfacing per-layer load failures as a warning banner instead of failing silently.Beyond the original scope: nvd3 is now completely gone. Three of the migrated charts didn't stop at a new data pipeline — they were subsequently rebuilt as native ECharts renderers, which meant the entire
nvd3/nvd3-forkdependency (and the last package that depended on it,preset-chart-nvd3) could be deleted from the codebase outright:universalTransition.markArearange bands, scatter/triangle target markers, and per-range tooltips.compare's percent-change re-basing (originally dropped as a "known limitation" of the ECharts migration — see below) was restored on the mainlineecharts_timeseries_linechart via arebase_percent_changeflag and a draggable baseline (EChartsgraphicoverlay +convertFromPixel+ grid-rect-clamped snapping).paired_ttest's table was also rebuilt on antdTable, dropping the unmaintainedreactabledependency (which turned out to have a real production-only rendering bug — it silently drops rows in a minified bundle, invisible in Jest — so this was a correctness fix, not just cleanup) and the Node-onlydistributionspackage.What was removed (#41750): the
explore_jsonroutes and their CSV/XLSX/query/results/samples helpers,superset/viz.pyand every importer (get_viz,Slice.viz,Slice.explore_json_url, legacy cache warm-up, legacy annotation data, theBaseVizoverload ofraise_for_access), theload_explore_json_into_cachecelery task, thecan_explore_jsonpublic-role grant, the CSRF exemption, and the frontenduseLegacyApiplumbing (ChartMetadatafield,shouldUseLegacyApi, all/explore_json/request branches, and a dataset-required warning banner that only ever applied to legacy chart types).Renames (#41751):
legacy-plugin-chart-{calendar,chord,country-map,horizon,paired-t-test,parallel-coordinates,partition,rose,world-map}→plugin-chart-*;legacy-preset-chart-nvd3→preset-chart-nvd3, which itself was later deleted outright once Bullet, Time-series Period Pivot, and Rose no longer needed it. "(legacy)" chart names and Legacy gallery tags are gone.Phase PRs (each merged green with bot-review threads resolved): #41715 (dead code), #41716
para, #41717country_map, #41718bullet(data-pipeline migration; later superseded by the ECharts rebuild below), #41719chord, #41720world_map, #41721paired_ttest, #41723time_table, #41724cal_heatmap, #41725horizon, #41726rose(data-pipeline migration; later superseded), #41727time_pivot(data-pipeline migration; later superseded), #41728bubble, #41729partition, #41730deck_multi, #41732 (ordering parity), #41738compare, #41750 (pipeline removal), #41751 (renames), #42225 (Bullet rebuilt on ECharts), #42245 (Time-series Period Pivot rebuilt on ECharts), #42247 (percent-change rebasing restored), #42381 (Rose rebuilt on ECharts,preset-chart-nvd3package deleted), #42530 (post-review hardening from a self-review pass over this branch — see below).Known behavior notes (also in UPDATING.md):
deck_multidashboard filter badges no longer aggregate child-layer filter metadata (each layer is now an independently-fetched chart, so there's no single payload to summarize from); the world bank example shipsbubble_v2. The two previously-listed limitations —compare's percent re-basing anddeck_multi's initial autozoom — have since been resolved (see above and the deck_multi autozoom fix that refits the viewport as each layer's async features arrive, rather than only once at load).Embedded/guest chart access — checked, mitigated. Charts on the v1
/api/v1/chart/datapipeline are subject to the guest-tamper check (query_context_modified) when embedded, which used to reject a request if the stored chart had noquery_contextand its storedparamsdidn't look like the posted payload —explore_jsonwas exempt, so legacy chart types embedded via guest tokens never hit it. Migrating 15 more chart types onto the v1 pipeline would have widened that exposure, but #42295 (already merged into this branch) added an equivalence table so the guest-tamper check compares against each chart type's own control-specific stored param keys (metric/x/y/size/entity/etc.) instead of requiring a storedquery_contextat all. Verified this is actually in the merged code (superset/security/manager.py,_columns_metrics_modified/_STORED_METRIC_PARAMS), not just present on master in isolation — flagging here so reviewers don't need to re-derive or re-flag it.Migration downgrade scoping — fixed via #42530.
MigrateViz.downgrade()selects rows byviz_type == target_viz_type AND params LIKE '%form_data_bak%', which isn't scoped to which migration produced them. Bothcompare→echarts_timeseries_line(this PR) and the already-shippedline→echarts_timeseries_linemigration target the sameviz_type, so runningalembic downgradeon the compare migration (orsuperset viz-migrations downgrade -t compare) used to also revert any previously-migratedlinecharts back to the now-deleted legacylineplugin. #42530 (already merged into this branch) fixed this:downgrade_slicenow checks each row's ownform_data_bak["viz_type"]againstsource_viz_typebefore reverting it, so a downgrade only touches the slices the matching migration actually upgraded — the SQL-level filter stays coarse (it's still justtarget_viz_type-based, for efficient row selection), but the per-row check now makes it precise. The CLI's separatePREVIOUS_VERSIONmapping, which had its own copy of this same source→target logic and could drift from it, was removed in favor of deriving the source viz type from each slice's own stored backup.Further hardening from a self-review pass (#42530, already merged into this branch): a stale-response race in
deck_multi(an out-of-order async layer response could clobber a newer one — fixed with a load-generation guard) plus removal of a dead code path reading a legacy payload shape the v1 endpoint never returns; the percent-change draggable baseline no longer reads its own series data back out ofchart.getOption()post-setOption()(now reads theechartOptionsprop directly) and gained its first test coverage; aquery_contextmissing the expected"queries"key (e.g. hand-edited via the API) no longer throws an uncaughtKeyErrormid-upgrade and leaves the slice half-migrated; Bullet's and Time-series Period Pivot's near-duplicate wrapped-legend-row estimation logic was deduped into one shared, tested helper (which also fixed a legend/chart overlap bug in Time-series Period Pivot);partition,paired_ttest, andparallel_coordinates's near-duplicate sort-metric/orderbybuildQuerylogic was likewise deduped; and Rose,partition'stransformProps, and the deck.glMulticontainer all gained test coverage they were missing.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Converted-in-place charts render identically (same renderer, same data shape, different endpoint). Bubble/compare charts re-open as their ECharts equivalents. Bullet, Time-series Period Pivot, and Rose render as newly-built ECharts charts (see screenshots below) rather than their old nvd3 renderers.
TESTING
superset db upgrademigrates savedbubble/comparecharts (reversible viaform_data_bak;superset viz-migrationsCLI also supports both types).POST /api/v1/chart/data;/superset/explore_json/no longer exists.Did manual tests of all affected charts... some are now better than they were on
master. Many screenshots below.SPECIAL BONUS: #42328 provides:
• New (not amazing, but I'm working on it) built-in example charts for ALL VISUALIZATIONS
• A new process to crawl and screencap ALL example charts as viz thumbnails
This can provide (a) evergreen thumbnails, and (b) regression testing for examples, a they should generally remain byte identical. In fact, I've been using that to find and fix bugs on this branch.
ADDITIONAL INFORMATION
bubble/compareslice counts (paginated updates); no downtime expected🤖 Generated with Claude Code
Screenshots from testing (see lots more on #42328)
New Nightengale Rose (ECharts)
Country Map:
World Map:
Chord:
Calendar Heatmap:
Horizon:
Bullet:
Timeseries Table
Nightengale Rose
Paired T-Test
After much ado, this one's working, and removes the reactable dependency to boot!

DeckGL Multi Layer
Timeseries pivot (not sure if this is working right... I never use this one. Investigating still...
Bubble
Partition
Parallel Coordinates
DeckGL Charts