diff --git a/pkg/github/codescanning.go b/pkg/github/codescanning.go index 05666286..a5ed1a1a 100644 --- a/pkg/github/codescanning.go +++ b/pkg/github/codescanning.go @@ -156,32 +156,42 @@ func (alerts CodeScanningWrapper) Frames() data.Frames { // https://docs.github.com/en/rest/reference/code-scanning#get-a-list-of-code-scanning-alerts-for-a-repository func GetCodeScanningAlerts(context context.Context, c models.Client, opt models.CodeScanningOptions, from time.Time, to time.Time) (CodeScanningWrapper, error) { var alerts []*googlegithub.Alert - var err error - // if there is no repository provided show alerts in organization level - if opt.Repository == "" { - alerts, _, err = c.ListAlertsForOrg( - context, - opt.Owner, - &googlegithub.AlertListOptions{ - State: opt.State, - Ref: opt.Ref, - }, - ) - } else { - alerts, _, err = c.ListAlertsForRepo( - context, - opt.Owner, - opt.Repository, - &googlegithub.AlertListOptions{ - State: opt.State, - Ref: opt.Ref, - }, - ) + listOpts := &googlegithub.AlertListOptions{ + State: opt.State, + Ref: opt.Ref, } + // Use offset pagination with a large page size. ListOptions is embedded + // explicitly (AlertListOptions also embeds ListCursorOptions) so Page/PerPage + // are unambiguous. + listOpts.ListOptions.PerPage = 100 + + page := 1 + for page != 0 { + listOpts.ListOptions.Page = page + + var ( + pageAlerts []*googlegithub.Alert + resp *googlegithub.Response + err error + ) + + // if there is no repository provided show alerts in organization level + if opt.Repository == "" { + pageAlerts, resp, err = c.ListAlertsForOrg(context, opt.Owner, listOpts) + } else { + pageAlerts, resp, err = c.ListAlertsForRepo(context, opt.Owner, opt.Repository, listOpts) + } + if err != nil { + return nil, err + } + + alerts = append(alerts, pageAlerts...) - if err != nil { - return nil, err + if resp == nil || resp.NextPage == 0 { + break + } + page = resp.NextPage } return CodeScanningWrapper(alerts), nil diff --git a/pkg/github/codescanning_test.go b/pkg/github/codescanning_test.go index c20f8188..30dccea6 100644 --- a/pkg/github/codescanning_test.go +++ b/pkg/github/codescanning_test.go @@ -11,12 +11,35 @@ import ( "github.com/grafana/github-datasource/pkg/models" ) +type mockAlertPage struct { + alerts []*googlegithub.Alert + nextPage int +} + type mockClient struct { - mockAlerts []*googlegithub.Alert - mockResponse *googlegithub.Response - expectedOwner string - expectedRepo string - t *testing.T + mockAlerts []*googlegithub.Alert + mockResponse *googlegithub.Response + // pages, when set, makes successive ListAlertsFor* calls return successive + // pages so pagination can be exercised. requestedPages records the Page value + // requested on each call. + pages []mockAlertPage + callCount int + requestedPages []int + expectedOwner string + expectedRepo string + t *testing.T +} + +// nextAlertPage returns the alerts and response for the current call when +// pagination is being simulated, or nil to fall back to mockAlerts/mockResponse. +func (m *mockClient) nextAlertPage(opts *googlegithub.AlertListOptions) ([]*googlegithub.Alert, *googlegithub.Response, bool) { + if len(m.pages) == 0 { + return nil, nil, false + } + m.requestedPages = append(m.requestedPages, opts.ListOptions.Page) + page := m.pages[m.callCount] + m.callCount++ + return page.alerts, &googlegithub.Response{NextPage: page.nextPage}, true } func (m *mockClient) Query(ctx context.Context, q interface{}, variables map[string]interface{}) error { @@ -40,6 +63,9 @@ func (m *mockClient) ListAlertsForRepo(ctx context.Context, owner, repo string, m.t.Errorf("Expected owner/repo to be %s/%s, got %s/%s", m.expectedOwner, m.expectedRepo, owner, repo) } + if alerts, resp, ok := m.nextAlertPage(opts); ok { + return alerts, resp, nil + } return m.mockAlerts, m.mockResponse, nil } @@ -49,6 +75,9 @@ func (m *mockClient) ListAlertsForOrg(ctx context.Context, owner string, opts *g m.t.Errorf("Expected owner to be %s, got %s", m.expectedOwner, owner) } + if alerts, resp, ok := m.nextAlertPage(opts); ok { + return alerts, resp, nil + } return m.mockAlerts, m.mockResponse, nil } @@ -193,3 +222,78 @@ func TestCodeScanningWrapperFrames(t *testing.T) { t.Errorf("Expected %d fields, got %d", expectedFields, len(frame.Fields)) } } + +// helper to build n alerts with distinct numbers +func makeAlerts(n int) []*googlegithub.Alert { + alerts := make([]*googlegithub.Alert, n) + for i := range alerts { + num := i + 1 + alerts[i] = &googlegithub.Alert{Number: &num} + } + return alerts +} + +// Regression test for https://github.com/grafana/github-datasource/issues/773: +// Code Scanning alerts must be paginated, not capped at the first page. +func TestGetCodeScanningAlertsPagination(t *testing.T) { + ctx := context.Background() + from := time.Now().Add(-30 * 24 * time.Hour) + to := time.Now() + + t.Run("repository walks all pages", func(t *testing.T) { + client := &mockClient{ + expectedOwner: "grafana", + expectedRepo: "grafana", + t: t, + pages: []mockAlertPage{ + {alerts: makeAlerts(3), nextPage: 2}, + {alerts: makeAlerts(3), nextPage: 3}, + {alerts: makeAlerts(2), nextPage: 0}, + }, + } + opts := models.CodeScanningOptions{Owner: "grafana", Repository: "grafana"} + + alerts, err := GetCodeScanningAlerts(ctx, client, opts, from, to) + if err != nil { + t.Fatal(err) + } + if len(alerts) != 8 { + t.Errorf("expected 8 alerts across 3 pages, got %d", len(alerts)) + } + if want := []int{1, 2, 3}; !equalInts(client.requestedPages, want) { + t.Errorf("expected requested pages %v, got %v", want, client.requestedPages) + } + }) + + t.Run("organization walks all pages", func(t *testing.T) { + client := &mockClient{ + expectedOwner: "grafana", + t: t, + pages: []mockAlertPage{ + {alerts: makeAlerts(3), nextPage: 2}, + {alerts: makeAlerts(1), nextPage: 0}, + }, + } + opts := models.CodeScanningOptions{Owner: "grafana"} // no repository -> org + + alerts, err := GetCodeScanningAlerts(ctx, client, opts, from, to) + if err != nil { + t.Fatal(err) + } + if len(alerts) != 4 { + t.Errorf("expected 4 alerts across 2 pages, got %d", len(alerts)) + } + }) +} + +func equalInts(a, b []int) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +}