Copilot commented on code in PR #42927:
URL: https://github.com/apache/superset/pull/42927#discussion_r3745874358
##########
superset/common/query_object.py:
##########
@@ -205,8 +206,77 @@ 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 the first parameter receives the DataFrame positionally and can
never
+ # be supplied as an option, and neither can a positional-only
parameter.
+ keyword_parameters = {
+ name
+ for position, (name, parameter) in enumerate(parameters.items())
+ if position > 0 and parameter.kind is not
inspect.Parameter.POSITIONAL_ONLY
+ }
Review Comment:
`keyword_parameters` currently includes `VAR_POSITIONAL` (`*args`)
parameters (since they’re not `POSITIONAL_ONLY`). But `*args` cannot be
supplied via `**options`, so treating it as a supported option can allow an
option key (eg `args`) to pass through and still raise `TypeError: got an
unexpected keyword argument`. Filter `keyword_parameters` to only include
parameters that can actually be provided as keywords (eg
`POSITIONAL_OR_KEYWORD` and `KEYWORD_ONLY`), excluding `VAR_POSITIONAL`.
##########
superset/common/query_object.py:
##########
@@ -205,8 +206,77 @@ 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 the first parameter receives the DataFrame positionally and can
never
+ # be supplied as an option, and neither can a positional-only
parameter.
+ keyword_parameters = {
+ name
+ for position, (name, parameter) in enumerate(parameters.items())
+ if position > 0 and parameter.kind is not
inspect.Parameter.POSITIONAL_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
+
+ logger.warning(
+ "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,
+ )
Review Comment:
This warning can become very noisy in production for frequently-rendered
legacy charts (and across multiple execution paths mentioned in the docstring),
potentially flooding logs. Consider reducing verbosity (eg `info`/`debug`)
and/or adding a lightweight deduplication/rate-limit mechanism (eg per-process
cache keyed by `(operation, frozenset(unsupported))`) so repeated requests
don’t emit the same warning indefinitely.
--
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]
