Skip to content

Commit 92b37cf

Browse files
theakshaypanttheakshaypant
committed
perf(github): cache getPullRequest result in Provider
Cache the GitHub API response for PR lookups to avoid repeated calls within the same event lifecycle. ACL checks now read from already-populated runevent fields instead of fetching independently. Fixes #2378 Signed-off-by: Akshay Pant <akpant@redhat.com>
1 parent b8ed140 commit 92b37cf

File tree

4 files changed

+188
-83
lines changed

4 files changed

+188
-83
lines changed

‎pkg/provider/github/acl.go‎

Lines changed: 11 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -209,11 +209,8 @@ func (v *Provider) aclCheckAll(ctx context.Context, rev *info.Event) (bool, erro
209209
// This can only happen with GithubApp and Bots.
210210
// Ex: dependabot, bots
211211
if rev.PullRequestNumber != 0 {
212-
isSameCloneURL, err := v.checkPullRequestForSameURL(ctx, rev)
213-
if err != nil {
214-
return false, err
215-
}
216-
if isSameCloneURL {
212+
isFromSameRepo := v.checkPullRequestForSameURL(ctx, rev)
213+
if isFromSameRepo {
217214
return true, nil
218215
}
219216
}
@@ -241,26 +238,18 @@ func (v *Provider) aclCheckAll(ctx context.Context, rev *info.Event) (bool, erro
241238
return v.IsAllowedOwnersFile(ctx, rev)
242239
}
243240

244-
// checkPullRequestForSameURL checks If PullRequests are for same clone URL and different branches
245-
// means if the user has access to create a branch in the repository without forking or having any permissions then PAC should allow to run CI.
241+
// checkPullRequestForSameURL checks if a pull request's head and base branches are from the same repository.
242+
// means if the user has access to create a branch in the repository without forking or having any
243+
// permissions then PAC should allow to run CI.
246244
//
247245
// ex: dependabot, *[bot] etc...
248-
func (v *Provider) checkPullRequestForSameURL(ctx context.Context, runevent *info.Event) (bool, error) {
249-
pr, resp, err := wrapAPI(v, "get_pull_request", func() (*github.PullRequest, *github.Response, error) {
250-
return v.Client().PullRequests.Get(ctx, runevent.Organization, runevent.Repository, runevent.PullRequestNumber)
251-
})
252-
if err != nil {
253-
return false, err
254-
}
255-
if resp != nil && resp.StatusCode == http.StatusNotFound {
256-
return false, nil
257-
}
258-
259-
if pr.GetHead().GetRepo().GetCloneURL() == pr.GetBase().GetRepo().GetCloneURL() && pr.GetHead().GetRef() != pr.GetBase().GetRef() {
260-
return true, nil
246+
//
247+
// HeadURL is set by getPullRequest() before aclCheckAll; if missing, fall through to other ACL checks.
248+
func (v *Provider) checkPullRequestForSameURL(_ context.Context, runevent *info.Event) bool {
249+
if runevent.HeadURL == "" {
250+
return false
261251
}
262-
263-
return false, nil
252+
return runevent.HeadURL == runevent.BaseURL && runevent.HeadBranch != runevent.BaseBranch
264253
}
265254

266255
// checkSenderOrgMembership Get sender user's organization. We can

‎pkg/provider/github/acl_test.go‎

Lines changed: 12 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package github
22

33
import (
44
"encoding/base64"
5-
"encoding/json"
65
"fmt"
76
"net/http"
87
"testing"
@@ -690,12 +689,10 @@ func TestAclCheckAll(t *testing.T) {
690689
}
691690

692691
func TestIfPullRequestIsForSameRepoWithoutFork(t *testing.T) {
693-
iddd := int64(1234)
694692
tests := []struct {
695693
name string
696694
event *info.Event
697695
commitFiles []*github.CommitFile
698-
pullRequest *github.PullRequest
699696
pullRequestNumber int
700697
allowed bool
701698
wantError bool
@@ -707,76 +704,36 @@ func TestIfPullRequestIsForSameRepoWithoutFork(t *testing.T) {
707704
Sender: "nonowner",
708705
Repository: "repo",
709706
PullRequestNumber: 1,
710-
},
711-
pullRequest: &github.PullRequest{
712-
ID: &iddd,
713-
Number: github.Ptr(1),
714-
Head: &github.PullRequestBranch{
715-
Ref: github.Ptr("main"),
716-
Repo: &github.Repository{
717-
CloneURL: github.Ptr("http://org.com/owner/repo"),
718-
},
719-
},
720-
Base: &github.PullRequestBranch{
721-
Ref: github.Ptr("dependabot"),
722-
Repo: &github.Repository{
723-
CloneURL: github.Ptr("http://org.com/owner/repo"),
724-
},
725-
},
707+
HeadURL: "http://org.com/owner/repo",
708+
BaseURL: "http://org.com/owner/repo",
709+
HeadBranch: "main",
710+
BaseBranch: "dependabot",
726711
},
727712
pullRequestNumber: 1,
728713
allowed: true,
729714
wantError: false,
730715
}, {
731-
name: "failed to get Pull Request",
716+
name: "when HeadURL is not populated on the event",
732717
event: &info.Event{
733718
Organization: "owner",
734719
Sender: "nonowner",
735720
Repository: "repo",
736-
PullRequestNumber: 2,
737-
},
738-
pullRequest: &github.PullRequest{
739-
ID: &iddd,
740-
Number: github.Ptr(1),
741-
Head: &github.PullRequestBranch{
742-
Ref: github.Ptr("main"),
743-
Repo: &github.Repository{
744-
CloneURL: github.Ptr("http://org.com/owner/repo"),
745-
},
746-
},
747-
Base: &github.PullRequestBranch{
748-
Ref: github.Ptr("dependabot"),
749-
Repo: &github.Repository{
750-
CloneURL: github.Ptr("http://org.com/owner/repo"),
751-
},
752-
},
721+
PullRequestNumber: 1,
753722
},
754723
pullRequestNumber: 1,
755724
allowed: false,
756-
wantError: true,
725+
wantError: false,
757726
}, {
758727
name: "when pull request raised by non owner to the repository where non owner don't have any permissions",
759728
event: &info.Event{
760729
Organization: "owner",
761730
Sender: "nonowner",
762731
Repository: "repo",
763732
PullRequestNumber: 1,
764-
},
765-
pullRequest: &github.PullRequest{
766-
ID: &iddd,
767-
Number: github.Ptr(1),
768-
Head: &github.PullRequestBranch{
769-
Ref: github.Ptr("main"),
770-
Repo: &github.Repository{
771-
CloneURL: github.Ptr("http://org.com/owner/repo"),
772-
},
773-
},
774-
Base: &github.PullRequestBranch{
775-
Ref: github.Ptr("dependabot"),
776-
Repo: &github.Repository{
777-
CloneURL: github.Ptr("http://org.com/owner/repo1"),
778-
},
779-
},
733+
HeadURL: "http://org.com/owner/repo",
734+
BaseURL: "http://org.com/owner/repo1",
735+
HeadBranch: "main",
736+
BaseBranch: "dependabot",
780737
},
781738
pullRequestNumber: 1,
782739
allowed: false,
@@ -785,14 +742,9 @@ func TestIfPullRequestIsForSameRepoWithoutFork(t *testing.T) {
785742
}
786743
for _, tt := range tests {
787744
t.Run(tt.name, func(t *testing.T) {
788-
fakeclient, mux, _, teardown := ghtesthelper.SetupGH()
745+
fakeclient, _, _, teardown := ghtesthelper.SetupGH()
789746
defer teardown()
790747
ctx, _ := rtesting.SetupFakeContext(t)
791-
mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/pulls/%d",
792-
tt.event.Organization, tt.event.Repository, tt.pullRequestNumber), func(rw http.ResponseWriter, _ *http.Request) {
793-
b, _ := json.Marshal(tt.pullRequest)
794-
fmt.Fprint(rw, string(b))
795-
})
796748

797749
repo := &v1alpha1.Repository{Spec: v1alpha1.RepositorySpec{
798750
Settings: &v1alpha1.Settings{},

‎pkg/provider/github/github.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ type Provider struct {
6363
triggerEvent string
6464
cachedChangedFiles *changedfiles.ChangedFiles
6565
commitInfo *github.Commit
66+
cachedPullRequest *github.PullRequest
6667
pacUserLogin string // user/bot login used by PAC
6768
clock clockwork.Clock
6869
graphQLClient *graphQLClient
@@ -547,14 +548,19 @@ func (v *Provider) concatAllYamlFiles(ctx context.Context, objects []*github.Tre
547548
return buf.String(), nil
548549
}
549550

550-
// getPullRequest get a pull request details.
551+
// getPullRequest get a pull request details, caching the result for the lifetime of the event.
551552
func (v *Provider) getPullRequest(ctx context.Context, runevent *info.Event) (*info.Event, error) {
553+
if v.cachedPullRequest != nil {
554+
return runevent, nil
555+
}
552556
pr, _, err := wrapAPI(v, "get_pull_request", func() (*github.PullRequest, *github.Response, error) {
553557
return v.Client().PullRequests.Get(ctx, runevent.Organization, runevent.Repository, runevent.PullRequestNumber)
554558
})
555559
if err != nil {
556560
return runevent, err
557561
}
562+
v.cachedPullRequest = pr
563+
558564
// Make sure to use the Base for Default BaseBranch or there would be a potential hijack
559565
runevent.DefaultBranch = pr.GetBase().GetRepo().GetDefaultBranch()
560566
runevent.URL = pr.GetBase().GetRepo().GetHTMLURL()

‎pkg/provider/github/github_test.go‎

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1646,6 +1646,164 @@ func TestCreateToken(t *testing.T) {
16461646
}
16471647
}
16481648

1649+
func TestGetPullRequest(t *testing.T) {
1650+
const (
1651+
org = "owner"
1652+
repo = "repo"
1653+
prNum = 42
1654+
)
1655+
prJSON := `{
1656+
"number": 42,
1657+
"title": "very cool feature pr",
1658+
"html_url": "https://github.com/owner/repo/pull/42",
1659+
"user": {"login": "author"},
1660+
"head": {
1661+
"sha": "headsha123",
1662+
"ref": "feature-branch",
1663+
"repo": {"html_url": "https://github.com/author/repo"}
1664+
},
1665+
"base": {
1666+
"ref": "main",
1667+
"repo": {
1668+
"html_url": "https://github.com/owner/repo",
1669+
"default_branch": "main",
1670+
"id": 9876
1671+
}
1672+
},
1673+
"labels": [{"name": "bug"}, {"name": "enhancement"}]
1674+
}`
1675+
1676+
tests := []struct {
1677+
name string
1678+
event *info.Event
1679+
apiReturn string
1680+
wantErr bool
1681+
wantErrStr string
1682+
wantSHA string
1683+
wantSHAURL string
1684+
wantURL string
1685+
wantDefaultBranch string
1686+
wantHeadBranch string
1687+
wantBaseBranch string
1688+
wantHeadURL string
1689+
wantBaseURL string
1690+
wantTitle string
1691+
wantSender string
1692+
wantEventType string
1693+
wantLabels []string
1694+
wantRepoID int64
1695+
}{
1696+
{
1697+
name: "populates all event fields from PR response",
1698+
event: &info.Event{
1699+
Organization: org,
1700+
Repository: repo,
1701+
PullRequestNumber: prNum,
1702+
},
1703+
apiReturn: prJSON,
1704+
wantSHA: "headsha123",
1705+
wantSHAURL: "https://github.com/owner/repo/pull/42/commit/headsha123",
1706+
wantURL: "https://github.com/owner/repo",
1707+
wantDefaultBranch: "main",
1708+
wantHeadBranch: "feature-branch",
1709+
wantBaseBranch: "main",
1710+
wantHeadURL: "https://github.com/author/repo",
1711+
wantBaseURL: "https://github.com/owner/repo",
1712+
wantTitle: "very cool feature pr",
1713+
wantSender: "author",
1714+
wantEventType: triggertype.PullRequest.String(),
1715+
wantLabels: []string{"bug", "enhancement"},
1716+
wantRepoID: 9876,
1717+
},
1718+
{
1719+
name: "returns error on API failure",
1720+
event: &info.Event{
1721+
Organization: org,
1722+
Repository: repo,
1723+
PullRequestNumber: prNum,
1724+
},
1725+
wantErr: true,
1726+
wantErrStr: "404",
1727+
},
1728+
}
1729+
1730+
for _, tt := range tests {
1731+
t.Run(tt.name, func(t *testing.T) {
1732+
ctx, _ := rtesting.SetupFakeContext(t)
1733+
fakeclient, mux, _, teardown := ghtesthelper.SetupGH()
1734+
defer teardown()
1735+
1736+
if tt.apiReturn != "" {
1737+
mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/pulls/%d", org, repo, prNum),
1738+
func(rw http.ResponseWriter, _ *http.Request) {
1739+
fmt.Fprint(rw, tt.apiReturn)
1740+
})
1741+
}
1742+
1743+
provider := &Provider{ghClient: fakeclient}
1744+
got, err := provider.getPullRequest(ctx, tt.event)
1745+
if tt.wantErr {
1746+
assert.Assert(t, err != nil)
1747+
if tt.wantErrStr != "" {
1748+
assert.ErrorContains(t, err, tt.wantErrStr)
1749+
}
1750+
return
1751+
}
1752+
assert.NilError(t, err)
1753+
1754+
assert.Equal(t, tt.wantSHA, got.SHA)
1755+
assert.Equal(t, tt.wantSHAURL, got.SHAURL)
1756+
assert.Equal(t, tt.wantURL, got.URL)
1757+
assert.Equal(t, tt.wantDefaultBranch, got.DefaultBranch)
1758+
assert.Equal(t, tt.wantHeadBranch, got.HeadBranch)
1759+
assert.Equal(t, tt.wantBaseBranch, got.BaseBranch)
1760+
assert.Equal(t, tt.wantHeadURL, got.HeadURL)
1761+
assert.Equal(t, tt.wantBaseURL, got.BaseURL)
1762+
assert.Equal(t, tt.wantTitle, got.PullRequestTitle)
1763+
assert.Equal(t, tt.wantSender, got.Sender)
1764+
assert.Equal(t, tt.wantEventType, got.EventType)
1765+
assert.DeepEqual(t, tt.wantLabels, got.PullRequestLabel)
1766+
assert.Equal(t, 1, len(provider.RepositoryIDs))
1767+
assert.Equal(t, tt.wantRepoID, provider.RepositoryIDs[0])
1768+
})
1769+
}
1770+
}
1771+
1772+
func TestGetPullRequestCaching(t *testing.T) {
1773+
ctx, _ := rtesting.SetupFakeContext(t)
1774+
fakeclient, mux, _, teardown := ghtesthelper.SetupGH()
1775+
defer teardown()
1776+
1777+
callCount := 0
1778+
mux.HandleFunc("/repos/owner/repo/pulls/1", func(rw http.ResponseWriter, _ *http.Request) {
1779+
callCount++
1780+
fmt.Fprint(rw, `{
1781+
"number": 1,
1782+
"title": "cached pr",
1783+
"html_url": "https://github.com/owner/repo/pull/1",
1784+
"user": {"login": "author"},
1785+
"head": {"sha": "abc", "ref": "head", "repo": {"html_url": "https://github.com/author/repo"}},
1786+
"base": {"ref": "main", "repo": {"html_url": "https://github.com/owner/repo", "default_branch": "main", "id": 1}},
1787+
"labels": [{"name": "bug"}, {"name": "enhancement"}]
1788+
}`)
1789+
})
1790+
1791+
event := &info.Event{
1792+
Organization: "owner",
1793+
Repository: "repo",
1794+
PullRequestNumber: 1,
1795+
}
1796+
provider := &Provider{ghClient: fakeclient}
1797+
1798+
_, err := provider.getPullRequest(ctx, event)
1799+
assert.NilError(t, err)
1800+
assert.Equal(t, 1, callCount, "expected exactly one API call on first invocation")
1801+
1802+
_, err = provider.getPullRequest(ctx, event)
1803+
assert.NilError(t, err)
1804+
assert.Equal(t, 1, callCount, "expected no additional API call on second invocation (cache hit)")
1805+
}
1806+
16491807
func TestIsHeadCommitOfBranch(t *testing.T) {
16501808
tests := []struct {
16511809
name string

0 commit comments

Comments
 (0)
⚡