fix(cache): warm native dashboard filter defaults - #43073
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43073 +/- ##
==========================================
+ Coverage 66.64% 66.67% +0.03%
==========================================
Files 2863 2863
Lines 162154 162116 -38
Branches 37404 37384 -20
==========================================
+ Hits 108065 108090 +25
+ Misses 52030 51943 -87
- Partials 2059 2083 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d07470c to
cccd3a5
Compare
|
The flagged issue is correct. In To resolve this, you should ensure that explicit filters take precedence. One approach is to filter out any dashboard-derived filters that conflict with the explicit filters before merging, or to maintain the previous behavior where explicit filters are treated as authoritative. Would you like me to implement a fix for this issue? I can also check the rest of the PR for other comments if you would like. superset/commands/chart/warm_up_cache.py |
Code Review Agent Run #8d7e21Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Thanks for taking a look at this. The flagged issue isn't correct. Although this query-building flow is not so straightforward to me, so I went back and checked both the previous behavior and the frontend implementation. Before this change, query.filter.extend(dashboard_filters) already combined both sets of filters. It did not replace the saved chart filters. This change only corrects their order to match buildQueryObject: dashboard or explicit filters first, followed by the saved chart filters. Explicit extra_filters still take precedence over filters read from the dashboard metadata. Filtering them by column would change the query and could bring back the cache-key mismatch. |
rusackas
left a comment
There was a problem hiding this comment.
Lookin good. Backed by an integration test that actually asserts is_cached=true on the browser-equivalent request; CI's are green and mergeable is clean. Solid, well-scoped. I'll merge soon...
SUMMARY
Fixes #43024.
Dashboard cache warm-up now includes persisted native filter defaults represented by
extra_form_data.filters. It reuses the existing dashboard filter-context helper for scope and default-value extraction, then preserves browser query ordering by placing dashboard filters before the chart's saved filters:Explicit
extra_filterssupplied to the warm-up endpoint keep their existing precedence. The change is intentionally limited to filter clauses; other native-filter context fields and plugin-specific query normalization are outside this PR.Related regression coverage: #43025.
Related discussion: #42382.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable. This change affects backend query construction and cache-key compatibility.
TESTING INSTRUCTIONS
Run the focused unit tests:
Run the causal integration regression:
The integration test persists a legacy default, a native select default, and a saved chart filter, warms the chart cache, sends the browser-equivalent request with
force=false, verifies thelegacy → native → chartorder, and assertsis_cached=true.ADDITIONAL INFORMATION