diff --git a/internal/bitbucket/bitbucket.go b/internal/bitbucket/bitbucket.go index bb28c847e..d412476e5 100644 --- a/internal/bitbucket/bitbucket.go +++ b/internal/bitbucket/bitbucket.go @@ -13,6 +13,10 @@ import ( "github.com/kosli-dev/cli/internal/types" ) +// defaultMaxPages bounds commit paging so next links that cycle end in an +// error. It is set far above the pages a real pull request needs. +const defaultMaxPages = 1000 + type Config struct { Username string Password string @@ -26,7 +30,11 @@ type Config struct { // parseRFC3339NanoTimestamp parses a timestamp string in RFC3339Nano format and returns its Unix timestamp. // The fieldName parameter is used for error messages to identify which field failed to parse. +// An empty string gives 0: the field was missing. func parseRFC3339NanoTimestamp(timestampStr, fieldName string) (int64, error) { + if timestampStr == "" { + return 0, nil + } parsedTime, err := time.Parse(time.RFC3339Nano, timestampStr) if err != nil { return 0, fmt.Errorf("failed to parse %s timestamp: %w", fieldName, err) @@ -34,6 +42,39 @@ func parseRFC3339NanoTimestamp(timestampStr, fieldName string) (int64, error) { return parsedTime.Unix(), nil } +// str, obj and boolean read a field from a decoded JSON object, giving the +// zero value when it is missing, null or of another type. +func str(m map[string]any, key string) string { + v, _ := m[key].(string) + return v +} + +func obj(m map[string]any, key string) map[string]any { + v, _ := m[key].(map[string]any) + return v +} + +func boolean(m map[string]any, key string) bool { + v, _ := m[key].(bool) + return v +} + +// objects reads a list of objects. A missing list is empty, but an item that +// is not an object is an error: it is an entry we can't read. owner says what +// the list belongs to. +func objects(m map[string]any, key, owner string) ([]map[string]any, error) { + items, _ := m[key].([]any) + out := make([]map[string]any, len(items)) + for i, item := range items { + o, ok := item.(map[string]any) + if !ok { + return nil, fmt.Errorf("%s[%d] for %s in the Bitbucket reply is null or not an object", key, i, owner) + } + out[i] = o + } + return out, nil +} + func (c *Config) ProviderAndLabel() (string, string) { return "bitbucket", "pull request" } @@ -97,16 +138,17 @@ func (c *Config) parseBitbucketResponse(commit string, response *requests.HTTPRe if err != nil { return pullRequestsEvidence, err } - pullRequests, ok := responseData["values"].([]any) - if !ok { - return pullRequestsEvidence, nil + pullRequests, err := objects(responseData, "values", "commit "+commit) + if err != nil { + return pullRequestsEvidence, err } - for _, prInterface := range pullRequests { - pr := prInterface.(map[string]any) - linksInterface := pr["links"].(map[string]any) - apiLinkMap := linksInterface["self"].(map[string]any) - htmlLinkMap := linksInterface["html"].(map[string]any) - evidence, err := c.getPullRequestDetailsFromBitbucket(apiLinkMap["href"].(string), htmlLinkMap["href"].(string), commit, version) + for _, pr := range pullRequests { + links := obj(pr, "links") + apiURL := str(obj(links, "self"), "href") + if apiURL == "" { + return pullRequestsEvidence, fmt.Errorf("a pull request in the Bitbucket reply has no links.self.href") + } + evidence, err := c.getPullRequestDetailsFromBitbucket(apiURL, str(obj(links, "html"), "href"), commit, version) if err != nil { return pullRequestsEvidence, err } @@ -141,25 +183,26 @@ func (c *Config) getPullRequestDetailsFromBitbucket(prApiUrl, prHtmlLink, commit evidence.URL = prHtmlLink evidence.MergeCommit = commit - evidence.State = responseData["state"].(string) - participants := responseData["participants"].([]any) + evidence.State = str(responseData, "state") + participants, err := objects(responseData, "participants", "pull request "+prApiUrl) + if err != nil { + return evidence, err + } approvers := []any{} if len(participants) > 0 { - for _, participantInterface := range participants { - p := participantInterface.(map[string]any) - if p["approved"].(bool) { - user := p["user"].(map[string]any) + for _, p := range participants { + if username := approvedBy(p); username != "" { if version == 1 { - approvers = append(approvers, user["display_name"].(string)) + approvers = append(approvers, username) } else { - approvalTimestamp, err := parseRFC3339NanoTimestamp(p["participated_on"].(string), "participated_on") + approvalTimestamp, err := parseRFC3339NanoTimestamp(str(p, "participated_on"), "participated_on") if err != nil { return evidence, err } approvers = append(approvers, types.PRApprovals{ - Username: user["display_name"].(string), - State: p["state"].(string), + Username: username, + State: str(p, "state"), Timestamp: approvalTimestamp, }) } @@ -170,22 +213,26 @@ func (c *Config) getPullRequestDetailsFromBitbucket(prApiUrl, prHtmlLink, commit c.Logger.Debug("no approvers found") } if version == 2 { - evidence.Author = responseData["author"].(map[string]any)["display_name"].(string) - createdAt, err := parseRFC3339NanoTimestamp(responseData["created_on"].(string), "created_on") + evidence.Author = str(obj(responseData, "author"), "display_name") + createdAt, err := parseRFC3339NanoTimestamp(str(responseData, "created_on"), "created_on") if err != nil { return evidence, err } evidence.CreatedAt = createdAt - mergedAt, err := parseRFC3339NanoTimestamp(responseData["updated_on"].(string), "updated_on") + mergedAt, err := parseRFC3339NanoTimestamp(str(responseData, "updated_on"), "updated_on") if err != nil { return evidence, err } evidence.MergedAt = mergedAt - evidence.Title = responseData["title"].(string) + evidence.Title = str(responseData, "title") evidence.HeadRef = bitbucketBranchName(responseData, "source") evidence.BaseRef = bitbucketBranchName(responseData, "destination") - prCommits, err := c.getPullRequestCommitsFromBitbucket(int(responseData["id"].(float64))) + prID, ok := responseData["id"].(float64) + if !ok { + return evidence, fmt.Errorf("the pull request at %s has no id", prApiUrl) + } + prCommits, err := c.getPullRequestCommitsFromBitbucket(int(prID)) if err != nil { return evidence, err } @@ -202,16 +249,24 @@ func (c *Config) getPullRequestDetailsFromBitbucket(prApiUrl, prHtmlLink, commit return evidence, nil } +// approvedBy names who approved in participant p, or gives "" when p is no +// approval. An approval with no identity, as from a deleted account, counts +// as none: approval evidence must name who gave it. +func approvedBy(p map[string]any) string { + if !boolean(p, "approved") { + return "" + } + return str(obj(p, "user"), "display_name") +} + // bitbucketReviews gives each participant's current decision. A participant // with no state who has commented is a comment; one who has done neither is // left out. -func bitbucketReviews(participants []any) ([]types.PRApprovals, error) { +func bitbucketReviews(participants []map[string]any) ([]types.PRApprovals, error) { reviews := []types.PRApprovals{} - for _, participantInterface := range participants { - p := participantInterface.(map[string]any) - user, _ := p["user"].(map[string]any) - username, _ := user["display_name"].(string) - participatedOn, _ := p["participated_on"].(string) + for _, p := range participants { + username := str(obj(p, "user"), "display_name") + participatedOn := str(p, "participated_on") var state string switch p["state"] { case "approved": @@ -231,18 +286,15 @@ func bitbucketReviews(participants []any) ([]types.PRApprovals, error) { if username == "" && state != types.ReviewChangesRequested { continue } - review := types.PRApprovals{ - Username: username, - State: state, - } - if participatedOn != "" { - timestamp, err := parseRFC3339NanoTimestamp(participatedOn, "participated_on") - if err != nil { - return nil, err - } - review.Timestamp = timestamp + timestamp, err := parseRFC3339NanoTimestamp(participatedOn, "participated_on") + if err != nil { + return nil, err } - reviews = append(reviews, review) + reviews = append(reviews, types.PRApprovals{ + Username: username, + State: state, + Timestamp: timestamp, + }) } return reviews, nil } @@ -250,7 +302,7 @@ func bitbucketReviews(participants []any) ([]types.PRApprovals, error) { // bitbucketBranchName extracts a branch name from a Bitbucket PR response for // the given side: "source" for the head branch, "destination" for the base. func bitbucketBranchName(prData map[string]any, side string) string { - return prData[side].(map[string]any)["branch"].(map[string]any)["name"].(string) + return str(obj(obj(prData, side), "branch"), "name") } // getPullRequestCommitsFromBitbucket gets the commits of a pull request from the Bitbucket API @@ -264,7 +316,10 @@ func (c *Config) getPullRequestCommitsFromBitbucket(prID int) ([]types.Commit, e allCommits := []types.Commit{} currentURL := url - for currentURL != "" { + for pages := 0; currentURL != ""; pages++ { + if pages == defaultMaxPages { + return nil, fmt.Errorf("aborting after %d pages of commits for pull request %d", defaultMaxPages, prID) + } reqParams := &requests.RequestParams{ Method: http.MethodGet, URL: currentURL, @@ -286,36 +341,32 @@ func (c *Config) getPullRequestCommitsFromBitbucket(prID int) ([]types.Commit, e return nil, err } - commits, ok := responseData["values"].([]any) - if !ok { - break + commits, err := objects(responseData, "values", fmt.Sprintf("pull request %d, page %d", prID, pages+1)) + if err != nil { + return nil, err } - for _, commitInterface := range commits { - commit := commitInterface.(map[string]any) - timestamp, err := parseRFC3339NanoTimestamp(commit["date"].(string), "date") + for _, commit := range commits { + if str(commit, "hash") == "" { + return nil, fmt.Errorf("commit %d for pull request %d has no hash", len(allCommits), prID) + } + timestamp, err := parseRFC3339NanoTimestamp(str(commit, "date"), "date") if err != nil { return nil, err } allCommits = append(allCommits, types.Commit{ - SHA: commit["hash"].(string), - Message: commit["message"].(string), - Author: commit["author"].(map[string]any)["raw"].(string), + SHA: str(commit, "hash"), + Message: str(commit, "message"), + Author: str(obj(commit, "author"), "raw"), Timestamp: timestamp, - URL: commit["links"].(map[string]any)["html"].(map[string]any)["href"].(string), + URL: str(obj(obj(commit, "links"), "html"), "href"), }) } - // Check for next page - nextInterface, hasNext := responseData["next"] - if !hasNext { - break - } - nextURL, ok := nextInterface.(string) - if !ok || nextURL == "" { + currentURL = str(responseData, "next") + if currentURL == "" { break } - currentURL = nextURL c.Logger.Debug("fetching next page of commits from %s", currentURL) } diff --git a/internal/bitbucket/odd_replies_test.go b/internal/bitbucket/odd_replies_test.go new file mode 100644 index 000000000..83df38da1 --- /dev/null +++ b/internal/bitbucket/odd_replies_test.go @@ -0,0 +1,425 @@ +package bitbucket + +import ( + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "regexp" + "strings" + "testing" + "time" + + "github.com/kosli-dev/cli/internal/logger" + "github.com/kosli-dev/cli/internal/requests" + "github.com/kosli-dev/cli/internal/testHelpers" + "github.com/kosli-dev/cli/internal/types" + "github.com/stretchr/testify/require" +) + +// Baseline replies for every call PREvidenceForCommitV2 makes. Shapes follow +// https://api.bitbucket.org/swagger.json: paginated_pullrequests (GET +// /repositories/{workspace}/{repo_slug}/commit/{commit}/pullrequests), +// pullrequest and participant (GET .../pullrequests/{pull_request_id}) and +// paginated_changeset of base_commit (GET .../pullrequests/{pull_request_id}/commits), +// as documented at +// https://developer.atlassian.com/cloud/bitbucket/rest/api-group-pullrequests/. +// {{base}} stands for https://api.bitbucket.org and is replaced with the test +// server's address when served, so links the code follows come back to it. +const ( + oddCommit = "4f1c2a9e8d7b6c5a4f3e2d1c0b9a8f7e6d5c4b3a" + + oddCommitPullRequests = `{ + "pagelen": 50, + "page": 1, + "size": 1, + "values": [{ + "type": "pullrequest", + "id": 7, + "title": "Add retry to uploads", + "state": "MERGED", + "author": {"type": "user", "display_name": "Ada Lovelace", "uuid": "{a1b2c3d4-0000-4000-8000-000000000001}", "account_id": "557058:ada", "nickname": "ada"}, + "source": {"branch": {"name": "feature/retry"}, "commit": {"type": "commit", "hash": "9a8b7c6d5e4f"}, "repository": {"type": "repository", "full_name": "acme/widgets", "name": "widgets", "uuid": "{a1b2c3d4-0000-4000-8000-0000000000aa}"}}, + "destination": {"branch": {"name": "main"}, "commit": {"type": "commit", "hash": "1a2b3c4d5e6f"}, "repository": {"type": "repository", "full_name": "acme/widgets", "name": "widgets", "uuid": "{a1b2c3d4-0000-4000-8000-0000000000aa}"}}, + "links": { + "self": {"href": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7"}, + "html": {"href": "https://bitbucket.org/acme/widgets/pull-requests/7"} + } + }] +}` + + oddPullRequest = `{ + "type": "pullrequest", + "id": 7, + "title": "Add retry to uploads", + "description": "Retries uploads on a 503.", + "state": "MERGED", + "draft": false, + "queued": false, + "reason": "", + "close_source_branch": true, + "comment_count": 2, + "task_count": 0, + "created_on": "2026-03-01T12:00:00.000000+00:00", + "updated_on": "2026-03-01T16:30:00.654321+00:00", + "author": {"type": "user", "display_name": "Ada Lovelace", "uuid": "{a1b2c3d4-0000-4000-8000-000000000001}", "account_id": "557058:ada", "nickname": "ada"}, + "closed_by": {"type": "user", "display_name": "Ada Lovelace", "uuid": "{a1b2c3d4-0000-4000-8000-000000000001}", "account_id": "557058:ada", "nickname": "ada"}, + "merge_commit": {"type": "commit", "hash": "4f1c2a9e8d7b"}, + "source": {"branch": {"name": "feature/retry"}, "commit": {"type": "commit", "hash": "9a8b7c6d5e4f"}, "repository": {"type": "repository", "full_name": "acme/widgets", "name": "widgets", "uuid": "{a1b2c3d4-0000-4000-8000-0000000000aa}"}}, + "destination": {"branch": {"name": "main"}, "commit": {"type": "commit", "hash": "1a2b3c4d5e6f"}, "repository": {"type": "repository", "full_name": "acme/widgets", "name": "widgets", "uuid": "{a1b2c3d4-0000-4000-8000-0000000000aa}"}}, + "reviewers": [ + {"type": "user", "display_name": "Grace Hopper", "uuid": "{a1b2c3d4-0000-4000-8000-000000000002}", "account_id": "557058:grace", "nickname": "grace"}, + {"type": "user", "display_name": "Alan Turing", "uuid": "{a1b2c3d4-0000-4000-8000-000000000003}", "account_id": "557058:alan", "nickname": "alan"} + ], + "participants": [ + {"type": "participant", "user": {"type": "user", "display_name": "Grace Hopper", "uuid": "{a1b2c3d4-0000-4000-8000-000000000002}", "account_id": "557058:grace", "nickname": "grace"}, "role": "REVIEWER", "approved": true, "state": "approved", "participated_on": "2026-03-01T13:00:00.123456+00:00"}, + {"type": "participant", "user": {"type": "user", "display_name": "Alan Turing", "uuid": "{a1b2c3d4-0000-4000-8000-000000000003}", "account_id": "557058:alan", "nickname": "alan"}, "role": "REVIEWER", "approved": false, "state": "changes_requested", "participated_on": "2026-03-01T14:00:00.000000+00:00"} + ], + "links": { + "self": {"href": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7"}, + "html": {"href": "https://bitbucket.org/acme/widgets/pull-requests/7"}, + "commits": {"href": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7/commits"} + } +}` + + // Commits come one per page so the code's paging is exercised. + oddPullRequestCommitsPage1 = `{ + "pagelen": 1, + "page": 1, + "values": [{ + "type": "commit", + "hash": "9a8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b", + "date": "2026-03-01T11:00:00+00:00", + "author": {"type": "author", "raw": "Ada Lovelace ", "user": {"type": "user", "display_name": "Ada Lovelace", "uuid": "{a1b2c3d4-0000-4000-8000-000000000001}"}}, + "message": "Add retry to uploads\n", + "parents": [{"type": "commit", "hash": "8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b9c"}], + "links": { + "self": {"href": "{{base}}/2.0/repositories/acme/widgets/commit/9a8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b"}, + "html": {"href": "https://bitbucket.org/acme/widgets/commits/9a8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b"} + } + }], + "next": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7/commits?page=2" +}` + + oddPullRequestCommitsPage2 = `{ + "pagelen": 1, + "page": 2, + "values": [{ + "type": "commit", + "hash": "8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b9c", + "date": "2026-03-01T10:00:00+00:00", + "author": {"type": "author", "raw": "Ada Lovelace ", "user": {"type": "user", "display_name": "Ada Lovelace", "uuid": "{a1b2c3d4-0000-4000-8000-000000000001}"}}, + "message": "Add upload test\n", + "parents": [{"type": "commit", "hash": "1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b"}], + "links": { + "self": {"href": "{{base}}/2.0/repositories/acme/widgets/commit/8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b9c"}, + "html": {"href": "https://bitbucket.org/acme/widgets/commits/8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b9c"} + } + }], + "previous": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7/commits?page=1" +}` +) + +// oddRoutes maps each reply name to the request that gets it. +var oddRoutes = map[string]string{ + "commit_pullrequests": "/2.0/repositories/acme/widgets/commit/" + oddCommit + "/pullrequests", + "pullrequest": "/2.0/repositories/acme/widgets/pullrequests/7", + "pullrequest_commits_1": "/2.0/repositories/acme/widgets/pullrequests/7/commits", + "pullrequest_commits_2": "/2.0/repositories/acme/widgets/pullrequests/7/commits?page=2", +} + +func oddBaselineReplies() map[string]string { + return map[string]string{ + "commit_pullrequests": oddCommitPullRequests, + "pullrequest": oddPullRequest, + "pullrequest_commits_1": oddPullRequestCommitsPage1, + "pullrequest_commits_2": oddPullRequestCommitsPage2, + } +} + +// redirectTransport sends requests for api.bitbucket.org to the test server +// and refuses any other host, so the test never reaches the network. +type redirectTransport struct { + server *url.URL +} + +func (rt redirectTransport) RoundTrip(req *http.Request) (*http.Response, error) { + switch req.URL.Host { + case "api.bitbucket.org": + req = req.Clone(req.Context()) + req.URL.Scheme = rt.server.Scheme + req.URL.Host = rt.server.Host + req.Host = rt.server.Host + case rt.server.Host: + default: + return nil, fmt.Errorf("test refuses request to %s", req.URL) + } + return http.DefaultTransport.RoundTrip(req) +} + +// runOddReplies serves replies and collects evidence for oddCommit through +// the given entry point: 1 for PREvidenceForCommitV1, 2 for V2. +func runOddReplies(t *testing.T, version int, replies map[string]string) ([]*types.PREvidence, error) { + t.Helper() + var base string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + for name, route := range oddRoutes { + if r.URL.RequestURI() == route { + _, _ = w.Write([]byte(strings.ReplaceAll(replies[name], "{{base}}", base))) + return + } + } + http.NotFound(w, r) + })) + defer server.Close() + base = server.URL + serverURL, err := url.Parse(server.URL) + require.NoError(t, err) + + c := &Config{ + Workspace: "acme", + Repository: "widgets", + AccessToken: "test-token", + Logger: logger.NewStandardLogger(), + KosliClient: &requests.Client{ + Logger: logger.NewStandardLogger(), + HttpClient: &http.Client{Transport: redirectTransport{server: serverURL}}, + }, + } + if version == 1 { + return c.PREvidenceForCommitV1(oddCommit) + } + return c.PREvidenceForCommitV2(oddCommit) +} + +func TestOddReplies_BaselineGivesFullEvidenceV1(t *testing.T) { + evidence, err := runOddReplies(t, 1, oddBaselineReplies()) + require.NoError(t, err) + require.Equal(t, []*types.PREvidence{{ + MergeCommit: oddCommit, + URL: "https://bitbucket.org/acme/widgets/pull-requests/7", + State: "MERGED", + Approvers: []any{"Grace Hopper"}, + }}, evidence) +} + +func TestOddReplies_BaselineGivesFullEvidenceV2(t *testing.T) { + evidence, err := runOddReplies(t, 2, oddBaselineReplies()) + require.NoError(t, err) + require.Equal(t, []*types.PREvidence{{ + MergeCommit: oddCommit, + URL: "https://bitbucket.org/acme/widgets/pull-requests/7", + State: "MERGED", + Approvers: []any{types.PRApprovals{ + Username: "Grace Hopper", State: "approved", Timestamp: 1772370000, + }}, + Author: "Ada Lovelace", + CreatedAt: 1772366400, + MergedAt: 1772382600, + Title: "Add retry to uploads", + HeadRef: "feature/retry", + BaseRef: "main", + Commits: []types.Commit{ + { + SHA: "9a8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b", + Message: "Add retry to uploads\n", + Author: "Ada Lovelace ", + Timestamp: 1772362800, + URL: "https://bitbucket.org/acme/widgets/commits/9a8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b", + }, + { + SHA: "8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b9c", + Message: "Add upload test\n", + Author: "Ada Lovelace ", + Timestamp: 1772359200, + URL: "https://bitbucket.org/acme/widgets/commits/8b7c6d5e4f3a2b1c0d9e8f7a6b5c4d3e2f1a0b9c", + }, + }, + Reviews: []types.PRApprovals{ + {Username: "Grace Hopper", State: types.ReviewApproved, Timestamp: 1772370000}, + {Username: "Alan Turing", State: types.ReviewChangesRequested, Timestamp: 1772373600}, + }, + }}, evidence) +} + +// oddAllowedErrors are the values the code needs for its next request, so +// making them (or anything holding them) null may fail. Keyed by entry point, +// reply and path. +var oddAllowedErrors = map[string]string{ + "V1 commit_pullrequests values[0].links.self.href": "the pull request's API URL, fetched next", + "V2 commit_pullrequests values[0].links.self.href": "the pull request's API URL, fetched next", + "V2 pullrequest id": "the pull request number, used to fetch its commits", + "V2 pullrequest_commits_1 values[0].hash": "a commit with no SHA can't be attested", + "V2 pullrequest_commits_2 values[0].hash": "a commit with no SHA can't be attested", +} + +// oddListOwners is what each read list belongs to, as its error names it. +var oddListOwners = map[string]string{ + "commit_pullrequests": "for commit " + oddCommit + " ", + "pullrequest": "/2.0/repositories/acme/widgets/pullrequests/7 ", + "pullrequest_commits_1": "for pull request 7, page 1 ", + "pullrequest_commits_2": "for pull request 7, page 2 ", +} + +// oddReadLists are the lists the code reads, per reply. A null item in one of +// them is an entry that can't be read, so it must fail. +var oddReadLists = map[string]string{ + "commit_pullrequests": "values", + "pullrequest": "participants", + "pullrequest_commits_1": "values", + "pullrequest_commits_2": "values", +} + +// oddNameless make a participant one with no identity, as for a deleted +// account. Its named entries must leave both lists, and V2's reviews must then +// hold the nameless entries given. A nameless approval is in neither list, so +// no policy counts it; a nameless change request stays, as dropping it would +// read as none given. +var oddNameless = map[string]struct { + who string + nameless []types.PRApprovals +}{ + "pullrequest participants[0].user": {"Grace Hopper", nil}, + "pullrequest participants[0].user.display_name": {"Grace Hopper", nil}, + "pullrequest participants[1].user": {"Alan Turing", []types.PRApprovals{{State: types.ReviewChangesRequested, Timestamp: 1772373600}}}, + "pullrequest participants[1].user.display_name": {"Alan Turing", []types.PRApprovals{{State: types.ReviewChangesRequested, Timestamp: 1772373600}}}, +} + +// requireNamelessHandled checks got against want, the evidence for the same +// replies with who still named: who's entries are gone from both lists, the +// other named entries are unchanged, and the only nameless reviews are those +// given (V2 only, as V1 records no reviews). +func requireNamelessHandled(t *testing.T, version int, want, got *types.PREvidence, who string, nameless []types.PRApprovals) { + t.Helper() + var wantApprovers []any + for _, a := range want.Approvers { + if approverName(a) != who { + wantApprovers = append(wantApprovers, a) + } + } + require.ElementsMatch(t, wantApprovers, got.Approvers) + if version == 1 { + return + } + var wantReviews []types.PRApprovals + for _, r := range want.Reviews { + if r.Username != who { + wantReviews = append(wantReviews, r) + } + } + wantReviews = append(wantReviews, nameless...) + require.ElementsMatch(t, wantReviews, got.Reviews) +} + +func approverName(a any) string { + switch a := a.(type) { + case string: + return a + case types.PRApprovals: + return a.Username + } + return fmt.Sprint(a) +} + +// oddMayError says whether path in reply holds, or is, a value in +// oddAllowedErrors for that entry point. +func oddMayError(entry, reply, path string) bool { + for key := range oddAllowedErrors { + allowed, ok := strings.CutPrefix(key, entry+" "+reply+" ") + if !ok { + continue + } + if path == "." || allowed == path || strings.HasPrefix(allowed, path+".") || strings.HasPrefix(allowed, path+"[") { + return true + } + } + return false +} + +// TestOddReplies makes each value in each reply null in turn, and empties each +// list, and checks the rule of kosli-dev/cli#1283 through both entry points: +// never a panic; a null field is no error unless oddAllowedErrors lists it; a +// null item in a list the code reads is an error; an emptied list is no error. +func TestOddReplies(t *testing.T) { + for _, version := range []int{1, 2} { + entry := fmt.Sprintf("V%d", version) + baseline, err := runOddReplies(t, version, oddBaselineReplies()) + require.NoError(t, err) + + for name := range oddRoutes { + if version == 1 && strings.HasPrefix(name, "pullrequest_commits") { + continue // V1 doesn't fetch commits. + } + variants, err := testHelpers.NullVariants([]byte(oddBaselineReplies()[name])) + require.NoError(t, err, name) + for _, v := range variants { + t.Run(entry+" "+name+" "+v.Path, func(t *testing.T) { + replies := oddBaselineReplies() + replies[name] = string(v.Body) + outcome := testHelpers.EvidenceOutcome(baseline, func() ([]*types.PREvidence, error) { + return runOddReplies(t, version, replies) + }) + require.False(t, strings.HasPrefix(outcome, "panic"), outcome) + namelessCase, nameless := oddNameless[name+" "+v.Path] + switch { + case strings.HasSuffix(v.Path, "[]"): + require.True(t, strings.HasPrefix(outcome, "ok"), "an emptied list must not fail: %s", outcome) + case regexp.MustCompile(`^` + oddReadLists[name] + `\[\d+\]$`).MatchString(v.Path): + require.Equal(t, "error", outcome, "a null list item must fail") + _, err := runOddReplies(t, version, replies) + require.ErrorContains(t, err, oddListOwners[name], "the error must say what the list belongs to") + case nameless: + evidence, err := runOddReplies(t, version, replies) + require.NoError(t, err) + requireNamelessHandled(t, version, baseline[0], evidence[0], namelessCase.who, namelessCase.nameless) + case oddMayError(entry, name, v.Path): + default: + require.True(t, strings.HasPrefix(outcome, "ok"), "a null field must not fail: %s", outcome) + } + }) + } + } + } +} + +// Commit pages whose next links cycle would be fetched forever: a page +// pointing at itself, or two pages pointing at each other. +func TestOddReplies_CommitPagesThatCycleFail(t *testing.T) { + for name, next := range map[string]string{ + "page 2 to itself": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7/commits?page=2", + "page 2 to page 1": "{{base}}/2.0/repositories/acme/widgets/pullrequests/7/commits", + } { + t.Run(name, func(t *testing.T) { + replies := oddBaselineReplies() + replies["pullrequest_commits_2"] = `{"pagelen": 1, "page": 2, "values": [], "next": "` + next + `"}` + done := make(chan error, 1) + go func() { + _, err := runOddReplies(t, 2, replies) + done <- err + }() + select { + case err := <-done: + require.ErrorContains(t, err, fmt.Sprintf("aborting after %d pages", defaultMaxPages)) + case <-time.After(20 * time.Second): + t.Fatal("paging did not stop") + } + }) + } +} + +// A commit with no hash can't be attested, so it fails rather than being +// recorded with an empty SHA. +func TestOddReplies_CommitWithNoHashFails(t *testing.T) { + // Commits are numbered across pages, so each index names one commit. + for page, index := range map[string]string{"pullrequest_commits_1": "0", "pullrequest_commits_2": "1"} { + t.Run(page, func(t *testing.T) { + replies := oddBaselineReplies() + hash := regexp.MustCompile(`"hash": "[0-9a-f]{40}",\n\t\t"date"`) + require.Len(t, hash.FindAllString(replies[page], -1), 1) + replies[page] = hash.ReplaceAllString(replies[page], `"hash": null, + "date"`) + _, err := runOddReplies(t, 2, replies) + require.ErrorContains(t, err, "commit "+index+" for pull request 7 has no hash") + }) + } +} diff --git a/internal/bitbucket/reviews_test.go b/internal/bitbucket/reviews_test.go index 3a0c20fe0..0abe34994 100644 --- a/internal/bitbucket/reviews_test.go +++ b/internal/bitbucket/reviews_test.go @@ -12,7 +12,7 @@ import ( // https://api.bitbucket.org/swagger.json: state is approved, changes_requested // or null; participated_on is the approval time, else the last comment, else null. func TestBitbucketReviews(t *testing.T) { - var participants []any + var participants []map[string]any require.NoError(t, json.Unmarshal([]byte(`[ {"user":{"display_name":"Ada"},"role":"REVIEWER","approved":true,"state":"approved","participated_on":"2026-03-01T13:00:00.123456+00:00"}, {"user":{"display_name":"Bob"},"role":"REVIEWER","approved":false,"state":"changes_requested","participated_on":"2026-03-01T14:00:00+00:00"}, @@ -20,7 +20,8 @@ func TestBitbucketReviews(t *testing.T) { {"user":{"display_name":"Di"},"role":"PARTICIPANT","approved":false,"state":null,"participated_on":"2026-03-01T15:00:00+00:00"}, {"user":{"display_name":"Ed"},"role":"REVIEWER","approved":false,"state":null,"participated_on":null}, {"user":null,"role":"REVIEWER","approved":false,"state":"changes_requested","participated_on":null}, - {"user":null,"role":"PARTICIPANT","approved":false,"state":null,"participated_on":"2026-03-01T16:00:00+00:00"} + {"user":null,"role":"PARTICIPANT","approved":false,"state":null,"participated_on":"2026-03-01T16:00:00+00:00"}, + {"user":{"display_name":null},"role":"REVIEWER","approved":true,"state":"approved","participated_on":"2026-03-01T17:00:00+00:00"} ]`), &participants)) reviews, err := bitbucketReviews(participants) @@ -35,7 +36,7 @@ func TestBitbucketReviews(t *testing.T) { } func TestBitbucketReviews_RejectsABadTimestamp(t *testing.T) { - var participants []any + var participants []map[string]any require.NoError(t, json.Unmarshal([]byte(`[ {"user":{"display_name":"Ada"},"approved":true,"state":"approved","participated_on":"yesterday"} ]`), &participants))