ktmud commented on a change in pull request #18998:
URL: https://github.com/apache/superset/pull/18998#discussion_r819739692
##########
File path: superset/utils/core.py
##########
@@ -1097,11 +1097,13 @@ def merge_extra_form_data(form_data: Dict[str, Any]) ->
None:
{"isExtra": True, **fltr} for fltr in append_adhoc_filters # type:
ignore
)
if append_filters:
- adhoc_filters.extend(
- simple_filter_to_adhoc({"isExtra": True, **fltr}) # type: ignore
- for fltr in append_filters
- if fltr
- )
+ for key, value in form_data.items():
+ if re.match("adhoc_filter.*", key):
+ value.extend(
+ simple_filter_to_adhoc({"isExtra": True, **fltr}) # type:
ignore
+ for fltr in append_filters
+ if fltr
+ )
Review comment:
I still believe in general regex should be used sparingly and explicit
is better than implicit. Since you can't stop someone from naming a new filter
field things like "series_b_filters", the bug may still come back even with a
regex.
In any case, we should need a consistent way of mapping field names to field
types because most of the time what you want is "do something special based on
filter type", not filed names. One obvious solution is to create an
`ADHOC_FILTER_KEYS` constant and put it together with `METRIC_KEYS` in some
`constants.py` file, but I agree this by itself probably doesn't make much
difference on the grander scheme of things.
--
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]
