sadpandajoe commented on code in PR #42597:
URL: https://github.com/apache/superset/pull/42597#discussion_r3684891819
##########
tests/unit_tests/queries/query_object_test.py:
##########
@@ -86,6 +86,34 @@ 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
Review Comment:
This assertion would also pass if `hash_from_dict` canonicalized every list,
which would collapse order-significant fields such as `columns`, `orderby`, or
`post_processing` and could serve another query's cached rows. Could you add a
negative control showing that reordering one of those query fields still
changes the cache key?
--
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]
