aminghadersohi commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4274813821
##########
superset/views/datasource/views.py:
##########
@@ -293,6 +425,74 @@ def save(self) -> FlaskResponse:
),
status=409,
)
+ # The mapping's one-transform invariant, against the columns this
+ # request is about to write. `update_from_object` sets every field in
+ # `update_from_object_fields` straight onto the column, so nothing
+ # between here and storage would notice a transform parked on a column
+ # the mapping does not mirror -- invisible in the editor, since no row
+ # but the mapped one renders a transform, and live the moment the
mapped
+ # column resolves back to it. `DatasetDAO.update` runs a model-level
+ # pass for this; this endpoint bypasses that along with the rest of
+ # `UpdateDatasetCommand`.
+ #
+ # Corrected rather than refused, unlike the gates below. Those are
about
+ # a value that may not be stored anywhere; this is about a value in the
+ # wrong place, and the payload a GET-then-POST client sends back
carries
+ # whatever a previous writer left there -- so a 422 would punish the
+ # client that is merely echoing the state it was given.
+ #
+ # Which is also why it runs *first*. A parked transform that is not a
+ # storable expression would otherwise fail the gate below and 422 every
+ # save, leaving the dataset uneditable over a value nobody asked to
+ # keep; dropped here, there is nothing left for that gate to refuse.
+ # What it still guards is the transform this request actually stores,
+ # the one on the mapped column.
+ #
+ # Read from the payload alone, with no fallback to the model: every
+ # field here rides in on `update_from_object`, which writes
+ # `obj.get(attr)` for each one, so what this request says *is* what the
+ # mapping will be.
+ if cleared := drop_unmapped_value_transforms(
+ datasource_dict.get("columns"),
+ partition_column=datasource_dict.get("partition_column"),
+
partition_mapped_column=datasource_dict.get("partition_mapped_column"),
+ main_dttm_col=datasource_dict.get("main_dttm_col"),
+ # So an unchanged echo of stored configuration survives: this
+ # endpoint's own client does GET-then-POST, which is exactly the
+ # shape that looked like a value being parked.
+ stored_by_name=None
+ if is_feature_enabled("PARTITION_FILTER_MAPPING")
+ else {
Review Comment:
Nit: This now holds only with the flag off; with it on, the echo is dropped
on purpose.
```suggestion
# With the flag off, an unchanged echo of stored configuration
# survives: this endpoint's own client does GET-then-POST. With
it
# on nothing is exempt, since no model-level pass runs here to
clear
# a transform the mapping no longer mirrors.
stored_by_name=None
if is_feature_enabled("PARTITION_FILTER_MAPPING")
else {
```
##########
superset/views/datasource/views.py:
##########
@@ -293,6 +425,74 @@ def save(self) -> FlaskResponse:
),
status=409,
)
+ # The mapping's one-transform invariant, against the columns this
+ # request is about to write. `update_from_object` sets every field in
+ # `update_from_object_fields` straight onto the column, so nothing
+ # between here and storage would notice a transform parked on a column
+ # the mapping does not mirror -- invisible in the editor, since no row
+ # but the mapped one renders a transform, and live the moment the
mapped
+ # column resolves back to it. `DatasetDAO.update` runs a model-level
+ # pass for this; this endpoint bypasses that along with the rest of
+ # `UpdateDatasetCommand`.
+ #
+ # Corrected rather than refused, unlike the gates below. Those are
about
+ # a value that may not be stored anywhere; this is about a value in the
+ # wrong place, and the payload a GET-then-POST client sends back
carries
+ # whatever a previous writer left there -- so a 422 would punish the
+ # client that is merely echoing the state it was given.
+ #
+ # Which is also why it runs *first*. A parked transform that is not a
+ # storable expression would otherwise fail the gate below and 422 every
+ # save, leaving the dataset uneditable over a value nobody asked to
+ # keep; dropped here, there is nothing left for that gate to refuse.
+ # What it still guards is the transform this request actually stores,
+ # the one on the mapped column.
+ #
+ # Read from the payload alone, with no fallback to the model: every
+ # field here rides in on `update_from_object`, which writes
+ # `obj.get(attr)` for each one, so what this request says *is* what the
+ # mapping will be.
+ if cleared := drop_unmapped_value_transforms(
+ datasource_dict.get("columns"),
+ partition_column=datasource_dict.get("partition_column"),
+
partition_mapped_column=datasource_dict.get("partition_mapped_column"),
+ main_dttm_col=datasource_dict.get("main_dttm_col"),
+ # So an unchanged echo of stored configuration survives: this
+ # endpoint's own client does GET-then-POST, which is exactly the
+ # shape that looked like a value being parked.
+ stored_by_name=None
Review Comment:
No test pins this: reverted to the stored-columns dict, `views_test.py`
stays green, since `_save_with_parked_transform` never stores `other_time`. A
case storing `other_time` with the echoed transform fails on that revert with
the flag on.
--
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]
