Repository navigation
Conversation
… cache isAdmin, isSummitAdmin, isRegistrationAdmin, isTester, isTrackChairAdmin and isOnGroup ran one Group_Members query per call through getGroupByCode(), the most executed statement on prod-12. They now resolve membership through belongsToGroup(), which answers from $groupMembershipCache after the first lookup per group code. getGroupByCode() consults and fills the same cache so a known negative answer costs no query. belongsToGroup() no longer caches the result when the query fails, so a transient error cannot stick as "not a member" for the request.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "fix(member): route group checks through the request-scoped membership cache" matches the main change. The raw summary shows that Member.php routes isAdmin, isSummitAdmin, isRegistrationAdmi… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "fix(member): route group checks through the request-scoped membership cache" matches the main change. The raw summary shows that Member.php routes isAdmin, isSummitAdmin, isRegistrationAdmin, isTester, isTrackChairAdmin, and isOnGroup through belongsToGroup(), which uses the cache. It also shows getGroupByCode() using the same cache, plus the failure handling that avoids caching a failed lookup. The title is specific and scoped to the primary change. It omits the failure-handling detail, which is acceptable for a short title.
-
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-654/ This page is automatically updated on each push to this PR. |
…the admin short-circuit test
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-654/ This page is automatically updated on each push to this PR. |
ref: https://app.clickup.com/t/86bcjg9a2
Summary
isAdmin(),isSummitAdmin(),isRegistrationAdmin(),isTester(),isTrackChairAdmin()andisOnGroup()now resolve membership throughbelongsToGroup(), which answers from the request-scoped$groupMembershipCacheafter the first lookup per group code. Before, each call ran oneGroup_Membersquery throughgetGroupByCode(), the most executed statement on prod-12 (16.1M executions since the 2026-10-13 restart, about 117 per second).getGroupByCode()consults and fills the same cache, so a known negative answer costs no query. It still queries when it needs theGroupentity.belongsToGroup()no longer caches the answer when the membership query fails. It logs a warning and returnsfalse(fail closed), and the next check retries instead of treating the member as "not on the group" for the rest of the request.Notes:
getGroupByCode()has no caller outsideMember.phpand every internal caller only null-checks it.setGroups(),clearGroups(),add2Group()andremoveFromGroup().Memberinstance kept in the identity map across jobs of a long-lived worker can still serve a stale membership. That exposure already exists forbelongsToGroup()and is not changed here.Test plan
tests/Unit/Models/MemberGroupMembershipFailureTest.php: a failed query is not cached, a successful one still is (no DB needed; fails against the previousbelongsToGroup())tests/Unit/Models/MemberGroupMembershipCacheTest.php(needs the DB) asserts that NisAdmin()calls on the same instance run 2 membership queries,isOnGroup()3, an admin short-circuits without a second lookup, andgetGroupByCode()answers a known negative from the cached11fcd04929dexecutions drop by at least 90% (from about 117 per second)🤖 Generated with Claude Code
Summary by CodeRabbit