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: [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]
