gabotorresruiz commented on code in PR #45294:
URL: https://github.com/apache/superset/pull/45294#discussion_r4288779453
##########
superset/common/query_context_factory.py:
##########
@@ -434,8 +434,19 @@ def _apply_granularity( # noqa: C901
if filter["col"] != filter_to_remove
]
- def _apply_filters(self, query_object: QueryObject) -> None:
+ def _apply_filters(
+ self, query_object: QueryObject, form_data: dict[str, Any] | None =
None
+ ) -> None:
+ """Apply the time range, retaining the source of dashboard
overrides."""
+ extra_form_data: object = (form_data or {}).get("extra_form_data")
+ dashboard_time_override: bool = (
+ isinstance(extra_form_data, dict)
+ and "time_range" in extra_form_data
+ and extra_form_data["time_range"] == query_object.time_range
+ )
if query_object.time_range:
for filter_object in query_object.filter:
if filter_object["op"] == "TEMPORAL_RANGE":
filter_object["val"] = query_object.time_range
+ if dashboard_time_override:
+ filter_object["isExtra"] = True
Review Comment:
This block worries me a bit. The stamp is keyed on the op, so it marks every
`TEMPORAL_RANGE` predicate on the query rather than the one the dashboard
override targets, and `validate_filter_columns` short-circuits on `isExtra`
before the metric branch. A chart-owned temporal filter therefore gets the
dashboard pass as well.
With a saved real range rather than a no-op, on `/api/v1/chart/data`:
| chart-owned filter | `0ce9b6ca46` | `9ae3a13aa5` | `224a5fbd` |
| --- | --- | --- | --- |
| `gone_col` `TEMPORAL_RANGE` `2024-01-01 : 2024-06-01`, no dashboard range
| 200 | 400 | 400 |
| the same, with a dashboard range | 200 | 400 | **200**, `gone_col`
rejected, provider queried |
| `total_amount` same shape, with a dashboard range | **400** | **400** |
**200**, `total_amount` rejected |
The third row is the one I would not want to ship: the metric refusal that
is already on `master` goes `400` to `200`, which also makes `metric filters
remain unsupported` in the `UPDATING.md` bullet untrue in that state. It is the
sibling of the ordering point you just fixed, reached through the other door.
The second row means the headline guarantee holds only on a dashboard with no
time-range filter.
@aminghadersohi's case is a saved `No filter` predicate, so scoping the
stamp to a value that was already unbounded closes it without handing out the
pass (needs `is_no_time_range` imported from `superset.utils.date_parser`):
```suggestion
if filter_object["op"] == "TEMPORAL_RANGE":
saved = filter_object.get("val")
was_unbounded = saved is None or (
isinstance(saved, str) and is_no_time_range(saved)
)
filter_object["val"] = query_object.time_range
if dashboard_time_override and was_unbounded:
filter_object["isExtra"] = True
```
I tried that plus the same guard in `dashboard_filter_context.py`: all three
rows go back to the `9ae3a13aa5` column, and `metric_filter_test.py`,
`tests/unit_tests/charts/test_chart_data_api.py` and `tests/unit_tests/common`
all stay green. Could a real-range case join
`test_dashboard_time_override_preserves_foreign_filter_provenance`? All four of
its params save `No filter`, which is why this shape slipped through. And since
the rule now lives in shared machinery,
`tests/unit_tests/common/test_query_context_factory.py` feels like the right
home for the stamping rule itself. Or am I misreading what the stamp is meant
to cover?
##########
superset/charts/data/dashboard_filter_context.py:
##########
@@ -382,5 +383,11 @@ def apply_dashboard_filter_context( # noqa: C901
for src_key, target_key in
EXTRA_FORM_DATA_OVERRIDE_REGULAR_MAPPINGS.items():
if src_key in extra_form_data:
query[target_key] = extra_form_data[src_key]
+ if src_key == "time_range" and extra_form_data[src_key]:
+ # The query factory rewrites temporal predicates with this
range.
+ filter_: QueryObjectFilterClause
+ for filter_ in query.get("filters") or []:
+ if filter_.get("op") == "TEMPORAL_RANGE":
+ filter_["isExtra"] = True
Review Comment:
Same scoping question here, and this path has a second caller:
`superset/dashboards/excel_export/workbook.py:223` also goes through
`apply_dashboard_filter_context`, so the stamp reaches dashboard Excel export
and not just the `filters_dashboard_id` route. Worth noting that guarding only
`_apply_filters` is not enough: with that one fixed, the `server` shape still
returned `200` for both regressed rows until the same value check landed here.
##########
superset/semantic_layers/mapper.py:
##########
@@ -1392,22 +1393,46 @@ def validate_filter_columns(
dimension_names: Collection[str],
metric_names: Collection[str],
) -> None:
- """Refuse chart-defined metric predicates; report incompatible extras
later."""
+ """Refuse chart-defined predicates on metrics or on columns the view lacks.
+
+ Dashboard (``isExtra``) filters are not refused: incompatible ones are
+ skipped and reported, so a dashboard filter cannot break the chart. A
+ "No filter" time range constrains nothing, so its column is not checked.
+ """
for filter_ in filters:
column: Column = filter_["col"]
if (
- isinstance(column, str)
- and column not in dimension_names
- and column in metric_names
- and not filter_.get("isExtra")
+ not isinstance(column, str)
+ or column in dimension_names
+ or filter_.get("isExtra")
+ or _is_no_op_time_range(filter_)
):
+ continue
+ if column in metric_names:
raise QueryObjectValidationError(
_(
"Filter column '%(column)s' is a metric; semantic views
only "
- "filter on dimensions",
+ "filter on dimensions. Select a dimension or remove the
filter.",
column=column,
)
)
+ raise QueryObjectValidationError(
Review Comment:
Confirmed closed, thanks. The metric no-op is back to `400` on `224a5fbd`:
`{"col": "total_amount", "op": "TEMPORAL_RANGE", "val": "No filter"}` and `val:
null` both return the metric error, matching `0ce9b6ca46`, and reverting the
two line move turns all three `test_metric_no_op_time_range_is_still_refused`
params red. One sibling of this same ordering shows up in the new
`_apply_filters` stamp, which I left on its own line.
--
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]
