(superset) branch master updated: fix(caching): sort extra_cache_keys before hashing (#34543) (#42597)

[ru...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git


The following commit(s) were added to refs/heads/master by this push:
     new 8f6587d0e67 fix(caching): sort extra_cache_keys before hashing (#34543) (#42597)
8f6587d0e67 is described below

commit 8f6587d0e67ff155e104b2f9ead1a26c0a4268ff
Author: Evan Rusackas <ev...@preset.io>
AuthorDate: Tue Aug 11 23:34:01 2026 -0700

    fix(caching): sort extra_cache_keys before hashing (#34543) (#42597)
    
    Co-authored-by: Claude Code <no...@anthropic.com>
---
 superset/common/query_object.py               | 16 +++++++
 tests/unit_tests/queries/query_object_test.py | 67 +++++++++++++++++++++++++++
 2 files changed, 83 insertions(+)

diff --git a/superset/common/query_object.py b/superset/common/query_object.py
index f2ef6dca55d..7dd030854ec 100644
--- a/superset/common/query_object.py
+++ b/superset/common/query_object.py
@@ -440,6 +440,22 @@ class QueryObject:  # pylint: disable=too-many-instance-attributes
         cache_dict: dict[str, Any] = dict(self.to_dict())
         cache_dict.update(extra)
 
+        if "extra_cache_keys" in cache_dict:
+            # Order carries no meaning here (an unordered set of opaque
+            # Jinja url_param()-derived values), but hash_from_dict only
+            # sorts dict keys, not list values, so an unsorted list makes
+            # the cache key depend on Python's per-process hash-randomized
+            # set iteration order (see SqlaTable.get_extra_cache_keys).
+            # Normalize once here so every producer of extra_cache_keys is
+            # safe by construction. Sort on (type name, str value) rather
+            # than a bare str() so values that stringify identically but
+            # differ in type (e.g. 1 and "1") still sort deterministically
+            # instead of falling back to input order.
+            cache_dict["extra_cache_keys"] = sorted(
+                cache_dict["extra_cache_keys"],
+                key=lambda value: (type(value).__name__, str(value)),
+            )
+
         # TODO: the below KVs can all be cleaned up and moved to `to_dict()` at some
         #  predetermined point in time when orgs are aware that the previously
         #  cached results will be invalidated.
diff --git a/tests/unit_tests/queries/query_object_test.py b/tests/unit_tests/queries/query_object_test.py
index 15fbc13c3eb..122e07ba4bc 100644
--- a/tests/unit_tests/queries/query_object_test.py
+++ b/tests/unit_tests/queries/query_object_test.py
@@ -21,6 +21,7 @@ from flask_appbuilder.security.sqla.models import User
 from superset.common.query_object import QueryObject
 from superset.connectors.sqla.models import SqlaTable
 from superset.models.core import Database
+from superset.superset_typing import Metric
 from superset.utils.core import override_user
 
 
@@ -86,6 +87,72 @@ def test_cache_key_changes_for_new_query_object_different_params():
     assert query_object2.cache_key() != cache_key1
 
 
+def test_cache_key_stable_regardless_of_extra_cache_keys_order():
+    """
+    Regression for #34543: the cache key must not depend on the order of
+    ``extra_cache_keys``.
+
+    ``SqlaTable.get_extra_cache_keys`` (superset/connectors/sqla/models.py)
+    returns ``list(set(extra_cache_keys))``. Python's string hashing is
+    randomized per-process (``PYTHONHASHSEED``), so the same set of values
+    can iterate in a different order in the Celery worker process (which
+    writes the query results to cache) than in the web process (which
+    re-derives the cache key to read them back). Because ``hash_from_dict``
+    only sorts dict keys and not list values, two ``extra_cache_keys`` lists
+    with identical Jinja ``url_param()`` values but different order hash to
+    different cache keys, causing async chart-data lookups to 422 with
+    "Error loading data from cache" whenever more than one url_param is
+    referenced (a single-element list has only one possible order, which is
+    why the bug is only visible with multiple parameters).
+    """
+    query_object1 = QueryObject(row_limit=1)
+    query_object2 = QueryObject(row_limit=1)
+    same_values_different_order = ["CAR_IDS=1,2,3", "CHASSIS_IDS=100,200"]
+    cache_key1 = query_object1.cache_key(extra_cache_keys=same_values_different_order)
+    cache_key2 = query_object2.cache_key(
+        extra_cache_keys=list(reversed(same_values_different_order))
+    )
+    assert cache_key1 == cache_key2
+
+
+def test_cache_key_stable_for_mixed_type_extra_cache_keys():
+    """
+    ``extra_cache_keys`` values are typed as ``Hashable``, so a mix of
+    strings and non-strings that stringify identically (e.g. ``1`` and
+    ``"1"``) can appear together. Sorting on a bare ``str()`` value treats
+    those as equal keys, so Python's stable sort would fall back to
+    whatever order they arrived in from ``list(set(...))`` -- which is not
+    deterministic across processes. The sort key must also account for
+    type so ordering doesn't silently regress to that non-determinism.
+    """
+    query_object1 = QueryObject(row_limit=1)
+    query_object2 = QueryObject(row_limit=1)
+    mixed_values = ["CAR_IDS=1,2,3", 1, "1", None]
+    cache_key1 = query_object1.cache_key(extra_cache_keys=mixed_values)
+    cache_key2 = query_object2.cache_key(extra_cache_keys=list(reversed(mixed_values)))
+    assert cache_key1 == cache_key2
+
+
+def test_cache_key_sensitive_to_orderby_order():
+    """
+    Negative control for the ``extra_cache_keys`` fix above: unlike that
+    field, ``orderby`` is order-significant (it determines sort direction
+    of the executed SQL), so the cache key must still change when the
+    order of its entries changes. This guards against a fix that
+    canonicalizes list values generically instead of targeting
+    ``extra_cache_keys`` specifically.
+    """
+    metric_a: Metric = "count"
+    metric_b: Metric = "sum__value"
+    query_object1 = QueryObject(
+        row_limit=1, orderby=[(metric_a, True), (metric_b, False)]
+    )
+    query_object2 = QueryObject(
+        row_limit=1, orderby=[(metric_b, False), (metric_a, True)]
+    )
+    assert query_object1.cache_key() != query_object2.cache_key()
+
+
 def test_cache_key_changes_for_new_query_object_same_params():
     """
     When a new query object is created with the same params,