bito-code-review[bot] commented on code in PR #43757:
URL: https://github.com/apache/superset/pull/43757#discussion_r4162861227
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -267,6 +271,52 @@ def _get_template_params(dataset: SqlaTable) -> dict[str,
Any]:
return params if isinstance(params, dict) else {}
+def drop_unusable_partition_transforms(config: dict[str, Any]) -> None:
+ """
+ Drop a partition value transform the save path would have rejected.
+
+ Import is the one door into this field that does not go through
+ `UpdateDatasetCommand`, so a bundle can carry a transform holding Jinja or
+ calling a non-deterministic function -- one that will never mirror a
filter,
+ and that the editor can only report as broken after the fact. Dropping it
on
+ the way in leaves the column with no transform, a state an owner can see
and
+ fix, rather than a stored expression that looks configured and is not.
+
+ Deliberately sanitizes rather than raises. A bundle is imported as a whole,
+ and failing someone's entire dataset over one unusable expression is a
worse
+ trade than importing the dataset without it. This is the same bargain
+ `DatasetDAO.clear_dangling_partition_mapping` already strikes for a mapping
+ whose column went away.
+ """
+ columns = config.get("columns")
+ if not columns or not is_feature_enabled(PARTITION_FILTER_MAPPING):
+ return
+ if not any(column.get("partition_value_transform") for column in columns):
+ return
+
+ database =
db.session.query(Database).filter_by(id=config["database_id"]).first()
+ if database is None:
+ return
+
+ for column in columns:
+ transform = column.get("partition_value_transform")
+ if not transform:
+ continue
+ if blocking := [
+ issue
+ for issue in validate_transform(transform, database.backend)
+ if issue.blocking
+ ]:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: Import Skips Expression Security Check</b></div>
<div id="fix">
The docstring promises parity with the save path, but
`UpdateDatasetCommand._validate_partition_mapping` also runs
`validate_stored_expression` on parseable transforms (update.py:524-543),
rejecting subqueries and set operations. `drop_unusable_partition_transforms`
applies only `validate_transform`'s blocking tier, so import stores transforms
the editor rejects on security grounds. The value is only parsed today
(`is_transform_active`), but the stored state diverges by entry path.
([CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
</div>
<small><i>Code Review Run #0d43e9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/datasets/commands/importers/v1/import_test.py:
##########
@@ -2804,3 +2806,81 @@ def
test_load_data_bounds_gzip_download_before_decompression(
mock_gzip_open.assert_called_once_with(bounded_raw)
# ...and the decompressed output is bounded again before parsing.
assert mock_read_bounded.call_args_list[1].args[0] is decompressed
+
+
+def _partition_mapping_config(database_id: int, transform: str) -> dict[str,
Any]:
+ return {
+ "table_name": "web_events",
+ "uuid": uuid.uuid4(),
+ "database_id": database_id,
+ "main_dttm_col": "event_time",
+ "partition_column": "dt_epoch",
+ "partition_mapped_column": "event_time",
+ "columns": [
+ {
+ "column_name": "event_time",
+ "is_dttm": True,
+ "partition_value_transform": transform,
+ "partition_transform_is_monotonic": True,
+ },
+ {"column_name": "dt_epoch"},
+ ],
+ "metrics": [],
+ }
+
+
+@with_feature_flags(PARTITION_FILTER_MAPPING=True)
[email protected](
+ "transform",
+ ["unix_timestamp({{ current_username() }})", "rand() + 0 * :value"],
+ ids=["jinja", "non-deterministic"],
+)
+def test_import_drops_a_transform_the_save_path_would_reject(
+ session: Session, transform: str
+) -> None:
+ """
+ Import is the one door into this field that does not go through
+ `UpdateDatasetCommand`, so a bundle can carry a transform that will never
+ mirror a filter. Dropping it leaves a state an owner can see and fix rather
+ than a stored expression that looks configured and is not.
+ """
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+ database = Database(database_name="hive_db",
sqlalchemy_uri="hive://localhost/db")
+ db.session.add(database)
+ db.session.flush()
+
+ config = _partition_mapping_config(database.id, transform)
+ drop_unusable_partition_transforms(config)
+
+ assert config["columns"][0]["partition_value_transform"] is None
+ assert config["columns"][0]["partition_transform_is_monotonic"] is False
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Tests bypass import wiring</b></div>
<div id="fix">
These tests are named and docstring'd as import-path behavior ("Import is
the one door..."), but all three call `drop_unusable_partition_transforms`
directly and never invoke `import_dataset`. If the call at `utils.py:559` were
dropped, these tests still pass while the import door silently stops
sanitizing. Exercise `import_dataset` once, or reframe names/docstrings as
helper-level tests.
</div>
</div>
<small><i>Code Review Run #0d43e9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/datasets/commands/importers/v1/import_test.py:
##########
@@ -2804,3 +2806,81 @@ def
test_load_data_bounds_gzip_download_before_decompression(
mock_gzip_open.assert_called_once_with(bounded_raw)
# ...and the decompressed output is bounded again before parsing.
assert mock_read_bounded.call_args_list[1].args[0] is decompressed
+
+
+def _partition_mapping_config(database_id: int, transform: str) -> dict[str,
Any]:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing docstring on helper</b></div>
<div id="fix">
`_partition_mapping_config` is the only new unit here without a docstring,
while the three tests it feeds all have one. Repo rule (BITO adaptive
12490/12147) requires docstrings on new test helpers. Add a one-liner, e.g.
"Build a minimal dataset import config whose dttm column carries `transform`."
</div>
</div>
<small><i>Code Review Run #0d43e9</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
