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