Abdulrehman-PIAIC80387 commented on PR #40839: URL: https://github.com/apache/superset/pull/40839#issuecomment-4646074677
**Bot review summary (addressed in [`92654da298`](https://github.com/apache/superset/pull/40839/commits/92654da298b8ccb419fc5c826fbd11e82e9c601a))** Thanks to the automated reviewers (`@codeant-ai-for-open-source`, `@bito-code-review`) for the careful pass. Recording status for the eventual human reviewer: **✅ Resolved** 1. **🔴 CRITICAL — Guest-user RLS leak** (`api.py:164`) `get_user_id()` returns `None` for guest/anonymous sessions, so embedded dashboards with different guest-token RLS would have shared a `"user": null` cache entry. Fix: cache key now includes `security_manager.get_rls_cache_key(datasource)` — the same canonical RLS fingerprint already used by `viz.py:479` and `query_context_processor.py:239`. Two guest sessions with different RLS now have distinct cache keys. Test: `test_get_column_values_cache_isolated_per_rls_context`. 2. **⚠️ Major — RLS policy changes don't bust cache** (`api.py:163`) Tightening an RLS rule wouldn't have evicted previously cached values until natural TTL expiry. Fix: same change as above. The RLS fingerprint includes both `get_rls_sorted(datasource)` clauses and `get_guest_rls_filters_str(datasource)`, so any policy change shifts the fingerprint and the next request misses the cache. **⚪ Deferred (already noted in the PR description)** 3. **⚠️ Major — Cache stampede / thundering herd** (`api.py:186`) Concurrent first-load requests can each miss the cache and execute the query. Doing this correctly across all `cache_manager.data_cache` backends (Redis `SETNX`, Memcached `add`, in-process `SimpleCache`) is non-trivial — ~80–100 lines with cross-backend tests. Out of scope for this PR; will open a follow-up if maintainers want it bundled. **Other automated checks** - `@bito-code-review` — "Actionable Suggestions — 0" after the bot accepted that the RLS fix addresses the flagged issue. - `@codecov` — the initial 5.88% patch coverage was reported before the integration tests ran; the seven cache tests in `tests/integration_tests/datasource/api_tests.py` cover the new code paths and the latest CI run shows 0 failures across 54 checks. I'll resolve the two addressed inline threads to keep the review focus on what's open. -- 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]
