aminghadersohi commented on PR #45814:
URL: https://github.com/apache/superset/pull/45814#issuecomment-6467203955
Product acceptance run on a real stack (docker compose: Postgres metadata +
warehouse, Redis data cache, embedded on, roles with RLS only on the physical
tables read by the virtual datasets), comparing the merge base
`a22013d37945c66aab94a70b3234f22b595fbefc` with this branch over real HTTP
sessions.
**Result:** all five cases pass on the branch and the leaks reproduce at the
merge base:
| Case | Merge base | Branch |
|---|---|---|
| Column values on a virtual dataset, viewers with different rules on the
inner table | second viewer gets a cache HIT with the first viewer's values (8
of the first tenant's values, 0 of its own) | own entry, own values |
| Same, via search (`q=`), a SQL Lab query datasource, and the MCP filter
validation helper | HIT with the other viewer's values | own values |
| Two embedded guests, token rules differing only on the inner table,
chart-backed annotation | second guest `is_cached: true` with the first guest's
annotation (57394.5; its own is 57396.0) | own annotation, 57396.0 |
| Same two predicates swapped between two inner tables | column values HIT
`['North']` for the `'South'` viewer; chart data also served from the other
viewer's cached result | own values and own chart result |
| Templated virtual SQL that does not parse raw | HIT with another viewer's
values | per-viewer entries |
| Identical effective RLS | shared | still shared (cache HIT) |
| Physical datasets | — | column-values Redis keys and chart cache keys
byte-identical to the merge base |
**One issue found and fixed in 1072cc722b2eced6c66f2ac6bbe337ca7db40ef6:**
`get_inner_rls_cache_key` looked up the database's default schema before
collecting predicates, even for datasets with a schema. That lookup opens a
connection, so every column-values request on a virtual dataset connected to
the warehouse, cache hits included. With the warehouse stopped, the merge base
still served cached filter values (HIT) while
6e2d289abdb2fe651087fec1306c71cd8d41865c returned a 500. The default schema is
now looked up only when the dataset or query has no schema.
- Regression test:
`test_cached_column_values_are_served_without_reaching_the_database` fails at
6e2d289abdb2fe651087fec1306c71cd8d41865c (`ConnectionError`) and passes at
1072cc722b2eced6c66f2ac6bbe337ca7db40ef6. In the stack, with the warehouse
stopped, the cached values are served again (HIT).
- Searched with `grep -rn "get_default_schema(" superset`.
- Siblings checked:
- `superset/connectors/sqla/models.py:3254` — fixed in
1072cc722b2eced6c66f2ac6bbe337ca7db40ef6
- `superset/models/sql_lab.py:426` — fixed in
1072cc722b2eced6c66f2ac6bbe337ca7db40ef6 (the chart cache key and the
annotation context call these two methods)
- `superset/utils/rls.py:222` — not affected: only reached for datasets
registered without a schema, inside the fail-closed `try`
- `superset/models/helpers.py:4128` — not affected: query execution, which
connects anyway
I also updated the description: binding predicates to their table changes
the chart cache key for virtual datasets with inner rules (one-time recompute
after upgrade), and it closes the swapped-rules collision in the chart-data
cache too.
--
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]
