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