rusackas commented on code in PR #42597:
URL: https://github.com/apache/superset/pull/42597#discussion_r3707362690
##########
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)
Review Comment:
Went with sorting at `cache_key()` over the producer since there's more than
one path feeding extra_cache_keys in, `get_extra_cache_keys()` and
`query_context_processor.py` both land there. One sort at the boundary covers
both instead of chasing each producer.
##########
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(
Review Comment:
`SqlaTable.get_extra_cache_keys` already does `list(set(extra_cache_keys))`
before this ever reaches `cache_key()`, so any positional signal from
`url_param()` call order is already gone by the time we see it. Nothing to lose
by sorting.
##########
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:
Added a negative control for that, orderby order still changes the cache key
so we're not over-canonicalizing.
--
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]
