feat(msgraph): batch Teams account→id resolution via startsWith advanced query#438
feat(msgraph): batch Teams account→id resolution via startsWith advanced query#438saurav-err-137 wants to merge 2 commits into
Conversation
…uery Replace the per-account GetUserByPrincipalName(account@domain) lookup with a batched, domain-agnostic GetUsersByAccounts: OR-combine startsWith(userPrincipalName,'account@') across the cache-missing accounts into chunked $filter queries (ConsistencyLevel: eventual + $count for the advanced query). Each account contributes both a lower- and upper-cased startsWith clause rather than relying on Graph case-insensitivity (accounts per query halved to stay within the clause budget); results map back by UPN local-part case-insensitively (first match wins). Drops the hardcoded TEAMS_EMAIL_DOMAIN (accounts may live under different domains), avoids per-account round-trips, and never enumerates the whole tenant. Follow-up to #378. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xT3cu9rqxJyNUzYsAv6sL
room-service depends on msgraph.Client only for CreateOnlineMeeting, but the user-lookup method sat on that same interface, forcing room-service's fakes to change whenever it changed. Move GetUsersByAccounts onto its own DirectoryReader interface + NewDirectoryClient constructor (mirroring PresenceReader), so Client is meetings-only again and room-service no longer depends on the user-lookup surface. The sync uses NewDirectoryClient; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xT3cu9rqxJyNUzYsAv6sL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughReplaces the single-user Graph lookup (GetUserByPrincipalName) with a batched, domain-agnostic user resolver (GetUsersByAccounts) via a new DirectoryReader interface in pkg/msgraph, updates user-presence-service sync reconciliation and config to use it, removes stale stub methods, updates mocks/tests, and revises the design spec accordingly. ChangesBatched directory lookup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant Cache as Valkey Cache
participant DirectoryReader
participant GraphAPI
Reconciler->>Cache: lookup presence:idmap:azure for active accounts
Cache-->>Reconciler: cached IDs, missing accounts
Reconciler->>DirectoryReader: GetUsersByAccounts(missing accounts)
DirectoryReader->>DirectoryReader: chunk accounts, build startsWith filters
DirectoryReader->>GraphAPI: GET /users?$filter=...&ConsistencyLevel=eventual
GraphAPI-->>DirectoryReader: value[] GraphUser
DirectoryReader-->>Reconciler: []GraphUser
Reconciler->>Reconciler: match by localPart (case-insensitive, first match wins)
Reconciler->>Cache: persist resolved account -> azureObjectID
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Summary
Follow-up to #378, addressing review feedback on how the Teams presence sync resolves accounts to Azure object IDs. Rebased on latest
main; two logically-distinct commits.1.
feat(msgraph): batch account->id resolution via startsWith advanced queryReplaces the per-account
GetUserByPrincipalName(account@domain)lookup with a batched, domain-agnosticGetUsersByAccounts:startsWith(userPrincipalName,'account@')across the cache-missing accounts into chunked$filterqueries (ConsistencyLevel: eventual+$countfor the advanced query).startsWithclause rather than relying on Graph case-insensitivity (accounts/query halved to stay within the clause budget).TEAMS_EMAIL_DOMAIN(accounts may live under different domains), avoids per-account round-trips, and never enumerates the whole tenant.2.
refactor(msgraph): move user lookup off Client onto DirectoryReaderroom-servicedepends onmsgraph.Clientonly forCreateOnlineMeeting, but the user-lookup method sat on that same interface — forcing room-service's test fakes to change whenever it changed. MovedGetUsersByAccountsonto its ownDirectoryReaderinterface +NewDirectoryClientconstructor (mirroring the existingPresenceReader), soClientis meetings-only again and room-service no longer depends on the user-lookup surface. No behavior change.Why
startsWithneeds the advanced-query headers, and Graph caps$filtercomplexity — hence chunking (≤15 clauses/query).presence:idmap:azurecache means steady-state runs make ~zero Graph calls; only genuinely new accounts are looked up.Testing
make lint— 0 issuesmake sast— gosec / govulncheck / semgrep all PASSpkg/msgraph(batch/chunk/cased-variants/empty),room-service,user-presence-service,.../sync-tags integration(run on CI)Each commit builds and passes tests independently.
🤖 Generated with Claude Code
https://claude.ai/code/session_011xT3cu9rqxJyNUzYsAv6sL
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes