Skip to content

fix: replace O(teams x members) REST scan in GetUserTeams with GraphQL - #2718

Closed
sav-hostaway wants to merge 1 commit into
diggerhq:developfrom
sav-hostaway:fix/get-user-teams-graphql
Closed

sav-hostaway wants to merge 1 commit into
diggerhq:developfrom
sav-hostaway:fix/get-user-teams-graphql

Conversation

@sav-hostaway

Copy link
Copy Markdown

GetUserTeams answers "which teams is this user in" by listing every team in the organisation and then paging through every team's members looking for that one login. That is one request per team, plus one per page of each team's members.

populatePolicyFieldsForJobs calls it once for the requester and once per PR approver, so the cost lands directly on the latency between a digger plan / digger apply comment and the job actually being dispatched.

On a 47-team organisation, measured:

API requests wall clock
current REST scan 48 21.3s
this change 2 1.2s

Both return the same team names. In production backend logs this showed up as a 9-27s gap (median 19s, n=19) between Successfully set PR status and Computed policy fields for jobs, bimodal depending on whether the PR had an approver, and uncorrelated with the number of projects in the batch.

Why two queries and not one

The obvious fix is a single teams(userLogins: [$login]) query. That returns the wrong answer.

userLogins matches direct membership only, while the REST /orgs/{org}/teams/{slug}/members endpoint also lists members inherited from child teams. On an organisation where team-devops has parent engineering:

REST scan (current):  ['engineering', 'team-devops']
GraphQL (naive):      ['team-devops']

So a naive swap silently drops parent teams, and any access-policy naming a parent team quietly stops matching. That is the same class of bug #2545 was fixing, in the opposite direction.

This change therefore runs a second query for the organisation's team-to-parent map and walks the chain upward from each direct team, which reproduces the REST semantics exactly. The walk carries a seen set so a cycle cannot hang the request.

Notes

  • No new dependencies. The GraphQL request goes through the existing go-github client via Client.NewRequest / Client.Do.
  • Signature and return semantics are unchanged, so callers and the ci.PullRequestService implementations for GitLab, Azure and Bitbucket are untouched.
  • GraphQL reports failures in the response body alongside HTTP 200, so the body's errors array is checked explicitly.
  • Results are sorted for deterministic output.
  • The second query fetches all organisation teams on each call. For very large organisations, caching that parent map per organisation would be a reasonable follow-up; it is left out here to keep the change reviewable.

Testing

  • go build ./... and go vet ./ci/github/ clean, gofmt clean
  • go test ./ci/... passes, other than the pre-existing TestListRepositoriesReturnsAllReposities failure which needs GITHUB_APP_ID set and fails identically on an unpatched tree
  • Query semantics verified against a real 47-team organisation, confirming the output matches the current REST scan exactly, including the inherited parent team

🤖 Generated with Claude Code

GetUserTeams listed every team in the organisation and then paged every team's
members looking for one user. On a 47-team organisation that is ~48 API requests
per call, and populatePolicyFieldsForJobs calls it once for the requester plus
once per PR approver, so a digger comment waits 9-27s before its job is
dispatched.

Two GraphQL queries replace the scan: one filters teams by userLogins, the other
builds the team-to-parent map. The parent walk is needed for equivalence, not
just completeness: userLogins matches direct membership only, while the REST
members endpoint also lists members inherited from child teams, so a naive swap
drops parent teams and silently narrows any policy that names one.

Measured against a 47-team organisation:
  before: 48 API requests, 21.3s
  after:   2 API requests,  1.2s
Both return the same team names.

🤖 Generated with Claude Code

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant