sadpandajoe commented on code in PR #45562:
URL: https://github.com/apache/superset/pull/45562#discussion_r4395547654
##########
superset/commands/dataset/update.py:
##########
@@ -491,6 +499,211 @@ def _validate_expressions(
)
)
+ def _validate_partition_mapping(self, exceptions: list[ValidationError])
-> None:
+ """
+ Validate the dataset's partition filter mapping.
+
+ Only the blocking (Tier 1) issues become validation errors. Tier 2
+ issues -- an unparseable transform, a transform missing `:value` --
+ deliberately let the save through and leave the mapping inactive, per
+ the PRD, so a half-written transform doesn't cost the owner the rest of
+ their edits. They are surfaced by the editor, not by rejecting the PUT.
+
+ The transform is authored by a dataset owner, the same principal and
+ trust level as a calculated-column expression, so it also goes through
+ `validate_stored_expression` -- the parser gate that already governs
+ stored expressions. That gate only applies to a transform that parses:
+ it rejects an unparseable expression outright, which would turn a
Tier-2
+ issue into a blocking one and undo the paragraph above. Skipping it
+ there costs nothing -- an unparseable transform is never emitted into a
+ query -- and it is not a hole for templating, because Jinja in a
+ transform is already a Tier-1 blocking issue of its own.
+
+ 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.
+ """
+ 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}
+ if columns is not None
+ else {column.column_name for column in self._model.columns}
+ )
+
+ partition_column = self._properties.get(
+ "partition_column", self._model.partition_column
+ )
+ partition_mapped_column = self._properties.get(
+ "partition_mapped_column", self._model.partition_mapped_column
+ )
+ main_dttm_col = self._properties.get("main_dttm_col",
self._model.main_dttm_col)
+ if not partition_column:
+ return
+
+ # A column payload drops every column it omits, and `update_columns`
+ # then runs `DatasetDAO.clear_dangling_partition_mapping` to drop a
+ # mapping whose columns went with them -- but that happens later,
during
+ # `run()`. Validate the state that cleanup leaves behind, or a metadata
+ # sync that legitimately removes the mapped column is rejected before
+ # the cleanup meant to handle it ever runs, which is exactly the
+ # orphaned case the cleanup exists for.
+ #
+ # Only a *stored* reference is forgiven. Asking in this very request to
+ # map onto a column the same request does not define is a mistake worth
+ # reporting, not something to quietly clean up.
+ if columns is not None:
+ if (
+ "partition_column" not in self._properties
+ and partition_column not in column_names
+ ):
+ return
+ if (
+ "partition_mapped_column" not in self._properties
+ and partition_mapped_column
+ and partition_mapped_column not in column_names
+ ):
+ partition_mapped_column = None
+
+ database = self._properties.get("database") or self._model.database
+ catalog = self._properties.get("catalog", self._model.catalog)
+ schema = self._properties.get("schema", self._model.schema)
+
+ 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,
+ partition_mapped_column=partition_mapped_column,
+ main_dttm_col=main_dttm_col,
+ transform=transform,
+ engine=database.backend,
+ ):
+ if issue.blocking and (issue.field, str(issue.message)) not in (
+ stored_issues
+ ):
+ exceptions.append(
+ ValidationError(str(issue.message), field_name=issue.field)
+ )
+
+ # Everything except a transform that is not SQL yet. A half-typed
+ # transform is a Tier-2 issue above -- the mapping saves and stays
+ # inactive -- and this gate fails closed on anything that does not
+ # 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 self._is_stored_transform(transform, database, catalog,
schema)
Review Comment:
This forgives a transform whenever its text equals the transform stored on
the *currently stored* mapped column, but doesn't check that the request still
maps the same column. If a dataset was saved with the flag off and column
holds a transform the expression gate would refuse, a PUT that sets to and
puts the same text on skips here, so a newly introduced transform is stored
without the gate that applies. Should also require the effective mapped
column to be unchanged (after rename-following)?
##########
tests/unit_tests/commands/dataset/update_test.py:
##########
@@ -1831,3 +1841,395 @@ def
test_update_dataset_rejects_malicious_fetch_values_predicate(
and "fetch_values_predicate" in (exc.field_name or "")
for exc in excinfo.value._exceptions
)
+
+
+def _mapping_command(
+ mocker: MockerFixture,
+ transform: str | None,
+ *,
+ partition_mapped_column: str | None = None,
+ main_dttm_col: str = "event_time",
+ database: Database | None = None,
+) -> UpdateDatasetCommand:
+ """
+ A command whose stored dataset maps `event_time` onto `dt_epoch`.
+
+ `database` takes a real one, for the gates that read more of it than
+ `backend` -- the function denylist is keyed on the engine spec's own name,
+ which a mock cannot supply.
+ """
+ mapped_column = mocker.MagicMock()
+ mapped_column.column_name = "event_time"
+ mapped_column.partition_value_transform = transform
+ partition_column = mocker.MagicMock()
+ partition_column.column_name = "dt_epoch"
+ # The partition column is the target of a mapping, never the source of one.
+ partition_column.partition_value_transform = None
+
+ mock_dataset = mocker.MagicMock(is_managed_externally=False)
+ if database is not None:
+ mock_dataset.database = database
+ else:
+ mock_dataset.database.backend = "sqlite"
+ mock_dataset.catalog = None
+ mock_dataset.schema = "main"
+ mock_dataset.columns = [mapped_column, partition_column]
+ mock_dataset.main_dttm_col = main_dttm_col
+ mock_dataset.partition_column = "dt_epoch"
+ mock_dataset.partition_mapped_column = partition_mapped_column
+
+ command = UpdateDatasetCommand(1, {})
+ command._model = mock_dataset
+ return command
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_an_unparseable_transform_does_not_block_the_save(
+ mocker: MockerFixture,
+) -> None:
+ """
+ An unparseable transform is a Tier-2 issue: the mapping saves and stays
+ inactive. `validate_stored_expression` rejects anything it cannot parse, so
+ running it here would turn that into a blocking error and cost the owner
the
+ rest of their edits.
+ """
+ gate =
mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(mocker, "unix_timestamp(:value")
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+ gate.assert_not_called()
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_parseable_transform_still_goes_through_the_stored_expression_gate(
+ mocker: MockerFixture,
+) -> None:
+ """
+ The gate that governs every other stored expression still runs -- now the
+ same `stored_expression_error` preview, import and the legacy save path
+ use, so the function denylist applies here too.
+ """
+ gate = mocker.patch(
+ "superset.commands.dataset.update.stored_expression_error",
return_value=None
+ )
+ command = _mapping_command(mocker, "unix_timestamp(:value)")
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+ gate.assert_called_once()
+ assert gate.call_args.args[-1] == "unix_timestamp(:value)"
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
[email protected](
+ "transform",
+ [
+ "unix_timestamp(:value) UNION ALL SELECT password FROM ab_user",
+ "unix_timestamp(:value); DROP TABLE ab_user",
+ "password || :value FROM ab_user",
+ ],
+ ids=["set-operation", "multi-statement", "bare-from"],
+)
+def test_a_transform_that_is_the_wrong_sql_still_reaches_the_gate(
+ mocker: MockerFixture, transform: str
+) -> None:
+ """
+ The companion to the test above, and the reason the skip condition is "not
+ SQL yet" rather than "does not parse". All three of these fail to parse as
+ a single statement, exactly as a half-typed transform does -- so a skip
+ keyed on parseability would have waved them through. None of them is on the
+ way to a valid transform.
+ """
+ command = _mapping_command(mocker, transform)
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ # Reported rather than silently accepted the way an unfinished transform
is.
+ assert exceptions
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_transform_calling_a_denied_function_is_refused_on_save(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A normal PUT was the one door into this field that skipped the function
+ denylist: preview, import and the legacy save path all ran
+ `stored_expression_error`, while this ran only the narrower parser gate. So
+ an owner could store what preview had just refused, and the probe would run
+ it on the next chart load.
+ """
+ command = _mapping_command(
+ mocker,
+ "schema_to_xml('public') || :value",
+ database=Database(database_name="pfm_pg",
sqlalchemy_uri="postgresql://u@h/d"),
+ )
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert [exc.field_name for exc in exceptions] ==
["partition_value_transform"]
+ assert "schema_to_xml" in str(exceptions[0].messages)
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_jinja_transform_is_still_rejected(mocker: MockerFixture) -> None:
+ """
+ Skipping the gate for unparseable transforms is not a hole for templating:
+ 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(: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)
+
+ assert [exc.field_name for exc in exceptions] ==
["partition_value_transform"]
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_description_only_put_survives_an_implicit_self_mapping(
+ mocker: MockerFixture,
+) -> None:
+ """
+ `fetch_metadata` could choose the partition column as the default datetime
+ column without anyone asking, and a blocking self-mapping error then made
+ the dataset unsaveable forever -- including by the very write that would
+ have set the override that fixes it, and including a write that changes
+ nothing but the description.
+
+ The mapping is inert in that state either way, since
+ `resolve_partition_mapping` bails out on it, so the issue is reported
+ rather than blocking.
+ """
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(
+ mocker, "unix_timestamp(:value)", main_dttm_col="dt_epoch"
+ )
+ command._properties["description"] = "a new description"
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_self_mapping_is_rejected_while_the_feature_is_on(
+ mocker: MockerFixture,
+) -> None:
+ """A column cannot stand in for itself: that is a Tier-1 blocking issue."""
+ 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 [exc.field_name for exc in exceptions] == ["partition_column"]
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=False)
+def test_a_mapping_the_request_breaks_is_rejected_while_the_feature_is_off(
+ mocker: MockerFixture,
+) -> None:
+ """
+ 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.
+ """
+ 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 [exc.field_name for exc in exceptions] == ["partition_column"]
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_sync_that_drops_the_mapped_column_is_not_blocked(
+ mocker: MockerFixture,
+) -> None:
+ """
+ `update_columns` clears a mapping whose columns the payload dropped, but
+ that runs later, during `run()`. Validating the pre-cleanup state would
+ reject the very sync the cleanup exists to absorb -- an `override_columns`
+ resync whose source table no longer has the mapped column.
+ """
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(
+ mocker, "unix_timestamp(:value)", partition_mapped_column="event_time"
+ )
+ command._properties["columns"] = [{"column_name": "dt_epoch"}]
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_sync_that_drops_the_partition_column_is_not_blocked(
+ mocker: MockerFixture,
+) -> None:
+ """Losing the partition column drops the whole mapping, not just the
half."""
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(
+ mocker, "unix_timestamp(:value)", partition_mapped_column="event_time"
+ )
+ command._properties["columns"] = [{"column_name": "event_time"}]
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_mapping_onto_a_column_the_same_request_omits_is_still_rejected(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Only a *stored* reference is forgiven. Asking in this request to map onto a
+ column the same request does not define is a mistake worth reporting, not
+ something to quietly clean up.
+ """
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(mocker, "unix_timestamp(:value)")
+ command._properties["columns"] = [{"column_name": "dt_epoch"}]
+ command._properties["partition_mapped_column"] = "event_time"
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert [exc.field_name for exc in exceptions] ==
["partition_mapped_column"]
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_self_mapping_is_still_rejected_alongside_a_column_payload(
+ mocker: MockerFixture,
+) -> None:
+ """Forgiving a dropped column does not forgive the checks that remain."""
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(mocker, "unix_timestamp(:value)")
+ command._properties["columns"] = [
+ {"column_name": "dt_epoch"},
+ {"column_name": "event_time"},
+ ]
+ command._properties["partition_mapped_column"] = "dt_epoch"
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert [exc.field_name for exc in exceptions] == ["partition_column"]
+
+
[email protected](
+ "stored",
+ [
+ {"partition_column": "no_such_column"},
+ {"partition_mapped_column": "no_such_column"},
+ {"transform": "rand() + 0 * :value"},
+ ],
+ ids=["dangling-partition", "dangling-mapped", "non-deterministic"],
+)
[email protected](
+ "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(
Review Comment:
leaves and as two different mocks, so is always false in these tests;
they only cover the stored-*issue* forgiveness. If the new expression-gate
exemption were deleted, every test here would still pass. Could you add a case
where the ids (and catalog/schema) match and returns an error, asserting an
unchanged transform saves without calling the gate, and one where the database
or schema changes and the gate's error is reported?
##########
superset/commands/dataset/update.py:
##########
@@ -491,6 +499,211 @@ def _validate_expressions(
)
)
+ def _validate_partition_mapping(self, exceptions: list[ValidationError])
-> None:
+ """
+ Validate the dataset's partition filter mapping.
+
+ Only the blocking (Tier 1) issues become validation errors. Tier 2
+ issues -- an unparseable transform, a transform missing `:value` --
+ deliberately let the save through and leave the mapping inactive, per
+ the PRD, so a half-written transform doesn't cost the owner the rest of
+ their edits. They are surfaced by the editor, not by rejecting the PUT.
+
+ The transform is authored by a dataset owner, the same principal and
+ trust level as a calculated-column expression, so it also goes through
+ `validate_stored_expression` -- the parser gate that already governs
+ stored expressions. That gate only applies to a transform that parses:
+ it rejects an unparseable expression outright, which would turn a
Tier-2
+ issue into a blocking one and undo the paragraph above. Skipping it
+ there costs nothing -- an unparseable transform is never emitted into a
+ query -- and it is not a hole for templating, because Jinja in a
+ transform is already a Tier-1 blocking issue of its own.
+
+ 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.
+ """
+ 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}
+ if columns is not None
+ else {column.column_name for column in self._model.columns}
+ )
+
+ partition_column = self._properties.get(
+ "partition_column", self._model.partition_column
+ )
+ partition_mapped_column = self._properties.get(
+ "partition_mapped_column", self._model.partition_mapped_column
+ )
+ main_dttm_col = self._properties.get("main_dttm_col",
self._model.main_dttm_col)
+ if not partition_column:
+ return
+
+ # A column payload drops every column it omits, and `update_columns`
+ # then runs `DatasetDAO.clear_dangling_partition_mapping` to drop a
+ # mapping whose columns went with them -- but that happens later,
during
+ # `run()`. Validate the state that cleanup leaves behind, or a metadata
+ # sync that legitimately removes the mapped column is rejected before
+ # the cleanup meant to handle it ever runs, which is exactly the
+ # orphaned case the cleanup exists for.
+ #
+ # Only a *stored* reference is forgiven. Asking in this very request to
+ # map onto a column the same request does not define is a mistake worth
+ # reporting, not something to quietly clean up.
+ if columns is not None:
+ if (
+ "partition_column" not in self._properties
+ and partition_column not in column_names
+ ):
+ return
+ if (
+ "partition_mapped_column" not in self._properties
+ and partition_mapped_column
+ and partition_mapped_column not in column_names
+ ):
+ partition_mapped_column = None
+
+ database = self._properties.get("database") or self._model.database
+ catalog = self._properties.get("catalog", self._model.catalog)
+ schema = self._properties.get("schema", self._model.schema)
+
+ 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,
+ partition_mapped_column=partition_mapped_column,
+ main_dttm_col=main_dttm_col,
+ transform=transform,
+ engine=database.backend,
+ ):
+ if issue.blocking and (issue.field, str(issue.message)) not in (
+ stored_issues
+ ):
+ exceptions.append(
+ ValidationError(str(issue.message), field_name=issue.field)
+ )
+
+ # Everything except a transform that is not SQL yet. A half-typed
+ # transform is a Tier-2 issue above -- the mapping saves and stays
+ # inactive -- and this gate fails closed on anything that does not
+ # 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 self._is_stored_transform(transform, database, catalog,
schema)
Review Comment:
This forgives a transform whenever its text equals the transform stored on
the *currently stored* mapped column, but doesn't check that the request still
maps the same column. If a dataset was saved with the flag off and column `a`
holds a transform the expression gate would refuse, a PUT that sets
`partition_mapped_column` to `b` and puts the same text on `b` skips
`stored_expression_error` here, so a newly introduced transform is stored
without the gate that `main` applies. Should `_is_stored_transform` also
require the effective mapped column to be unchanged (after rename-following)?
##########
tests/unit_tests/commands/dataset/update_test.py:
##########
@@ -1831,3 +1841,395 @@ def
test_update_dataset_rejects_malicious_fetch_values_predicate(
and "fetch_values_predicate" in (exc.field_name or "")
for exc in excinfo.value._exceptions
)
+
+
+def _mapping_command(
+ mocker: MockerFixture,
+ transform: str | None,
+ *,
+ partition_mapped_column: str | None = None,
+ main_dttm_col: str = "event_time",
+ database: Database | None = None,
+) -> UpdateDatasetCommand:
+ """
+ A command whose stored dataset maps `event_time` onto `dt_epoch`.
+
+ `database` takes a real one, for the gates that read more of it than
+ `backend` -- the function denylist is keyed on the engine spec's own name,
+ which a mock cannot supply.
+ """
+ mapped_column = mocker.MagicMock()
+ mapped_column.column_name = "event_time"
+ mapped_column.partition_value_transform = transform
+ partition_column = mocker.MagicMock()
+ partition_column.column_name = "dt_epoch"
+ # The partition column is the target of a mapping, never the source of one.
+ partition_column.partition_value_transform = None
+
+ mock_dataset = mocker.MagicMock(is_managed_externally=False)
+ if database is not None:
+ mock_dataset.database = database
+ else:
+ mock_dataset.database.backend = "sqlite"
+ mock_dataset.catalog = None
+ mock_dataset.schema = "main"
+ mock_dataset.columns = [mapped_column, partition_column]
+ mock_dataset.main_dttm_col = main_dttm_col
+ mock_dataset.partition_column = "dt_epoch"
+ mock_dataset.partition_mapped_column = partition_mapped_column
+
+ command = UpdateDatasetCommand(1, {})
+ command._model = mock_dataset
+ return command
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_an_unparseable_transform_does_not_block_the_save(
+ mocker: MockerFixture,
+) -> None:
+ """
+ An unparseable transform is a Tier-2 issue: the mapping saves and stays
+ inactive. `validate_stored_expression` rejects anything it cannot parse, so
+ running it here would turn that into a blocking error and cost the owner
the
+ rest of their edits.
+ """
+ gate =
mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(mocker, "unix_timestamp(:value")
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+ gate.assert_not_called()
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_parseable_transform_still_goes_through_the_stored_expression_gate(
+ mocker: MockerFixture,
+) -> None:
+ """
+ The gate that governs every other stored expression still runs -- now the
+ same `stored_expression_error` preview, import and the legacy save path
+ use, so the function denylist applies here too.
+ """
+ gate = mocker.patch(
+ "superset.commands.dataset.update.stored_expression_error",
return_value=None
+ )
+ command = _mapping_command(mocker, "unix_timestamp(:value)")
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+ gate.assert_called_once()
+ assert gate.call_args.args[-1] == "unix_timestamp(:value)"
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
[email protected](
+ "transform",
+ [
+ "unix_timestamp(:value) UNION ALL SELECT password FROM ab_user",
+ "unix_timestamp(:value); DROP TABLE ab_user",
+ "password || :value FROM ab_user",
+ ],
+ ids=["set-operation", "multi-statement", "bare-from"],
+)
+def test_a_transform_that_is_the_wrong_sql_still_reaches_the_gate(
+ mocker: MockerFixture, transform: str
+) -> None:
+ """
+ The companion to the test above, and the reason the skip condition is "not
+ SQL yet" rather than "does not parse". All three of these fail to parse as
+ a single statement, exactly as a half-typed transform does -- so a skip
+ keyed on parseability would have waved them through. None of them is on the
+ way to a valid transform.
+ """
+ command = _mapping_command(mocker, transform)
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ # Reported rather than silently accepted the way an unfinished transform
is.
+ assert exceptions
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_transform_calling_a_denied_function_is_refused_on_save(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A normal PUT was the one door into this field that skipped the function
+ denylist: preview, import and the legacy save path all ran
+ `stored_expression_error`, while this ran only the narrower parser gate. So
+ an owner could store what preview had just refused, and the probe would run
+ it on the next chart load.
+ """
+ command = _mapping_command(
+ mocker,
+ "schema_to_xml('public') || :value",
+ database=Database(database_name="pfm_pg",
sqlalchemy_uri="postgresql://u@h/d"),
+ )
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert [exc.field_name for exc in exceptions] ==
["partition_value_transform"]
+ assert "schema_to_xml" in str(exceptions[0].messages)
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_jinja_transform_is_still_rejected(mocker: MockerFixture) -> None:
+ """
+ Skipping the gate for unparseable transforms is not a hole for templating:
+ 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(: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)
+
+ assert [exc.field_name for exc in exceptions] ==
["partition_value_transform"]
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_description_only_put_survives_an_implicit_self_mapping(
+ mocker: MockerFixture,
+) -> None:
+ """
+ `fetch_metadata` could choose the partition column as the default datetime
+ column without anyone asking, and a blocking self-mapping error then made
+ the dataset unsaveable forever -- including by the very write that would
+ have set the override that fixes it, and including a write that changes
+ nothing but the description.
+
+ The mapping is inert in that state either way, since
+ `resolve_partition_mapping` bails out on it, so the issue is reported
+ rather than blocking.
+ """
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(
+ mocker, "unix_timestamp(:value)", main_dttm_col="dt_epoch"
+ )
+ command._properties["description"] = "a new description"
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_self_mapping_is_rejected_while_the_feature_is_on(
+ mocker: MockerFixture,
+) -> None:
+ """A column cannot stand in for itself: that is a Tier-1 blocking issue."""
+ 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 [exc.field_name for exc in exceptions] == ["partition_column"]
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=False)
+def test_a_mapping_the_request_breaks_is_rejected_while_the_feature_is_off(
+ mocker: MockerFixture,
+) -> None:
+ """
+ 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.
+ """
+ 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 [exc.field_name for exc in exceptions] == ["partition_column"]
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_sync_that_drops_the_mapped_column_is_not_blocked(
+ mocker: MockerFixture,
+) -> None:
+ """
+ `update_columns` clears a mapping whose columns the payload dropped, but
+ that runs later, during `run()`. Validating the pre-cleanup state would
+ reject the very sync the cleanup exists to absorb -- an `override_columns`
+ resync whose source table no longer has the mapped column.
+ """
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(
+ mocker, "unix_timestamp(:value)", partition_mapped_column="event_time"
+ )
+ command._properties["columns"] = [{"column_name": "dt_epoch"}]
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_sync_that_drops_the_partition_column_is_not_blocked(
+ mocker: MockerFixture,
+) -> None:
+ """Losing the partition column drops the whole mapping, not just the
half."""
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(
+ mocker, "unix_timestamp(:value)", partition_mapped_column="event_time"
+ )
+ command._properties["columns"] = [{"column_name": "event_time"}]
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert exceptions == []
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_mapping_onto_a_column_the_same_request_omits_is_still_rejected(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Only a *stored* reference is forgiven. Asking in this request to map onto a
+ column the same request does not define is a mistake worth reporting, not
+ something to quietly clean up.
+ """
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(mocker, "unix_timestamp(:value)")
+ command._properties["columns"] = [{"column_name": "dt_epoch"}]
+ command._properties["partition_mapped_column"] = "event_time"
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert [exc.field_name for exc in exceptions] ==
["partition_mapped_column"]
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
+def test_a_self_mapping_is_still_rejected_alongside_a_column_payload(
+ mocker: MockerFixture,
+) -> None:
+ """Forgiving a dropped column does not forgive the checks that remain."""
+ mocker.patch("superset.commands.dataset.update.validate_stored_expression")
+ command = _mapping_command(mocker, "unix_timestamp(:value)")
+ command._properties["columns"] = [
+ {"column_name": "dt_epoch"},
+ {"column_name": "event_time"},
+ ]
+ command._properties["partition_mapped_column"] = "dt_epoch"
+
+ exceptions: list[ValidationError] = []
+ command._validate_partition_mapping(exceptions)
+
+ assert [exc.field_name for exc in exceptions] == ["partition_column"]
+
+
[email protected](
+ "stored",
+ [
+ {"partition_column": "no_such_column"},
+ {"partition_mapped_column": "no_such_column"},
+ {"transform": "rand() + 0 * :value"},
+ ],
+ ids=["dangling-partition", "dangling-mapped", "non-deterministic"],
+)
[email protected](
+ "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(
Review Comment:
`_mapping_command` leaves `model.database_id` and `model.database.id` as two
different mocks, so `_is_stored_transform()` is always false in these tests;
they only cover the stored-*issue* forgiveness. If the new expression-gate
exemption were deleted, every test here would still pass. Could you add a case
where the ids (and catalog/schema) match and `stored_expression_error` returns
an error, asserting an unchanged transform saves without calling the gate, and
one where the database or schema changes and the gate's error is reported?
--
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]
