fix(caching): sort extra_cache_keys before hashing (#34543) - #42597
Conversation
dosubot
Bot
added
global:async-query
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42597 +/- ##
=======================================
Coverage 66.74% 66.74%
=======================================
Files 2862 2862
Lines 161748 161750 +2
Branches 37312 37313 +1
=======================================
+ Hits 107962 107966 +4
+ Misses 51729 51727 -2
Partials 2057 2057
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The current regression test uses only strings, which masks potential Here is the updated test case: query_object1 = QueryObject(row_limit=1)
query_object2 = QueryObject(row_limit=1)
# Mixed types to test sorting robustness
mixed_values_different_order = ["CAR_IDS=1,2,3", 123, None]
cache_key1 = query_object1.cache_key(extra_cache_keys=mixed_values_different_order)
cache_key2 = query_object2.cache_key(
extra_cache_keys=list(reversed(mixed_values_different_order))
)
assert cache_key1 == cache_key2Would you like me to check the rest of the comments on this PR and implement fixes for them as well? tests/unit_tests/queries/query_object_test.py |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #fc1c01
Actionable Suggestions - 1
-
tests/unit_tests/queries/query_object_test.py - 1
- Test reveals unfixed cache key ordering bug · Line 89-114
Review Details
-
Files reviewed - 1 · Commit Range:
a0e2dc6..a0e2dc6- tests/unit_tests/queries/query_object_test.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| 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) |
There was a problem hiding this comment.
This assertion requires canonicalizing extra_cache_keys inside QueryObject.cache_key, so fixing the nondeterminism at SqlaTable.get_extra_cache_keys would resolve both this path and the legacy BaseViz consumer but still leave this test red. Could this exercise the producer boundary instead so the regression test does not force the narrower fix location?
There was a problem hiding this 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.
| 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( |
There was a problem hiding this comment.
The real producer appends only raw url_param() values, so list position is what distinguishes which template call produced each value. Making ["1", "2"] equivalent to ["2", "1"] lets swapped parameters render different SQL while sharing a data-cache key and can return the wrong cached rows. Could this test deterministic ordered deduplication at get_extra_cache_keys() instead of declaring the final list order-insensitive?
There was a problem hiding this 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.
rusackas
changed the title
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #c49b89Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Per review feedback on #42597: the extra_cache_keys ordering fix canonicalizes only that field, not list values generically. Add a companion test asserting orderby order still changes the cache key, so a future refactor can't accidentally widen the canonicalization. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per review feedback on #42597: the extra_cache_keys ordering fix canonicalizes only that field, not list values generically. Add a companion test asserting orderby order still changes the cache key, so a future refactor can't accidentally widen the canonicalization. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3d6bc70 to
dd86b72
Compare
Code Review Agent Run #9fedffActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #ac7024Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Traced the full producer→consumer path at 096583f to settle the correctness question behind the two open threads. Summary up front so Joe can decide on his threads.
Producer boundary: the values reach cache_key() already set-derived
Every path into QueryObject.cache_key's extra_cache_keys comes through QueryContextProcessor.query_cache_key → datasource.get_extra_cache_keys(...). The SqlaTable implementation ends with:
# superset/connectors/sqla/models.py:2242
return list(set(extra_cache_keys))The other implementations (Query, semantic layer) return []. So by the time the list reaches cache_key(), the raw positional url_param() appends in cache_key_wrapper (jinja_context.py:254) have already been collapsed into a set — position is destroyed and values are deduplicated before this code ever sees them.
That makes the in-code comment ("order carries no meaning — an unordered set") accurate for the real value domain, and it means sorting a set-derived list is a pure canonicalization: sorted(set(X)) maps each distinct set to exactly one deterministic list under the total order (type name, str value). It cannot merge two distinct sets, so it introduces zero collisions the existing set() didn't already have.
Re: the data-corruption thread (["1","2"] ≡ ["2","1"] serving wrong rows)
url_param appends the interpolated value only (jinja_context.py:304), not a (param, value) pair, and the producer already set()s the result. So {"1","2"} and {"2","1"} were the same set — and thus the same intended cache key — before this PR. Pre-PR they only sometimes collided (whichever way set iteration happened to land that process); the actual observable bug was the opposite of corruption — a cache miss across web/worker processes with different PYTHONHASHSEED. This PR removes that nondeterminism without widening any equivalence class. I don't see a data-correctness regression on this path.
Fix-location thread — agree on merit
The legacy BaseViz.cache_key consumes the same set-derived list and is not fixed by this PR:
# superset/viz.py:489, 502
cache_dict["extra_cache_keys"] = self.datasource.get_extra_cache_keys(query_obj)
...
json_data = self.json_dumps(cache_dict, sort_keys=True) # sorts dict KEYS, not list VALUESsort_keys=True canonicalizes dict keys but leaves the extra_cache_keys list order untouched, so the legacy path keeps the exact nondeterminism this PR removes from QueryObject. Fixing at the producer — return sorted(set(extra_cache_keys), key=lambda v: (type(v).__name__, str(v))) in SqlaTable.get_extra_cache_keys — would cover both consumers in one place, and the code comment's "so every producer of extra_cache_keys is safe by construction" would then actually hold for every consumer too. viz.py is @deprecated(3.0), so this is lower-urgency and defensible to defer, but Joe's architectural point is correct: the producer is the more complete location. Worth at least a comment noting the legacy path is knowingly left as-is. (These two points are Joe's to resolve, not mine.)
Other scope (no blocking findings)
- TypeError / mixed types: safe. The key
(type(value).__name__, str(value))yields a(str, str)tuple for everyHashable(None→('NoneType','None'),1→('int','1'),"1"→('str','1'), tuples→('tuple', '...')), which is always totally ordered. No incomparable-typeTypeErrorreaches the chart-data path.codeant's earlierkey=strtie concern is genuinely resolved by the type-qualified key. - Blast radius: the branch is a strict no-op when
extra_cache_keysis absent (guarded byif "extra_cache_keys" in cache_dict), so only Jinja/url_paramcharts are touched. Expect a one-time cache-invalidation event on deploy for those entries (they were keyed on unstable order and were already missing cross-process), which is acceptable and consistent with the existing invalidation TODO just below. - Negative control: present and correct.
test_cache_key_sensitive_to_orderby_orderasserts reorderingorderbystill changes the key, which would fail under a generic "canonicalize every list" fix — so it properly pins the fix toextra_cache_keysspecifically rather than to a blankethash_from_dictchange.
Rule-26 note (test env here lacks superset_core, so this is by inspection): reverting only the query_object.py hunk fails the two order-stability tests and leaves the orderby negative control passing — the tests target the production line rather than tautologies.
|
@aminghadersohi thanks for tracing that through, matches how I read it too. Went to add a note on the legacy |
This is a test-only PR opened as a TDD-style validation of issue #34543. #34543 (filed 2025-08) reports that embedded dashboards with multiple Jinja url_param() filters fail async cache retrieval with a 422 "Error loading data from cache", while a single url_param works fine. Root cause: SqlaTable.get_extra_cache_keys() (superset/connectors/sqla/models.py) returns list(set(extra_cache_keys)). Python randomizes string hashing per-process, so the same set of url_param values can iterate in a different order in the Celery worker (which writes the query results to cache) than in the web process (which re-derives the cache key to read them back). hash_from_dict() only sorts dict keys, not list values, so two extra_cache_keys lists with identical values but different order hash to different cache keys. A single-element list has only one possible order, which is why the bug only appears with multiple parameters. This PR adds one regression test on QueryObject.cache_key(): 1. test_cache_key_stable_regardless_of_extra_cache_keys_order - asserts the cache key is identical for two otherwise-equal query objects whose extra_cache_keys differ only in order. Closes #34543 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
QueryObject.cache_key() merges extra_cache_keys (produced by SqlaTable.get_extra_cache_keys, ultimately from Jinja url_param() calls) straight into the hashed cache_dict in whatever order the caller passed. hash_from_dict only sorts dict keys via json.dumps(sort_keys=True), never list contents, so two otherwise- identical queries whose extra_cache_keys list the same values in a different order hash to different cache keys. SqlaTable.get_extra_cache_keys itself returns list(set(...)), and Python's per-process string-hash randomization means that set can iterate in a different order in the Celery worker that writes a chart's cached result than in the web process that later re-derives the cache key to read it back, whenever 2+ url_params are involved (a single-element list has only one possible order, matching the reported single-vs-multi-parameter split exactly). Order carries no meaning for this field, it's a set of opaque Jinja-derived values, so sort it once where it enters cache_key() rather than at every producer, making any future extra_cache_keys source safe by construction (this also covers the pre-existing call site in superset/common/query_context_processor.py, which passes datasource.get_extra_cache_keys() straight through the same cache_key(extra_cache_keys=...) path). Closes #34543 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per review feedback on #42597: the extra_cache_keys ordering fix canonicalizes only that field, not list values generically. Add a companion test asserting orderby order still changes the cache key, so a future refactor can't accidentally widen the canonicalization. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`sorted(..., key=str)` treats values with identical string representations (e.g. `1` and `"1"`) as equal, so ties could still fall back to non-deterministic set-iteration order. Sort on (type name, str value) instead so ties can't happen, plus a regression test for it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
49fd9c0 to
49593d1
Compare
Code Review Agent Run #bb3da7Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
4 participants
SUMMARY
Fixes #34543. Embedded dashboards with multiple Jinja
url_param()filters failed async chart-data cache retrieval with a422 Unprocessable Entity/ "Error loading data from cache", while a singleurl_paramworked fine.Root cause:
SqlaTable.get_extra_cache_keys()(superset/connectors/sqla/models.py) returnslist(set(extra_cache_keys)). Python randomizes string hashing per-process (PYTHONHASHSEED), so the same set ofurl_param()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 later re-derives the cache key to read them back).QueryObject.cache_key()merges that list straight into the dict it hashes, in whatever order it arrives;hash_from_dict()only sorts dict keys, never list values. A single-element list has only one possible order, which is exactly why the bug is only visible with 2+ url_params, matching the reported single-vs-multi-parameter split precisely.This was originally opened as a test-only TDD PR pinning the gap down; this update adds the actual fix.
THE FIX
QueryObject.cache_key()(superset/common/query_object.py): sortextra_cache_keys(bystr, since entries are justHashable, not guaranteed mutually orderable) right where it enters the dict that gets hashed, before callinghash_from_dict(). Order carries no meaning for this field, it's an unordered set of opaque Jinja-derived values, so normalizing it here makes any current or future producer ofextra_cache_keyssafe by construction, rather than patching each producer individually.TESTING INSTRUCTIONS
test_cache_key_stable_regardless_of_extra_cache_keys_orderwas expected/confirmed red before the fix (asserts two otherwise-identical query objects withextra_cache_keysdiffering only in order produce the same cache key); now green. Confirmed the rest oftests/unit_tests/queries/,tests/unit_tests/common/, andtests/unit_tests/charts/(344 tests) are unaffected.ADDITIONAL INFORMATION
GLOBAL_ASYNC_QUERIES(only needed to reproduce the end-to-end symptom; the fix and test operate on the cache-key logic directly)🤖 Generated with Claude Code