Use slug-based APIs for team operations (#212)

This reduces the total number of requests by eliminating calls to
enumerate all teams, which were previously required to discover all team
IDs and map them to names.

These APIs are only available on GitHub.com and in GitHub Enterprise
2.21 or newer, so policy-bot is no longer compatible with older versions
of GitHub Enterprise.

go.mod

@@ -10,7 +10,6 @@ require (
 	github.com/c2h5oh/datasize v0.0.0-20171227191756-4eba002a5eae
 	github.com/die-net/lrucache v0.0.0-20181227122439-19a39ef22a11
 	github.com/google/go-github/v32 v32.0.0
-	github.com/google/go-querystring v1.0.0
 	github.com/gregjones/httpcache v0.0.0-20190611155906-901d90724c79
 	github.com/inconshreveable/mousetrap v1.0.0 // indirect
 	github.com/palantir/go-baseapp v0.2.1

pull/github_membership.go

@@ -26,7 +26,6 @@ type GitHubMembershipContext struct {
 	ctx    context.Context
 	client *github.Client
 
-	teamIDs     map[string]int64
 	membership  map[string]bool
 	orgMembers  map[string][]string
 	teamMembers map[string][]string
@@ -36,7 +35,6 @@ func NewGitHubMembershipContext(ctx context.Context, client *github.Client) *Git
 	return &GitHubMembershipContext{
 		ctx:         ctx,
 		client:      client,
-		teamIDs:     make(map[string]int64),
 		membership:  make(map[string]bool),
 		orgMembers:  make(map[string][]string),
 		teamMembers: make(map[string][]string),
@@ -47,68 +45,36 @@ func membershipKey(group, user string) string {
 	return group + ":" + user
 }
 
+func splitTeam(team string) (org, slug string, err error) {
+	parts := strings.Split(team, "/")
+	if len(parts) != 2 {
+		return "", "", errors.Errorf("invalid team format: %s", team)
+	}
+	return parts[0], parts[1], nil
+}
+
 func (mc *GitHubMembershipContext) IsTeamMember(team, user string) (bool, error) {
 	key := membershipKey(team, user)
 
-	id, err := mc.getTeamID(team)
+	org, slug, err := splitTeam(team)
 	if err != nil {
-		return false, errors.Errorf("failed to get ID for team %s", team)
+		return false, err
 	}
 
 	isMember, ok := mc.membership[key]
 	if ok {
 		return isMember, nil
 	}
 
-	membership, _, err := GetTeamMembership(mc.ctx, mc.client, id, user)
+	membership, _, err := mc.client.Teams.GetTeamMembershipBySlug(mc.ctx, org, slug, user)
 	if err != nil && !isNotFound(err) {
 		return false, errors.Wrap(err, "failed to get team membership")
 	}
 
 	isMember = membership != nil && membership.GetState() == "active"
-
 	mc.membership[key] = isMember
-	return isMember, nil
-}
 
-func (mc *GitHubMembershipContext) getTeamID(team string) (int64, error) {
-	org := strings.Split(team, "/")[0]
-	id, ok := mc.teamIDs[team]
-	if !ok {
-		if err := mc.cacheTeamIDs(org); err != nil {
-			return 0, err
-		}
-
-		id, ok = mc.teamIDs[team]
-		if !ok {
-			return 0, errors.Errorf("failed to get ID for team %s", team)
-		}
-	}
-	return id, nil
-}
-
-func (mc *GitHubMembershipContext) cacheTeamIDs(org string) error {
-	opt := github.ListOptions{
-		PerPage: 100,
-	}
-
-	for {
-		teams, res, err := mc.client.Teams.ListTeams(mc.ctx, org, &opt)
-		if err != nil {
-			return errors.Wrap(err, "failed to list organization teams")
-		}
-
-		for _, t := range teams {
-			key := org + "/" + t.GetSlug()
-			mc.teamIDs[key] = t.GetID()
-		}
-
-		if res.NextPage == 0 {
-			break
-		}
-		opt.Page = res.NextPage
-	}
-	return nil
+	return isMember, nil
 }
 
 func (mc *GitHubMembershipContext) IsOrgMember(org, user string) (bool, error) {
@@ -175,13 +141,13 @@ func (mc *GitHubMembershipContext) TeamMembers(team string) ([]string, error) {
 			},
 		}
 
-		teamID, err := mc.getTeamID(team)
+		org, slug, err := splitTeam(team)
 		if err != nil {
-			return nil, errors.Wrapf(err, "Unable to get information for team %s", team)
+			return nil, err
 		}
 
 		for {
-			users, resp, err := ListTeamMembers(mc.ctx, mc.client, teamID, opt)
+			users, resp, err := mc.client.Teams.ListTeamMembersBySlug(mc.ctx, org, slug, opt)
 			if err != nil {
 				return nil, errors.Wrapf(err, "failed to list team %s members page %d", team, opt.Page)
 			}

pull/github_test.go

@@ -276,24 +276,20 @@ func TestNoComments(t *testing.T) {
 
 func TestIsTeamMember(t *testing.T) {
 	rp := &ResponsePlayer{}
-	teamsRule := rp.AddRule(
-		ExactPathMatcher("/orgs/testorg/teams"),
-		"testdata/responses/teams_testorg.yml",
-	)
 	yesRule1 := rp.AddRule(
-		ExactPathMatcher("/teams/123/memberships/mhaypenny"),
+		ExactPathMatcher("/orgs/testorg/teams/yes-team/memberships/mhaypenny"),
 		"testdata/responses/membership_team123_mhaypenny.yml",
 	)
 	yesRule2 := rp.AddRule(
-		ExactPathMatcher("/teams/123/memberships/ttest"),
+		ExactPathMatcher("/orgs/testorg/teams/yes-team/memberships/ttest"),
 		"testdata/responses/membership_team123_ttest.yml",
 	)
 	noRule1 := rp.AddRule(
-		ExactPathMatcher("/teams/456/memberships/mhaypenny"),
+		ExactPathMatcher("/orgs/testorg/teams/no-team/memberships/mhaypenny"),
 		"testdata/responses/membership_team456_mhaypenny.yml",
 	)
 	noRule2 := rp.AddRule(
-		ExactPathMatcher("/teams/456/memberships/ttest"),
+		ExactPathMatcher("/orgs/testorg/teams/no-team/memberships/ttest"),
 		"testdata/responses/membership_team456_ttest.yml",
 	)
 
@@ -303,38 +299,33 @@ func TestIsTeamMember(t *testing.T) {
 	require.NoError(t, err)
 
 	assert.True(t, isMember, "user is not a member")
-	assert.Equal(t, 2, teamsRule.Count, "no http request was made for teams")
 	assert.Equal(t, 1, yesRule1.Count, "no http request was made")
 
 	isMember, err = ctx.IsTeamMember("testorg/yes-team", "ttest")
 	require.NoError(t, err)
 
 	assert.True(t, isMember, "user is not a member")
-	assert.Equal(t, 2, teamsRule.Count, "cached team IDs were not used")
 	assert.Equal(t, 1, yesRule2.Count, "no http request was made")
 
 	// not a member because missing from team
 	isMember, err = ctx.IsTeamMember("testorg/no-team", "mhaypenny")
 	require.NoError(t, err)
 
 	assert.False(t, isMember, "user is a member")
-	assert.Equal(t, 2, teamsRule.Count, "cached team IDs were not used")
 	assert.Equal(t, 1, noRule1.Count, "no http request was made")
 
 	// not a member because membership state is pending
 	isMember, err = ctx.IsTeamMember("testorg/no-team", "ttest")
 	require.NoError(t, err)
 
 	assert.False(t, isMember, "user is a member")
-	assert.Equal(t, 2, teamsRule.Count, "cached team IDs were not used")
 	assert.Equal(t, 1, noRule2.Count, "no http request was made")
 
 	// verify that team membership is cached
 	isMember, err = ctx.IsTeamMember("testorg/yes-team", "mhaypenny")
 	require.NoError(t, err)
 
 	assert.True(t, isMember, "user is not a member")
-	assert.Equal(t, 2, teamsRule.Count, "cached team IDs were not used")
 	assert.Equal(t, 1, yesRule1.Count, "cached membership was not used")
 }