Copilot commented on code in PR #38910:
URL: https://github.com/apache/superset/pull/38910#discussion_r3017337875
##########
tests/integration_tests/charts/data/api_tests.py:
##########
@@ -1677,6 +1677,91 @@ def test_data_cache_default_timeout(
assert rv.json["result"][0]["cache_timeout"] == 3456
[email protected](
+ "flask.current_app.config",
+ {
+ **app.config,
+ "CACHE_DEFAULT_TIMEOUT": 100000,
+ "DATA_CACHE_CONFIG": {
+ **app.config["DATA_CACHE_CONFIG"],
+ "CACHE_DEFAULT_TIMEOUT": 3456,
+ },
+ "FILTER_STATE_CACHE_CONFIG": {
+ **app.config["FILTER_STATE_CACHE_CONFIG"],
+ "CACHE_DEFAULT_TIMEOUT": 7890,
+ },
+ },
+)
Review Comment:
The three tests duplicate a large, nested config patch payload, which makes
future adjustments error-prone (e.g., if config keys change). Consider
extracting a small helper/fixture that builds the patched config (optionally
parameterized by `filter_state_timeout` present/absent and desired timeouts) to
reduce duplication and improve readability.
##########
tests/integration_tests/charts/data/api_tests.py:
##########
@@ -1677,6 +1677,91 @@ def test_data_cache_default_timeout(
assert rv.json["result"][0]["cache_timeout"] == 3456
[email protected](
+ "flask.current_app.config",
+ {
+ **app.config,
+ "CACHE_DEFAULT_TIMEOUT": 100000,
+ "DATA_CACHE_CONFIG": {
+ **app.config["DATA_CACHE_CONFIG"],
+ "CACHE_DEFAULT_TIMEOUT": 3456,
+ },
+ "FILTER_STATE_CACHE_CONFIG": {
+ **app.config["FILTER_STATE_CACHE_CONFIG"],
+ "CACHE_DEFAULT_TIMEOUT": 7890,
+ },
+ },
+)
+def test_native_filter_uses_filter_state_cache_timeout(
+ test_client,
+ login_as_admin,
+ physical_query_context,
+):
+ physical_query_context["form_data"] = {
+ "native_filter_id": "NATIVE_FILTER-TEST",
+ "viz_type": "filter_select",
+ }
+ rv = test_client.post(CHART_DATA_URI, json=physical_query_context)
+ assert rv.json["result"][0]["cache_timeout"] == 7890
+
+
[email protected](
+ "flask.current_app.config",
+ {
+ **app.config,
+ "CACHE_DEFAULT_TIMEOUT": 100000,
+ "DATA_CACHE_CONFIG": {
+ **app.config["DATA_CACHE_CONFIG"],
+ "CACHE_DEFAULT_TIMEOUT": 3456,
+ },
+ "FILTER_STATE_CACHE_CONFIG": {
+ **app.config["FILTER_STATE_CACHE_CONFIG"],
+ "CACHE_DEFAULT_TIMEOUT": 7890,
+ },
+ },
+)
+def test_non_filter_viz_type_with_native_filter_id_uses_data_cache_timeout(
+ test_client,
+ login_as_admin,
+ physical_query_context,
+):
+ physical_query_context["form_data"] = {
+ "native_filter_id": "NATIVE_FILTER-TEST",
+ "viz_type": "table",
+ }
+ rv = test_client.post(CHART_DATA_URI, json=physical_query_context)
+ assert rv.json["result"][0]["cache_timeout"] == 3456
+
+
[email protected](
+ "flask.current_app.config",
+ {
+ **app.config,
+ "CACHE_DEFAULT_TIMEOUT": 100000,
+ "DATA_CACHE_CONFIG": {
+ **app.config["DATA_CACHE_CONFIG"],
+ "CACHE_DEFAULT_TIMEOUT": 3456,
+ },
+ "FILTER_STATE_CACHE_CONFIG": {
+ key: value
+ for key, value in app.config["FILTER_STATE_CACHE_CONFIG"].items()
+ if key != "CACHE_DEFAULT_TIMEOUT"
+ },
+ },
+)
Review Comment:
The three tests duplicate a large, nested config patch payload, which makes
future adjustments error-prone (e.g., if config keys change). Consider
extracting a small helper/fixture that builds the patched config (optionally
parameterized by `filter_state_timeout` present/absent and desired timeouts) to
reduce duplication and improve readability.
##########
tests/integration_tests/charts/data/api_tests.py:
##########
@@ -1677,6 +1677,91 @@ def test_data_cache_default_timeout(
assert rv.json["result"][0]["cache_timeout"] == 3456
[email protected](
+ "flask.current_app.config",
+ {
+ **app.config,
+ "CACHE_DEFAULT_TIMEOUT": 100000,
+ "DATA_CACHE_CONFIG": {
+ **app.config["DATA_CACHE_CONFIG"],
+ "CACHE_DEFAULT_TIMEOUT": 3456,
+ },
+ "FILTER_STATE_CACHE_CONFIG": {
+ **app.config["FILTER_STATE_CACHE_CONFIG"],
+ "CACHE_DEFAULT_TIMEOUT": 7890,
+ },
+ },
+)
Review Comment:
`mock.patch("flask.current_app.config", ...)` is brittle because
`current_app` is a `LocalProxy`; patching its forwarded attributes can be
error-prone depending on context handling. Prefer patching the symbol as
imported/used by the code under test (e.g., patch
`superset.common.query_context_processor.current_app.config`) or updating
config via an app context (`with app.app_context(): app.config.update(...)`) to
make the test less flaky.
##########
tests/integration_tests/charts/data/api_tests.py:
##########
@@ -1677,6 +1677,91 @@ def test_data_cache_default_timeout(
assert rv.json["result"][0]["cache_timeout"] == 3456
[email protected](
+ "flask.current_app.config",
+ {
+ **app.config,
+ "CACHE_DEFAULT_TIMEOUT": 100000,
+ "DATA_CACHE_CONFIG": {
+ **app.config["DATA_CACHE_CONFIG"],
+ "CACHE_DEFAULT_TIMEOUT": 3456,
+ },
+ "FILTER_STATE_CACHE_CONFIG": {
+ **app.config["FILTER_STATE_CACHE_CONFIG"],
+ "CACHE_DEFAULT_TIMEOUT": 7890,
+ },
+ },
+)
+def test_native_filter_uses_filter_state_cache_timeout(
+ test_client,
+ login_as_admin,
+ physical_query_context,
+):
+ physical_query_context["form_data"] = {
+ "native_filter_id": "NATIVE_FILTER-TEST",
+ "viz_type": "filter_select",
+ }
+ rv = test_client.post(CHART_DATA_URI, json=physical_query_context)
+ assert rv.json["result"][0]["cache_timeout"] == 7890
+
+
[email protected](
+ "flask.current_app.config",
+ {
+ **app.config,
+ "CACHE_DEFAULT_TIMEOUT": 100000,
+ "DATA_CACHE_CONFIG": {
+ **app.config["DATA_CACHE_CONFIG"],
+ "CACHE_DEFAULT_TIMEOUT": 3456,
+ },
+ "FILTER_STATE_CACHE_CONFIG": {
+ **app.config["FILTER_STATE_CACHE_CONFIG"],
+ "CACHE_DEFAULT_TIMEOUT": 7890,
+ },
+ },
+)
Review Comment:
The three tests duplicate a large, nested config patch payload, which makes
future adjustments error-prone (e.g., if config keys change). Consider
extracting a small helper/fixture that builds the patched config (optionally
parameterized by `filter_state_timeout` present/absent and desired timeouts) to
reduce duplication and improve readability.
##########
superset/common/query_context_processor.py:
##########
@@ -383,8 +383,29 @@ def get_payload(
return return_value
def get_cache_timeout(self) -> int:
- if cache_timeout_rv := self._query_context.get_cache_timeout():
+ if (cache_timeout_rv := self._query_context.get_cache_timeout()) is
not None:
return cache_timeout_rv
Review Comment:
This change intentionally fixes the case where an explicit cache timeout of
`0` should be honored (previously `0` would be treated as falsy and ignored).
There isn’t test coverage in this PR for that specific regression/behavior;
please add a targeted test asserting that when the query context returns `0`,
`get_cache_timeout()` returns `0` and does not fall back to config defaults.
--
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]
