diff --git a/superset/common/query_object.py b/superset/common/query_object.py index 7dd030854ecc..6365af67e6a0 100644 --- a/superset/common/query_object.py +++ b/superset/common/query_object.py @@ -17,6 +17,7 @@ # pylint: disable=invalid-name from __future__ import annotations +import inspect import logging from datetime import datetime from pprint import pformat @@ -205,8 +206,86 @@ def is_str_or_adhoc(metric: Metric) -> bool: def _set_post_processing( self, post_processing: list[dict[str, Any] | None] | None ) -> None: - post_processing = post_processing or [] - self.post_processing = [post_proc for post_proc in post_processing if post_proc] + self.post_processing = [ + self._drop_unsupported_options(post_proc) + for post_proc in post_processing or [] + if post_proc + ] + + @staticmethod + def _drop_unsupported_options(post_proc: dict[str, Any]) -> dict[str, Any]: + """ + Drop options that the post-processing operation no longer accepts. + + A chart's ``query_context`` is written when the chart is saved and is + never rewritten afterwards, while Explore rebuilds the query from + ``form_data`` at every render. A chart saved by an older version of + Superset can therefore reference an option that has since been removed + from the operation. ``exec_post_processing`` passes the stored options + as keyword arguments, so that option raises a bare ``TypeError`` on + every path that replays the stored ``query_context`` -- the chart data + endpoint, alerts and reports, thumbnails, CSV export -- while the same + chart still renders correctly in Explore. + + Comparing against the signature avoids a hard-coded list of removed + option names, which would need extending at each release. + """ + operation = post_proc.get("operation") + function = ( + getattr(pandas_postprocessing, operation, None) + if isinstance(operation, str) + else None + ) + if function is None: + # A missing or unknown operation is left untouched, so that + # exec_post_processing reports it as InvalidPostProcessingError. + return post_proc + + parameters = inspect.signature(function).parameters + if any( + parameter.kind is inspect.Parameter.VAR_KEYWORD + for parameter in parameters.values() + ): + return post_proc + + # `exec_post_processing` calls the operation as `operation(df, **options)`, + # so an option can only reach a parameter that a caller may fill by + # keyword. That excludes the first parameter, which receives the + # DataFrame positionally, and any positional-only or `*args` parameter. + keyword_parameters = { + name + for position, (name, parameter) in enumerate(parameters.items()) + if position > 0 + and parameter.kind + in ( + inspect.Parameter.POSITIONAL_OR_KEYWORD, + inspect.Parameter.KEYWORD_ONLY, + ) + } + + options = post_proc.get("options") or {} + unsupported = {key for key in options if key not in keyword_parameters} + if not unsupported: + return post_proc + + # Logged at info: a chart saved before the option was removed hits this + # on every render, so a warning would repeat for as long as the chart + # is not resaved, without anything new to report. + logger.info( + "Dropping unsupported option(s) %s of post-processing operation " + "`%s`. The chart's stored query_context predates the current " + "signature of that operation.", + sorted(unsupported), + operation, + ) + return { + **post_proc, + "options": { + key: value + for key, value in options.items() + if key in keyword_parameters + }, + } def _init_series_columns( self, diff --git a/superset/utils/pandas_postprocessing/utils.py b/superset/utils/pandas_postprocessing/utils.py index aa2667ed9d55..79b4796a7372 100644 --- a/superset/utils/pandas_postprocessing/utils.py +++ b/superset/utils/pandas_postprocessing/utils.py @@ -15,7 +15,7 @@ # specific language governing permissions and limitations # under the License. from collections.abc import Sequence -from functools import partial +from functools import partial, wraps from typing import Any, Callable import numpy as np @@ -122,6 +122,10 @@ def scalar_to_sequence(val: Any) -> Sequence[str]: def validate_column_args(*argnames: str) -> Callable[..., Any]: def wrapper(func: Callable[..., Any]) -> Callable[..., Any]: + # `wraps` keeps `func` reachable through `__wrapped__`, so that + # `inspect.signature` reports the parameters of the decorated operation + # rather than the `(df, **options)` of this wrapper. + @wraps(func) def wrapped(df: DataFrame, **options: Any) -> Any: if _is_multi_index_on_columns(df): # MultiIndex column validate first level diff --git a/tests/unit_tests/pandas_postprocessing/test_utils.py b/tests/unit_tests/pandas_postprocessing/test_utils.py index 058cefcd6c72..c039085d7043 100644 --- a/tests/unit_tests/pandas_postprocessing/test_utils.py +++ b/tests/unit_tests/pandas_postprocessing/test_utils.py @@ -14,7 +14,13 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -from superset.utils.pandas_postprocessing import escape_separator, unescape_separator +import inspect + +from superset.utils.pandas_postprocessing import ( + escape_separator, + pivot, + unescape_separator, +) def test_escape_separator(): @@ -28,3 +34,19 @@ def test_escape_separator(): escape_string = escape_separator("hello,world") assert escape_string == r"hello\,world" assert unescape_separator(escape_string) == "hello,world" + + +def test_validate_column_args_preserves_signature(): + """ + The decorator must not hide the signature of the operation it wraps. + + `inspect.signature` follows `__wrapped__`, which `functools.wraps` sets. + Without it every decorated operation reports `(df, **options)`, and code + that inspects the signature -- see `QueryObject._drop_unsupported_options` + -- cannot tell a supported option from an unsupported one. + """ + parameters = inspect.signature(pivot).parameters + + assert pivot.__name__ == "pivot" + assert "options" not in parameters + assert {"index", "aggregates", "columns"} <= set(parameters) diff --git a/tests/unit_tests/queries/query_object_test.py b/tests/unit_tests/queries/query_object_test.py index 122e07ba4bc6..3663f703b1b2 100644 --- a/tests/unit_tests/queries/query_object_test.py +++ b/tests/unit_tests/queries/query_object_test.py @@ -22,6 +22,7 @@ from superset.connectors.sqla.models import SqlaTable from superset.models.core import Database from superset.superset_typing import Metric +from superset.utils import pandas_postprocessing from superset.utils.core import override_user @@ -438,3 +439,143 @@ def test_cache_key_cache_impersonation_on_with_different_user_and_db_impersonati ], any_order=True, ) + + +def test_post_processing_drops_unsupported_options(): + """ + An option that the operation no longer accepts is dropped, not passed on. + + A chart saved by an older version of Superset stores `flatten_columns` in + the options of its `pivot` operation. `pivot` lost that parameter when + flattening became its own operation, so replaying the stored query_context + raised `TypeError: pivot() got an unexpected keyword argument + 'flatten_columns'`. + """ + query_object = QueryObject( + row_limit=1, + post_processing=[ + { + "operation": "pivot", + "options": { + "index": ["__timestamp"], + "columns": ["genre"], + "aggregates": {"count": {"operator": "mean"}}, + "drop_missing_columns": False, + "flatten_columns": True, + "reset_index": True, + }, + } + ], + ) + + options = query_object.post_processing[0]["options"] + assert "flatten_columns" not in options + assert "reset_index" not in options + assert options["drop_missing_columns"] is False + assert options["index"] == ["__timestamp"] + + +def test_post_processing_keeps_supported_options(): + """Options the operation accepts are left alone.""" + post_processing = [ + { + "operation": "pivot", + "options": {"index": ["__timestamp"], "aggregates": {}}, + } + ] + query_object = QueryObject(row_limit=1, post_processing=post_processing) + + assert query_object.post_processing == post_processing + + +def test_post_processing_keeps_unknown_operation(): + """ + An unknown operation is kept, so that `exec_post_processing` can report it + as an `InvalidPostProcessingError` rather than being silently dropped here. + """ + query_object = QueryObject( + row_limit=1, + post_processing=[{"operation": "does_not_exist", "options": {"a": 1}}, None], + ) + + assert query_object.post_processing == [ + {"operation": "does_not_exist", "options": {"a": 1}} + ] + + +def test_post_processing_drops_the_dataframe_parameter(): + """ + The DataFrame parameter is not an option. + + `exec_post_processing` calls `operation(df, **options)`, so an option named + after the first parameter would raise `TypeError: pivot() got multiple + values for argument 'df'`. + """ + query_object = QueryObject( + row_limit=1, + post_processing=[ + { + "operation": "pivot", + "options": {"df": "malformed", "index": ["a"], "aggregates": {}}, + } + ], + ) + + options = query_object.post_processing[0]["options"] + assert "df" not in options + assert options["index"] == ["a"] + + +def test_post_processing_keeps_options_of_a_variadic_operation(): + """An operation that accepts `**kwargs` accepts every option.""" + + def variadic(df, **kwargs): + return df + + post_processing = [{"operation": "variadic", "options": {"anything": 1}}] + with patch.object(pandas_postprocessing, "variadic", variadic, create=True): + query_object = QueryObject(row_limit=1, post_processing=post_processing) + + assert query_object.post_processing == post_processing + + +def test_post_processing_drops_a_variadic_positional_option(): + """ + A `*args` parameter cannot be filled by a keyword argument. + + `exec_post_processing` calls the operation as `operation(df, **options)`, + so an option named after a `*args` parameter would raise `TypeError: + variadic_positional() got an unexpected keyword argument 'args'` even + though the name appears in the signature. + """ + + def variadic_positional(df, *args, index=None): # pylint: disable=unused-argument + return df + + with patch.object( + pandas_postprocessing, "variadic_positional", variadic_positional, create=True + ): + query_object = QueryObject( + row_limit=1, + post_processing=[ + { + "operation": "variadic_positional", + "options": {"args": [1], "index": ["a"]}, + } + ], + ) + + options = query_object.post_processing[0]["options"] + assert "args" not in options + assert options["index"] == ["a"] + + +def test_post_processing_keeps_an_entry_without_an_operation(): + """ + An entry that names no operation is kept, so that `exec_post_processing` + reports it as an `InvalidPostProcessingError`. + """ + post_processing = [{"options": {"a": 1}}] + query_object = QueryObject(row_limit=1, post_processing=post_processing) + + assert query_object.post_processing == post_processing