You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "no-hup (via GitHub)" <gi...@apache.org> on 2026/09/08 11:17:24 UTC
[PR] fix(dashboard): load chart editors and viewers up front [superset]
no-hup opened a new pull request, #43990:
URL: https://github.com/apache/superset/pull/43990
### SUMMARY
`GET /api/v1/dashboard/<id_or_slug>` narrows its `charts` field by calling `can_access_chart` on every member chart. That check reads `Slice.editors` and `Slice.viewers`, and both are plain lazy relationships, so the query count grows with the number of charts on the dashboard. `GET /<id_or_slug>/charts` does the same thing through `_serialize_dashboard_chart`.
This adds `DashboardDAO.prefetch_chart_access`, which loads both relationships for the whole set in two queries, and calls it from those two places.
Admins short-circuit on `is_admin()` before any of this runs, which is probably why it hasn't come up.
I counted statements with a SQLAlchemy `before_cursor_execute` listener — non-admin user, dashboard seeded with N charts — comparing this branch against 574d121.
Charts the user reaches through their own editors/viewers:
| charts | editor: before → after | viewer: before → after |
|-------:|-----------------------:|-----------------------:|
| 5 | 13 → 11 | 23 → 16 |
| 20 | 43 → 26 | 83 → 46 |
| 40 | 83 → 46 | 163 → 86 |
Charts reached through `datasource_access` instead:
| charts | before → after |
|-------:|---------------:|
| 5 | 59 → 52 |
| 20 | 224 → 187 |
| 40 | 444 → 367 |
I want to be straight about that second table: this does not stop the endpoint scaling with chart count. It removes the two lazy loads per chart, and that's all. What's left is one `SELECT EXISTS ... ab_permission_view` per chart from `can_access_datasource`, which is a bigger N+1 and I haven't touched it here. Happy to look at that separately if you'd want it.
### TESTING INSTRUCTIONS
`pytest tests/unit_tests/dao/dashboard_test.py`
The new test uses `inspect(slc).unloaded` to assert both relationships start unloaded and are loaded on every member chart after the prefetch.
I also ran `tests/unit_tests/dao/`, `tests/unit_tests/security/manager_test.py`, `tests/unit_tests/tasks/test_utils.py`, `tests/unit_tests/dashboards/` and `tests/unit_tests/models/`: 978 pass, 21 fail. The same 21 fail on 574d121 in the same environment — 15 in `models/test_hours_offset_bound_truncation.py`, 5 in `models/core_test.py`, 1 in `security/manager_test.py` — so none of them come from this change.
`ruff check` and `ruff format --check` are clean on the three changed files.
### ADDITIONAL INFORMATION
- [ ] 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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5584301069
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
Re: [PR] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5718519328
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 an incremental AI Review.
- `/review full` - 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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev merged PR #43990:
URL: https://github.com/apache/superset/pull/43990
--
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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "no-hup (via GitHub)" <gi...@apache.org>.
no-hup commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5768578683
Good call — added that in e007c6a. It's a companion to the /charts test: materialize the slices (like `schema.dump` does), prefetch, then read editors/viewers per slice and assert no extra SQL. I checked it's load-bearing — drop the `prefetch_chart_access` call and it fails with 6 statements for 3 charts, so it'll catch that call being removed or moved after serialization.
--
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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5613222223
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 an incremental AI Review.
- `/review full` - 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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "no-hup (via GitHub)" <gi...@apache.org>.
no-hup commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5775346270
You're right, and thanks for pulling the prefetch out to check — that's the gap. The test mirrors the api.py sequence (dump → prefetch → read editors/viewers) rather than driving the handler, so it pins the ordering but not the call itself. Removing `prefetch_chart_access` from api.py leaves it green, exactly as you saw.
The reason I stopped short of `inspect.unwrap(DashboardRestApi.get)` is that the real handler runs `schema.dump(dash)` first, and that serialization issues its own queries over the dashboard — so a statement count around the whole call isn't clean, it has to isolate the editors/viewers reads from the dump's own SQL. Doable, just more than the DAO-level test needed.
I'll add the handler-level version: unwrap `get`, invoke it in a request context with `can_access_chart` stubbed to read editors/viewers, count only those reads, and confirm it fails when the prefetch line is removed. I don't want to push it until I've actually run it green both ways instead of eyeballing it — my local unit env is currently wedged on an unrelated flask-caching init error, so it'll take me a moment to sort that first. Happy to leave it as a follow-up if you'd rather not hold the PR on a non-blocking test.
--
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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "no-hup (via GitHub)" <gi...@apache.org>.
no-hup commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5601295171
One correction to what I said above, since you raised the 1,000-chart case and my number doesn't hold there.
I said the count stays at 3. That's only true inside the first `selectinload` batch. It batches its own IN list at 500, so each relationship adds a statement per 500 charts. Measured on the branch:
| charts | statements |
|---|---|
| 3 | 3 |
| 25 | 3 |
| 600 | 5 |
| 1200 | 7 |
So it's `1 + 2 * ceil(N / 500)`, not constant. Still 7 against 2400 for a 1,200-chart dashboard, and the outer query is one bind parameter at every size, so your portability point is fully addressed — but "constant-query contract" was the wrong words and my test name said it too. Renamed it and wrote the batching into the docstring in 4f7abee.
Nothing else changed.
--
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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #43990:
URL: https://github.com/apache/superset/pull/43990#discussion_r4087723874
##########
superset/daos/dashboard.py:
##########
@@ -203,6 +208,37 @@ def get_by_id_or_slug(cls, id_or_slug: int | str) -> Dashboard:
return dashboard
+ @staticmethod
+ def prefetch_chart_access(dashboard: Dashboard) -> None:
+ """
+ Load the editors and viewers of a dashboard's charts up front.
+
+ The per-chart access check reads both on every slice, so without this
+ they are two lazy loads per chart rather than two queries in total.
Review Comment:
The editor and viewer paths still grow with chart count after this. `is_editor` and `is_viewer` each call `get_user_subject_ids`, which runs a query on every call, and that's the one-per-chart left in your first table (11, 26, 46 for editors, two per chart for viewers).
Fine to leave it for the follow-up you mentioned, but could the description say so next to the `can_access_datasource` caveat? Caching the subject ids for the request would make both paths flat.
--
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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5770117270
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 an incremental AI Review.
- `/review full` - 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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #43990:
URL: https://github.com/apache/superset/pull/43990#discussion_r3965468227
##########
superset/daos/dashboard.py:
##########
@@ -203,6 +203,23 @@ def get_by_id_or_slug(cls, id_or_slug: int | str) -> Dashboard:
return dashboard
+ @staticmethod
+ def prefetch_chart_access(dashboard: Dashboard) -> None:
+ """
+ Load the editors and viewers of a dashboard's charts up front.
+
+ The per-chart access check reads both on every slice, so without this
+ they are two lazy loads per chart rather than two queries in total.
+ """
+ if slice_ids := [slc.id for slc in dashboard.slices]:
Review Comment:
The PR description calls out that admins short-circuit before these relationships are read, but this helper runs before `can_access_chart()`. That means an admin request will still execute the `Slice` query plus both relationship loads on each endpoint, turning the existing zero-query access-check path into three extra statements for a typical dashboard. Could we preserve the admin short-circuit here (or at the call sites) and cover that path in a test?
##########
tests/unit_tests/dao/dashboard_test.py:
##########
@@ -168,3 +171,45 @@ def test_set_dash_metadata_updates_refresh_frequency_when_present(
assert md["refresh_frequency"] == 0, (
"refresh_frequency should be updated when present in data"
)
+
+
+def test_prefetch_chart_access_loads_editors_and_viewers(
+ session: Session,
+) -> None:
+ """The per-chart access check reads editors and viewers on every slice.
+
+ Without the prefetch those are two lazy loads per chart, so the dashboard
+ GET issues a pair of queries for each member chart it narrows.
+ """
+ Dashboard.metadata.create_all(session.get_bind())
+
+ editor = Subject(label="editor", type=SubjectType.ROLE)
+ viewer = Subject(label="viewer", type=SubjectType.ROLE)
+ dashboard = Dashboard(dashboard_title="prefetch", slug="prefetch")
+ for i in range(3):
+ dashboard.slices.append(
+ Slice(
+ slice_name=f"chart-{i}",
+ datasource_type="table",
+ datasource_id=1,
+ viz_type="table",
+ editors=[editor],
+ viewers=[viewer],
+ )
+ )
+ session.add(dashboard)
+ session.flush()
+
+ # Drop everything from the identity map so the relationships start unloaded.
+ session.expire_all()
+ dashboard = session.query(Dashboard).filter_by(slug="prefetch").one()
+ assert all("editors" in inspect(slc).unloaded for slc in dashboard.slices)
+
+ DashboardDAO.prefetch_chart_access(dashboard)
Review Comment:
Could this test assert the number of SQL statements as well as the loaded state? An implementation that loops over the slices and touches `editors`/`viewers` would still pass this test while bringing back the exact `2N` behavior we're fixing. A `before_cursor_execute` listener around the prefetch, ideally comparing dashboards of two sizes, would make the constant-query contract explicit.
##########
superset/daos/dashboard.py:
##########
@@ -203,6 +203,23 @@ def get_by_id_or_slug(cls, id_or_slug: int | str) -> Dashboard:
return dashboard
+ @staticmethod
+ def prefetch_chart_access(dashboard: Dashboard) -> None:
+ """
+ Load the editors and viewers of a dashboard's charts up front.
+
+ The per-chart access check reads both on every slice, so without this
+ they are two lazy loads per chart rather than two queries in total.
+ """
+ if slice_ids := [slc.id for slc in dashboard.slices]:
+ db.session.query(Slice).options(
+ # The rows are already in the session, we only want the two
+ # relationships, so don't re-fire the model's own eager loads.
+ lazyload("*"),
+ selectinload(Slice.editors),
+ selectinload(Slice.viewers),
+ ).filter(Slice.id.in_(slice_ids)).all()
Review Comment:
This outer `IN` expands to one bind parameter per chart; `selectinload` chunks its own relationship queries, but it doesn't chunk this filter. Superset's SQLite code treats 999 variables as the portability floor, so a dashboard with 1,000+ charts can make this GET fail instead of merely getting slower. Could we select through `dashboard_slices` using `dashboard.id` (or explicitly chunk these IDs) to avoid the unbounded parameter list?
--
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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5601059748
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 2e5f22037f55abe6a9d965c0fae651d532e7531b |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6aa141a694d7790008810e01 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-43990--superset-docs-preview.netlify.app](https://deploy-preview-43990--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> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctLCoAwDADRu2RtMIHi7wYeIzaVCmpLW0ER7253s3jzQkzhiAUmgAaWJKf1tdftblWyX4IkReslFRRrXc54YtyvjFy5uriHZ9Y6dCJsWLrRaN-PRDQMTI4Yvh8) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43990--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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "no-hup (via GitHub)" <gi...@apache.org>.
no-hup commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5682529848
Heads-up on the red checks: the whole matrix is failing on the DB setup step, not on anything in this PR. The error is:
```
ERROR [flask_migrate] Error: Multiple head revisions are present for given argument 'head';
please specify a specific target revision, '<branchname>@head' to narrow to a specific head, ...
```
That's a migration multiple-heads state on `master` — `superset db upgrade` can't pick a single head, so every job that boots the app (backend db tests, playwright, cypress, docker-build) errors out before running. This branch is already merged up to current `master` (`12da1c4e`) and touches only `daos/dashboard.py`, `dashboards/api.py`, and one unit test — no migrations. So there's nothing here to fix; it'll go green once a merge migration lands on `master` and I re-sync. Just flagging so the red isn't read as this PR being broken.
--
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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "no-hup (via GitHub)" <gi...@apache.org>.
no-hup commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5715953026
Good catch, you're right. `get_charts_for_dashboard` returned `dashboard.slices` after the prefetch, so when the collection started unloaded the prefetched charts had nothing keeping them alive — the return reloaded them with editors/viewers unloaded again and paid the per-chart queries.
Fixed in 566366b: hold `charts = dashboard.slices` before prefetching and return that same list, so the prefetched relationships stay on the instances the caller reads.
Added a regression test (`test_get_charts_for_dashboard_returns_the_prefetched_charts`) along the lines of your reproduction: it seeds a dashboard, expunges the session so the lookup returns it with slices unloaded, runs it through the helper, then counts statements while reading the returned charts' editors/viewers. It's 0 with the reorder, and I checked it fails on the old ordering. The dao test file is 7 passed locally — unit tests only, I didn't run the integration/Postgres suite.
--
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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "no-hup (via GitHub)" <gi...@apache.org>.
no-hup commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5621159519
Note on the red `unit-tests` check: it isn't the tests. The run on the current commit (6e4df11, freshly rebased on master) shows `15030 passed, 5 skipped, 2 xfailed` — the whole suite is green. The job then gets marked failed because the post-test codecov `upload-coverage` step is cancelled: the upload itself returns 200 ("Upload queued for processing"), and immediately after that the log shows `##[error]The operation was canceled.` It's cancelled at the coverage-upload step, not in any test our diff touches.
I've refreshed the base to re-trigger everything and it reproduces the same way, so I don't think there's anything actionable in the diff here. Flagging it once rather than repeatedly asking for re-runs.
--
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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5593968540
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 an incremental AI Review.
- `/review full` - 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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "no-hup (via GitHub)" <gi...@apache.org>.
no-hup commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5601080346
Thanks — all three were right, and the admin one was a genuine regression. Fixed in 2e5f220.
**Admin path.** You're correct, and I had this backwards in the description. The short-circuit is inside `is_editor`/`is_viewer` (`if self.is_admin(): return True`), which is *after* my helper runs, so an admin was paying three statements for relationships nothing would read. `prefetch_chart_access` now returns early on `is_admin()`, and there's a test asserting it issues zero statements in that case.
Worth noting this doesn't add a new context requirement: `is_editor`/`is_viewer` already call `is_admin()` first, so anything reaching the access check needed `g.user` regardless. It just moves the same call a few lines earlier in the same request.
**Parameter list.** Also right, and I checked the 999 figure against the repo rather than taking it as folklore — `migrations/shared/catalogs.py:126` sets `max_sqlite_in = 999` and `tasks/version_history_retention.py:245` documents the same floor. It now selects through `dashboard_slices` on `dashboard.id`, so it's one bind parameter no matter how many charts.
**Test.** Fair, and the old assertion really was too weak. It now counts statements at 3 charts and 25 and requires them equal. I checked it actually catches the case you described by temporarily swapping in a loop-and-touch implementation:
```
prefetch scaled with chart count: 8 statements for 3 charts, 52 for 25
```
With the real implementation both sizes are 3 — the slice query plus one per relationship.
`pytest tests/unit_tests/dao tests/unit_tests/dashboards` is 258 passed. One thing I did not check: any of this against a real Postgres or the integration suite — I only ran the unit tests locally.
--
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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #43990:
URL: https://github.com/apache/superset/pull/43990#discussion_r4066172628
##########
tests/unit_tests/dao/dashboard_test.py:
##########
@@ -168,3 +172,183 @@ def test_set_dash_metadata_updates_refresh_frequency_when_present(
assert md["refresh_frequency"] == 0, (
"refresh_frequency should be updated when present in data"
)
+
+
+def _make_dashboard(slug: str, n_charts: int) -> Dashboard:
+ """A dashboard with n_charts member charts, each with one editor and viewer."""
+ editor = Subject(label=f"editor-{slug}", type=SubjectType.ROLE)
+ viewer = Subject(label=f"viewer-{slug}", type=SubjectType.ROLE)
+ dashboard = Dashboard(dashboard_title=slug, slug=slug)
+ for i in range(n_charts):
+ dashboard.slices.append(
+ Slice(
+ slice_name=f"{slug}-chart-{i}",
+ datasource_type="table",
+ datasource_id=1,
+ viz_type="table",
+ editors=[editor],
+ viewers=[viewer],
+ )
+ )
+ db.session.add(dashboard)
+ db.session.flush()
+ return dashboard
+
+
+def _count_statements(session: Session, fn) -> int:
+ """Number of SQL statements fn issues."""
+ seen = []
+
+ def before(conn, cursor, statement, params, ctx, many): # noqa: PLR0913
+ seen.append(statement)
+
+ bind = session.get_bind()
+ event.listen(bind, "before_cursor_execute", before)
+ try:
+ fn()
+ finally:
+ event.remove(bind, "before_cursor_execute", before)
+ return len(seen)
+
+
+def test_prefetch_chart_access_loads_editors_and_viewers(
+ session: Session,
+) -> None:
+ """The per-chart access check reads editors and viewers on every slice.
+
+ Without the prefetch those are two lazy loads per chart, so the dashboard
+ GET issues a pair of queries for each member chart it narrows.
+ """
+ Dashboard.metadata.create_all(session.get_bind())
+
+ editor = Subject(label="editor", type=SubjectType.ROLE)
+ viewer = Subject(label="viewer", type=SubjectType.ROLE)
+ dashboard = Dashboard(dashboard_title="prefetch", slug="prefetch")
+ for i in range(3):
+ dashboard.slices.append(
+ Slice(
+ slice_name=f"chart-{i}",
+ datasource_type="table",
+ datasource_id=1,
+ viz_type="table",
+ editors=[editor],
+ viewers=[viewer],
+ )
+ )
+ session.add(dashboard)
+ session.flush()
+
+ # Drop everything from the identity map so the relationships start unloaded.
+ session.expire_all()
+ dashboard = session.query(Dashboard).filter_by(slug="prefetch").one()
+ assert all("editors" in inspect(slc).unloaded for slc in dashboard.slices)
+
+ # The access check short-circuits for an admin without reading either
+ # relationship, so the prefetch is a non-admin path.
+ with patch.object(security_manager, "is_admin", return_value=False):
+ DashboardDAO.prefetch_chart_access(dashboard)
+
+ for slc in dashboard.slices:
+ unloaded = inspect(slc).unloaded
+ assert "editors" not in unloaded
+ assert "viewers" not in unloaded
+ assert [s.label for s in slc.editors] == ["editor"]
+ assert [s.label for s in slc.viewers] == ["viewer"]
+
+
+def test_prefetch_chart_access_does_no_per_chart_work(
+ session: Session,
+) -> None:
+ """Adding charts must not add statements.
+
+ Asserting only that the relationships end up loaded would also pass for an
+ implementation that walks the slices and touches each one, which is the 2N
+ behaviour this is meant to remove. So count the statements at two sizes and
+ require the same number.
+
+ Not literally constant forever: selectinload batches its own IN lists at 500,
+ so each relationship adds one statement per 500 charts (1200 charts is 7
+ statements, against 2400 before). Both sizes here sit inside the first batch,
+ which is what makes the equality check meaningful.
+ """
+ Dashboard.metadata.create_all(session.get_bind())
+
+ small = _make_dashboard("count-small", 3)
+ large = _make_dashboard("count-large", 25)
+ session.expire_all()
+
+ with patch.object(security_manager, "is_admin", return_value=False):
+ small = session.query(Dashboard).filter_by(slug="count-small").one()
+ small_n = _count_statements(
+ session, lambda: DashboardDAO.prefetch_chart_access(small)
+ )
+ large = session.query(Dashboard).filter_by(slug="count-large").one()
+ large_n = _count_statements(
+ session, lambda: DashboardDAO.prefetch_chart_access(large)
+ )
+
+ assert small_n == large_n, (
+ f"prefetch scaled with chart count: {small_n} statements for 3 charts, "
+ f"{large_n} for 25"
+ )
+ assert small_n == 3, f"expected slices + editors + viewers, got {small_n}"
+
+
+def test_prefetch_chart_access_skips_the_query_for_admins(
+ session: Session,
+) -> None:
+ """An admin's access check never reads editors or viewers, so don't prefetch.
+
+ is_editor and is_viewer both return True on is_admin() before touching
+ either relationship. Prefetching for an admin would turn a zero-query
+ access check into three extra statements.
+ """
+ Dashboard.metadata.create_all(session.get_bind())
+
+ dashboard = _make_dashboard("admin-skip", 5)
+ session.expire_all()
+ dashboard = session.query(Dashboard).filter_by(slug="admin-skip").one()
+
+ with patch.object(security_manager, "is_admin", return_value=True):
+ n = _count_statements(
+ session, lambda: DashboardDAO.prefetch_chart_access(dashboard)
+ )
+
+ assert n == 0, f"prefetch issued {n} statements for an admin"
+
+
+def test_get_charts_for_dashboard_returns_the_prefetched_charts(
+ session: Session,
+) -> None:
+ """The charts endpoint must read editors/viewers without per-chart queries.
+
+ get_charts_for_dashboard looks the dashboard up with its slices unloaded,
+ so the prefetched charts and the returned collection have to be the same
+ instances. If the method reloaded dashboard.slices after prefetching, the
+ weak identity map could drop the prefetched charts and reading their
+ editors/viewers would fall back to a query per chart -- the 2N cost this is
+ meant to remove.
+ """
+ Dashboard.metadata.create_all(session.get_bind())
+
+ _make_dashboard("charts-endpoint", 3)
+ # Detach everything so the lookup returns a dashboard with slices unloaded,
+ # matching the real request path.
+ session.expunge_all()
+ dashboard = session.query(Dashboard).filter_by(slug="charts-endpoint").one()
+
+ with (
+ patch.object(security_manager, "is_admin", return_value=False),
+ patch.object(DashboardDAO, "get_by_id_or_slug", return_value=dashboard),
+ ):
+ charts = DashboardDAO.get_charts_for_dashboard("charts-endpoint")
+
+ def read_relationships() -> None:
+ for slc in charts:
+ assert [s.label for s in slc.editors] == ["editor-charts-endpoint"]
+ assert [s.label for s in slc.viewers] == ["viewer-charts-endpoint"]
+
+ n = _count_statements(session, read_relationships)
+
+ assert len(charts) == 3
+ assert n == 0, f"reading the returned charts issued {n} statements"
Review Comment:
Thanks for adding this regression test. Starting with `expunge_all()` and counting queries when the returned relationships are consumed captures the weak-identity-map issue from the previous review. Together with keeping `charts` alive in the DAO, this addresses that concern.
##########
superset/dashboards/api.py:
##########
@@ -682,7 +682,9 @@ def get(
if "charts" in result:
# Only name the member charts the caller can access, consistent with
# the per-object narrowing applied to the dashboard's datasets and
- # charts sub-resources.
+ # charts sub-resources. The check reads each chart's editors and
+ # viewers, so load them for the whole set first.
+ DashboardDAO.prefetch_chart_access(dash)
Review Comment:
Optional test follow-up: could we cover this call site too? It works because `schema.dump(dash)` reads `Dashboard.charts` and materializes `dash.slices` before the prefetch. The new DAO regression test protects the `/charts` path, but wouldn't catch this call being removed or moved ahead of serialization. A focused test starting with unloaded slices and checking that the chart-access loop reads editors/viewers without additional SQL would protect the main dashboard GET as well.
--
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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5584538167
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43990?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 66.32%. Comparing base ([`53070fa`](https://app.codecov.io/gh/apache/superset/commit/53070fa4c04c192118cb393c3e5bbe68eb1abbcb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b5b4c21`](https://app.codecov.io/gh/apache/superset/commit/b5b4c21c6dfca695967b78d088f63dd054c10fd1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 11 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (53070fa) and HEAD (b5b4c21). Click for more details.
>
> <details><summary>HEAD has 137 uploads less than BASE</summary>
>
>| Flag | BASE (53070fa) | HEAD (b5b4c21) |
>|------|------|------|
>|python|96|3|
>|presto|16|1|
>|hive|15|1|
>|sqlite|16|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43990 +/- ##
===========================================
- Coverage 79.81% 66.32% -13.49%
===========================================
Files 2914 2914
Lines 170467 170480 +13
Branches 39518 39519 +1
===========================================
- Hits 136060 113074 -22986
- Misses 31885 55059 +23174
+ Partials 2522 2347 -175
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43990/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/43990/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.63% <33.33%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/43990/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/43990/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/43990/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.51% <33.33%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/43990/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.19% <100.00%> (-27.00%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/43990/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.06% <100.00%> (+<0.01%)` | :arrow_up: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/43990/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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 Harness](https://app.codecov.io/gh/apache/superset/pull/43990?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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #43990:
URL: https://github.com/apache/superset/pull/43990#discussion_r4069225310
##########
tests/unit_tests/dao/dashboard_test.py:
##########
@@ -168,3 +172,221 @@ def test_set_dash_metadata_updates_refresh_frequency_when_present(
assert md["refresh_frequency"] == 0, (
"refresh_frequency should be updated when present in data"
)
+
+
+def _make_dashboard(slug: str, n_charts: int) -> Dashboard:
+ """A dashboard with n_charts member charts, each with one editor and viewer."""
+ editor = Subject(label=f"editor-{slug}", type=SubjectType.ROLE)
+ viewer = Subject(label=f"viewer-{slug}", type=SubjectType.ROLE)
+ dashboard = Dashboard(dashboard_title=slug, slug=slug)
+ for i in range(n_charts):
+ dashboard.slices.append(
+ Slice(
+ slice_name=f"{slug}-chart-{i}",
+ datasource_type="table",
+ datasource_id=1,
+ viz_type="table",
+ editors=[editor],
+ viewers=[viewer],
+ )
+ )
+ db.session.add(dashboard)
+ db.session.flush()
+ return dashboard
+
+
+def _count_statements(session: Session, fn) -> int:
+ """Number of SQL statements fn issues."""
+ seen = []
+
+ def before(conn, cursor, statement, params, ctx, many): # noqa: PLR0913
+ seen.append(statement)
+
+ bind = session.get_bind()
+ event.listen(bind, "before_cursor_execute", before)
+ try:
+ fn()
+ finally:
+ event.remove(bind, "before_cursor_execute", before)
+ return len(seen)
+
+
+def test_prefetch_chart_access_loads_editors_and_viewers(
+ session: Session,
+) -> None:
+ """The per-chart access check reads editors and viewers on every slice.
+
+ Without the prefetch those are two lazy loads per chart, so the dashboard
+ GET issues a pair of queries for each member chart it narrows.
+ """
+ Dashboard.metadata.create_all(session.get_bind())
+
+ editor = Subject(label="editor", type=SubjectType.ROLE)
+ viewer = Subject(label="viewer", type=SubjectType.ROLE)
+ dashboard = Dashboard(dashboard_title="prefetch", slug="prefetch")
+ for i in range(3):
+ dashboard.slices.append(
+ Slice(
+ slice_name=f"chart-{i}",
+ datasource_type="table",
+ datasource_id=1,
+ viz_type="table",
+ editors=[editor],
+ viewers=[viewer],
+ )
+ )
+ session.add(dashboard)
+ session.flush()
+
+ # Drop everything from the identity map so the relationships start unloaded.
+ session.expire_all()
+ dashboard = session.query(Dashboard).filter_by(slug="prefetch").one()
+ assert all("editors" in inspect(slc).unloaded for slc in dashboard.slices)
+
+ # The access check short-circuits for an admin without reading either
+ # relationship, so the prefetch is a non-admin path.
+ with patch.object(security_manager, "is_admin", return_value=False):
+ DashboardDAO.prefetch_chart_access(dashboard)
+
+ for slc in dashboard.slices:
+ unloaded = inspect(slc).unloaded
+ assert "editors" not in unloaded
+ assert "viewers" not in unloaded
+ assert [s.label for s in slc.editors] == ["editor"]
+ assert [s.label for s in slc.viewers] == ["viewer"]
+
+
+def test_prefetch_chart_access_does_no_per_chart_work(
+ session: Session,
+) -> None:
+ """Adding charts must not add statements.
+
+ Asserting only that the relationships end up loaded would also pass for an
+ implementation that walks the slices and touches each one, which is the 2N
+ behaviour this is meant to remove. So count the statements at two sizes and
+ require the same number.
+
+ Not literally constant forever: selectinload batches its own IN lists at 500,
+ so each relationship adds one statement per 500 charts (1200 charts is 7
+ statements, against 2400 before). Both sizes here sit inside the first batch,
+ which is what makes the equality check meaningful.
+ """
+ Dashboard.metadata.create_all(session.get_bind())
+
+ small = _make_dashboard("count-small", 3)
+ large = _make_dashboard("count-large", 25)
+ session.expire_all()
+
+ with patch.object(security_manager, "is_admin", return_value=False):
+ small = session.query(Dashboard).filter_by(slug="count-small").one()
+ small_n = _count_statements(
+ session, lambda: DashboardDAO.prefetch_chart_access(small)
+ )
+ large = session.query(Dashboard).filter_by(slug="count-large").one()
+ large_n = _count_statements(
+ session, lambda: DashboardDAO.prefetch_chart_access(large)
+ )
+
+ assert small_n == large_n, (
+ f"prefetch scaled with chart count: {small_n} statements for 3 charts, "
+ f"{large_n} for 25"
+ )
+ assert small_n == 3, f"expected slices + editors + viewers, got {small_n}"
+
+
+def test_prefetch_chart_access_skips_the_query_for_admins(
+ session: Session,
+) -> None:
+ """An admin's access check never reads editors or viewers, so don't prefetch.
+
+ is_editor and is_viewer both return True on is_admin() before touching
+ either relationship. Prefetching for an admin would turn a zero-query
+ access check into three extra statements.
+ """
+ Dashboard.metadata.create_all(session.get_bind())
+
+ dashboard = _make_dashboard("admin-skip", 5)
+ session.expire_all()
+ dashboard = session.query(Dashboard).filter_by(slug="admin-skip").one()
+
+ with patch.object(security_manager, "is_admin", return_value=True):
+ n = _count_statements(
+ session, lambda: DashboardDAO.prefetch_chart_access(dashboard)
+ )
+
+ assert n == 0, f"prefetch issued {n} statements for an admin"
+
+
+def test_get_charts_for_dashboard_returns_the_prefetched_charts(
+ session: Session,
+) -> None:
+ """The charts endpoint must read editors/viewers without per-chart queries.
+
+ get_charts_for_dashboard looks the dashboard up with its slices unloaded,
+ so the prefetched charts and the returned collection have to be the same
+ instances. If the method reloaded dashboard.slices after prefetching, the
+ weak identity map could drop the prefetched charts and reading their
+ editors/viewers would fall back to a query per chart -- the 2N cost this is
+ meant to remove.
+ """
+ Dashboard.metadata.create_all(session.get_bind())
+
+ _make_dashboard("charts-endpoint", 3)
+ # Detach everything so the lookup returns a dashboard with slices unloaded,
+ # matching the real request path.
+ session.expunge_all()
+ dashboard = session.query(Dashboard).filter_by(slug="charts-endpoint").one()
+
+ with (
+ patch.object(security_manager, "is_admin", return_value=False),
+ patch.object(DashboardDAO, "get_by_id_or_slug", return_value=dashboard),
+ ):
+ charts = DashboardDAO.get_charts_for_dashboard("charts-endpoint")
+
+ def read_relationships() -> None:
+ for slc in charts:
+ assert [s.label for s in slc.editors] == ["editor-charts-endpoint"]
+ assert [s.label for s in slc.viewers] == ["viewer-charts-endpoint"]
+
+ n = _count_statements(session, read_relationships)
+
+ assert len(charts) == 3
+ assert n == 0, f"reading the returned charts issued {n} statements"
+
+
+def test_prefetch_before_the_main_get_access_loop_reads_no_extra_sql(
+ session: Session,
+) -> None:
+ """The dashboard GET narrows ``charts`` with the same per-slice access check.
+
+ ``DashboardApi.get`` prefetches, then keeps only the slices the caller can
+ access by reading each one's editors and viewers (superset/dashboards/api.py).
+ The /charts test above protects ``get_charts_for_dashboard``; this protects
+ the main GET call site, which relies on the prefetch running before the loop.
+ If that call were removed or moved after serialization the loop would fall
+ back to two lazy loads per chart -- the 2N cost the prefetch removes.
+ """
+ Dashboard.metadata.create_all(session.get_bind())
+
+ _make_dashboard("main-get", 3)
+ # Detach everything so the dashboard comes back with its slices unloaded,
+ # matching the request path.
+ session.expunge_all()
+ dashboard = session.query(Dashboard).filter_by(slug="main-get").one()
+
+ with patch.object(security_manager, "is_admin", return_value=False):
+ # ``schema.dump(dash)`` reads ``Dashboard.charts`` and materializes the
+ # slices before the prefetch runs; hold the same instances so the loop
+ # below reads their relationships rather than reloading the collection.
+ slices = list(dashboard.slices)
+ DashboardDAO.prefetch_chart_access(dashboard)
Review Comment:
Thanks for picking this up. Could we invoke the actual `DashboardRestApi.get` handler here instead of reproducing its sequence in the test? I temporarily removed `DashboardDAO.prefetch_chart_access(dash)` from `superset/dashboards/api.py` and all eight tests still passed, including this one. Calling the helper directly here means the test can't detect removal or reordering at the API call site.
A focused unit test could use `inspect.unwrap(DashboardRestApi.get)` inside a request context, keep the real schema serialization, and have the `can_access_chart` stub read editors/viewers while counting those SQL statements. That would exercise the production ordering and fail when the handler's prefetch is removed. This remains a non-blocking test suggestion.
--
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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #43990:
URL: https://github.com/apache/superset/pull/43990#discussion_r4035505094
##########
superset/daos/dashboard.py:
##########
@@ -215,7 +251,10 @@ def get_tabs_for_dashboard(id_or_slug: str) -> dict[str, Any]:
@staticmethod
def get_charts_for_dashboard(id_or_slug: str) -> list[Slice]:
- return DashboardDAO.get_by_id_or_slug(id_or_slug).slices
+ dashboard = DashboardDAO.get_by_id_or_slug(id_or_slug)
+ # The caller narrows each chart by access, which reads these.
+ DashboardDAO.prefetch_chart_access(dashboard)
+ return dashboard.slices
Review Comment:
Could we load `dashboard.slices` before calling the prefetch and keep that collection for the return? For a non-admin dashboard editor (or an explicit dashboard viewer), `get_by_id_or_slug()` can return without loading `slices`. The prefetch then discards its `.all()` result, and SQLAlchemy’s weak identity map does not keep those chart objects alive. Accessing `dashboard.slices` afterward loads fresh instances with `editors` and `viewers` still unloaded, so this path pays for the prefetch and still does the per-chart queries.
I reproduced this by seeding three charts, calling `session.expunge_all()`, re-querying the dashboard, and passing it through `get_charts_for_dashboard()` with the dashboard lookup mocked. Reading the returned charts’ editors/viewers issued **six more SQL statements**. Changing the order to `charts = dashboard.slices`, then prefetching, then `return charts` brought that count to zero; all six existing tests plus the regression test passed. Could we also cover that initially-unloaded collection in the tests? The loaded-state test touches `dashboard.slices` before prefetching, and the query-count test stops before consuming the relationships, so neither catches this case.
--
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] fix(dashboard): load chart editors and viewers up front [superset]
Posted by "no-hup (via GitHub)" <gi...@apache.org>.
no-hup commented on PR #43990:
URL: https://github.com/apache/superset/pull/43990#issuecomment-5806540174
Good point — you're right that `is_editor`/`is_viewer` each call `get_user_subject_ids`, so the editor and viewer reads stay one query per chart even after the prefetch loads the relationships. I've noted that next to the `can_access_datasource` caveat in the description.
The actual fix for it is in #44017: it memoises `get_user_subject_ids` per request, which flattens the editor/viewer paths (and `can_access_datasource`) to a constant number of subject-id lookups. I kept it in a separate PR since it touches the security manager rather than this DAO.
Also refreshed this branch on master (e2ab684) to clear the merge conflict.
--
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