Ujjwaljain16 commented on PR #38910: URL: https://github.com/apache/superset/pull/38910#issuecomment-4700220753
@rusackas Thanks for the detailed feedback i spent some time tracing the full native filter request flow again and revisited the previous assumptions you were right about the `metrics` check the earlier detection logic was incorrect because `nativeFilters/utils.ts::getFormData()` always populates `form_data` with `metrics: ["count"]` for native filter requests the tests in the previous iteration were using a synthetic payload that did not accurately represent the real frontend contract which meant the new path would never have been exercised in production i updated the detection logic to rely on the stable fields that identify native filter requests: `native_filter_id` together with the `filter_*` viz type prefix and added tests using realistic native filter payloads to prevent this from regressing also while revisiting the implementation i realized that using `FILTER_STATE_CACHE_CONFIG` was the wrong abstraction native filter option loading is still a chart-data query that goes through `/api/v1/chart/data`, so it should continue using the existing data cache infrastructure the actual requirement is not a separate cache backend but an independent freshness policy for these dataset-derived filter values the new approach introduces `NATIVE_FILTER_OPTIONS_CACHE_TIMEOUT` (default `None`) which preserves the existing behavior unless operators explicitly configure a separate TTL when configured it is evaluated before the dataset/database timeout resolution so cases where datasets have a long cache timeout (for example 24 hours) can still configure more frequently refreshed native filter values i also verified the timeout precedence chain to make sure explicit request-level overrides still win the final order is: `custom_cache_timeout` → `NATIVE_FILTER_OPTIONS_CACHE_TIMEOUT` → slice/dataset/database timeout → `DATA_CACHE_CONFIG` → global default. test coverage has been updated to cover: * realistic native filter requests (`metrics: ["count"]`) * native filter timeout override * dataset timeout override (ensuring dataset-level TTLs do not mask the native filter TTL) * standard chart behavior remaining unchanged * cache disabled semantics using `CACHE_DISABLED_TIMEOUT` * false positive protection for non-filter visualizations * request-level `custom_cache_timeout` precedence i also expanded the detection comment with the frontend contract details explaining why `metrics` should not be used as a signal. thanks again for pushing back on the initial approach the review comment helped uncover a few incorrect assumptions in the original implementation and i think the final design aligns much better with the existing cache ownership and timeout hierarchy please take another look whenever you get a chance happy to make any additional adjustments.... -- 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]
