codeant-ai-for-open-source[bot] commented on code in PR #45814:
URL: https://github.com/apache/superset/pull/45814#discussion_r4400534633
##########
superset/connectors/sqla/models.py:
##########
@@ -3239,6 +3227,35 @@ class and any keys added via `ExtraCache`.
return list(set(extra_cache_keys))
+ def get_inner_rls_cache_key(self) -> list[str]:
+ """
+ The table-bound RLS predicates execution injects into the tables read
+ by this dataset's SQL, as cache key components.
+
+ ``security_manager.get_rls_cache_key`` covers only the dataset's own
+ rules, so a cache of a virtual dataset's results must also key on
these,
+ or viewers whose RLS differs only on an inner table share entries.
Empty
+ for a physical dataset. If the predicates cannot be collected, a
+ viewer-scoped marker is returned instead.
+
+ :return: The RLS predicates, one cache key component each
+ """
+ from superset.utils.rls import collect_rls_predicates_for_sql
+
+ if not (self.is_virtual and self.sql):
+ return []
+ # The default schema is looked up only when the dataset has none: the
+ # lookup connects to the database, which serving a cached entry must
+ # not depend on.
+ return collect_rls_predicates_for_sql(
+ self.sql,
+ self.database,
+ self.catalog,
+ self.schema or self.database.get_default_schema(self.catalog) or
"",
Review Comment:
**Suggestion:** `get_default_schema()` opens a database connection before
the collector can catch failures, so a schema-less virtual dataset cannot build
its annotation cache key during an outage.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Cache`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7790cb7458f0416cb82117b28268c798&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7790cb7458f0416cb82117b28268c798&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/connectors/sqla/models.py
**Line:** 3254:3254
**Comment:**
*Cache: `get_default_schema()` opens a database connection before the
collector can catch failures, so a schema-less virtual dataset cannot build its
annotation cache key during an outage.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45814&comment_hash=5fe0269ce8095279f194c0b6100241f9f1bd03cc06c1ac8629d766bfd75a9669&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45814&comment_hash=5fe0269ce8095279f194c0b6100241f9f1bd03cc06c1ac8629d766bfd75a9669&reaction=dislike'>👎</a>
##########
superset/datasource/api.py:
##########
@@ -102,6 +103,13 @@ def _column_values_cache_key(
rls=security_manager.get_rls_cache_key(datasource),
changed_on=str(getattr(datasource, "changed_on", "")),
)
+ # Executing a virtual dataset (or a SQL Lab query) also injects the RLS of
+ # the tables its SQL reads, which ``get_rls_cache_key`` does not cover.
+ # Only added when non-empty, so keys without inner rules keep their bytes.
+ if isinstance(datasource, (SqlaTable, Query)) and (
+ inner_rls := datasource.get_inner_rls_cache_key()
+ ):
Review Comment:
**Suggestion:** `get_inner_rls_cache_key()` looks up the default schema
before the collector’s fallback; on a schema-less virtual dataset or SQL Lab
query, an outage raises before a cached value can be read.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Cache`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8f21d6ab16354c6a8cdc99c09ea70d11&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8f21d6ab16354c6a8cdc99c09ea70d11&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/datasource/api.py
**Line:** 109:111
**Comment:**
*Cache: `get_inner_rls_cache_key()` looks up the default schema before
the collector’s fallback; on a schema-less virtual dataset or SQL Lab query, an
outage raises before a cached value can be read.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45814&comment_hash=76919aa8b4f0f819bb40293d300d1e03ad88d6e6d303f6bd53348c294e188bbf&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45814&comment_hash=76919aa8b4f0f819bb40293d300d1e03ad88d6e6d303f6bd53348c294e188bbf&reaction=dislike'>👎</a>
--
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]
