This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch viz-pipeline-followups in repository https://gitbox.apache.org/repos/asf/superset.git
commit 8bd085613fe691e38b5b5298c4ffbe8b0f940473 Author: Claude Code <[email protected]> AuthorDate: Tue Jul 28 00:44:06 2026 -0700 fix(migrations): scope downgrade to the migration that actually upgraded a slice Two related bugs found in a post-merge self-review, both stemming from multiple MigrateViz subclasses sharing one target_viz_type (e.g. both MigrateLineChart and MigrateCompareChart migrate onto echarts_timeseries_line): - MigrateViz.downgrade()'s SQL filter (target_viz_type + a form_data_bak marker) is coarse by necessity and matches slices from ANY subclass sharing that target. Running e.g. `compare`'s downgrade would therefore also revert already-settled `line`-sourced slices back to the legacy `line` viz_type -- a plugin this same PR deletes. Each row's own form_data_bak was still correct (no data corruption), but the migration wasn't independently revertible the way its name implies. Fixed by checking form_data_bak["viz_type"] == cls.source_viz_type in downgrade_slice() before touching a row -- the backup already records which migration produced it, no new field needed. - The `superset viz-migrations downgrade --id` CLI path had the same root cause one level up: PREVIOUS_VERSION was keyed by target_viz_type, so building it from MIGRATIONS silently let the last-registered subclass for a shared target win the dict, and `migrate_by_id` would invoke the wrong class's downgrade_slice for any chart from the losing subclass. With the fix above, that would have started silently no-op'ing instead of misfiring, which just relocated the bug. Removed PREVIOUS_VERSION entirely and look the migration up by the slice's own backed-up source viz_type via MIGRATIONS (already correctly keyed 1:1 by source type). Added a regression test exercising the exact MigrateLineChart / MigrateCompareChart collision. Co-Authored-By: Claude Sonnet 5 <[email protected]> --- superset/cli/viz_migrations.py | 21 +++++--- superset/migrations/shared/migrate_viz/base.py | 13 ++++- .../migrations/viz/downgrade_scope_test.py | 56 ++++++++++++++++++++++ 3 files changed, 82 insertions(+), 8 deletions(-) diff --git a/superset/cli/viz_migrations.py b/superset/cli/viz_migrations.py index 793142a425c..1240b6df9c9 100644 --- a/superset/cli/viz_migrations.py +++ b/superset/cli/viz_migrations.py @@ -26,6 +26,7 @@ from flask.cli import with_appcontext from superset import db from superset.migrations.shared.migrate_viz.base import ( + FORM_DATA_BAK_FIELD_NAME, MigrateViz, Slice, ) @@ -44,7 +45,7 @@ from superset.migrations.shared.migrate_viz.processors import ( MigrateSunburst, MigrateTreeMap, ) -from superset.migrations.shared.utils import paginated_update +from superset.migrations.shared.utils import paginated_update, try_load_json class VizType(str, Enum): @@ -79,10 +80,6 @@ MIGRATIONS: dict[VizType, Type[MigrateViz]] = { VizType.TREEMAP: MigrateTreeMap, } -PREVIOUS_VERSION = { - migration.target_viz_type: migration for migration in MIGRATIONS.values() -} - @click.group() def migrate_viz() -> None: @@ -174,7 +171,19 @@ def migrate_by_id(ids: tuple[int, ...], is_downgrade: bool = False) -> None: ), ): if is_downgrade: - PREVIOUS_VERSION[slc.viz_type].downgrade_slice(slc) + # Look up the migration by the slice's OWN backed-up original + # viz_type rather than its current one: several source viz types + # can migrate onto the same target (e.g. both `line` and + # `compare` migrate onto `echarts_timeseries_line`), so a lookup + # keyed by the current viz_type can't tell which migration + # actually produced this slice's backup. + form_data = try_load_json(slc.params) + source_viz_type = form_data.get(FORM_DATA_BAK_FIELD_NAME, {}).get( + "viz_type" + ) + migration = MIGRATIONS.get(source_viz_type) if source_viz_type else None + if migration: + migration.downgrade_slice(slc) elif slc.viz_type in MIGRATIONS: MIGRATIONS[slc.viz_type].upgrade_slice(slc) diff --git a/superset/migrations/shared/migrate_viz/base.py b/superset/migrations/shared/migrate_viz/base.py index 1c9ba255a60..86e97a99490 100644 --- a/superset/migrations/shared/migrate_viz/base.py +++ b/superset/migrations/shared/migrate_viz/base.py @@ -185,8 +185,10 @@ class MigrateViz: 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") @@ -214,6 +216,13 @@ class MigrateViz: @classmethod def downgrade(cls, session: Session) -> None: + # This SQL-level filter is intentionally coarse: several MigrateViz + # subclasses can share one target_viz_type (e.g. MigrateLineChart and + # MigrateCompareChart both migrate onto echarts_timeseries_line), so + # it will also match slices another subclass upgraded. downgrade_slice + # does the precise per-row check (form_data_bak["viz_type"] == + # cls.source_viz_type) so this class's downgrade only reverts the + # slices it upgraded, not every slice currently at the same target. slices = session.query(Slice).filter( and_( Slice.viz_type == cls.target_viz_type, diff --git a/tests/unit_tests/migrations/viz/downgrade_scope_test.py b/tests/unit_tests/migrations/viz/downgrade_scope_test.py new file mode 100644 index 00000000000..934e5a44542 --- /dev/null +++ b/tests/unit_tests/migrations/viz/downgrade_scope_test.py @@ -0,0 +1,56 @@ +# 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. +from superset.migrations.shared.migrate_viz import MigrateCompareChart, MigrateLineChart +from superset.models.slice import Slice +from superset.utils import json + + +def test_downgrade_slice_ignores_a_slice_from_a_different_source_migration() -> None: + """MigrateCompareChart and MigrateLineChart both migrate onto + echarts_timeseries_line, so `downgrade()`'s SQL filter (target_viz_type + + a form_data_bak marker) matches slices from either migration. Calling + the wrong class's `downgrade_slice` on a slice it didn't upgrade must be + a no-op rather than reverting it to the wrong source viz_type.""" + line_source = { + "viz_type": "line", + "datasource": "1__table", + "x_axis_label": "x", + } + + slc = Slice( + viz_type="line", + datasource_type="table", + params=json.dumps(line_source), + query_context=f'{{"form_data": {json.dumps(line_source)}, "queries": []}}', + ) + MigrateLineChart.upgrade_slice(slc) + assert slc.viz_type == "echarts_timeseries_line" + upgraded_params = json.loads(slc.params) + + # MigrateCompareChart's downgrade must not touch a slice that + # MigrateLineChart (not MigrateCompareChart) upgraded, even though both + # target echarts_timeseries_line and the slice's params contain a + # form_data_bak key. + MigrateCompareChart.downgrade_slice(slc) + + assert slc.viz_type == "echarts_timeseries_line" + assert json.loads(slc.params) == upgraded_params + + # The matching migration still downgrades it correctly. + MigrateLineChart.downgrade_slice(slc) + assert slc.viz_type == "line" + assert json.loads(slc.params) == line_source
