rusackas commented on code in PR #43025:
URL: https://github.com/apache/superset/pull/43025#discussion_r3772784468
##########
tests/unit_tests/views/test_utils.py:
##########
@@ -51,3 +57,83 @@ def test_get_form_data_handles_non_dict_json_body() -> None:
assert form_data == {}
assert slc is None
+
+
+def test_get_dashboard_extra_filters_includes_native_filter_defaults(
+ session: Session,
+) -> None:
+ """
+ get_dashboard_extra_filters must surface native filter defaults, not just
+ the legacy Filter Box ``default_filters``/``filter_scopes`` metadata.
+
+ Reported in apache/superset#43024 (originally raised in discussion #42382):
+ ``ChartWarmUpCacheCommand`` relies on this function to reconstruct a
+ dashboard's applied filters for cache warming. Because it only reads the
+ legacy fields, warming a
+ dashboard that uses native filters (the standard mechanism today, not the
+ deprecated Filter Box) silently drops every native filter's default
+ value, so the warmed cache key never matches what the browser actually
+ requests and warm_up_cache is effectively a no-op for such dashboards.
+
+ A working native-filter extractor already exists and is wired into the
+ real ``/api/v1/chart/data`` endpoint (see
+
``superset.charts.data.dashboard_filter_context.get_dashboard_filter_context``)
+ -- this function just isn't using it.
+ """
+ Dashboard.metadata.create_all(session.get_bind())
+
+ dataset = SqlaTable(
+ table_name="extra_filters_table",
+ database=Database(database_name="extra_filters_db",
sqlalchemy_uri="sqlite://"),
+ )
+ db.session.add(dataset)
+ db.session.flush()
+
+ chart = Slice(
+ slice_name="chart_with_native_filter",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ )
+
+ native_filter_configuration = [
+ {
+ "id": "NATIVE_FILTER-1",
+ "name": "Region filter",
+ "type": "NATIVE_FILTER",
+ "scope": {"rootPath": ["ROOT_ID"], "excluded": []},
+ "targets": [{"column": {"name": "region"}}],
+ "defaultDataMask": {
+ "extraFormData": {
+ "filters": [{"col": "region", "op": "IN", "val": ["APAC"]}]
+ },
+ "filterState": {"value": ["APAC"]},
+ },
+ "controlValues": {},
+ }
+ ]
+ dashboard = Dashboard(
+ dashboard_title="native_filter_dash",
+ slices=[chart],
+ published=True,
+ json_metadata=json.dumps(
+ {"native_filter_configuration": native_filter_configuration}
+ ),
+ position_json="{}",
+ )
+ db.session.add_all([chart, dashboard])
+ db.session.flush()
+
+ extra_filters = get_dashboard_extra_filters(chart.id, dashboard.id)
+
+ # Pin the actual predicate, not just non-emptiness -- a fix that returns
+ # any placeholder/non-empty value without extracting the native filter's
+ # own col/op/val (from defaultDataMask.extraFormData.filters) would
+ # otherwise still pass this test while producing a cache key that
+ # diverges from what the browser actually requests.
+ assert {"col": "region", "op": "IN", "val": ["APAC"]} in extra_filters, (
+ "get_dashboard_extra_filters must surface the native filter's "
+ "region=APAC predicate from defaultDataMask.extraFormData.filters -- "
+ "it currently only reads the legacy default_filters/filter_scopes "
+ "metadata, so native filter defaults are silently dropped from "
+ f"cache warming. Got: {extra_filters!r}"
+ )
Review Comment:
Right call, but moot now, the actual fix landed on master via #43073 with
equivalent (and more thorough) coverage of this exact scenario. Closing this
one out as superseded.
--
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]
