[dbernstein]

Description

Answer the five Admin.is_* permission checks from the roles relationship instead of HasSessionCache.

HasSessionCache._cache_lookup only caches a lookup that returns a row — the insert is guarded by if obj is not None. Used as an existence check, a miss therefore re-queries the database on every single call.

Asking whether an admin is a librarian for a library in which they have no associated roles cost ~5 uncached queries (is_system_admin, is_sitewide_library_manager, is_library_manager, is_sitewide_librarian, is_librarian, each a fresh get_one). /admin/collections asks that question once per collection/library pair — in can_see_collection, which loops over a collection's associated libraries, and again in configured_service_library_info for every library configuration. On a consortium CM where each collection is associated with every member library, that is libraries × collections × 5 queries.

remove_role now also detaches the role from the loaded roles collection. SQLAlchemy does not prune a collection when one of its members is deleted, so a removed role would otherwise keep granting access for the rest of the session. Three existing tests cover this.

Motivation and Context

Library staff (any admin who is not a system admin) could not load the Lists page on a CM with a large number of libraries (ie over 1000). The page showed a loading bar and an error; the underlying cause was /admin/collections returning 504.

This only affected non-system-admins, and the cache is why. A system admin has a system role row, so the very first lookup hits, gets cached, and every later check is a dict read. A library admin has no matching row for any library but their own, so nothing ever caches. The asymmetry belongs to the cache, not to the permission model.

Measured on a consortium with 14 libraries and 11 collections:

	services	visible	queries
system admin	11	11	35
librarian	11	9	548

After this change the librarian issues 35 queries — identical to the system admin — and the per-library factor is gone.

Note that this is not a recent regression. The N+1 predates the current release; a consortium simply has to grow past the point where libraries × collections × 5 exceeds the gateway timeout.

How Has This Been Tested?

Two new regression tests, both confirmed to fail without the fix:

• test_role_checks_read_roles_once (tests/manager/sqlalchemy/model/test_admin.py) — asserts a permission check reads the admin's roles once however many libraries it is asked about. Fails on main with assert 71 <= 2.
• test_collections_get_checks_roles_once_for_non_system_admins (tests/manager/api/admin/controller/test_collections.py) — asserts the collections listing issues at most one adminroles query. Fails on main with assert 86 <= 1.

Full runs:

• 1,976 tests passing across tests/manager/api and tests/manager/sqlalchemy
• uv run mypy clean on 1,174 source files
• pre-commit run clean

Verified end-to-end against a local CM restored from consortium data: signed in as a library admin, loaded the Lists page, and confirmed it renders fully and auto-redirects into list editing — the redirect that the fetch error was previously blocking. All admin endpoints reachable by a librarian returned 200.

Follow-ups (not in this PR)

An audit of the rest of the admin API turned up no other misuse of the session cache as an existence check, but did find several ordinary lazy-load N+1s that grow with the number of libraries (1–2 queries per library, versus the ~37 per library fixed here), and /admin/stats is slow for an unrelated reason — five GROUP BY scans over licensepools, 2.2 s on a small dev dataset. Both are being handled separately.

Checklist

• I have updated the documentation accordingly.
• All new and existing tests passed.

🤖 Generated with Claude Code

[claude]

Claude finished @dbernstein's task in 2m 23s —— View job

---

Code review

• Gather context
• Review admin.py model changes
• Review individual_admin_settings.py changes
• Review tests and fixture
• Post review

Summary

No actionable issues at ed4a812. The core change is correct: _has_role matches the old get_one filters (sitewide roles ignore library_id, library roles match on library_id). Comparing library_id is safe because every AdminRole in src/ is created through add_role, which goes through get_one_or_create and then create, and that flushes. Those callers are handle_roles, authenticated_admin in base.py:89, and the setup path. Nothing builds AdminRole(...) directly, so a loaded role can't have a None library_id.

Session lifetime: the loaded roles collection can't go stale across requests. The scoped session is removed in teardown_appcontext, the same lifetime the old HasSessionCache had.

remove_role: detaching the role and relying on cascade="all, delete-orphan" behaves as intended. A self-edit (where user is admin) sees removals at the same point it did before, when autoflush made the DELETE visible to the next get_one.

handle_roles: snapshotting with list(admin.roles) fixes the skip-on-remove problem. A side effect is that handle_password now iterates the roles as they are after the edit, not a list that still held deleted rows. I traced the permission outcomes and they come out the same, and the result is more consistent with what is_sitewide_library_manager() already reported.

Removing HasSessionCache from AdminRole: nothing else used it. Nothing calls AdminRole.by_id / by_cache_key / cache_warm, and the cache-stats logger in routes.py just walks whatever classes are present in _db.info.

Tests: the two query-count tests are meaningful regression guards. Each pins the adminroles lookups (or total statements) to a constant, and the second collection now forces the full can_see_collection loop.

I reviewed by reading the code only and did not run the test suite.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues remain.

Summary

Permission checks now read Admin.roles instead of making repeated database lookups for missing roles.

• Admin permission checks reuse the roles already loaded for the session.
• Role edits remove dropped roles from the active admin session.

No new actionable issues were found. Tests were not run during this review.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Permission check] --> B[Read Admin.roles]
    B --> C[Load roles once if needed]
    C --> D[Check role and library]
    E[Remove role] --> F[Remove from Admin.roles]
    F --> G[Permission checks see removal immediately]
    F --> H[Delete orphan row on flush]

Loading

_{Reviews (6) · Last reviewed commit: "Drop the redundant delete from remove_ro..." · Reviewed by Greptile}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.87%. Comparing base (d577f98) to head (ed4a812).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3897   +/-   ##
=======================================
  Coverage   93.87%   93.87%           
=======================================
  Files         512      512           
  Lines       46956    46926   -30     
  Branches     6373     6368    -5     
=======================================
- Hits        44079    44051   -28     
  Misses       1857     1857           
+ Partials     1020     1018    -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dbernstein]

Addressed both reviews in cf8ce61.

The role-removal bug (claude, greptile P1) — real, and confirmed by a failing test before the fix.

handle_roles assigned old_roles = admin.roles, the live collection rather than a copy, and then called admin.remove_role(...) while iterating it. Since this PR made remove_role detach the role from that same collection, removing an element shifted the next one past the loop's position. A request dropping two library roles only dropped the first; the second kept its database row and kept granting access, and the request still returned 200.

I reproduced it before changing anything — test_individual_admins_post_edit_removes_every_dropped_role fails on the previous commit with:

assert [<AdminRole: ...>] == []
  Left contains one more item: <AdminRole: role=librarian library=L2 admin=admin@nypl.org>

Fixed by snapshotting with old_roles = list(admin.roles). That also makes old_roles genuinely old: add_role appends to the same collection, so the removal loop was previously walking roles the request had just added.

handle_roles is the only production caller that iterates while removing — I checked every remove_role call site.

Dead session cache on AdminRole (claude, minor) — removed.

Confirmed there are no remaining AdminRole.by_cache_key / by_id / cache_warm calls anywhere in src/ or tests/, and no listener hooks referencing the mixin, so HasSessionCache and cache_key() are both gone from AdminRole. Admin keeps its mixin, since authenticate still uses it.

While there I also dropped the if admin_role in self.roles guard in remove_role. get_one filters on admin=self, so any row it finds is already in the loaded collection — the guard was an unreachable branch.

Type hints (greptile P2) — done. Both record callbacks are annotated and test_role_checks_read_roles_once has -> None.

Coverage. codecov/patch was at 90% against a 93.87% target. The uncovered diff lines were the library is None branches in is_library_manager / is_librarian and the two branches in remove_role. Added test_role_checks_without_a_library and test_remove_role_not_held, and removing the unreachable guard dropped one branch outright. model/admin.py is now at 97%, with the only remaining misses on lines that predate this PR.

Verification: 1,198 tests passing across tests/manager/api/admin and tests/manager/sqlalchemy, uv run mypy clean on 1,174 files, pre-commit run clean.

---

One thing worth flagging separately, since it is not caused by this PR but I ran into it: test_collections.py registers mock APIs into the shared services_fixture LicenseProvidersRegistry singleton (lines 1038, 1131, 1158) and never unregisters them. Any test that later lands in the same xdist worker and reads .protocols hits MockApi, which has no label(). It does not reproduce at directory scope — the full tests/manager/api/admin run is green, as is CI — but a narrow multi-file selection can shuffle the xdist distribution enough to expose it. Worth cleaning up on its own at some point; I left it alone here to keep this diff to the bug.

[dbernstein]

Both suggestions taken, in 33a41f7.

Shared statement recorder. Extracted to DatabaseTransactionFixture.record_statements() in tests/fixtures/database.py, as suggested. Both regression tests now read:

with db.record_statements() as statements:
    ...
assert len(statements) <= 2

contextmanager, Generator, Connection, event and Any were all already imported in that fixture module, so the helper added no new dependencies, and the imports the inline recorders had needed are gone from both test files.

Worth noting for the follow-up N+1 work: #3898 is already open and branched from main, so it won't see this helper until this merges. Not worth coupling the two PRs over — flagging it so the duplication is a known choice rather than an oversight.

remove_role looking the role up in self.roles. Done. It was issuing two queries for one removal, and the unguarded .remove() depended on the row get_one found also being in the loaded list — the relationship is the source of truth for _has_role, so it should be here too.

I checked the one thing this turns on before making the change: the library_id comparison is only safe if roles are always flushed. add_role goes through get_one_or_create → create, which calls flush(db) (util.py:80), so library_id is always populated by the time anything looks at it. That is the same assumption _has_role already makes, so the two are now consistent rather than each relying on something different.

Verification: 1,198 tests passing across tests/manager/api/admin and tests/manager/sqlalchemy — which covers every remove_role call site in the suite, since they all live in those two trees — plus uv run mypy clean on 1,174 files and pre-commit clean. model/admin.py patch coverage is unchanged at 97%, with the only misses on pre-existing lines.

[tdilauro]

This looks good! 🥇

A couple of small issues for your review below.

[tdilauro]

Minor - It looks like we wouldn't completely exercise can_see_collection here.

Roles are checked in two places:

• can_see_collection: Can the admin see the collection at all?
• configured_service_library_info: For the collections the admin can see, check each library.

We currently have only a single collection. The librarian has a role for L1, and L1 is that collection's first library. So can_see_collection checks L1, finds a match, and stops. It woule never checks the other 10 libraries. So, only configured_service_library_info checks all 11.

Adding a collection with associations to the other libraries, but no association to the admin's library, would force all the libraries to be checked in both:

Suggested change

	collection.associated_libraries += [db.library() for _ in range(10)]
	collection.associated_libraries += [db.library() for _ in range(10)]
	# (probably warrants an explanatory comment here, however you want to phrase it)
	unassociated = db.collection(name="Unassociated Collection", protocol=OverdriveAPI)
	unassociated.associated_libraries += [db.library() for _ in range(10)]

[dbernstein]

good thinking. I'll modify.

[dbernstein]

You're right — the loop was only ever running one iteration. db.collection(library=library) appends L1 first, so can_see_collection matched on the first element and the other ten libraries were never reached. Only configured_service_library_info was seeing all eleven.

Took the suggestion, plus an assertion that the response contains only the Consortium Collection — the new collection does its job only if it is actually walked and rejected, and without that assertion a future change that skipped it early would quietly return the test to its old coverage while still passing.

The second collection makes the loop run all ten iterations instead of matching on the first — against the pre-fix code the recorded role queries go from 86 to 157, which is the ten extra libraries being walked. Both versions of the test fail pre-fix, so this is not a better detector of the bug being fixed; it is coverage of the exhaustive no-match path, which the old test never reached.

One caveat — the new names assertion passes against the pre-fix code too. It guards the test's setup; it is not a second regression detector.

[tdilauro]

Minor - I don't think the explicit delete is strictly necessary here. AdminRole has a delete orphan cascade, so that will take care of this. And we already have a test that should cover this, if someone were to remove the cascade.

It doesn't hurt to leave the delete in here, but at a minimum the comment is misleading.

Suggested change

	# Detach the role as well as deleting the row. SQLAlchemy does not
	# prune a loaded collection when a member is deleted, so a role left
	# in it would keep granting access for the rest of the session.
	self.roles.remove(admin_role)
	Session.object_session(self).delete(admin_role)
	self.roles.remove(admin_role)

[dbernstein]

I'll take a closer look.

[dbernstein]

Agreed on both counts. Admin.roles is cascade="all, delete-orphan", so self.roles.remove() orphans the role and the cascade deletes the row — the explicit delete() was redundant. The comment was worse than redundant: it had the causality backwards, describing the removal as bookkeeping on top of the delete when the removal is what does the work.

Dropped the delete and rewrote the comment to say why the removal alone suffices. And you're right that it's covered: test_individual_admin_post_removes_all_roles_not_in_form flushes and queries AdminRole directly, asserting the rows are gone rather than merely detached, so dropping the cascade would fail a test.