codeant-ai-for-open-source[bot] commented on code in PR #42938:
URL: https://github.com/apache/superset/pull/42938#discussion_r3744384588
##########
superset/utils/rls.py:
##########
@@ -181,6 +182,12 @@ def collect_rls_predicates_for_sql(
}
)
except Exception:
- # If we can't parse the SQL, return empty list
- # This ensures RLS application failure doesn't break caching
- return []
+ # If we can't parse the SQL, we can't tell which (if any) RLS
+ # predicates would apply, so we can't contribute a meaningful cache
+ # key component. Returning an empty list here would make every
+ # user's failure collapse onto the same (missing) contribution,
+ # which is unsafe when different users have different RLS scopes on
+ # the underlying tables. Fall back to a per-user marker instead, so
+ # the cache key still varies by user even though we don't know the
+ # actual predicates.
+ return [f"rls-predicate-parse-failed-for-user-{get_user_id()}"]
Review Comment:
**Suggestion:** The fallback is not actually isolated for guest-token
requests: `get_user_id()` returns `None` for `GuestUser`, while guest RLS rules
are derived from the token's `rls_rules` and can differ between tokens.
Consequently, parse failures for different guest scopes all contribute
`rls-predicate-parse-failed-for-user-None`, allowing their virtual-dataset
cache entries to collide. Use a stable identifier derived from the guest token
or its RLS scope instead of only the database user id. [cache]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Guest RLS cache contributions collide during parse failures.
- ❌ Virtual-dataset results may cross guest scopes.
- ⚠️ Embedded dashboards are affected when SQL parsing fails.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2dd4b582e7684e00a75175f5f7b70ec0&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=2dd4b582e7684e00a75175f5f7b70ec0&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/utils/rls.py
**Line:** 193:193
**Comment:**
*Cache: The fallback is not actually isolated for guest-token requests:
`get_user_id()` returns `None` for `GuestUser`, while guest RLS rules are
derived from the token's `rls_rules` and can differ between tokens.
Consequently, parse failures for different guest scopes all contribute
`rls-predicate-parse-failed-for-user-None`, allowing their virtual-dataset
cache entries to collide. Use a stable identifier derived from the guest token
or its RLS scope instead of only the database user id.
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%2F42938&comment_hash=d44dac41a455f344409bba717e3a1294f7c313812dafa9cc631f40d3d3de1c07&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42938&comment_hash=d44dac41a455f344409bba717e3a1294f7c313812dafa9cc631f40d3d3de1c07&reaction=dislike'>👎</a>
##########
superset/commands/database/sync_permissions.py:
##########
@@ -313,14 +318,14 @@ def _rename_database_in_permissions(
@celery_app.task(name="sync_database_permissions", soft_time_limit=600)
def sync_database_permissions_task(
- database_id: int, username: str, old_db_connection_name: str
+ database_id: int, user_id: int, old_db_connection_name: str
Review Comment:
**Suggestion:** The Celery task's second argument changed from the queued
username value to a user id without preserving compatibility with messages
already published under the previous contract. During a rolling deployment, a
worker running this version can consume an old three-argument message, pass the
username string to `get_user_by_id()`, and then swallow
`UserNotFoundInSessionError`, leaving that permission sync undone. Version the
task name or accept and translate the legacy payload while old messages may
remain in the broker. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Rolling deployments can strand queued permission synchronizations.
- ❌ Database permissions remain stale after failed task consumption.
- ⚠️ The task logs failure without retrying the incompatible message.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7b331ec979b84693af35a230d328b2ed&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=7b331ec979b84693af35a230d328b2ed&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/commands/database/sync_permissions.py
**Line:** 320:321
**Comment:**
*Api Mismatch: The Celery task's second argument changed from the
queued username value to a user id without preserving compatibility with
messages already published under the previous contract. During a rolling
deployment, a worker running this version can consume an old three-argument
message, pass the username string to `get_user_by_id()`, and then swallow
`UserNotFoundInSessionError`, leaving that permission sync undone. Version the
task name or accept and translate the legacy payload while old messages may
remain in the broker.
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%2F42938&comment_hash=d6a8a6f9fed9af61f4a4c4068f66cc0e667b4eef8582d2196464489461863bc7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42938&comment_hash=d6a8a6f9fed9af61f4a4c4068f66cc0e667b4eef8582d2196464489461863bc7&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]
