-
Notifications
You must be signed in to change notification settings - Fork 17.2k
docs: Superset 6.1 documentation catch-up — batch 4 #39446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -178,6 +178,20 @@ Then on configuration: | |||||
| WEBDRIVER_AUTH_FUNC = auth_driver | ||||||
| ``` | ||||||
|
|
||||||
| ## ETag Support for Thumbnails | ||||||
|
|
||||||
| Thumbnail and screenshot endpoints return `ETag` response headers based on the cached content digest. Clients can use conditional requests to avoid downloading unchanged images: | ||||||
|
|
||||||
| ``` | ||||||
| GET /api/v1/chart/42/thumbnail/ | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The example request path is incorrect because the chart thumbnail API requires a Severity Level: Major
|
||||||
| GET /api/v1/chart/42/thumbnail/ | |
| GET /api/v1/chart/42/thumbnail/<digest>/ |
Steps of Reproduction ✅
1. Open `docs/admin_docs/configuration/cache.mdx` and locate the thumbnail example at
lines 162–171 (from Grep: lines 162–172), which documents `GET
/api/v1/chart/42/thumbnail/` as the chart thumbnail endpoint used for ETag-based
conditional requests.
2. Inspect the actual chart thumbnail route in `superset/superset/charts/api.py:729–733`,
where the endpoint is defined as `@expose("/<pk>/thumbnail/<digest>/", methods=("GET",))`
and the view `thumbnail(self, pk: int, digest: str, **kwargs)` requires a `digest` path
parameter.
3. Inspect how thumbnail URLs are generated in `superset/superset/models/slice.py:247`,
which returns `f"/api/v1/chart/{self.id}/thumbnail/{digest}/"`, and see this same URL
shape exercised in `tests/integration_tests/thumbnails_tests.py:237–240` where
`thumbnail_url` (including the digest) is fetched and expected to return `200`.
4. Run Superset with thumbnails enabled and issue `GET /api/v1/chart/42/thumbnail/`
(missing digest): because no route is defined for `/api/v1/chart/<pk>/thumbnail/` (only
`/api/v1/chart/<pk>/thumbnail/<digest>/` exists), Flask will return `404 Not Found`, so a
reader following the documented example cannot reproduce the described conditional request
behavior until they add the missing `<digest>` segment.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docs/admin_docs/configuration/cache.mdx
**Line:** 167:167
**Comment:**
*Logic Error: The example request path is incorrect because the chart thumbnail API requires a `digest` path segment (`/<pk>/thumbnail/<digest>/`). Using the documented URL without `digest` will not hit the intended endpoint, so readers won't be able to reproduce conditional requests 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 Architect Review — HIGH
The ETag example for thumbnails uses a non-existent endpoint (GET /api/v1/chart/42/thumbnail/), while the actual API requires a digest segment (/api/v1/chart/{id}/thumbnail/{digest}/) and is typically accessed via the image_url returned from the screenshot/thumbnail caching APIs. This incorrect path and implied flow will lead clients following the docs to 404s.
Suggestion: Update the example to use the correct digest-based endpoint shape (e.g. GET /api/v1/chart/42/thumbnail/{digest}/) and clarify that clients should first obtain the image_url (including the digest) from the screenshot/thumbnail caching APIs, then issue conditional GETs with If-None-Match against that URL.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** docs/admin_docs/configuration/cache.mdx
**Line:** 164:172
**Comment:**
*HIGH: The ETag example for thumbnails uses a non-existent endpoint (`GET /api/v1/chart/42/thumbnail/`), while the actual API requires a digest segment (`/api/v1/chart/{id}/thumbnail/{digest}/`) and is typically accessed via the `image_url` returned from the screenshot/thumbnail caching APIs. This incorrect path and implied flow will lead clients following the docs to 404s.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation claims that thumbnail and screenshot endpoints return ETag headers for conditional requests, but the actual endpoints in charts/api.py and dashboards/api.py do not use the etag_cache decorator or add ETag headers. Additionally, the example URL /api/v1/chart/42/thumbnail/ is missing the required parameter; the actual endpoint is /api/v1/chart/42/thumbnail//. This could mislead users expecting ETag functionality that isn't implemented.
Code Review Run #d696cc
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: This statement over-promises behavior that is not implemented in the current thumbnail/screenshot handlers (they don't set
ETagheaders or performIf-None-Matchconditional handling). Reword to avoid documenting unsupported caching semantics. [possible bug]Severity Level: Major⚠️
Steps of Reproduction ✅
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖