no-hup opened a new pull request, #44017: URL: https://github.com/apache/superset/pull/44017
### SUMMARY `is_editor` and `is_viewer` both start by resolving the current user's subject IDs, and both take a single resource. Anything that checks a list of objects calls them in a loop, so the subject lookup runs once per object. The lookup isn't cheap — `get_user_subject_ids_subquery` is a three-way `UNION ALL` over the user's own subject, their direct and group-derived roles, and their groups. Loading a dashboard runs `can_access_chart` per chart, which is where I noticed it. Counting the union queries while checking 40 charts as a non-admin: | | subject-union queries | |---|---| | master | 80 | | this branch | 1 | Two per chart, because the editor check and the viewer check each do their own lookup. The fix memoises the result on `g`, keyed by user id. Nothing in a request reads a user's own subjects after changing them, so the entry can't go stale underneath a caller. Two things I was deliberate about: - **Outside a request context the behaviour is unchanged.** It's guarded by `has_request_context()`, so Celery tasks and CLI commands keep reading through every time. That matters because `g` is bound to the app context, not the request, and a worker can hold one open for a long time. - **It returns a copy.** One caller hands the list into the bootstrap payload that `COMMON_BOOTSTRAP_OVERRIDES_FUNC` is allowed to edit, so handing out the cached list itself would let a customisation hook corrupt it. Happy to change the cache key or drop it behind a config flag if you'd rather — this seemed like the smallest version that works. ### TESTING INSTRUCTIONS `pytest tests/unit_tests/subjects tests/unit_tests/commands tests/unit_tests/dao` — 1237 passed. Three new tests in `tests/unit_tests/subjects/test_utils.py` cover the cases I cared about: it runs once per user per request, a second user is a separate entry, it still reads through with no request context, and mutating the returned list doesn't poison the cache. `ruff check`, `ruff format --check` and `mypy` are clean on both 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]
