You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "kgabryje (via GitHub)" <gi...@apache.org> on 2026/02/12 19:19:37 UTC
[PR] perf(dashboard): Remove thumbnail_url from detail API response [superset]
kgabryje opened a new pull request, #37941:
URL: https://github.com/apache/superset/pull/37941
### SUMMARY
The `GET /api/v1/dashboard/{id}` endpoint computes thumbnail_url in its response, which triggers `get_dashboard_digest() β _adjust_string_with_rls()`. This evaluates RLS filters for every unique datasource in the dashboard, adding significant latency when there are many RLS filters.
The thumbnail_url is unnecessary on the detail endpoint (you don't need a thumbnail of the dashboard you're already viewing) so it is safe to remove it from the schema.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
Call GET /api/v1/dashboard/{id} and confirm the response no longer includes thumbnail_url
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(dashboard): Batch RLS filter lookups for dashboard digest computation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37941:
URL: https://github.com/apache/superset/pull/37941#issuecomment-3909904643
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(dashboard): Batch RLS filter lookups for dashboard digest computation [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #37941:
URL: https://github.com/apache/superset/pull/37941#issuecomment-3898235151
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | 0e8c2c66dc434232fff543727d90e6cfaf992417 |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/698f55140ca3c300082b3063 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-37941--superset-docs-preview.netlify.app](https://deploy-preview-37941--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-37941--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(dashboard): Remove thumbnail_url from detail API response [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #37941:
URL: https://github.com/apache/superset/pull/37941#issuecomment-3893040237
Talked with @kgabryje here, we might need to change the approach as the Dashboards list uses `thumbnail_url` for `CardView`.
One thing I was thinking, is that currently:
1. The Dashboards list on `ListView` loads with `select_columns` to avoid including `thumbnail_url` (for performance reasons).
2. When switching to `CardView`, a new request fires to `/api/v1/dashboard/{id}` for each item in the list, so that `thumbnail_url` is available.
At least on my end, when I change from `ListView` to `CardView`, the thumbnails don't really load, unsure if this flow is working properly or not. One suggestion: Could we change the process so that when switching to `CardView`, we trigger a new call to `/api/v1/dashboard/` with `select_columns` this time including `thumbnail_url`? It would:
1. Reduce the amount of API calls (get a list as opposed to one per item)
2. Allow us to remove the `thumbnail_url` from `/api/v1/dashboard/{id}`
Other pending question here is that if `thumbnail_url` is used anywhere else.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(dashboard): Remove thumbnail_url from detail API response [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #37941:
URL: https://github.com/apache/superset/pull/37941#issuecomment-3892963731
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/37941?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 65.87%. Comparing base ([`dbf5e1f`](https://app.codecov.io/gh/apache/superset/commit/dbf5e1f1314735dd5e19c6d244cf6bebb38a77a0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`475e4b9`](https://app.codecov.io/gh/apache/superset/commit/475e4b94387897e63a8a35e1deea6390c798cd71?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #37941 +/- ##
===========================================
+ Coverage 0 65.87% +65.87%
===========================================
Files 0 668 +668
Lines 0 51318 +51318
Branches 0 5776 +5776
===========================================
+ Hits 0 33804 +33804
- Misses 0 16119 +16119
- Partials 0 1395 +1395
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/37941/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/37941/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.47% <ΓΈ> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/37941/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.48% <ΓΈ> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/37941/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.85% <ΓΈ> (?)` | |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/37941/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.20% <ΓΈ> (?)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/37941/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ΓΈ> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/37941?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(dashboard): Batch RLS filter lookups for dashboard digest computation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37941:
URL: https://github.com/apache/superset/pull/37941#issuecomment-3898499565
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(dashboard): Batch RLS filter lookups for dashboard digest computation [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje merged PR #37941:
URL: https://github.com/apache/superset/pull/37941
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(dashboard): Batch RLS filter lookups for dashboard digest computation [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #37941:
URL: https://github.com/apache/superset/pull/37941#discussion_r2804984253
##########
superset/security/manager.py:
##########
@@ -2701,6 +2701,17 @@ def get_rls_filters(self, table: "BaseDatasource | Explorable") -> list[SqlaQuer
if not (hasattr(g, "user") and g.user is not None):
return []
+ # Check request-scoped cache keyed by (user_identifier, table_id).
+ # Uses user_id for regular users and username for guest users.
+ cache: dict[tuple[int | str, int | str], list[SqlaQuery]] = getattr(
Review Comment:
The dict type is a bit of a monster; can we add a type alias to describe what these mean? Something like this?
```python
RequestUser = int | str
RequestTable = int | str # isn't this in fact just int?
dict[tuple[RequestUser, RequestTable], list[SqlaQuery]]
```
##########
superset/security/manager.py:
##########
@@ -2701,6 +2701,17 @@ def get_rls_filters(self, table: "BaseDatasource | Explorable") -> list[SqlaQuer
if not (hasattr(g, "user") and g.user is not None):
return []
+ # Check request-scoped cache keyed by (user_identifier, table_id).
+ # Uses user_id for regular users and username for guest users.
+ cache: dict[tuple[int | str, int | str], list[SqlaQuery]] = getattr(
+ g, "_rls_filter_cache", {}
+ )
+ user_key: int | str | None = get_user_id() or getattr(g.user, "username", None)
Review Comment:
Can't we just use `get_username()` from `superset.utils.core`? I mean don't all users have `username`, so we don't need to have either `id` or `username`, as we can key just with `username` which is `str | None`?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(dashboard): Batch RLS filter lookups for dashboard digest computation [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #37941:
URL: https://github.com/apache/superset/pull/37941#discussion_r2813416108
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -1223,3 +1223,213 @@ def test_get_rls_filters_uses_table_id_directly(
# If it uses table.id directly (correct behavior), it will complete successfully
result = sm.get_rls_filters(table)
assert isinstance(result, list)
+
+
+def test_get_rls_filters_returns_cached_result(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that get_rls_filters() returns cached results on subsequent calls
+ for the same user and table, avoiding redundant DB queries.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_user = mocker.MagicMock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_user.roles = [mocker.MagicMock(id=1)]
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_user)
+ # Prevent MagicMock from auto-creating _rls_filter_cache
+ del mock_g._rls_filter_cache
+ mocker.patch("superset.security.manager.get_username", return_value="admin")
+ mocker.patch.object(sm, "get_user_roles", return_value=mock_user.roles)
+
+ table = mocker.MagicMock()
+ table.id = 42
+
+ # First call populates the cache
+ result1 = sm.get_rls_filters(table)
+
+ # Verify cache was populated keyed by (username, table_id)
+ assert ("admin", 42) in mock_g._rls_filter_cache
+
+ # Replace session query with something that would fail if called
+ mocker.patch.object(
+ sm.session,
+ "query",
+ side_effect=AssertionError("DB should not be queried on cache hit"),
+ )
+
+ # Second call should return cached result without querying DB
+ result2 = sm.get_rls_filters(table)
+ assert result1 == result2
+
+
+def test_prefetch_rls_filters_populates_cache(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that prefetch_rls_filters() populates the cache for all provided
+ table_ids, including empty results for tables with no matching filters.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_user = mocker.MagicMock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_user.roles = [mocker.MagicMock(id=10)]
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_user)
+ del mock_g._rls_filter_cache
Review Comment:
**Suggestion:** In this test, patching `g` as a `MagicMock` and deleting `_rls_filter_cache` means `prefetch_rls_filters()` populates a mock instead of a real dict, so accessing cached entries via attribute lookups and list semantics doesn't behave as expected and the assertions on cached filter contents will fail; use a plain object for `g` so `_rls_filter_cache` is a real dictionary. [logic error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β `prefetch_rls_filters` test fails due to MagicMock cache.
- β οΈ Batch RLS prefetch behavior not validated by unit tests.
- β οΈ CI for this PR will fail on this unit test.
```
</details>
```suggestion
class G:
pass
g_obj = G()
g_obj.user = mock_user
mock_g = mocker.patch("superset.security.manager.g", new=g_obj)
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run `pytest
tests/unit_tests/security/manager_test.py::test_prefetch_rls_filters_populates_cache`.
2. In `test_prefetch_rls_filters_populates_cache`
(`tests/unit_tests/security/manager_test.py:1269-1311`), `g` is patched to a `MagicMock`
(`mocker.patch("superset.security.manager.g", user=mock_user)`) and `_rls_filter_cache` is
deleted on that mock (lines 1279-1284).
3. In `SupersetSecurityManager.prefetch_rls_filters()`
(`superset/security/manager.py:2783-2874`), `hasattr(g, "_rls_filter_cache")` against a
`MagicMock` returns `True` even when the attribute was deleted, so `_rls_filter_cache` is
never initialized to `{}` and remains a `MagicMock`.
4. The comprehension `uncached_ids = [tid for tid in table_ids if (username, tid) not in
g._rls_filter_cache]` then calls `__contains__` on a `MagicMock`, which returns another
`MagicMock` that is truthy, producing `uncached_ids == []`. The function returns early
without querying or populating the cache, so `mock_g._rls_filter_cache[("admin", 1)]` is a
`MagicMock` instead of a list, and `len(cached) == 2` (and subsequent attribute
assertions) fails; using a simple object for `g` makes `_rls_filter_cache` a proper dict
and the test correctly exercises and validates prefetch behavior.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/manager_test.py
**Line:** 1283:1284
**Comment:**
*Logic Error: In this test, patching `g` as a `MagicMock` and deleting `_rls_filter_cache` means `prefetch_rls_filters()` populates a mock instead of a real dict, so accessing cached entries via attribute lookups and list semantics doesn't behave as expected and the assertions on cached filter contents will fail; use a plain object for `g` so `_rls_filter_cache` is a real dictionary.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37941&comment_hash=fa0834a0c7d7d0a3e4007b31d46a217026f7e307d5144d47a4c29b88d92303f2&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37941&comment_hash=fa0834a0c7d7d0a3e4007b31d46a217026f7e307d5144d47a4c29b88d92303f2&reaction=dislike'>π</a>
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -1223,3 +1223,213 @@ def test_get_rls_filters_uses_table_id_directly(
# If it uses table.id directly (correct behavior), it will complete successfully
result = sm.get_rls_filters(table)
assert isinstance(result, list)
+
+
+def test_get_rls_filters_returns_cached_result(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that get_rls_filters() returns cached results on subsequent calls
+ for the same user and table, avoiding redundant DB queries.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_user = mocker.MagicMock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_user.roles = [mocker.MagicMock(id=1)]
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_user)
+ # Prevent MagicMock from auto-creating _rls_filter_cache
+ del mock_g._rls_filter_cache
+ mocker.patch("superset.security.manager.get_username", return_value="admin")
Review Comment:
**Suggestion:** Patching `g` as a `MagicMock` and deleting `_rls_filter_cache` causes `get_rls_filters()` to see `_rls_filter_cache` as a mock instead of a real dict, so membership checks and assignments don't work correctly and the cache assertion in this test will fail; use a simple object for `g` instead of a mock so attribute access behaves like Flask's `g`. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ RLS cache-hit behavior for regular users not truly verified.
- β οΈ Regressions in `get_rls_filters` caching may go undetected.
- β οΈ Test relies on MagicMock semantics instead of real dict operations.
```
</details>
```suggestion
class G:
pass
g_obj = G()
g_obj.user = mock_user
mock_g = mocker.patch("superset.security.manager.g", new=g_obj)
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run `pytest
tests/unit_tests/security/manager_test.py::test_get_rls_filters_returns_cached_result`.
2. In `test_get_rls_filters_returns_cached_result`
(`tests/unit_tests/security/manager_test.py:1228-1266`), `g` is patched to a `MagicMock`
via `mocker.patch("superset.security.manager.g", user=mock_user)` and `_rls_filter_cache`
is deleted on that mock (lines 1238-1245).
3. Inside `SupersetSecurityManager.get_rls_filters()`
(`superset/security/manager.py:2703-2781`), `cache = getattr(g, "_rls_filter_cache", {})`
returns a `MagicMock` (because `g` is a `MagicMock` that never raises `AttributeError`),
so `cache_key in cache` and `cache[cache_key]` operate on mocks rather than a real dict,
and no real cache population occurs.
4. The test still passes (no exception, assertions succeed), but it never verifies that
`_rls_filter_cache` is a real dictionary or that `get_rls_filters()` actually writes to
it, so regressions in the caching logic would not be caught; using a plain object for `g`
would make `_rls_filter_cache` a real dict and make the test meaningfully exercise the
cache behavior.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/manager_test.py
**Line:** 1242:1245
**Comment:**
*Logic Error: Patching `g` as a `MagicMock` and deleting `_rls_filter_cache` causes `get_rls_filters()` to see `_rls_filter_cache` as a mock instead of a real dict, so membership checks and assignments don't work correctly and the cache assertion in this test will fail; use a simple object for `g` instead of a mock so attribute access behaves like Flask's `g`.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37941&comment_hash=580448bfbc896c2727119f6761001a11ca4b1620ed0091e8732a1112cbd3d39c&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37941&comment_hash=580448bfbc896c2727119f6761001a11ca4b1620ed0091e8732a1112cbd3d39c&reaction=dislike'>π</a>
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -1223,3 +1223,213 @@ def test_get_rls_filters_uses_table_id_directly(
# If it uses table.id directly (correct behavior), it will complete successfully
result = sm.get_rls_filters(table)
assert isinstance(result, list)
+
+
+def test_get_rls_filters_returns_cached_result(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that get_rls_filters() returns cached results on subsequent calls
+ for the same user and table, avoiding redundant DB queries.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_user = mocker.MagicMock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_user.roles = [mocker.MagicMock(id=1)]
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_user)
+ # Prevent MagicMock from auto-creating _rls_filter_cache
+ del mock_g._rls_filter_cache
+ mocker.patch("superset.security.manager.get_username", return_value="admin")
+ mocker.patch.object(sm, "get_user_roles", return_value=mock_user.roles)
+
+ table = mocker.MagicMock()
+ table.id = 42
+
+ # First call populates the cache
+ result1 = sm.get_rls_filters(table)
+
+ # Verify cache was populated keyed by (username, table_id)
+ assert ("admin", 42) in mock_g._rls_filter_cache
+
+ # Replace session query with something that would fail if called
+ mocker.patch.object(
+ sm.session,
+ "query",
+ side_effect=AssertionError("DB should not be queried on cache hit"),
+ )
+
+ # Second call should return cached result without querying DB
+ result2 = sm.get_rls_filters(table)
+ assert result1 == result2
+
+
+def test_prefetch_rls_filters_populates_cache(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that prefetch_rls_filters() populates the cache for all provided
+ table_ids, including empty results for tables with no matching filters.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_user = mocker.MagicMock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_user.roles = [mocker.MagicMock(id=10)]
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_user)
+ del mock_g._rls_filter_cache
+ mocker.patch("superset.security.manager.get_username", return_value="admin")
+ mocker.patch.object(sm, "get_user_roles", return_value=mock_user.roles)
+
+ # Mock the batch query to return filters for table 1 but not table 2
+ mock_query = mocker.MagicMock()
+ mock_query.join.return_value = mock_query
+ mock_query.filter.return_value = mock_query
+ mock_query.all.return_value = [
+ (1, 100, "group_a", "id > 0"), # table_id=1
+ (1, 101, None, "active = 1"), # table_id=1
+ ]
+ mocker.patch.object(sm.session, "query", return_value=mock_query)
+
+ sm.prefetch_rls_filters([1, 2])
+
+ # Table 1 should have 2 filters with named attribute access
+ cached = mock_g._rls_filter_cache[("admin", 1)]
+ assert len(cached) == 2
+ assert cached[0].id == 100
+ assert cached[0].group_key == "group_a"
+ assert cached[0].clause == "id > 0"
+ assert cached[1].id == 101
+ assert cached[1].group_key is None
+ assert cached[1].clause == "active = 1"
+ # Table 2 should have empty list
+ assert mock_g._rls_filter_cache[("admin", 2)] == []
+
+
+def test_prefetch_rls_filters_skips_cached_ids(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that prefetch_rls_filters() skips table_ids already in cache
+ and returns early when all ids are cached.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_user = mocker.MagicMock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_user.roles = [mocker.MagicMock(id=10)]
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_user)
+ mocker.patch("superset.security.manager.get_username", return_value="admin")
+ mocker.patch.object(sm, "get_user_roles", return_value=mock_user.roles)
+
+ # Pre-populate cache for table 1
+ mock_g._rls_filter_cache = {("admin", 1): [(100, "group_a", "id > 0")]}
+
+ # If it queries the DB, this will fail
+ mocker.patch.object(
+ sm.session,
+ "query",
+ side_effect=AssertionError("DB should not be queried for cached ids"),
+ )
+
+ # All ids already cached -> should return immediately
+ sm.prefetch_rls_filters([1])
+
+
+def test_prefetch_rls_filters_no_user(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that prefetch_rls_filters() returns early when no user is present.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+ mock_g = mocker.patch("superset.security.manager.g")
+ del mock_g.user
+
+ # Should not attempt any DB queries
+ mocker.patch.object(
+ sm.session,
+ "query",
+ side_effect=AssertionError("DB should not be queried without a user"),
+ )
+ sm.prefetch_rls_filters([1, 2])
+
+
+def test_get_rls_filters_cache_works_for_guest_user(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that get_rls_filters() caches results for guest users
+ using the same (username, table_id) cache key as regular users.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_guest = mocker.MagicMock()
+ mock_guest.username = "guest_user"
+ mock_guest.roles = [mocker.MagicMock(id=99)]
+
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_guest)
Review Comment:
**Suggestion:** For guest-user caching, patching `g` as a `MagicMock` and deleting `_rls_filter_cache` again causes `_rls_filter_cache` to be a mock, so cache membership and equality checks behave incorrectly and the test does not meaningfully validate guest caching; switch `g` to a basic object so the cache is a real dict. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ Guest-user RLS cache-hit behavior not truly validated.
- β οΈ Regressions in guest caching may slip past unit tests.
- β οΈ Test semantics depend on MagicMock quirks, not dict behavior.
```
</details>
```suggestion
class G:
pass
g_obj = G()
g_obj.user = mock_guest
mock_g = mocker.patch("superset.security.manager.g", new=g_obj)
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run `pytest
tests/unit_tests/security/manager_test.py::test_get_rls_filters_cache_works_for_guest_user`.
2. In `test_get_rls_filters_cache_works_for_guest_user`
(`tests/unit_tests/security/manager_test.py:1365-1403`), `g` is patched to a `MagicMock`
with a `user` attribute (`mock_guest`) and `_rls_filter_cache` is deleted on that mock
(lines 1375-1381).
3. In `SupersetSecurityManager.get_rls_filters()`
(`superset/security/manager.py:2703-2781`), `cache = getattr(g, "_rls_filter_cache", {})`
returns a `MagicMock`, so the first call treats the cache as a mock, and `cache_key in
cache` / `cache[cache_key]` operate on mocks rather than a real dict; no actual cache
population is guaranteed.
4. The test asserts that `("guest_user", 42) in mock_g._rls_filter_cache` after the first
call, but because `_rls_filter_cache` is a `MagicMock`, membership tests and equality
comparisons are based on mock behavior rather than real dictionary semantics, so the test
can pass even if `get_rls_filters()`'s guest caching logic regresses; using a plain `G`
object for `g` ensures `_rls_filter_cache` is a real dict and makes the test reliably
validate guest cache behavior.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/manager_test.py
**Line:** 1379:1379
**Comment:**
*Logic Error: For guest-user caching, patching `g` as a `MagicMock` and deleting `_rls_filter_cache` again causes `_rls_filter_cache` to be a mock, so cache membership and equality checks behave incorrectly and the test does not meaningfully validate guest caching; switch `g` to a basic object so the cache is a real dict.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37941&comment_hash=31d4391fe272038a42bd70d1f85d3042712d859bcc42e2c869462d20b414dc7e&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37941&comment_hash=31d4391fe272038a42bd70d1f85d3042712d859bcc42e2c869462d20b414dc7e&reaction=dislike'>π</a>
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -1223,3 +1223,213 @@ def test_get_rls_filters_uses_table_id_directly(
# If it uses table.id directly (correct behavior), it will complete successfully
result = sm.get_rls_filters(table)
assert isinstance(result, list)
+
+
+def test_get_rls_filters_returns_cached_result(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that get_rls_filters() returns cached results on subsequent calls
+ for the same user and table, avoiding redundant DB queries.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_user = mocker.MagicMock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_user.roles = [mocker.MagicMock(id=1)]
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_user)
+ # Prevent MagicMock from auto-creating _rls_filter_cache
+ del mock_g._rls_filter_cache
+ mocker.patch("superset.security.manager.get_username", return_value="admin")
+ mocker.patch.object(sm, "get_user_roles", return_value=mock_user.roles)
+
+ table = mocker.MagicMock()
+ table.id = 42
+
+ # First call populates the cache
+ result1 = sm.get_rls_filters(table)
+
+ # Verify cache was populated keyed by (username, table_id)
+ assert ("admin", 42) in mock_g._rls_filter_cache
+
+ # Replace session query with something that would fail if called
+ mocker.patch.object(
+ sm.session,
+ "query",
+ side_effect=AssertionError("DB should not be queried on cache hit"),
+ )
+
+ # Second call should return cached result without querying DB
+ result2 = sm.get_rls_filters(table)
+ assert result1 == result2
+
+
+def test_prefetch_rls_filters_populates_cache(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that prefetch_rls_filters() populates the cache for all provided
+ table_ids, including empty results for tables with no matching filters.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_user = mocker.MagicMock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_user.roles = [mocker.MagicMock(id=10)]
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_user)
+ del mock_g._rls_filter_cache
+ mocker.patch("superset.security.manager.get_username", return_value="admin")
+ mocker.patch.object(sm, "get_user_roles", return_value=mock_user.roles)
+
+ # Mock the batch query to return filters for table 1 but not table 2
+ mock_query = mocker.MagicMock()
+ mock_query.join.return_value = mock_query
+ mock_query.filter.return_value = mock_query
+ mock_query.all.return_value = [
+ (1, 100, "group_a", "id > 0"), # table_id=1
+ (1, 101, None, "active = 1"), # table_id=1
+ ]
+ mocker.patch.object(sm.session, "query", return_value=mock_query)
+
+ sm.prefetch_rls_filters([1, 2])
+
+ # Table 1 should have 2 filters with named attribute access
+ cached = mock_g._rls_filter_cache[("admin", 1)]
+ assert len(cached) == 2
+ assert cached[0].id == 100
+ assert cached[0].group_key == "group_a"
+ assert cached[0].clause == "id > 0"
+ assert cached[1].id == 101
+ assert cached[1].group_key is None
+ assert cached[1].clause == "active = 1"
+ # Table 2 should have empty list
+ assert mock_g._rls_filter_cache[("admin", 2)] == []
+
+
+def test_prefetch_rls_filters_skips_cached_ids(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that prefetch_rls_filters() skips table_ids already in cache
+ and returns early when all ids are cached.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_user = mocker.MagicMock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_user.roles = [mocker.MagicMock(id=10)]
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_user)
+ mocker.patch("superset.security.manager.get_username", return_value="admin")
+ mocker.patch.object(sm, "get_user_roles", return_value=mock_user.roles)
+
+ # Pre-populate cache for table 1
+ mock_g._rls_filter_cache = {("admin", 1): [(100, "group_a", "id > 0")]}
+
+ # If it queries the DB, this will fail
+ mocker.patch.object(
+ sm.session,
+ "query",
+ side_effect=AssertionError("DB should not be queried for cached ids"),
+ )
+
+ # All ids already cached -> should return immediately
+ sm.prefetch_rls_filters([1])
+
+
+def test_prefetch_rls_filters_no_user(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that prefetch_rls_filters() returns early when no user is present.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+ mock_g = mocker.patch("superset.security.manager.g")
+ del mock_g.user
+
+ # Should not attempt any DB queries
+ mocker.patch.object(
+ sm.session,
+ "query",
+ side_effect=AssertionError("DB should not be queried without a user"),
+ )
+ sm.prefetch_rls_filters([1, 2])
+
+
+def test_get_rls_filters_cache_works_for_guest_user(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that get_rls_filters() caches results for guest users
+ using the same (username, table_id) cache key as regular users.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_guest = mocker.MagicMock()
+ mock_guest.username = "guest_user"
+ mock_guest.roles = [mocker.MagicMock(id=99)]
+
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_guest)
+ del mock_g._rls_filter_cache
+ mocker.patch("superset.security.manager.get_username", return_value="guest_user")
+ mocker.patch.object(sm, "get_user_roles", return_value=mock_guest.roles)
+
+ table = mocker.MagicMock()
+ table.id = 42
+
+ # First call runs the query
+ result1 = sm.get_rls_filters(table)
+
+ # Verify cache was populated with (username, table_id) key
+ assert ("guest_user", 42) in mock_g._rls_filter_cache
+
+ # Replace session query to detect if it's called again
+ mocker.patch.object(
+ sm.session,
+ "query",
+ side_effect=AssertionError("DB should not be queried on cache hit"),
+ )
+
+ # Second call should use cache
+ result2 = sm.get_rls_filters(table)
+ assert result1 == result2
+
+
+def test_prefetch_rls_filters_works_for_guest_user(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that prefetch_rls_filters() works for guest users using the
+ same (username, table_id) cache key as regular users.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_guest = mocker.MagicMock()
+ mock_guest.username = "guest_user"
+ mock_guest.roles = [mocker.MagicMock(id=99)]
+
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_guest)
Review Comment:
**Suggestion:** In this guest-user prefetch test, patching `g` as a `MagicMock` and deleting `_rls_filter_cache` leads to the cache becoming a mock, so the code under test doesn't actually populate a real dict and the final equality checks on `_rls_filter_cache` entries can fail or be meaningless; use a simple object for `g` so `_rls_filter_cache` is a real dictionary. [logic error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β Guest prefetch RLS test fails due to MagicMock cache.
- β οΈ Guest batch-prefetch behavior not validated by tests.
- β οΈ Failing unit test will block CI for this PR.
```
</details>
```suggestion
class G:
pass
g_obj = G()
g_obj.user = mock_guest
mock_g = mocker.patch("superset.security.manager.g", new=g_obj)
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run `pytest
tests/unit_tests/security/manager_test.py::test_prefetch_rls_filters_works_for_guest_user`.
2. In `test_prefetch_rls_filters_works_for_guest_user`
(`tests/unit_tests/security/manager_test.py:1405-1435`), `g` is patched to a `MagicMock`
with a `user` attribute (`mock_guest`), and `_rls_filter_cache` is deleted from that mock
(lines 1415-1421).
3. In `SupersetSecurityManager.prefetch_rls_filters()`
(`superset/security/manager.py:2783-2874`), `hasattr(g, "_rls_filter_cache")` on a
`MagicMock` returns `True`, so the function does not initialize `_rls_filter_cache` to
`{}` and the attribute remains a `MagicMock`.
4. The list comprehension that determines `uncached_ids` operates against
`g._rls_filter_cache` (a `MagicMock`), so it computes `uncached_ids == []` and returns
early without querying or populating a real cache; the final assertions
`mock_g._rls_filter_cache[("guest_user", 10)] == []` and for table 20 then compare
`MagicMock` instances to `[]`, causing the test to fail. Switching `g` to a simple object
ensures `_rls_filter_cache` is a real dict, `uncached_ids` is computed correctly, and the
test meaningfully validates guest prefetch behavior.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/manager_test.py
**Line:** 1419:1419
**Comment:**
*Logic Error: In this guest-user prefetch test, patching `g` as a `MagicMock` and deleting `_rls_filter_cache` leads to the cache becoming a mock, so the code under test doesn't actually populate a real dict and the final equality checks on `_rls_filter_cache` entries can fail or be meaningless; use a simple object for `g` so `_rls_filter_cache` is a real dictionary.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37941&comment_hash=2fbae3c08442fcd2bfdf02998aed1a812031d8bc0784761f205897191a34bbe4&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37941&comment_hash=2fbae3c08442fcd2bfdf02998aed1a812031d8bc0784761f205897191a34bbe4&reaction=dislike'>π</a>
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -1223,3 +1223,213 @@ def test_get_rls_filters_uses_table_id_directly(
# If it uses table.id directly (correct behavior), it will complete successfully
result = sm.get_rls_filters(table)
assert isinstance(result, list)
+
+
+def test_get_rls_filters_returns_cached_result(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that get_rls_filters() returns cached results on subsequent calls
+ for the same user and table, avoiding redundant DB queries.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_user = mocker.MagicMock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_user.roles = [mocker.MagicMock(id=1)]
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_user)
+ # Prevent MagicMock from auto-creating _rls_filter_cache
+ del mock_g._rls_filter_cache
+ mocker.patch("superset.security.manager.get_username", return_value="admin")
+ mocker.patch.object(sm, "get_user_roles", return_value=mock_user.roles)
+
+ table = mocker.MagicMock()
+ table.id = 42
+
+ # First call populates the cache
+ result1 = sm.get_rls_filters(table)
+
+ # Verify cache was populated keyed by (username, table_id)
+ assert ("admin", 42) in mock_g._rls_filter_cache
+
+ # Replace session query with something that would fail if called
+ mocker.patch.object(
+ sm.session,
+ "query",
+ side_effect=AssertionError("DB should not be queried on cache hit"),
+ )
+
+ # Second call should return cached result without querying DB
+ result2 = sm.get_rls_filters(table)
+ assert result1 == result2
+
+
+def test_prefetch_rls_filters_populates_cache(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that prefetch_rls_filters() populates the cache for all provided
+ table_ids, including empty results for tables with no matching filters.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_user = mocker.MagicMock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_user.roles = [mocker.MagicMock(id=10)]
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_user)
+ del mock_g._rls_filter_cache
+ mocker.patch("superset.security.manager.get_username", return_value="admin")
+ mocker.patch.object(sm, "get_user_roles", return_value=mock_user.roles)
+
+ # Mock the batch query to return filters for table 1 but not table 2
+ mock_query = mocker.MagicMock()
+ mock_query.join.return_value = mock_query
+ mock_query.filter.return_value = mock_query
+ mock_query.all.return_value = [
+ (1, 100, "group_a", "id > 0"), # table_id=1
+ (1, 101, None, "active = 1"), # table_id=1
+ ]
+ mocker.patch.object(sm.session, "query", return_value=mock_query)
+
+ sm.prefetch_rls_filters([1, 2])
+
+ # Table 1 should have 2 filters with named attribute access
+ cached = mock_g._rls_filter_cache[("admin", 1)]
+ assert len(cached) == 2
+ assert cached[0].id == 100
+ assert cached[0].group_key == "group_a"
+ assert cached[0].clause == "id > 0"
+ assert cached[1].id == 101
+ assert cached[1].group_key is None
+ assert cached[1].clause == "active = 1"
+ # Table 2 should have empty list
+ assert mock_g._rls_filter_cache[("admin", 2)] == []
+
+
+def test_prefetch_rls_filters_skips_cached_ids(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that prefetch_rls_filters() skips table_ids already in cache
+ and returns early when all ids are cached.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+
+ mock_user = mocker.MagicMock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_user.roles = [mocker.MagicMock(id=10)]
+ mock_g = mocker.patch("superset.security.manager.g", user=mock_user)
+ mocker.patch("superset.security.manager.get_username", return_value="admin")
+ mocker.patch.object(sm, "get_user_roles", return_value=mock_user.roles)
+
+ # Pre-populate cache for table 1
+ mock_g._rls_filter_cache = {("admin", 1): [(100, "group_a", "id > 0")]}
+
+ # If it queries the DB, this will fail
+ mocker.patch.object(
+ sm.session,
+ "query",
+ side_effect=AssertionError("DB should not be queried for cached ids"),
+ )
+
+ # All ids already cached -> should return immediately
+ sm.prefetch_rls_filters([1])
+
+
+def test_prefetch_rls_filters_no_user(
+ mocker: MockerFixture,
+ app_context: None,
+) -> None:
+ """
+ Test that prefetch_rls_filters() returns early when no user is present.
+ """
+ sm = SupersetSecurityManager(appbuilder)
+ mock_g = mocker.patch("superset.security.manager.g")
+ del mock_g.user
+
Review Comment:
**Suggestion:** This test intends to simulate the absence of a user, but patching `g` as a `MagicMock` and deleting `user` fails because `hasattr(g, "user")` on a MagicMock always returns True, so `prefetch_rls_filters()` will execute and hit the DB query mocked to raise, causing the test to fail; patch `g` to a simple object without a `user` attribute so the early-return path is exercised correctly. [logic error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β `prefetch_rls_filters` no-user guard test currently fails.
- β οΈ Early-return behavior without a user is not validated.
- β οΈ CI for this PR is blocked by this failing unit test.
```
</details>
```suggestion
class G:
pass
mock_g = mocker.patch("superset.security.manager.g", new=G())
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run `pytest
tests/unit_tests/security/manager_test.py::test_prefetch_rls_filters_no_user`.
2. In `test_prefetch_rls_filters_no_user`
(`tests/unit_tests/security/manager_test.py:1345-1362`), `g` is patched to a bare
`MagicMock` (`mock_g = mocker.patch("superset.security.manager.g")`) and `del mock_g.user`
is used to try to simulate the absence of a user (lines 1353-1355).
3. In `SupersetSecurityManager.prefetch_rls_filters()`
(`superset/security/manager.py:2783-2874`), the guard `if not (hasattr(g, "user") and
g.user is not None): return` is evaluated; for a `MagicMock`, `hasattr(g, "user")` returns
`True` and `g.user` returns another `MagicMock`, so the condition is false and the
function does not early-return.
4. Because the test also patches `sm.session.query` with `side_effect=AssertionError("DB
should not be queried without a user")`, the function proceeds to execute
`self.session.query(...)` and the assertion error is raised, causing the test to fail;
patching `g` to a simple object with no `user` attribute makes `hasattr(g, "user")` return
`False` and ensures the early-return path (no DB query) is properly exercised.
```
</details>
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/security/manager_test.py
**Line:** 1354:1355
**Comment:**
*Logic Error: This test intends to simulate the absence of a user, but patching `g` as a `MagicMock` and deleting `user` fails because `hasattr(g, "user")` on a MagicMock always returns True, so `prefetch_rls_filters()` will execute and hit the DB query mocked to raise, causing the test to fail; patch `g` to a simple object without a `user` attribute so the early-return path is exercised correctly.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37941&comment_hash=aa31ff1c437f79ad90bcd872254a25525dea79bf97e3050a889b6b5b52ac2f64&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37941&comment_hash=aa31ff1c437f79ad90bcd872254a25525dea79bf97e3050a889b6b5b52ac2f64&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] perf(dashboard): Remove thumbnail_url from detail API response [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37941:
URL: https://github.com/apache/superset/pull/37941#issuecomment-3892922346
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org