bito-code-review[bot] commented on code in PR #42597:
URL: https://github.com/apache/superset/pull/42597#discussion_r3707363638
##########
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:
<!-- Bito Reply -->
The reviewer's suggestion to include mixed-type cache-key components in the
regression test was evaluated. Local testing confirms that sorting by `str()`
representation avoids `TypeError` for mixed types, and the proposed test case
correctly handles heterogeneous values without error. The current
implementation is appropriate for verifying the cache key stability.
--
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]
