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