This is an automated email from the ASF dual-hosted git repository. eschutho pushed a commit to branch fix-cache-max-value-size-default in repository https://gitbox.apache.org/repos/asf/superset.git
commit 9f9711ef12835d9e695d1e948683df96944d07dd Author: Elizabeth Thompson <[email protected]> AuthorDate: Fri Sep 25 19:00:05 2026 +0000 fix(cache): default DATA_CACHE_MAX_VALUE_SIZE to 10 MB and apply it to every data-cache writer - Default DATA_CACHE_MAX_VALUE_SIZE to 10 MB so a single oversized value cannot flood the data cache backend. - Extract the size check into exceeds_max_cache_value_size() and use it in set_and_log_cache and in the writers that bypassed it: the SQL executor's result cache and the datasource API's column-values and compatible metrics/dimensions caches. Each skip logs a WARNING and increments skip_cache_value_too_large. - Async chart queries: when the query succeeds but the result is not in the data cache (e.g. skipped for size), fail the task with a clear error instead of reporting success, which made the client resubmit indefinitely. - Update config comment, docs and UPDATING.md; add tests. Co-Authored-By: Claude Opus 5.5 <[email protected]> --- UPDATING.md | 20 +++ docs/admin_docs/configuration/cache.mdx | 14 +- superset/config.py | 23 ++- superset/datasource/api.py | 14 +- superset/sql/execution/executor.py | 6 + superset/tasks/async_queries.py | 20 +++ superset/utils/cache.py | 50 ++++-- tests/unit_tests/datasource/cache_size_cap_test.py | 160 ++++++++++++++++++ tests/unit_tests/sql/execution/test_executor.py | 73 ++++++++ tests/unit_tests/tasks/test_async_queries.py | 61 +++++++ tests/unit_tests/utils/cache_test.py | 183 +++++++++++++++++++++ 11 files changed, 594 insertions(+), 30 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 8a396f8ad35..89ec881a732 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -24,6 +24,26 @@ assists people when migrating to a new version. ## Next +### `DATA_CACHE_MAX_VALUE_SIZE` now defaults to 10 MB + +The upper bound on the serialized size of a single value written to the data +cache now defaults to `10 * 1024 * 1024` (10 MB) instead of `None`. Values whose +serialized size exceeds the limit are no longer written to the data cache — the +request still succeeds, but the next load re-queries the datasource instead of +getting a cache hit. The limit applies to every data-cache writer: chart results, +SQL query results from the SQL executor, filter dropdown column values, and +compatible metrics/dimensions. Each skip emits a WARNING log +naming the key and byte size and increments the `skip_cache_value_too_large` +statsd counter. This protects the cache backend (e.g. Redis) from being driven +toward its memory limit by a heavy tail of very large results. To restore the +previous unbounded behavior, set `DATA_CACHE_MAX_VALUE_SIZE = None`; to allow +larger cached results, raise the limit further. + +With `GLOBAL_ASYNC_QUERIES` enabled, a chart whose result exceeds the limit now +fails with a clear error instead of being retried indefinitely, because async +chart delivery depends on reading the result back from the data cache. Raise the +limit, narrow the query, or disable async queries for such charts. + ### Guest token RLS rules without a dataset apply inside sub-queries A guest token RLS rule with no `dataset` key applies to every dataset. Such diff --git a/docs/admin_docs/configuration/cache.mdx b/docs/admin_docs/configuration/cache.mdx index 7c724ab9d3a..76abbd22ad5 100644 --- a/docs/admin_docs/configuration/cache.mdx +++ b/docs/admin_docs/configuration/cache.mdx @@ -131,17 +131,19 @@ retention policy, it keeps async results in the cache longer and modestly increa ## Limiting Cached Result Size Very large chart or SQL query results can flood the cache backend (Redis/Memcached), evicting many -smaller useful entries or exhausting memory. To cap the size of any single value written to the data -cache, set `DATA_CACHE_MAX_VALUE_SIZE` (in bytes) in `superset_config.py`: +smaller useful entries or exhausting memory. The size of any single value written to the data cache +(chart and SQL query results, filter dropdown values, and compatible metrics/dimensions) is bounded by `DATA_CACHE_MAX_VALUE_SIZE` (in bytes), which defaults to `10 * 1024 * 1024` (10 MB). To +change it, set it in `superset_config.py`: ```python -DATA_CACHE_MAX_VALUE_SIZE = 10 * 1024 * 1024 # 10 MB +DATA_CACHE_MAX_VALUE_SIZE = 20 * 1024 * 1024 # 20 MB ``` When a result's serialized size exceeds this threshold it is not written to the data cache — the -chart still renders, but the next load re-queries the datasource instead of getting a cache hit. The -`skip_cache_value_too_large` statsd metric is incremented each time this happens. Set to `None` (the -default) to disable the check. +chart still renders, but the next load re-queries the datasource instead of getting a cache hit. Each +skip emits a WARNING log naming the key and byte size, and increments the `skip_cache_value_too_large` +statsd metric. Set to `None` to disable the check entirely (no serialization overhead is then +incurred). ## SQL Lab Query Results diff --git a/superset/config.py b/superset/config.py index bd2fb106f1a..ddb0eb3c8f5 100644 --- a/superset/config.py +++ b/superset/config.py @@ -1423,13 +1423,22 @@ CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "NullCache"} DATA_CACHE_CONFIG: CacheConfig = {"CACHE_TYPE": "NullCache"} # Upper bound, in bytes, on the serialized size of a single value written to the -# data cache (chart and SQL query results). When a result's pickled size exceeds -# this threshold the value is NOT written to the cache: the chart still renders, -# but the next load re-queries the datasource instead of getting a cache hit. This -# protects the cache backend (e.g. Redis/Memcached) from being flooded by very -# large result sets. Set to ``None`` to disable the check (the default). Example: -# 10 * 1024 * 1024 for a 10 MB limit. -DATA_CACHE_MAX_VALUE_SIZE: int | None = None +# data cache (chart and SQL query results, filter dropdown values, and compatible +# metrics/dimensions). When a value's pickled size exceeds this threshold it is +# NOT written to the cache: the request still succeeds, but the next load +# re-queries the datasource instead of getting a cache hit. This protects the +# cache backend (e.g. Redis/Memcached) from being flooded by a heavy tail of very +# large result sets, which can drive the backend toward its memory limit and +# evict many smaller, useful entries. Each skip emits a WARNING log naming the key +# and byte size and increments the ``skip_cache_value_too_large`` statsd counter. +# With GLOBAL_ASYNC_QUERIES, async chart delivery reads the result back from this +# cache, so an oversized async chart result fails with a clear error instead. +# The default of 10 MB comfortably exceeds typical chart/query payloads while +# excluding the multi-tens-of-MB outliers responsible for cache pressure; raise it +# if legitimate results are being skipped, or set it to ``None`` to disable the +# check entirely (no serialization overhead is then incurred). Example: +# 20 * 1024 * 1024 for 20 MB. +DATA_CACHE_MAX_VALUE_SIZE: int | None = 10 * 1024 * 1024 # Include per-query lifecycle timing in /api/v1/chart/data JSON responses. # The default keeps the public response contract unchanged. diff --git a/superset/datasource/api.py b/superset/datasource/api.py index e951ca5d93c..efc07284d39 100644 --- a/superset/datasource/api.py +++ b/superset/datasource/api.py @@ -48,6 +48,7 @@ from superset.extensions import cache_manager from superset.semantic_layers.mapper import SUPPORTED_FILTER_OPERATORS from superset.superset_typing import FlaskResponse from superset.utils import json +from superset.utils.cache import exceeds_max_cache_value_size from superset.utils.core import ( apply_max_row_limit, DatasourceType, @@ -59,6 +60,15 @@ from superset.views.base_api import BaseSupersetApi, protect_read, statsd_metric logger = logging.getLogger(__name__) + +def _set_data_cache(cache_key: str, value: Any, timeout: int) -> None: + """Write ``value`` to the data cache unless it exceeds + ``DATA_CACHE_MAX_VALUE_SIZE``. An oversized value is left uncached, so the next + request misses and recomputes it from the datasource.""" + if not exceeds_max_cache_value_size(cache_key, value): + cache_manager.data_cache.set(cache_key, value, timeout=timeout) + + # Cache lifetime for search-filtered column values, in seconds. SEARCH_CACHE_TIMEOUT = 60 @@ -281,7 +291,7 @@ class DatasourceRestApi(BaseSupersetApi): # Every distinct search term is its own key, so a few users typing # would otherwise pin one entry per keystroke for the full timeout. timeout = min(timeout, SEARCH_CACHE_TIMEOUT) - cache_manager.data_cache.set(cache_key, payload, timeout=timeout) + _set_data_cache(cache_key, payload, timeout) logger.debug( "column-values cache MISS: uid=%s col=%s", datasource.uid, column_name ) @@ -575,7 +585,7 @@ class DatasourceRestApi(BaseSupersetApi): timeout = datasource.cache_timeout or app.config.get( "CACHE_DEFAULT_TIMEOUT", 300 ) - cache_manager.data_cache.set(cache_key, result, timeout=timeout) + _set_data_cache(cache_key, result, timeout) return self.response(200, result=result) diff --git a/superset/sql/execution/executor.py b/superset/sql/execution/executor.py index e357d396216..0629b49ebf2 100644 --- a/superset/sql/execution/executor.py +++ b/superset/sql/execution/executor.py @@ -80,6 +80,7 @@ from superset.exceptions import ( from superset.extensions import cache_manager from superset.sql.parse import SQLScript from superset.utils import core as utils +from superset.utils.cache import exceeds_max_cache_value_size if TYPE_CHECKING: from superset_core.queries.types import ( @@ -1021,6 +1022,11 @@ class SQLExecutor: "total_execution_time_ms": result.total_execution_time_ms, } + # An oversized result is left uncached; ``_get_from_cache`` then misses and + # the query re-runs on the next request. + if exceeds_max_cache_value_size(cache_key, cached_data): + return + cache_manager.data_cache.set( cache_key, cached_data, diff --git a/superset/tasks/async_queries.py b/superset/tasks/async_queries.py index 71436da5241..1ed1772337d 100644 --- a/superset/tasks/async_queries.py +++ b/superset/tasks/async_queries.py @@ -33,6 +33,7 @@ from superset.common.query_serialization import ( from superset.constants import CacheRegion from superset.exceptions import SupersetException from superset.extensions import ( + cache_manager, security_manager, ) from superset.tasks.ambient_context import get_context @@ -318,6 +319,25 @@ def execute_chart_query( with _capture_query_cancellation(query_context): result = query_context.get_df_payload_result(query_obj) if cache_key := result.payload.get(CACHE_KEY_PAYLOAD_KEY): + # Async delivery works by the client reading this result back from the + # DATA cache under ``cache_key``. When the query succeeded but the value + # was not persisted -- most commonly because its serialized size exceeds + # DATA_CACHE_MAX_VALUE_SIZE and the oversized-value skip dropped it -- the + # read-back would miss and the client would resubmit this task without + # end (its force_cached read raises CacheLoadError, which the chart-data + # endpoint treats as "not ready yet" and reschedules). Fail loudly here + # instead so the user sees a clear terminal error rather than an + # invisible retry loop. (Async is never scheduled against a NullCache + # DATA backend, so a missing entry here means a real skipped write.) + if not result.payload.get("error") and not cache_manager.data_cache.has( + cache_key + ): + raise SupersetException( + "The query result could not be cached and cannot be returned " + "asynchronously. This usually means it exceeds " + "DATA_CACHE_MAX_VALUE_SIZE; narrow the query, raise the limit, " + "or run the request without async." + ) # Write synchronously: a dependent contribution query reads this # cache key via get_dependency_payloads once the DAG gate releases, # so it must not sit in the throttle buffer. diff --git a/superset/utils/cache.py b/superset/utils/cache.py index fb0d31811c8..46522e8ecde 100644 --- a/superset/utils/cache.py +++ b/superset/utils/cache.py @@ -54,6 +54,38 @@ def generate_cache_key(values_dict: dict[str, Any], key_prefix: str = "") -> str return cache_key +def exceeds_max_cache_value_size(cache_key: str, value: Any) -> bool: + """Check a value against ``DATA_CACHE_MAX_VALUE_SIZE`` before a data-cache write. + + Every writer to the data cache calls this so that one oversized value cannot + flood the cache backend (e.g. Redis/Memcached) and evict many smaller entries. + When the serialized (pickled) size of ``value`` is larger than the limit, a + WARNING naming the key and size is logged and the + ``skip_cache_value_too_large`` statsd counter is incremented; the caller must + then skip the write. A skipped write is an ordinary cache miss: the next read + re-runs the query. + + :returns: ``True`` when the value is too large and must not be cached. Always + ``False`` when ``DATA_CACHE_MAX_VALUE_SIZE`` is ``None``, in which case the + value is not serialized and no overhead is incurred. + """ + max_value_size = app.config.get("DATA_CACHE_MAX_VALUE_SIZE") + if max_value_size is None: + return False + value_size = len(pickle.dumps(value, protocol=pickle.HIGHEST_PROTOCOL)) + if value_size <= max_value_size: + return False + logger.warning( + "Skipping cache set for key %s: serialized value size %d bytes " + "exceeds DATA_CACHE_MAX_VALUE_SIZE (%d bytes)", + cache_key, + value_size, + max_value_size, + ) + app.config["STATS_LOGGER"].incr("skip_cache_value_too_large") + return True + + def set_and_log_cache( cache_instance: Cache, cache_key: str, @@ -91,21 +123,9 @@ def set_and_log_cache( # Skip caching results that are too large to protect the cache backend # (e.g. Redis/Memcached) from being flooded by huge result sets. The chart # still renders; the value is simply not cached, causing a re-query on the - # next load instead of a cache hit. Disabled when DATA_CACHE_MAX_VALUE_SIZE - # is None (the default), in which case no serialization overhead is incurred. - max_value_size = app.config.get("DATA_CACHE_MAX_VALUE_SIZE") - if max_value_size is not None: - value_size = len(pickle.dumps(value, protocol=pickle.HIGHEST_PROTOCOL)) - if value_size > max_value_size: - logger.warning( - "Skipping cache set for key %s: serialized value size %d bytes " - "exceeds DATA_CACHE_MAX_VALUE_SIZE (%d bytes)", - cache_key, - value_size, - max_value_size, - ) - app.config["STATS_LOGGER"].incr("skip_cache_value_too_large") - return False + # next load instead of a cache hit. + if exceeds_max_cache_value_size(cache_key, value): + return False # Flask-Caching's set() returns bool | None: cachelib backends can report # a failed write by returning False without raising, while some backends diff --git a/tests/unit_tests/datasource/cache_size_cap_test.py b/tests/unit_tests/datasource/cache_size_cap_test.py new file mode 100644 index 00000000000..b8c6d4d161d --- /dev/null +++ b/tests/unit_tests/datasource/cache_size_cap_test.py @@ -0,0 +1,160 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""``DATA_CACHE_MAX_VALUE_SIZE`` applies to the datasource API's cache writes. + +The column-values endpoint (filter dropdown values) and the compatible +metrics/dimensions endpoint write to the data cache directly. An oversized +payload is left uncached and still returned to the caller; a normal payload is +cached as usual. +""" + +from typing import Any +from unittest.mock import call, MagicMock + +import pytest +from flask import current_app +from pytest_mock import MockerFixture + +# A cap small enough for the "large" payloads below to exceed it, while the +# "small" payloads stay under it. +MAX_VALUE_SIZE = 1024 + + [email protected] +def datasource(mocker: MockerFixture) -> MagicMock: + datasource = MagicMock() + datasource.uid = "1__table" + datasource.cache_timeout = None + datasource.normalize_columns = False + datasource.changed_on = "2024-01-01" + mocker.patch( + "superset.datasource.api.DatasourceDAO.get_datasource", + return_value=datasource, + ) + mocker.patch( + "superset.datasource.api.security_manager.get_rls_cache_key", + return_value=[], + ) + return datasource + + [email protected] +def data_cache(mocker: MockerFixture) -> MagicMock: + cache = mocker.patch("superset.datasource.api.cache_manager").data_cache + cache.get.return_value = None + return cache + + [email protected] +def stats_logger(mocker: MockerFixture) -> MagicMock: + stats_logger = MagicMock() + mocker.patch.dict( + current_app.config, + { + "DATA_CACHE_MAX_VALUE_SIZE": MAX_VALUE_SIZE, + "STATS_LOGGER": stats_logger, + }, + ) + return stats_logger + + +def _get_values(client: Any) -> Any: + return client.get("/api/v1/datasource/table/1/column/name/values/") + + +def _post_compatible(client: Any) -> Any: + return client.post( + "/api/v1/datasource/table/1/compatible", + json={"selected_metrics": [], "selected_dimensions": []}, + ) + + +def test_column_values_oversized_payload_is_not_cached( + client: Any, + full_api_access: None, + datasource: MagicMock, + data_cache: MagicMock, + stats_logger: MagicMock, + mocker: MockerFixture, +) -> None: + values = [f"value-{i:05d}" for i in range(500)] + datasource.values_for_column.return_value = values + mock_logger = mocker.patch("superset.utils.cache.logger") + + response = _get_values(client) + + assert response.status_code == 200 + assert response.json["result"] == values + data_cache.set.assert_not_called() + stats_logger.incr.assert_any_call("skip_cache_value_too_large") + mock_logger.warning.assert_called_once() + + +def test_column_values_normal_payload_is_cached( + client: Any, + full_api_access: None, + datasource: MagicMock, + data_cache: MagicMock, + stats_logger: MagicMock, +) -> None: + datasource.values_for_column.return_value = ["a", "b"] + + response = _get_values(client) + + assert response.status_code == 200 + data_cache.set.assert_called_once() + assert data_cache.set.call_args.args[1] == ["a", "b"] + assert call("skip_cache_value_too_large") not in stats_logger.incr.mock_calls + + +def test_compatible_oversized_result_is_not_cached( + client: Any, + full_api_access: None, + datasource: MagicMock, + data_cache: MagicMock, + stats_logger: MagicMock, +) -> None: + metrics = [f"metric_{i:05d}" for i in range(500)] + datasource.get_compatible_metrics.return_value = metrics + datasource.get_compatible_dimensions.return_value = [] + + response = _post_compatible(client) + + assert response.status_code == 200 + assert response.json["result"]["compatible_metrics"] == metrics + data_cache.set.assert_not_called() + stats_logger.incr.assert_any_call("skip_cache_value_too_large") + + +def test_compatible_normal_result_is_cached( + client: Any, + full_api_access: None, + datasource: MagicMock, + data_cache: MagicMock, + stats_logger: MagicMock, +) -> None: + datasource.get_compatible_metrics.return_value = ["count"] + datasource.get_compatible_dimensions.return_value = ["country"] + + response = _post_compatible(client) + + assert response.status_code == 200 + data_cache.set.assert_called_once() + assert data_cache.set.call_args.args[1] == { + "compatible_metrics": ["count"], + "compatible_dimensions": ["country"], + } diff --git a/tests/unit_tests/sql/execution/test_executor.py b/tests/unit_tests/sql/execution/test_executor.py index ac171ff2580..03075c2198c 100644 --- a/tests/unit_tests/sql/execution/test_executor.py +++ b/tests/unit_tests/sql/execution/test_executor.py @@ -2725,3 +2725,76 @@ def test_store_in_cache_skips_when_identity_unknown( executor._store_in_cache(result, "SELECT * FROM salaries", QueryOptions()) mock_cache_set.assert_not_called() + + +def _store_in_cache_with_cap( + mocker: MockerFixture, database: Database, row_count: int +) -> tuple[MagicMock, MagicMock, MagicMock]: + """Run ``_store_in_cache`` for a ``row_count``-row result under a 1 KB cap. + + :returns: the mocked ``data_cache.set``, stats logger, and cache-module logger + """ + from superset_core.queries.types import ( + QueryResult as QueryResultType, + StatementResult, + ) + + from superset.extensions import cache_manager + from superset.sql.execution.executor import SQLExecutor + + stats_logger = MagicMock() + mocker.patch.dict( + current_app.config, + { + "DATA_CACHE_MAX_VALUE_SIZE": 1024, + "STATS_LOGGER": stats_logger, + "CACHE_DEFAULT_TIMEOUT": 300, + }, + ) + mock_cache_set = mocker.patch.object(cache_manager.data_cache, "set") + mock_logger = mocker.patch("superset.utils.cache.logger") + + result = QueryResultType( + status=QueryStatus.SUCCESS, + statements=[ + StatementResult( + original_sql="SELECT name FROM users", + executed_sql="SELECT name FROM users", + data=pd.DataFrame( + {"name": [f"user-{i:05d}" for i in range(row_count)]} + ), + row_count=row_count, + ) + ], + ) + SQLExecutor(database)._store_in_cache( + result, "SELECT name FROM users", QueryOptions() + ) + return mock_cache_set, stats_logger, mock_logger + + +def test_store_in_cache_skips_oversized_result( + mocker: MockerFixture, database: Database, app_context: None +) -> None: + """A result larger than ``DATA_CACHE_MAX_VALUE_SIZE`` is not written; the skip + is logged and counted. A later ``_get_from_cache`` simply misses and the query + re-runs.""" + mock_cache_set, stats_logger, mock_logger = _store_in_cache_with_cap( + mocker, database, row_count=500 + ) + + mock_cache_set.assert_not_called() + stats_logger.incr.assert_called_once_with("skip_cache_value_too_large") + mock_logger.warning.assert_called_once() + + +def test_store_in_cache_writes_result_under_cap( + mocker: MockerFixture, database: Database, app_context: None +) -> None: + """A result under ``DATA_CACHE_MAX_VALUE_SIZE`` is cached as usual.""" + mock_cache_set, stats_logger, _ = _store_in_cache_with_cap( + mocker, database, row_count=2 + ) + + mock_cache_set.assert_called_once() + stats_logger.incr.assert_not_called() diff --git a/tests/unit_tests/tasks/test_async_queries.py b/tests/unit_tests/tasks/test_async_queries.py index 1641e7829ff..8bd51949abb 100644 --- a/tests/unit_tests/tasks/test_async_queries.py +++ b/tests/unit_tests/tasks/test_async_queries.py @@ -169,6 +169,11 @@ def test_execute_chart_query_publishes_cache_key_payload( "superset.tasks.async_queries.get_context", return_value=task_context, ) + # The result was persisted, so the client's read-back will hit. + mocker.patch( + "superset.tasks.async_queries.cache_manager.data_cache.has", + return_value=True, + ) execute_chart_query.func(_serialized_query(), user_id=7) @@ -178,6 +183,58 @@ def test_execute_chart_query_publishes_cache_key_payload( ) +def test_execute_chart_query_fails_loudly_when_result_not_cached( + mocker: MockerFixture, +) -> None: + """A successful-but-unpersisted result fails the task instead of resubmitting. + + When a result exceeds ``DATA_CACHE_MAX_VALUE_SIZE`` the cache write is skipped + but the query still succeeds. Async delivery relies on the client reading the + result back from the DATA cache, so a missing entry makes the endpoint's + ``force_cached`` read raise ``CacheLoadError`` and reschedule this task. If the + task reported success, the client would resubmit it endlessly. The task must + raise so the client sees a clear terminal error, and must not publish a + cache-key payload that would drive another read-back/resubmit cycle. + """ + from superset.exceptions import SupersetException + from superset.tasks.async_queries import execute_chart_query + + query_obj = mocker.MagicMock() + query_context = mocker.MagicMock() + query_context.queries = [query_obj] + # Query succeeded (no "error"), but the value was skipped for size. + query_context.get_df_payload_result.return_value.payload = { + "cache_key": "oversized-cache-key", + "error": None, + } + task_context = mocker.MagicMock() + + mocker.patch( + "superset.tasks.async_queries._resolve_user", + return_value=mocker.MagicMock(), + ) + mocker.patch("superset.tasks.async_queries.override_user") + mocker.patch( + "superset.tasks.async_queries.load_serialized_query", + return_value=query_context, + ) + mocker.patch( + "superset.tasks.async_queries.get_context", + return_value=task_context, + ) + # The oversized value was never written, so the read-back would miss. + mocker.patch( + "superset.tasks.async_queries.cache_manager.data_cache.has", + return_value=False, + ) + + with pytest.raises(SupersetException, match="DATA_CACHE_MAX_VALUE_SIZE"): + execute_chart_query.func(_serialized_query(), user_id=7) + + # It must NOT publish a cache-key payload the client would resubmit against. + task_context.update_task.assert_not_called() + + def test_execute_chart_query_reestablishes_form_data( mocker: MockerFixture, ) -> None: @@ -293,6 +350,10 @@ def test_execute_chart_query_reads_totals_key_from_dependency_payload( "superset.tasks.async_queries.get_context", return_value=task_context, ) + mocker.patch( + "superset.tasks.async_queries.cache_manager.data_cache.has", + return_value=True, + ) execute_chart_query.func(_serialized_query(), user_id=7, requires_totals=True) diff --git a/tests/unit_tests/utils/cache_test.py b/tests/unit_tests/utils/cache_test.py index c1fbafb7cf4..6adac429d22 100644 --- a/tests/unit_tests/utils/cache_test.py +++ b/tests/unit_tests/utils/cache_test.py @@ -397,6 +397,40 @@ def test_set_and_log_cache_under_threshold_metadata_db(mocker: MockerFixture) -> mock_session.add.assert_called_once_with(mock_cache_key.return_value) +def test_data_cache_max_value_size_default_is_protective() -> None: + """The shipped default caps oversized data-cache entries out of the box. + + The oversized-value skip is only effective when ``DATA_CACHE_MAX_VALUE_SIZE`` + has a value; a ``None`` default disables it and lets very large results pile + up in the cache backend. This asserts the default is a positive cap that still + exceeds ordinary chart/query payloads. Operators can raise it or set it to + ``None`` explicitly. + """ + import superset.config as config + + assert config.DATA_CACHE_MAX_VALUE_SIZE is not None + assert isinstance(config.DATA_CACHE_MAX_VALUE_SIZE, int) + # Comfortably above typical payloads, well below the multi-tens-of-MB outliers. + assert 1024 * 1024 <= config.DATA_CACHE_MAX_VALUE_SIZE <= 20 * 1024 * 1024 + + +def test_set_and_log_cache_applies_default_timeout(mocker: MockerFixture) -> None: + """Every persisted value carries a TTL: an unset timeout falls back to + ``CACHE_DEFAULT_TIMEOUT`` and is passed atomically to ``cache.set`` (SETEX), + never written TTL-less. Guards against a code path that could leave a data-cache + key with no expiry, which would let it linger in Redis indefinitely.""" + from superset.utils.cache import set_and_log_cache + + _patch_config(mocker) # CACHE_DEFAULT_TIMEOUT == 100, no explicit timeout + cache_instance = _make_cache_instance(mocker) + + set_and_log_cache(cache_instance, "my_key", {"df": "small"}) + + cache_instance.set.assert_called_once() + _, kwargs = cache_instance.set.call_args + assert kwargs["timeout"] == 100 + + def test_set_and_log_cache_set_failure_logs(mocker: MockerFixture) -> None: """A failure inside the try block is caught and logged as 'Could not cache key'.""" from superset.utils.cache import set_and_log_cache @@ -412,3 +446,152 @@ def test_set_and_log_cache_set_failure_logs(mocker: MockerFixture) -> None: mock_logger.warning.assert_called_once_with("Could not cache key %s", "my_key") mock_logger.exception.assert_called_once_with(boom) + + +def test_set_and_log_cache_shipped_default_caches_normal_payload( + mocker: MockerFixture, +) -> None: + """With the *shipped* default cap active, an ordinary payload still caches. + + Uses the real ``config.DATA_CACHE_MAX_VALUE_SIZE`` rather than a patched value, + so it proves the value we actually ship does not wrongly skip typical + chart/query results (which sit far below the cap). + """ + import superset.config as config + from superset.utils.cache import set_and_log_cache + + cfg = _patch_config( + mocker, DATA_CACHE_MAX_VALUE_SIZE=config.DATA_CACHE_MAX_VALUE_SIZE + ) + cache_instance = _make_cache_instance(mocker) + + set_and_log_cache(cache_instance, "my_key", {"df": "a normal-sized result"}) + + cache_instance.set.assert_called_once() + cfg["STATS_LOGGER"].incr.assert_any_call("set_cache_key") + assert ( + mocker.call("skip_cache_value_too_large") + not in cfg["STATS_LOGGER"].incr.mock_calls + ) + + +def test_set_and_log_cache_shipped_default_skips_oversized_payload( + mocker: MockerFixture, +) -> None: + """With the *shipped* default cap active, an over-cap payload is skipped. + + Builds a payload larger than the real ``config.DATA_CACHE_MAX_VALUE_SIZE`` and + asserts the end-to-end skip behavior the default exists to provide: no cache + write, a WARNING, and the ``skip_cache_value_too_large`` counter. + """ + import superset.config as config + from superset.utils.cache import set_and_log_cache + + max_size = config.DATA_CACHE_MAX_VALUE_SIZE + assert max_size is not None + cfg = _patch_config(mocker, DATA_CACHE_MAX_VALUE_SIZE=max_size) + cache_instance = _make_cache_instance(mocker) + mock_logger = mocker.patch("superset.utils.cache.logger") + + # A string this long pickles to more than ``max_size`` bytes. + set_and_log_cache(cache_instance, "my_key", {"df": "x" * (max_size + 1024)}) + + cache_instance.set.assert_not_called() + cfg["STATS_LOGGER"].incr.assert_called_once_with("skip_cache_value_too_large") + mock_logger.warning.assert_called_once() + + +def test_set_and_log_cache_one_byte_over_threshold(mocker: MockerFixture) -> None: + """A value one byte OVER the threshold is skipped (guard is strict ``>``). + + Complements ``test_set_and_log_cache_equal_threshold`` (== is cached) by pinning + the threshold to one below the exact serialized size. + """ + import pickle + + from superset.utils.cache import set_and_log_cache + + cache_value = {"df": "boundary"} + dttm = "2021-01-01T00:00:00" + value = {**cache_value, "dttm": dttm} + exact_size = len(pickle.dumps(value, protocol=pickle.HIGHEST_PROTOCOL)) + + cfg = _patch_config(mocker, DATA_CACHE_MAX_VALUE_SIZE=exact_size - 1) + cache_instance = _make_cache_instance(mocker) + mock_datetime = mocker.patch("superset.utils.cache.datetime") + mock_datetime.now.return_value.replace.return_value.isoformat.return_value = dttm + + set_and_log_cache(cache_instance, "my_key", cache_value) + + cache_instance.set.assert_not_called() + cfg["STATS_LOGGER"].incr.assert_called_once_with("skip_cache_value_too_large") + + +def test_set_and_log_cache_explicit_timeout_passthrough(mocker: MockerFixture) -> None: + """An explicit ``cache_timeout`` is forwarded verbatim to ``cache.set``, + taking precedence over ``CACHE_DEFAULT_TIMEOUT``.""" + from superset.utils.cache import set_and_log_cache + + _patch_config(mocker, DATA_CACHE_MAX_VALUE_SIZE=10 * 1024 * 1024) + cache_instance = _make_cache_instance(mocker) + + set_and_log_cache(cache_instance, "my_key", {"df": "small"}, cache_timeout=4242) + + cache_instance.set.assert_called_once() + _, kwargs = cache_instance.set.call_args + assert kwargs["timeout"] == 4242 + + +def test_set_and_log_cache_zero_timeout_never_expires(mocker: MockerFixture) -> None: + """A ``cache_timeout`` of 0 ("never expires") is preserved, not treated as + disabled. Only ``CACHE_DISABLED_TIMEOUT`` (-1) skips the write; 0 must pass + through to ``cache.set`` so the backend stores the key without expiry.""" + from superset.utils.cache import set_and_log_cache + + _patch_config(mocker, DATA_CACHE_MAX_VALUE_SIZE=10 * 1024 * 1024) + cache_instance = _make_cache_instance(mocker) + + set_and_log_cache(cache_instance, "my_key", {"df": "small"}, cache_timeout=0) + + cache_instance.set.assert_called_once() + _, kwargs = cache_instance.set.call_args + assert kwargs["timeout"] == 0 + + +def test_exceeds_max_cache_value_size_disabled_skips_serialization( + mocker: MockerFixture, +) -> None: + """With the cap set to ``None`` nothing is too large and nothing is pickled.""" + from superset.utils.cache import exceeds_max_cache_value_size + + _patch_config(mocker, DATA_CACHE_MAX_VALUE_SIZE=None) + mock_dumps = mocker.patch("superset.utils.cache.pickle.dumps") + + assert exceeds_max_cache_value_size("my_key", ["x" * 1024]) is False + mock_dumps.assert_not_called() + + +def test_exceeds_max_cache_value_size_boundary(mocker: MockerFixture) -> None: + """A value exactly at the cap fits; one byte over does not and is reported.""" + import pickle + + from superset.utils.cache import exceeds_max_cache_value_size + + value = ["a", "b", "c"] + exact_size = len(pickle.dumps(value, protocol=pickle.HIGHEST_PROTOCOL)) + mock_logger = mocker.patch("superset.utils.cache.logger") + + config = _patch_config(mocker, DATA_CACHE_MAX_VALUE_SIZE=exact_size) + assert exceeds_max_cache_value_size("my_key", value) is False + config["STATS_LOGGER"].incr.assert_not_called() + mock_logger.warning.assert_not_called() + + config = _patch_config(mocker, DATA_CACHE_MAX_VALUE_SIZE=exact_size - 1) + assert exceeds_max_cache_value_size("my_key", value) is True + config["STATS_LOGGER"].incr.assert_called_once_with("skip_cache_value_too_large") + mock_logger.warning.assert_called_once() + assert mock_logger.warning.call_args.args[1:] == ( + "my_key", + exact_size, + exact_size - 1, + )
