codeant-ai-for-open-source[bot] commented on code in PR #43395:
URL: https://github.com/apache/superset/pull/43395#discussion_r3831866462
##########
superset/utils/screenshots.py:
##########
@@ -315,6 +341,7 @@ def compute_and_cache( # pylint: disable=too-many-arguments
window_size = window_size or self.window_size
thumb_size = thumb_size or self.thumb_size
logger.info("Processing url for thumbnail: %s", cache_key)
+ cache_payload.set_scope(self.cache_scope)
Review Comment:
**Suggestion:** Legacy cache entries with a valid `UPDATED` image have no
scope, so `should_trigger_task()` returns false and the method exits before
reaching this line. Consequently, the entry is never migrated with a scope,
while the dashboard and chart screenshot endpoints now reject it, leaving the
thumbnail unavailable indefinitely unless a forced recomputation occurs. Treat
an `UPDATED` entry without the expected scope as a cache miss or persist the
scope before the early-return check. [cache]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Existing dashboard screenshots return 404 after scope validation.
- ⚠️ Legacy cache entries are not automatically recomputed.
- ⚠️ Chart screenshot reads have the same scope rejection at
`superset/charts/api.py:1119-1120`.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/screenshots.py
**Line:** 344:344
**Comment:**
*Cache: Legacy cache entries with a valid `UPDATED` image have no
scope, so `should_trigger_task()` returns false and the method exits before
reaching this line. Consequently, the entry is never migrated with a scope,
while the dashboard and chart screenshot endpoints now reject it, leaving the
thumbnail unavailable indefinitely unless a forced recomputation occurs. Treat
an `UPDATED` entry without the expected scope as a cache miss or persist the
scope before the early-return check.
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%2F43395&comment_hash=10ae065a04949b0dbf8a273fb5928af5971a2d892fd92996ab7e5efbe1088207&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43395&comment_hash=10ae065a04949b0dbf8a273fb5928af5971a2d892fd92996ab7e5efbe1088207&reaction=dislike'>👎</a>
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -4222,13 +4261,15 @@ def test_screenshot_invalid_download_format(
):
self.login(ADMIN_USERNAME)
mock_cache_task.return_value = None
- mock_get_from_cache_key.return_value = ScreenshotCachePayload(b"fake
png data")
dashboard = (
db.session.query(Dashboard)
.filter(Dashboard.dashboard_title == "dash with tag")
.first()
)
+ mock_get_from_cache_key.return_value = ScreenshotCachePayload(
+ b"fake png data", scope=f"dashboard:{dashboard.id}"
+ )
Review Comment:
**Suggestion:** The newly added cache payload is immediately replaced with
an empty `ScreenshotCachePayload` before the GET request, so the request
returns 404 because no image exists and never exercises the
`download_format=invalid` branch. Keep a valid scoped payload in place for the
GET, then assert that the invalid format is rejected. [incomplete
implementation]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Invalid-format handling is not actually regression-tested.
- ⚠️ The test can pass while format validation remains unverified.
- ⚠️ Screenshot endpoint behavior is masked by an earlier cache-scope
failure.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/integration_tests/dashboards/api_tests.py
**Line:** 4270:4272
**Comment:**
*Incomplete Implementation: The newly added cache payload is
immediately replaced with an empty `ScreenshotCachePayload` before the GET
request, so the request returns 404 because no image exists and never exercises
the `download_format=invalid` branch. Keep a valid scoped payload in place for
the GET, then assert that the invalid format is rejected.
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%2F43395&comment_hash=7be5579015824ae21442d50d85ef73b551f22c3ac748585c5968b56ec58f124f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43395&comment_hash=7be5579015824ae21442d50d85ef73b551f22c3ac748585c5968b56ec58f124f&reaction=dislike'>👎</a>
##########
superset/dashboards/api.py:
##########
@@ -2019,6 +2062,12 @@ def screenshot(self, pk: int, digest: str) ->
WerkzeugResponse:
# fetch the dashboard screenshot using the current user and cache if
set
if cache_payload := DashboardScreenshot.get_from_cache_key(digest):
+ # The digest is caller-supplied and cache entries are shared across
+ # every dashboard (and, via the same backend, charts) -- without
+ # this check any cache_key learned for one dashboard would serve
+ # its image under a different, merely-accessible `pk`.
+ if cache_payload.get_scope() != f"dashboard:{dashboard.id}":
+ return self.response_404()
Review Comment:
**Suggestion:** This rejects any cached screenshot created before the scope
field was introduced because those payloads deserialize with `get_scope()`
equal to `None`. Such entries are still treated as successfully updated by
`should_trigger_task()`, so `cache_dashboard_screenshot` will not enqueue a
recomputation; the API returns a successful cache response, but the returned
image URL permanently produces 404 until the old cache entry expires or is
manually removed. Treat an unscoped entry as a cache miss and trigger
recomputation, or explicitly invalidate it when building the response. [cache]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard screenshot requests return unusable image URLs.
- ⚠️ Existing cache entries require expiry or forced recomputation.
- ⚠️ Rolling deployments can expose stale screenshot payloads.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/dashboards/api.py
**Line:** 2069:2070
**Comment:**
*Cache: This rejects any cached screenshot created before the scope
field was introduced because those payloads deserialize with `get_scope()`
equal to `None`. Such entries are still treated as successfully updated by
`should_trigger_task()`, so `cache_dashboard_screenshot` will not enqueue a
recomputation; the API returns a successful cache response, but the returned
image URL permanently produces 404 until the old cache entry expires or is
manually removed. Treat an unscoped entry as a cache miss and trigger
recomputation, or explicitly invalidate it when building the response.
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%2F43395&comment_hash=b059cba7216d70164d8c20763458a437ccc142b721fef7d2852589317a5266bd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43395&comment_hash=b059cba7216d70164d8c20763458a437ccc142b721fef7d2852589317a5266bd&reaction=dislike'>👎</a>
##########
superset/thumbnails/digest.py:
##########
@@ -62,9 +62,18 @@ def _adjust_string_with_rls(
Add the RLS filters to the unique string based on current executor.
"""
+ # Prefer the ambient guest user (the actual requesting principal) over a
+ # DB-user lookup by username: for guest requests `executor` is the
+ # token-supplied username, which can collide with a real DB username. If
+ # find_user() were tried first, a collision would compute RLS under the
+ # unrelated DB user's identity, and the token's own per-token rls claims
+ # (surfaced via get_guest_rls_filters(), which reads the ambient guest
+ # user installed by override_user() below) would never enter the digest --
+ # letting two guest tokens with the same username but different rls
+ # collide on one cache entry.
user = (
- security_manager.find_user(executor)
- or security_manager.get_current_guest_user_if_guest()
+ security_manager.get_current_guest_user_if_guest()
+ or security_manager.find_user(executor)
Review Comment:
**Suggestion:** The digest now always selects the ambient guest user when
one exists, even when `get_executor()` selected a different executor such as a
fixed user. In paths such as chart thumbnail generation, the task renders under
the configured executor while the digest contains the guest's RLS filters, so
the cache key does not represent the identity that produced the image and can
cause cache misses or incorrect cache reuse. Only prefer the guest user when
the selected executor is the guest/current-user identity. [stale reference]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Fixed-executor guest thumbnails use mismatched cache identities.
- ⚠️ Embedded chart thumbnail requests can recompute unnecessarily.
- ⚠️ Dashboard thumbnail caches fragment across guest RLS scopes.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/thumbnails/digest.py
**Line:** 74:76
**Comment:**
*Stale Reference: The digest now always selects the ambient guest user
when one exists, even when `get_executor()` selected a different executor such
as a fixed user. In paths such as chart thumbnail generation, the task renders
under the configured executor while the digest contains the guest's RLS
filters, so the cache key does not represent the identity that produced the
image and can cause cache misses or incorrect cache reuse. Only prefer the
guest user when the selected executor is the guest/current-user identity.
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%2F43395&comment_hash=87f9cacbf9689822c9957e26db0be9e29ef23f2fb1d150acac2ebe919ee82a61&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43395&comment_hash=87f9cacbf9689822c9957e26db0be9e29ef23f2fb1d150acac2ebe919ee82a61&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]
