You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by hu...@apache.org on 2026/10/21 08:18:15 UTC
(superset) branch hughhhh/pfm-lifecycle-boundaries updated (1764827c2b3 -> ebe661af595)
This is an automated email from the ASF dual-hosted git repository.
hughhhh pushed a change to branch hughhhh/pfm-lifecycle-boundaries
in repository https://gitbox.apache.org/repos/asf/superset.git
from 1764827c2b3 fix(samples): skip the unbounded COUNT(*) on partitioned datasets
add f4b48deb2c0 refactor(datasets): move the ungated sync and fieldset fixes to their own PRs
add 0f376ba9f97 fix(semantic-layers): show per-view errors when adding semantic views (#45524)
add 190873e9832 fix(version-history): preview banner, history panel layout, scope labels and current-version marking (#45471)
add 6dc0641c91a fix(reports): keep the first column header in XLSX exports of semantic-view tables (#45527)
add 4e7e2b7390d fix(tests): narrow Optional StatementResult.data in async result tests (#45517)
add 836c0f3f95d fix(table): reset default margins on the pagination list (#45365)
add 8d3ade7e0a8 test(trino): close db_engine_spec test gaps (#45361)
add 3d902d9eb3a chore(deps): bump the security group across 2 directories with 2 updates (#45542)
add 527021d5819 chore(deps): bump react-virtualized-auto-sizer from 1.0.26 to 2.0.3 in /superset-frontend (#45197)
add c11f993af37 fix(auth): close forced-password-change redirect loop and sweep stale legacy password perms from all roles (#44857)
add 7d0e1c939ee fix(frontend): retry a failed lazy import before surfacing ChunkLoadError (#43215)
add abc1b50f65d fix(mcp): clean up nits from PNG chart preview review (#45509)
add f597190d432 Merge branch 'master' into hughhhh/pfm-6-explore-indicator
new ebe661af595 fix(datasets): keep partition mappings intact across renames, exports and flag flips
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
UPDATING.md | 45 +-
docs/yarn.lock | 6 +-
superset-embedded-sdk/package-lock.json | 26 +-
superset-frontend/package-lock.json | 327 ++----
superset-frontend/package.json | 2 +-
.../dashboard/version-history-placement.spec.ts | 39 +-
.../tests/version-history/activity-log.spec.ts | 14 +-
.../plugins/plugin-chart-table/src/Styles.tsx | 4 +
.../plugin-chart-table/test/TableChart.test.tsx | 73 ++
.../SqlLab/components/ResultSet/ResultSet.test.tsx | 13 +-
.../src/SqlLab/components/ResultSet/index.tsx | 10 +-
.../SqlLab/components/SqlEditor/SqlEditor.test.tsx | 17 +-
.../src/SqlLab/components/SqlEditor/index.tsx | 10 +-
.../TableExploreTree/TableExploreTree.test.tsx | 13 +-
.../SqlLab/components/TableExploreTree/index.tsx | 10 +-
.../components/TablePreview/TablePreview.test.tsx | 13 +-
.../src/SqlLab/components/TablePreview/index.tsx | 8 +-
.../DatasourceModal/DatasourceModal.test.tsx | 64 --
.../Datasource/DatasourceModal/index.tsx | 15 +-
.../FoldersEditor/FoldersEditor.test.tsx | 17 +-
.../components/Datasource/FoldersEditor/index.tsx | 12 +-
.../CollectionTable/CollectionTable.test.tsx | 112 +--
.../components/CollectionTable/index.tsx | 83 +-
.../DashboardBuilder/DashboardBuilder.test.tsx | 137 ++-
.../DashboardBuilder/DashboardBuilder.tsx | 155 +--
.../DashboardBuilder/DashboardWrapper.tsx | 2 +-
.../src/dashboard/components/SliceAdder.test.tsx | 2 +-
.../src/dashboard/components/SliceAdder.tsx | 8 +-
.../src/dashboard/containers/DashboardPage.tsx | 5 +-
superset-frontend/src/embedded/index.tsx | 5 +-
.../DatasourcePanel.real-autosizer.test.tsx | 7 +-
.../DatasourcePanel/DatasourcePanel.test.tsx | 13 +-
.../explore/components/DatasourcePanel/index.tsx | 10 +-
.../components/ExploreViewContainer/index.tsx | 6 +-
.../components/controls/JSEditorControl.test.tsx | 9 +-
.../components/controls/JSEditorControl.tsx | 10 +-
.../semanticViews/AddSemanticViewModal.test.tsx | 251 ++++-
.../semanticViews/AddSemanticViewModal.tsx | 121 ++-
.../src/features/semanticViews/creationResult.ts | 90 ++
.../src/features/versionHistory/ActionRow.tsx | 5 +-
.../versionHistory/CurrentVersionSection.tsx | 12 +-
.../DashboardVersionHistory.test.tsx | 4 +-
.../features/versionHistory/SaveGroupItem.test.tsx | 88 +-
.../src/features/versionHistory/SaveGroupItem.tsx | 66 +-
.../versionHistory/VersionHistoryPanel.test.tsx | 107 +-
.../versionHistory/VersionHistoryPanel.tsx | 53 +-
.../src/features/versionHistory/display.test.ts | 28 +
.../src/features/versionHistory/display.ts | 28 +
.../versionHistory/useVersionActivity.test.ts | 4 +-
superset-frontend/src/utils/lazyWithRetry.test.tsx | 140 +++
superset-frontend/src/utils/lazyWithRetry.ts | 135 +++
superset-frontend/src/views/routes.tsx | 83 +-
superset/charts/client_processing.py | 41 +-
superset/commands/dataset/export.py | 26 +-
superset/commands/dataset/importers/v1/utils.py | 64 +-
superset/commands/dataset/update.py | 89 +-
superset/connectors/sqla/models.py | 11 +-
superset/connectors/sqla/partition_mapping.py | 49 +
superset/daos/dataset.py | 7 +
.../mcp_service/chart/tool/get_chart_preview.py | 10 +-
superset/mcp_service/middleware.py | 29 +-
superset/mcp_service/utils/response_size_utils.py | 4 +
...ep_legacy_password_view_perms_from_all_roles.py | 120 +++
superset/security/password_change.py | 101 +-
superset/translations/messages.pot | 15 +
superset/views/datasource/views.py | 33 +-
tests/unit_tests/charts/test_client_processing.py | 188 ++++
tests/unit_tests/commands/dataset/update_test.py | 119 ++-
tests/unit_tests/connectors/sqla/models_test.py | 41 -
tests/unit_tests/dao/dataset_test.py | 118 +++
tests/unit_tests/datasets/commands/export_test.py | 57 +-
.../datasets/commands/importers/v1/import_test.py | 49 +-
tests/unit_tests/db_engine_specs/test_trino.py | 1052 +++++++++++++++++++-
.../chart/tool/test_get_chart_preview.py | 40 +
.../test_sweep_legacy_password_view_perms.py | 190 ++++
tests/unit_tests/security/test_password_change.py | 63 ++
.../unit_tests/security/test_password_recovery.py | 12 +-
.../unit_tests/sql/execution/test_async_results.py | 2 +
tests/unit_tests/views/datasource/views_test.py | 66 ++
79 files changed, 4048 insertions(+), 1065 deletions(-)
create mode 100644 superset-frontend/src/features/semanticViews/creationResult.ts
create mode 100644 superset-frontend/src/utils/lazyWithRetry.test.tsx
create mode 100644 superset-frontend/src/utils/lazyWithRetry.ts
create mode 100644 superset/migrations/versions/2026-09-30_00-00_d623a0cb6bb0_sweep_legacy_password_view_perms_from_all_roles.py
create mode 100644 tests/unit_tests/migrations/test_sweep_legacy_password_view_perms.py
(superset) 01/01: fix(datasets): keep partition mappings intact across renames, exports and flag flips
Posted by hu...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
hughhhh pushed a commit to branch hughhhh/pfm-lifecycle-boundaries
in repository https://gitbox.apache.org/repos/asf/superset.git
commit ebe661af595390853d0fc568154f17ae40997eeb
Author: Hugh A Miles II <hu...@gmail.com>
AuthorDate: Wed Oct 21 09:15:52 2026 +0100
fix(datasets): keep partition mappings intact across renames, exports and flag flips
Three lifecycle boundaries raised in review of #43870:
- Renames: `partition_column` / `partition_mapped_column` store column names,
while the upsert path renames a column in place by id. Unfollowed, renaming
the partition column dropped the whole mapping and renaming the mapped column
re-pointed it at `main_dttm_col` and deleted the transform. The references now
follow an id-preserving rename when the request omits them or echoes the
stored name (what the dataset editor sends), in both the DAO and the update
command's validation.
- Export compatibility: the four mapping fields are left out of an export while
they hold their defaults, so a dataset that does not use the feature exports a
bundle older Superset import schemas accept. The v1 importer reads an omitted
field as its default, so an overwrite still clears a mapping the bundle lacks.
- Flag-off writes: storing a mapping is validated with the flag on or off (the
REST PUT, the legacy datasource save and the importer's blocking transform
checks), and a save is refused only over a problem the request introduces,
not one the stored mapping already has, so an unrelated edit to a dataset
stored broken no longer fails.
Co-Authored-By: Claude Opus 5.5 <no...@anthropic.com>
---
superset/commands/dataset/export.py | 26 ++++-
superset/commands/dataset/importers/v1/utils.py | 64 ++++++-----
superset/commands/dataset/update.py | 89 +++++++++++++--
superset/connectors/sqla/partition_mapping.py | 49 +++++++++
superset/daos/dataset.py | 7 ++
superset/views/datasource/views.py | 33 ++++--
tests/unit_tests/commands/dataset/update_test.py | 119 +++++++++++++++++++--
tests/unit_tests/dao/dataset_test.py | 118 ++++++++++++++++++++
tests/unit_tests/datasets/commands/export_test.py | 57 +++++++---
.../datasets/commands/importers/v1/import_test.py | 49 +++++++--
tests/unit_tests/views/datasource/views_test.py | 66 ++++++++++++
11 files changed, 604 insertions(+), 73 deletions(-)
diff --git a/superset/commands/dataset/export.py b/superset/commands/dataset/export.py
index 26448f194d5..97a89de6b97 100644
--- a/superset/commands/dataset/export.py
+++ b/superset/commands/dataset/export.py
@@ -18,7 +18,7 @@
import logging
from collections.abc import Iterator
-from typing import Callable
+from typing import Any, Callable
import yaml
@@ -36,6 +36,28 @@ logger = logging.getLogger(__name__)
JSON_KEYS = {"params", "template_params", "extra"}
+def _omit_default_partition_fields(payload: dict[str, Any]) -> None:
+ """
+ Leave the partition mapping fields out while they hold their defaults.
+
+ So a dataset that does not use partition filter mapping exports a bundle an
+ older Superset can import: its schemas refuse a field they do not know. The
+ importer reads an omitted field as its default, so nothing is lost.
+
+ The monotonic declaration describes the transform, so without one it is
+ left out whatever it says.
+ """
+ for key in ("partition_column", "partition_mapped_column"):
+ if payload.get(key) is None:
+ payload.pop(key, None)
+ for attributes in payload.get("columns", []):
+ if attributes.get("partition_value_transform") is None:
+ attributes.pop("partition_value_transform", None)
+ attributes.pop("partition_transform_is_monotonic", None)
+ elif not attributes.get("partition_transform_is_monotonic"):
+ attributes.pop("partition_transform_is_monotonic", None)
+
+
class ExportDatasetsCommand(ExportModelsCommand):
dao = DatasetDAO
not_found = DatasetNotFoundError
@@ -74,6 +96,8 @@ class ExportDatasetsCommand(ExportModelsCommand):
"Unable to decode `extra` field: %s", attributes["extra"]
)
+ _omit_default_partition_fields(payload)
+
payload["version"] = EXPORT_VERSION
payload["database_uuid"] = str(model.database.uuid)
diff --git a/superset/commands/dataset/importers/v1/utils.py b/superset/commands/dataset/importers/v1/utils.py
index efa26c6d9e9..7c756f0398f 100644
--- a/superset/commands/dataset/importers/v1/utils.py
+++ b/superset/commands/dataset/importers/v1/utils.py
@@ -35,7 +35,7 @@ from sqlalchemy import BigInteger, Boolean, Date, DateTime, Float, String, Text
from sqlalchemy.exc import MultipleResultsFound
from sqlalchemy.types import TypeEngine
-from superset import db, is_feature_enabled, security_manager
+from superset import db, security_manager
from superset.commands.dataset.exceptions import (
DatasetAccessDeniedError,
DatasetForbiddenDataURI,
@@ -52,7 +52,6 @@ from superset.commands.importers.v1.utils import (
from superset.connectors.sqla.models import SqlaTable
from superset.connectors.sqla.partition_mapping import (
drop_unmapped_value_transforms,
- FEATURE_FLAG as PARTITION_FILTER_MAPPING,
is_unfinished,
stored_expression_error,
validate_transform,
@@ -420,6 +419,25 @@ def _default_omitted_monotonic_declarations(columns: list[dict[str, Any]]) -> No
column.setdefault("partition_transform_is_monotonic", False)
+def _default_omitted_partition_fields(config: dict[str, Any]) -> None:
+ """
+ Read an omitted partition mapping field as its default.
+
+ `ExportDatasetsCommand` leaves these fields out of a bundle while they hold
+ their defaults, so that a dataset that does not use the feature exports a
+ bundle an older Superset -- whose import schemas refuse a field they do not
+ know -- can still import. Omitted therefore has to mean "the default", not
+ "keep what is stored": the import applies the raw config, so on an overwrite
+ an omitted key would otherwise keep a mapping the source dataset does not
+ have.
+ """
+ config.setdefault("partition_column", None)
+ config.setdefault("partition_mapped_column", None)
+ for column in config.get("columns") or []:
+ column.setdefault("partition_value_transform", None)
+ column.setdefault("partition_transform_is_monotonic", False)
+
+
def drop_unusable_partition_transforms(
config: dict[str, Any], database: Database | None = None
) -> None:
@@ -439,14 +457,12 @@ def drop_unusable_partition_transforms(
`DatasetDAO.clear_dangling_partition_mapping` already strikes for a mapping
whose column went away.
- Two kinds of check, and only one of them answers to the feature flag. The
- usability checks ask whether a transform will ever mirror a filter, which
- is a question about a live feature; with the flag off nothing mirrors, so
- rewriting the bundle would discard configuration for no gain. The
- structural check asks whether the expression is one this product stores at
- all, and that answer does not change with a flag -- an import written while
- the flag was off would otherwise sit in the metadata DB fully armed,
- waiting for an operator to turn the flag on.
+ Neither check answers to the feature flag. The structural check asks
+ whether the expression is one this product stores at all; the blocking
+ tier of `validate_transform` asks the same question `UpdateDatasetCommand`
+ asks of every save, flag on or off. Skipped while the flag was off, a
+ transform no save would accept was imported, stored, and waiting for an
+ operator to turn the flag on.
``database`` is accepted already resolved for the v0 importer, whose bundles
nest a dataset under the connection being imported and so carry no
@@ -467,7 +483,6 @@ def drop_unusable_partition_transforms(
if database is None:
return
- check_usability = is_feature_enabled(PARTITION_FILTER_MAPPING)
catalog = config.get("catalog")
schema = config.get("schema")
@@ -489,12 +504,11 @@ def drop_unusable_partition_transforms(
if not is_unfinished(transform, database.backend):
if reason := stored_expression_error(database, catalog, schema, transform):
reasons.append(reason)
- if check_usability:
- reasons.extend(
- str(issue.message)
- for issue in validate_transform(transform, database.backend)
- if issue.blocking
- )
+ reasons.extend(
+ str(issue.message)
+ for issue in validate_transform(transform, database.backend)
+ if issue.blocking
+ )
if reasons:
logger.warning(
@@ -761,6 +775,7 @@ def import_dataset( # noqa: C901
)
attributes["extra"] = None
+ _default_omitted_partition_fields(config)
drop_unusable_partition_transforms(config)
# And the mapping's one-transform invariant, against the columns this bundle
# writes. `DatasetDAO.clear_unmapped_partition_transforms` runs on both
@@ -772,18 +787,13 @@ def import_dataset( # noqa: C901
# same bargain the sanitizer above strikes and for the same reason: a bundle
# is imported whole.
#
- # Each reference falls back to the stored dataset, because an overwrite
- # keeps whatever the bundle omits -- so a bundle that says nothing about
- # `partition_column` is still mapping onto the stored one.
+ # The mapping's own references are always in `config` by now, but
+ # `main_dttm_col` falls back to the stored dataset, because an overwrite
+ # keeps whatever the bundle omits.
if cleared := drop_unmapped_value_transforms(
config.get("columns"),
- partition_column=config.get(
- "partition_column", getattr(existing, "partition_column", None)
- ),
- partition_mapped_column=config.get(
- "partition_mapped_column",
- getattr(existing, "partition_mapped_column", None),
- ),
+ partition_column=config["partition_column"],
+ partition_mapped_column=config["partition_mapped_column"],
main_dttm_col=config.get(
"main_dttm_col", getattr(existing, "main_dttm_col", None)
),
diff --git a/superset/commands/dataset/update.py b/superset/commands/dataset/update.py
index beca6c7ee80..7962ceca187 100644
--- a/superset/commands/dataset/update.py
+++ b/superset/commands/dataset/update.py
@@ -51,6 +51,7 @@ from superset.commands.dataset.exceptions import (
from superset.commands.utils import compute_subjects, raise_if_managed_externally
from superset.connectors.sqla.models import SqlaTable, validate_stored_expression
from superset.connectors.sqla.partition_mapping import (
+ follow_partition_column_renames,
is_unfinished,
stored_expression_error,
validate_partition_mapping,
@@ -518,16 +519,28 @@ class UpdateDatasetCommand(UpdateMixin, BaseCommand):
query -- and it is not a hole for templating, because Jinja in a
transform is already a Tier-1 blocking issue of its own.
- Every path that *reads* a mapping is gated on the feature flag, so this
- one is too: with the flag off nothing mirrors, and rejecting a save over
- a mapping that can never be consumed would be a validation error the
- owner has no way to act on.
+ Not gated on the feature flag. Reading a mapping is, but storing one is
+ not: with this check skipped while the flag was off, a PUT could store a
+ reference to a column the dataset does not have, or a transform no
+ flag-on save would accept, and nothing complained until the flag went on.
+
+ And only what *this request* gets wrong is refused. A problem the stored
+ mapping already has -- written before this check, or by a path that
+ never ran it -- is not the request's doing, and refusing the request
+ over it made every later save fail, including a description-only one
+ that says nothing about the mapping and gives the owner no field to fix.
+ That is also what keeps the ungated check harmless on a deployment that
+ never turns the flag on: an editor that echoes the stored mapping back
+ unchanged never trips it.
"""
- if not is_feature_enabled("PARTITION_FILTER_MAPPING"):
- return
-
self._model = cast(SqlaTable, self._model)
+ # Before anything below reads the names: a rename the mapping has not
+ # followed reads as a reference to a column that is gone.
+ self._properties = follow_partition_column_renames(
+ self._model, self._properties
+ )
+
columns = self._properties.get("columns")
column_names = (
{column["column_name"] for column in columns}
@@ -576,6 +589,7 @@ class UpdateDatasetCommand(UpdateMixin, BaseCommand):
effective_mapped_column = partition_mapped_column or main_dttm_col
transform = self._effective_transform(columns, effective_mapped_column)
+ stored_issues = self._stored_mapping_issues(database.backend)
for issue in validate_partition_mapping(
column_names=column_names,
partition_column=partition_column,
@@ -584,7 +598,9 @@ class UpdateDatasetCommand(UpdateMixin, BaseCommand):
transform=transform,
engine=database.backend,
):
- if issue.blocking:
+ if issue.blocking and (issue.field, str(issue.message)) not in (
+ stored_issues
+ ):
exceptions.append(
ValidationError(str(issue.message), field_name=issue.field)
)
@@ -595,7 +611,11 @@ class UpdateDatasetCommand(UpdateMixin, BaseCommand):
# parse, so asking it would refuse the save mid-keystroke. Narrower
# than "parses as a single expression": a set-operation or a
# multi-statement transform fails that too, and has to reach this gate.
- if transform and not is_unfinished(transform, database.backend):
+ if (
+ transform
+ and not self._is_stored_transform(transform, database, catalog, schema)
+ and not is_unfinished(transform, database.backend)
+ ):
# `stored_expression_error` rather than `validate_stored_expression`
# alone, which it already includes: it is the gate preview, import
# and the legacy save path all use, and running a narrower one here
@@ -608,6 +628,57 @@ class UpdateDatasetCommand(UpdateMixin, BaseCommand):
ValidationError(reason, field_name="partition_value_transform")
)
+ def _stored_mapping_issues(self, engine: str) -> set[tuple[str, str]]:
+ """
+ The blocking issues the stored mapping already has, as (field, message).
+
+ What `_validate_partition_mapping` forgives: an issue that is here too
+ was not introduced by the request, so refusing the request over it
+ would only make the dataset unsaveable.
+ """
+ model = cast(SqlaTable, self._model)
+ if not model.partition_column:
+ return set()
+ return {
+ (issue.field, str(issue.message))
+ for issue in validate_partition_mapping(
+ column_names={column.column_name for column in model.columns},
+ partition_column=model.partition_column,
+ partition_mapped_column=model.partition_mapped_column,
+ main_dttm_col=model.main_dttm_col,
+ transform=self._effective_transform(
+ None, model.partition_mapped_column or model.main_dttm_col
+ ),
+ engine=engine,
+ )
+ if issue.blocking
+ }
+
+ def _is_stored_transform(
+ self,
+ transform: str,
+ database: Database,
+ catalog: str | None,
+ schema: str | None,
+ ) -> bool:
+ """
+ Whether `transform` is the stored one, against the same source.
+
+ The expression gate depends on where the transform runs as well as on
+ its text, so moving the dataset re-asks it even for an unchanged
+ transform.
+ """
+ model = cast(SqlaTable, self._model)
+ stored = self._effective_transform(
+ None, model.partition_mapped_column or model.main_dttm_col
+ )
+ return (
+ transform == stored
+ and database.id == model.database_id
+ and catalog == model.catalog
+ and schema == model.schema
+ )
+
def _effective_transform(
self,
columns: list[dict[str, Any]] | None,
diff --git a/superset/connectors/sqla/partition_mapping.py b/superset/connectors/sqla/partition_mapping.py
index bf2335877f4..309e1ab8e6c 100644
--- a/superset/connectors/sqla/partition_mapping.py
+++ b/superset/connectors/sqla/partition_mapping.py
@@ -2374,6 +2374,55 @@ def validate_partition_mapping( # pylint: disable=too-many-arguments
return issues
+def follow_partition_column_renames(
+ model: SqlaTable, attributes: dict[str, Any]
+) -> dict[str, Any]:
+ """
+ Point the partition mapping at the new name of a column renamed in place.
+
+ `partition_column` and `partition_mapped_column` store column *names*,
+ while `DatasetDAO._upsert_columns` identifies a column by its primary key
+ and lets the same request rename it. Without this, renaming either end of
+ the mapping reads as deleting it: `DatasetDAO.clear_dangling_partition_mapping`
+ drops the whole mapping for the partition column, and for the mapped column
+ it drops the override, which re-points the mapping at `main_dttm_col` and
+ takes the renamed column's transform with it.
+
+ A reference is followed when the request leaves it alone -- either by
+ omitting it or by echoing the stored name back, which is what the
+ dataset editor does on every save. A request that names a different
+ column has said what it wants, and is left to say it.
+
+ Not for `override_columns=true`, which matches columns by name: there a
+ rename is a different column, and the mapping going with the old one is
+ correct.
+
+ Returns a new dict rather than mutating the caller's, and is idempotent:
+ once followed, the reference no longer matches the stored name.
+ """
+ columns = attributes.get("columns")
+ if not columns or attributes.get("override_columns"):
+ return attributes
+
+ names_by_id = {column.id: column.column_name for column in model.columns}
+ renames = {
+ names_by_id[column["id"]]: column["column_name"]
+ for column in columns
+ if column.get("id") in names_by_id
+ and column.get("column_name")
+ and column["column_name"] != names_by_id[column["id"]]
+ }
+ if not renames:
+ return attributes
+
+ followed = dict(attributes)
+ for field in ("partition_column", "partition_mapped_column"):
+ stored = getattr(model, field)
+ if stored in renames and followed.get(field, stored) == stored:
+ followed[field] = renames[stored]
+ return followed
+
+
def drop_unmapped_value_transforms(
columns: list[dict[str, Any]] | None,
*,
diff --git a/superset/daos/dataset.py b/superset/daos/dataset.py
index d241806298a..1c0d9018ddf 100644
--- a/superset/daos/dataset.py
+++ b/superset/daos/dataset.py
@@ -35,6 +35,7 @@ from superset.connectors.sqla.models import (
from superset.connectors.sqla.partition_mapping import (
drop_unmapped_value_transforms,
FEATURE_FLAG as PARTITION_FILTER_MAPPING_FLAG,
+ follow_partition_column_renames,
)
from superset.constants import EPOCH_FORMATS
from superset.daos.base import BaseDAO, ColumnOperator, ColumnOperatorEnum
@@ -452,6 +453,12 @@ class DatasetDAO(BaseDAO[SqlaTable]):
delete_metric_ids removes only explicitly selected metrics, without
replacing or replaying the remaining metrics.
"""
+ # Before anything reads the mapping: every pass below compares the
+ # mapping's names against the surviving columns, so a rename it has not
+ # followed looks exactly like a deletion.
+ if item is not None and attributes:
+ attributes = follow_partition_column_renames(item, attributes)
+
# Snapshot the keys: the column and metric branches below `pop` theirs
# out of `attributes`, so by the time the partition-mapping cleanup runs
# the dict no longer says what the request contained.
diff --git a/superset/views/datasource/views.py b/superset/views/datasource/views.py
index eb5205f82df..1c54ee2e931 100644
--- a/superset/views/datasource/views.py
+++ b/superset/views/datasource/views.py
@@ -191,13 +191,13 @@ def _partition_mapping_reference_error(
design in `validate_partition_mapping`, and refusing it here would recreate
exactly the unsaveable dataset this function exists to prevent.
- Gated on the feature flag, like `UpdateDatasetCommand`'s own call: with the
- flag off nothing mirrors, and a deployment that never turned the feature on
- should not start failing saves over a reference nothing reads.
+ Not gated on the feature flag, and only for what this request breaks --
+ the bargain `UpdateDatasetCommand._validate_partition_mapping` strikes, and
+ for its reasons. Gated, a flag-off save stored any reference and the first
+ flag-on save refused it; refusing a reference that was already broken in
+ storage meant this endpoint's own GET-then-POST client could not save the
+ dataset at all, whatever it changed.
"""
- if not is_feature_enabled("PARTITION_FILTER_MAPPING"):
- return None
-
partition_column = datasource_dict.get("partition_column")
if not partition_column:
return None
@@ -214,6 +214,25 @@ def _partition_mapping_reference_error(
for column in datasource_dict.get("columns") or []
if column.get("column_name")
}
+ stored_issues = (
+ {
+ (issue.field, str(issue.message))
+ for issue in validate_partition_mapping(
+ column_names={
+ column.column_name
+ for column in getattr(orm_datasource, "columns", None) or []
+ },
+ partition_column=orm_datasource.partition_column,
+ partition_mapped_column=orm_datasource.partition_mapped_column,
+ main_dttm_col=orm_datasource.main_dttm_col,
+ transform=None,
+ engine=database.backend,
+ )
+ if issue.blocking
+ }
+ if getattr(orm_datasource, "partition_column", None)
+ else set()
+ )
for issue in validate_partition_mapping(
column_names=column_names,
partition_column=partition_column,
@@ -222,7 +241,7 @@ def _partition_mapping_reference_error(
transform=None,
engine=database.backend,
):
- if issue.blocking:
+ if issue.blocking and (issue.field, str(issue.message)) not in stored_issues:
return str(issue.message)
return None
diff --git a/tests/unit_tests/commands/dataset/update_test.py b/tests/unit_tests/commands/dataset/update_test.py
index f907bb6dc9e..abf939e99f3 100644
--- a/tests/unit_tests/commands/dataset/update_test.py
+++ b/tests/unit_tests/commands/dataset/update_test.py
@@ -1985,7 +1985,14 @@ def test_a_jinja_transform_is_still_rejected(mocker: MockerFixture) -> None:
Jinja is a blocking issue of its own, reported before the gate is reached.
"""
mocker.patch("superset.commands.dataset.update.validate_stored_expression")
- command = _mapping_command(mocker, "unix_timestamp({{ current_user_id() }})")
+ command = _mapping_command(mocker, "unix_timestamp(:value)")
+ command._properties["columns"] = [
+ {
+ "column_name": "event_time",
+ "partition_value_transform": "unix_timestamp({{ current_user_id() }})",
+ },
+ {"column_name": "dt_epoch"},
+ ]
exceptions: list[ValidationError] = []
command._validate_partition_mapping(exceptions)
@@ -2036,24 +2043,22 @@ def test_a_self_mapping_is_rejected_while_the_feature_is_on(
@with_feature_flags(PARTITION_FILTER_MAPPING=False)
-def test_no_mapping_validation_runs_while_the_feature_is_off(
+def test_a_mapping_the_request_breaks_is_rejected_while_the_feature_is_off(
mocker: MockerFixture,
) -> None:
"""
- With the flag off nothing mirrors, so a stored mapping can never be
- consumed. Rejecting the save over it would hand the owner a validation
- error they have no way to act on -- and every path that reads a mapping is
- gated the same way.
+ Reading a mapping is gated on the flag; storing one is not. Skipped while
+ the flag was off, this let a PUT store a mapping no flag-on save accepts,
+ and the first flag-on save -- whatever it changed -- was refused over it.
"""
- gate = mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
command = _mapping_command(mocker, "unix_timestamp(:value)")
command._properties["partition_mapped_column"] = "dt_epoch"
exceptions: list[ValidationError] = []
command._validate_partition_mapping(exceptions)
- assert exceptions == []
- gate.assert_not_called()
+ assert [exc.field_name for exc in exceptions] == ["partition_column"]
@with_feature_flags(PARTITION_FILTER_MAPPING=True)
@@ -2132,3 +2137,99 @@ def test_a_self_mapping_is_still_rejected_alongside_a_column_payload(
command._validate_partition_mapping(exceptions)
assert [exc.field_name for exc in exceptions] == ["partition_column"]
+
+
+@pytest.mark.parametrize(
+ "stored",
+ [
+ {"partition_column": "no_such_column"},
+ {"partition_mapped_column": "no_such_column"},
+ {"transform": "rand() + 0 * :value"},
+ ],
+ ids=["dangling-partition", "dangling-mapped", "non-deterministic"],
+)
+@pytest.mark.parametrize(
+ "request_properties",
+ [{"description": "x"}, {"metrics": [{"metric_name": "cnt"}]}],
+ ids=["description-only", "metrics-only"],
+)
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_an_unrelated_save_survives_a_mapping_broken_in_storage(
+ mocker: MockerFixture,
+ stored: dict[str, str],
+ request_properties: dict[str, Any],
+) -> None:
+ """
+ A mapping stored broken -- while this check was flag-gated, or by a path
+ that never ran it -- is not the request's doing. Refusing the request over
+ it made every later save fail, including one that says nothing about the
+ mapping and gives the owner no field to fix.
+ """
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(
+ mocker,
+ stored.get("transform", "unix_timestamp(:value)"),
+ partition_mapped_column=stored.get("partition_mapped_column"),
+ )
+ if "partition_column" in stored:
+ cast(Any, command._model).partition_column = stored["partition_column"]
+ command._properties.update(request_properties)
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_new_problem_is_still_refused_alongside_a_stored_one(
+ mocker: MockerFixture,
+) -> None:
+ """Forgiving what storage already has does not forgive what the request adds."""
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(
+ mocker, "unix_timestamp(:value)", partition_mapped_column="no_such_column"
+ )
+ command._properties["partition_column"] = "also_missing"
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert [exc.field_name for exc in exceptions] == ["partition_column"]
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_the_editor_echo_after_a_rename_validates_against_the_new_name(
+ mocker: MockerFixture,
+) -> None:
+ """
+ The editor renames a column in place and sends the stored mapping back
+ unchanged. Validated as sent, the old name is not among the columns and the
+ save is refused; the reference has to follow the rename first.
+ """
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(
+ mocker, "unix_timestamp(:value)", partition_mapped_column="event_time"
+ )
+ mapped, partition = cast(Any, command._model).columns
+ mapped.id, partition.id = 10, 11
+ command._properties.update(
+ {
+ "partition_column": "dt_epoch",
+ "partition_mapped_column": "event_time",
+ "columns": [
+ {
+ "id": 10,
+ "column_name": "event_ts",
+ "partition_value_transform": "unix_timestamp(:value)",
+ },
+ {"id": 11, "column_name": "dt_epoch"},
+ ],
+ }
+ )
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+ assert command._properties["partition_mapped_column"] == "event_ts"
diff --git a/tests/unit_tests/dao/dataset_test.py b/tests/unit_tests/dao/dataset_test.py
index 46ce995d0db..373a9c28cb1 100644
--- a/tests/unit_tests/dao/dataset_test.py
+++ b/tests/unit_tests/dao/dataset_test.py
@@ -1166,3 +1166,121 @@ def test_a_transform_on_a_column_added_in_the_same_request_is_cleared(
}
assert by_name["region_key"].partition_value_transform is None
assert by_name["region_key"].partition_transform_is_monotonic is False
+
+
+def _renamable_mapping(session: Session, name: str) -> Any:
+ from superset import db
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.models.core import Database
+
+ SqlaTable.metadata.create_all(session.get_bind())
+ database = Database(database_name=f"{name}_db", sqlalchemy_uri="sqlite://")
+ table = SqlaTable(
+ table_name=name, schema="main", database=database, main_dttm_col="event_time"
+ )
+ table.columns = [
+ TableColumn(column_name="event_time"),
+ TableColumn(column_name="dt_part"),
+ TableColumn(column_name="country", partition_value_transform="lower(:value)"),
+ ]
+ table.partition_column = "dt_part"
+ table.partition_mapped_column = "country"
+ db.session.add_all([database, table])
+ db.session.flush()
+ return table
+
+
+@pytest.mark.parametrize("echo", [False, True], ids=["omitted", "echoed"])
+@pytest.mark.parametrize("renamed", ["dt_part", "country"])
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_rename_in_place_carries_the_mapping_with_it(
+ session: Session, renamed: str, echo: bool
+) -> None:
+ """
+ The mapping stores column names; the upsert path identifies a column by id
+ and lets the same request rename it. Unfollowed, renaming the partition
+ column dropped the whole mapping, and renaming the mapped column dropped the
+ override -- re-pointing the mapping at `main_dttm_col` and deleting the
+ renamed column's transform.
+
+ Followed whether the request omits the reference or echoes the stored name
+ back, which is what the dataset editor sends on every save.
+ """
+ from superset import db
+
+ table = _renamable_mapping(session, f"rn_{renamed}_{echo}")
+ ids = {c.column_name: c.id for c in table.columns}
+ attributes: dict[str, Any] = {
+ "columns": [
+ {
+ "id": ids[name],
+ "column_name": f"{name}_v2" if name == renamed else name,
+ **(
+ {"partition_value_transform": "lower(:value)"}
+ if name == "country"
+ else {}
+ ),
+ }
+ for name in ("event_time", "dt_part", "country")
+ ]
+ }
+ if echo:
+ attributes["partition_column"] = "dt_part"
+ attributes["partition_mapped_column"] = "country"
+
+ DatasetDAO.update(table, attributes)
+ db.session.flush()
+
+ assert table.partition_column == (
+ "dt_part_v2" if renamed == "dt_part" else "dt_part"
+ )
+ assert table.partition_mapped_column == (
+ "country_v2" if renamed == "country" else "country"
+ )
+ mapped = table.partition_mapped_column
+ assert {c.column_name: c.partition_value_transform for c in table.columns}[
+ mapped
+ ] == "lower(:value)"
+
+
+def test_a_rename_does_not_override_a_reference_the_request_sets(
+ session: Session,
+) -> None:
+ """A request that names a different column has said what it wants."""
+ from superset import db
+
+ table = _renamable_mapping(session, "rn_explicit")
+ ids = {c.column_name: c.id for c in table.columns}
+
+ DatasetDAO.update(
+ table,
+ {
+ "partition_mapped_column": "event_time",
+ "columns": [
+ {"id": ids["event_time"], "column_name": "event_time"},
+ {"id": ids["dt_part"], "column_name": "dt_part"},
+ {"id": ids["country"], "column_name": "country_v2"},
+ ],
+ },
+ )
+ db.session.flush()
+
+ assert table.partition_mapped_column == "event_time"
+
+
+def test_follow_partition_column_renames_skips_override_columns(
+ session: Session,
+) -> None:
+ """`override_columns=true` matches by name, so a rename is another column."""
+ from superset.connectors.sqla.partition_mapping import (
+ follow_partition_column_renames,
+ )
+
+ table = _renamable_mapping(session, "rn_override")
+ ids = {c.column_name: c.id for c in table.columns}
+ attributes = {
+ "override_columns": True,
+ "columns": [{"id": ids["dt_part"], "column_name": "dt_part_v2"}],
+ }
+
+ assert follow_partition_column_renames(table, attributes) is attributes
diff --git a/tests/unit_tests/datasets/commands/export_test.py b/tests/unit_tests/datasets/commands/export_test.py
index 6567caae2df..ff0bf33e220 100644
--- a/tests/unit_tests/datasets/commands/export_test.py
+++ b/tests/unit_tests/datasets/commands/export_test.py
@@ -216,8 +216,6 @@ folders:
- uuid: 00000000-0000-0000-0000-000000000005
type: column
name: profit
-partition_column: null
-partition_mapped_column: null
uuid: {payload["uuid"]}
metrics:
- metric_name: cnt
@@ -246,8 +244,6 @@ columns:
datetime_format: null
extra:
certified_by: User
- partition_value_transform: null
- partition_transform_is_monotonic: false
uuid: 00000000-0000-0000-0000-000000000005
- column_name: ds
verbose_name: null
@@ -262,8 +258,6 @@ columns:
python_date_format: null
datetime_format: null
extra: null
- partition_value_transform: null
- partition_transform_is_monotonic: false
uuid: 00000000-0000-0000-0000-000000000006
- column_name: user_id
verbose_name: null
@@ -278,8 +272,6 @@ columns:
python_date_format: null
datetime_format: null
extra: null
- partition_value_transform: null
- partition_transform_is_monotonic: false
uuid: 00000000-0000-0000-0000-000000000007
- column_name: revenue
verbose_name: null
@@ -294,8 +286,6 @@ columns:
python_date_format: null
datetime_format: null
extra: null
- partition_value_transform: null
- partition_transform_is_monotonic: false
uuid: 00000000-0000-0000-0000-000000000008
- column_name: expenses
verbose_name: null
@@ -310,8 +300,6 @@ columns:
python_date_format: null
datetime_format: null
extra: null
- partition_value_transform: null
- partition_transform_is_monotonic: false
uuid: 00000000-0000-0000-0000-000000000009
version: 1.0.0
database_uuid: {database.uuid}
@@ -440,3 +428,48 @@ def test_export_two_datasets_same_table_name_different_schema(
assert schemas_in_yaml == {"prod", "dev"}, (
f"Expected both prod and dev schemas in export, got {schemas_in_yaml}"
)
+
+
+def test_export_writes_the_partition_mapping_only_when_set(session: Session) -> None:
+ """
+ An older Superset's import schemas refuse a field they do not know, so the
+ mapping fields stay out of the bundle while they hold their defaults -- and
+ only then. The importer reads an omitted field as its default.
+ """
+ from superset.commands.dataset.export import ExportDatasetsCommand
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.models.core import Database
+
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+ database = Database(database_name="my_database", sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+ table = SqlaTable(
+ table_name="web_events",
+ database=database,
+ columns=[
+ TableColumn(
+ column_name="event_time",
+ partition_value_transform="unix_timestamp(:value)",
+ partition_transform_is_monotonic=True,
+ ),
+ TableColumn(column_name="dt_epoch"),
+ ],
+ partition_column="dt_epoch",
+ partition_mapped_column="event_time",
+ )
+ db.session.add(table)
+ db.session.flush()
+
+ payload = yaml.safe_load(ExportDatasetsCommand._file_content(table))
+
+ assert payload["partition_column"] == "dt_epoch"
+ assert payload["partition_mapped_column"] == "event_time"
+ columns = {column["column_name"]: column for column in payload["columns"]}
+ assert columns["event_time"]["partition_value_transform"] == (
+ "unix_timestamp(:value)"
+ )
+ assert columns["event_time"]["partition_transform_is_monotonic"] is True
+ assert "partition_value_transform" not in columns["dt_epoch"]
+ assert "partition_transform_is_monotonic" not in columns["dt_epoch"]
diff --git a/tests/unit_tests/datasets/commands/importers/v1/import_test.py b/tests/unit_tests/datasets/commands/importers/v1/import_test.py
index 8e81b0ee125..22cd892da23 100644
--- a/tests/unit_tests/datasets/commands/importers/v1/import_test.py
+++ b/tests/unit_tests/datasets/commands/importers/v1/import_test.py
@@ -3818,13 +3818,13 @@ def test_import_keeps_a_usable_transform(session: Session) -> None:
@with_feature_flags(PARTITION_FILTER_MAPPING=False)
-def test_import_leaves_transforms_alone_while_the_feature_is_off(
+def test_import_drops_an_unsavable_transform_while_the_feature_is_off(
session: Session,
) -> None:
"""
- No *usability* check runs with the flag off: nothing mirrors, so there is
- only stored configuration to lose. The structural check still runs, and
- Jinja passes it -- balanced blocks are substituted out before parsing.
+ The blocking checks do not answer to the flag. Skipped while it was off, a
+ transform no save accepts was imported and stored, and the first flag-on
+ save of the dataset was refused over it.
"""
engine = db.session.get_bind()
SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
@@ -3837,10 +3837,7 @@ def test_import_leaves_transforms_alone_while_the_feature_is_off(
)
drop_unusable_partition_transforms(config)
- assert (
- config["columns"][0]["partition_value_transform"]
- == "unix_timestamp({{ current_username() }})"
- )
+ assert config["columns"][0]["partition_value_transform"] is None
#: Expressions the save path refuses outright, rather than ones that merely
@@ -3977,3 +3974,39 @@ def test_an_unfinished_transform_survives_the_export_import_round_trip(
drop_unusable_partition_transforms(config)
assert config["columns"][0]["partition_value_transform"] == transform
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_an_overwrite_reads_omitted_mapping_fields_as_their_defaults(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ Export leaves the mapping fields out while they hold their defaults, so an
+ older Superset can import the bundle. Omitted therefore means "the default":
+ read as "keep what is stored", overwriting a mapped dataset with a bundle
+ from one that is not mapped kept a mapping the source does not have.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+ database = Database(database_name="pm_db", sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ config = _partition_mapping_config(database.id, "lower(:value)")
+ mapped = import_dataset(copy.deepcopy(config), overwrite=True)
+ assert mapped.partition_column == "dt_epoch"
+
+ unmapped = copy.deepcopy(config)
+ for key in ("partition_column", "partition_mapped_column"):
+ del unmapped[key]
+ for column in unmapped["columns"]:
+ column.pop("partition_value_transform", None)
+ column.pop("partition_transform_is_monotonic", None)
+ dataset = import_dataset(unmapped, overwrite=True)
+
+ assert dataset.partition_column is None
+ assert dataset.partition_mapped_column is None
+ assert all(c.partition_value_transform is None for c in dataset.columns)
+ assert not any(c.partition_transform_is_monotonic for c in dataset.columns)
diff --git a/tests/unit_tests/views/datasource/views_test.py b/tests/unit_tests/views/datasource/views_test.py
index 9083b1f4a21..9134d0d773e 100644
--- a/tests/unit_tests/views/datasource/views_test.py
+++ b/tests/unit_tests/views/datasource/views_test.py
@@ -2016,3 +2016,69 @@ def test_save_keeps_an_echoed_transform_only_while_the_flag_is_off(
)
assert on[0]["partition_value_transform"] is None
assert on[0]["partition_transform_is_monotonic"] is False
+
+
+def _legacy_mapping_orm(partition_column: str | None) -> MagicMock:
+ columns = []
+ for name in ("event_time", "dt_epoch"):
+ column = MagicMock()
+ column.column_name = name
+ columns.append(column)
+ orm = MagicMock()
+ orm.database.backend = "sqlite"
+ orm.columns = columns
+ orm.main_dttm_col = "event_time"
+ orm.partition_column = partition_column
+ orm.partition_mapped_column = None
+ return orm
+
+
+def _legacy_mapping_payload(partition_column: str) -> dict[str, Any]:
+ return {
+ "main_dttm_col": "event_time",
+ "partition_column": partition_column,
+ "partition_mapped_column": None,
+ "columns": [{"column_name": "event_time"}, {"column_name": "dt_epoch"}],
+ }
+
+
+@pytest.mark.parametrize("feature_enabled", [True, False])
+def test_legacy_save_refuses_a_reference_the_request_breaks(
+ app: Flask, feature_enabled: bool
+) -> None:
+ """Storing a mapping is not flag-gated: reading one is."""
+ from superset.views.datasource.views import _partition_mapping_reference_error
+
+ original = app.config["DEFAULT_FEATURE_FLAGS"].get("PARTITION_FILTER_MAPPING")
+ app.config["DEFAULT_FEATURE_FLAGS"]["PARTITION_FILTER_MAPPING"] = feature_enabled
+ try:
+ error = _partition_mapping_reference_error(
+ _legacy_mapping_orm("dt_epoch"), _legacy_mapping_payload("no_such_column")
+ )
+ finally:
+ app.config["DEFAULT_FEATURE_FLAGS"]["PARTITION_FILTER_MAPPING"] = original
+
+ assert error is not None
+ assert "no_such_column" in error
+
+
+def test_legacy_save_survives_a_reference_already_broken_in_storage(
+ app: Flask,
+) -> None:
+ """
+ This endpoint's client does GET-then-POST, so it echoes a reference stored
+ broken. Refusing it meant the dataset could not be saved at all.
+ """
+ from superset.views.datasource.views import _partition_mapping_reference_error
+
+ original = app.config["DEFAULT_FEATURE_FLAGS"].get("PARTITION_FILTER_MAPPING")
+ app.config["DEFAULT_FEATURE_FLAGS"]["PARTITION_FILTER_MAPPING"] = True
+ try:
+ error = _partition_mapping_reference_error(
+ _legacy_mapping_orm("no_such_column"),
+ _legacy_mapping_payload("no_such_column"),
+ )
+ finally:
+ app.config["DEFAULT_FEATURE_FLAGS"]["PARTITION_FILTER_MAPPING"] = original
+
+ assert error is None