sadpandajoe commented on code in PR #41714: URL: https://github.com/apache/superset/pull/41714#discussion_r3697430533
########## UPDATING.md: ########## @@ -233,6 +233,10 @@ are the intended model going forward; deprecating and removing implicit viewersh in a later major version. - [41044](https://github.com/apache/superset/issues/41044): Removes the deprecated `AVOID_COLORS_COLLISION` feature flag (it defaulted to `True`). Color-collision avoidance is now permanently enabled; any config override setting it to `False` is ignored. +- [41714](https://github.com/apache/superset/pull/41714): **Breaking — the legacy `explore_json` chart-data pipeline is removed** at its long-declared `5.0.0` EOL. The `/superset/explore_json/` and `/superset/explore_json/data/<cache_key>` endpoints, `superset/viz.py`, the `Slice.viz` property, the `get_viz` factory, the `load_explore_json_into_cache` celery task and the `viz=` overload of `security_manager.raise_for_access` are gone. Anything importing `superset.viz` must migrate to the QueryContext / `pandas_postprocessing` pipeline behind `/api/v1/chart/data`. All 15 remaining legacy charts were migrated first: most keep their `viz_type` and renderer (no action needed for saved charts), while saved nvd3 Bubble charts are auto-migrated to the ECharts Bubble Chart (`bubble_v2`) and saved "Time-series Percent Change" (`compare`) charts to the ECharts Line Chart — the nvd3 renderer's interactive percent re-basing is not preserved. The deck.gl Multiple Layers chart now fetches its layers entirely client-side; its initial autozoom falls back to the saved viewport, and dashboard filter badges no longer aggregate child-layer filter metadata. Review Comment: This release note still says percent re-basing is not preserved and deck_multi autozoom falls back to the saved viewport, but both limitations were restored later in this branch. Could this describe the current behavior so operators are not warned about regressions that no longer exist? ########## superset/migrations/shared/migrate_viz/base.py: ########## @@ -185,14 +204,25 @@ def upgrade_slice(cls, slc: Slice) -> None: def downgrade_slice(cls, slc: Slice) -> None: try: form_data = try_load_json(slc.params) - if "viz_type" in ( - form_data_bak := form_data.get(FORM_DATA_BAK_FIELD_NAME, {}) + form_data_bak = form_data.get(FORM_DATA_BAK_FIELD_NAME, {}) + if ( + "viz_type" in form_data_bak + and form_data_bak["viz_type"] == cls.source_viz_type ): slc.params = json.dumps(form_data_bak) slc.viz_type = form_data_bak.get("viz_type") query_context = try_load_json(slc.query_context) queries_bak = form_data.get(QUERIES_BAK_FIELD_NAME, {}) - if queries_bak: + if ( + isinstance(queries_bak, dict) + and FULL_CONTEXT_BAK_KEY in queries_bak + ): + # The original context had no "queries" key, so it was + # backed up wholesale on upgrade -- restore it verbatim + # rather than patching "queries" onto the upgraded + # context. + slc.query_context = json.dumps(queries_bak[FULL_CONTEXT_BAK_KEY]) Review Comment: An original query context with `"queries": []` is backed up as an empty list, so this truthiness check falls through and sets `query_context = None` on downgrade, losing the stored datasource and form data. Could this distinguish `None` from an empty query list and add an empty-list round-trip test? -- 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]
