diff --git a/platform/README.md b/platform/README.md index 9fdcdd0ca..5b0f8482f 100644 --- a/platform/README.md +++ b/platform/README.md @@ -14,6 +14,7 @@ Cross-domain packages shared by SubmitQueue, Stovepipe, and other services in th - **pipeline/** — Typed engine that assembles a service's queue topology into one lifecycle component. - **publish/** — Topic-key publish plumbing and the message-ID convention that controls deduplication. - **buildkite/**, **githubactions/** — Vendor CI clients over `platform/http`: the REST calls and provider-specific vocabulary (state strings, id encoding) shared by every domain's `BuildRunner` backend. They deliberately define no interface, so they are plumbing rather than extensions — each domain keeps its own `BuildRunner` contract under `{domain}/extension/buildrunner` and adapts these clients to it. +- **githubapi/** — Request conventions every direct GitHub API caller shares: the REST headers pinned to one API version, and the GraphQL envelope with its errors. Not a client: callers keep their own `*http.Client` and typed requests. - **base/** — Shared domain entities (`change`, `messagequeue`, and related subpackages). Root package `base` is documentation-only. - **extension/** — Shared extension interfaces and implementations reused across domains (`counter`, `messagequeue`, and backends such as `mysql`). A package belongs here only if it defines a behavioral interface with its `Config` and `Factory` interface; a vendor client with no interface belongs directly under `platform/`. diff --git a/platform/githubactions/BUILD.bazel b/platform/githubactions/BUILD.bazel index c86265a45..7d5287863 100644 --- a/platform/githubactions/BUILD.bazel +++ b/platform/githubactions/BUILD.bazel @@ -5,7 +5,10 @@ go_library( srcs = ["client.go"], importpath = "github.com/uber/submitqueue/platform/githubactions", visibility = ["//visibility:public"], - deps = ["//platform/http:go_default_library"], + deps = [ + "//platform/githubapi:go_default_library", + "//platform/http:go_default_library", + ], ) go_test( diff --git a/platform/githubactions/client.go b/platform/githubactions/client.go index cede10351..2bd7171f8 100644 --- a/platform/githubactions/client.go +++ b/platform/githubactions/client.go @@ -31,11 +31,10 @@ import ( "net/url" "strconv" + "github.com/uber/submitqueue/platform/githubapi" phttp "github.com/uber/submitqueue/platform/http" ) -const githubAPIVersion = "2026-03-10" - // ErrNotFound is returned when the GitHub Actions API responds with 404 to a // request for a resource by ID (e.g. GetRun on an unknown run, or CancelRun // on an already-deleted run). @@ -136,7 +135,7 @@ func (c *Client) GetRun(ctx context.Context, runID int64) (WorkflowRun, error) { // run is already terminal or otherwise not cancellable (HTTP 409/422) — the // BuildRunner contract treats that as a no-op. func (c *Client) CancelRun(ctx context.Context, runID int64) error { - status, _, err := phttp.SendRequest(ctx, c.httpClient, http.MethodPost, c.runPath(runID)+"/cancel", nil, c.setHeaders) + status, _, err := phttp.SendRequest(ctx, c.httpClient, http.MethodPost, c.runPath(runID)+"/cancel", nil, githubapi.SetRESTHeaders) if err != nil { return err } @@ -181,7 +180,7 @@ func (c *Client) runPath(runID int64) string { // 2xx response, decodes the body into out (when non-nil). A 404 is reported // as ErrNotFound so callers can distinguish it from other failures. func (c *Client) do(ctx context.Context, method, rawURL string, body []byte, out any) error { - status, respBody, err := phttp.SendRequest(ctx, c.httpClient, method, rawURL, body, c.setHeaders) + status, respBody, err := phttp.SendRequest(ctx, c.httpClient, method, rawURL, body, githubapi.SetRESTHeaders) if err != nil { return err } @@ -203,12 +202,6 @@ func (c *Client) do(ctx context.Context, method, rawURL string, body []byte, out return nil } -func (c *Client) setHeaders(req *http.Request) { - req.Header.Set("Accept", "application/vnd.github+json") - req.Header.Set("Content-Type", "application/json") - req.Header.Set("X-GitHub-Api-Version", githubAPIVersion) -} - // EncodeRunID encodes a GitHub Actions workflow run id as an opaque build id // string. func EncodeRunID(runID int64) string { diff --git a/platform/githubapi/BUILD.bazel b/platform/githubapi/BUILD.bazel new file mode 100644 index 000000000..f50e6c5ab --- /dev/null +++ b/platform/githubapi/BUILD.bazel @@ -0,0 +1,20 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["githubapi.go"], + importpath = "github.com/uber/submitqueue/platform/githubapi", + visibility = ["//visibility:public"], + deps = ["//platform/http:go_default_library"], +) + +go_test( + name = "go_default_test", + srcs = ["githubapi_test.go"], + embed = [":go_default_library"], + deps = [ + "//platform/http:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + ], +) diff --git a/platform/githubapi/README.md b/platform/githubapi/README.md new file mode 100644 index 000000000..9331aaeab --- /dev/null +++ b/platform/githubapi/README.md @@ -0,0 +1,14 @@ +# GitHub API conventions + +The parts of a GitHub API request that every direct caller would otherwise repeat: + +- `SetRESTHeaders` sets the JSON media type and pins the REST API version (`Version`), so every REST caller moves to a new version together. Pass it as `platform/http.SendRequest`'s `setHeaders`. +- `QueryGraphQL` posts a `GraphQLRequest` to `/graphql` and decodes the response's `data` into the caller's type. A non-200 reply is a `platform/http.StatusError`, so `platform/errs/http` classifies it like any other HTTP failure; a 200 reply that carries GraphQL errors fails even when it also carries data. + +This package is not a client. Each caller keeps its own `*http.Client` (API root from `platform/http.BaseURLTransport`, auth from a transport) and its own typed requests and responses, and decides what a missing resource means for it. + +## Who uses it + +- `platform/githubactions` and `runway/extension/merger/github` (REST) +- `submitqueue/extension/changeprovider/github` and `submitqueue/extension/validator/github` (GraphQL) +- `test/testutil/githubtestrepo` (REST) diff --git a/platform/githubapi/githubapi.go b/platform/githubapi/githubapi.go new file mode 100644 index 000000000..34e92e2f3 --- /dev/null +++ b/platform/githubapi/githubapi.go @@ -0,0 +1,111 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package githubapi holds the request conventions shared by every component +// that calls the GitHub API directly: the REST headers, pinned to one API +// version, and the GraphQL request and response envelope. +// +// It is not a client. Callers keep their own *http.Client, with the API root +// resolved by platform/http.BaseURLTransport and auth set by a transport, and +// their own typed requests; this package only removes the parts every caller +// would otherwise repeat. +package githubapi + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "strings" + + phttp "github.com/uber/submitqueue/platform/http" +) + +// Version is the GitHub REST API version every REST request pins. +const Version = "2026-03-10" + +// SetRESTHeaders sets the headers of a JSON request to the GitHub REST API. It +// has the signature platform/http.SendRequest takes for its setHeaders. +func SetRESTHeaders(req *http.Request) { + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-GitHub-Api-Version", Version) +} + +// GraphQLRequest is the body of a request to the GitHub GraphQL API. +type GraphQLRequest struct { + // Query is the GraphQL document. + Query string `json:"query"` + // Variables binds the query's variables by name. + Variables map[string]any `json:"variables"` +} + +type graphQLResponse struct { + Data json.RawMessage `json:"data"` + Errors graphQLErrors `json:"errors"` +} + +type graphQLError struct { + Type string `json:"type"` + Message string `json:"message"` +} + +// graphQLErrors is the errors array of a response GitHub answered with 200. +type graphQLErrors []graphQLError + +func (e graphQLErrors) Error() string { + msgs := make([]string, len(e)) + for i, err := range e { + msgs[i] = err.Type + ": " + err.Message + } + return "GraphQL errors: " + strings.Join(msgs, "; ") +} + +// QueryGraphQL posts req to the client's /graphql and decodes the response's +// data into data. A non-200 reply is a *platform/http.StatusError, so error +// classifiers can read its code; a 200 reply that carries errors fails even +// when it also carries data. A null data leaves data untouched. +func QueryGraphQL(ctx context.Context, client *http.Client, req GraphQLRequest, data any) error { + body, err := json.Marshal(req) + if err != nil { + return fmt.Errorf("marshal GraphQL request: %w", err) + } + // The path is relative: the client's transport roots it at the API. + status, respBody, err := phttp.SendRequest(ctx, client, http.MethodPost, "/graphql", body, setGraphQLHeaders) + if err != nil { + return err + } + if status != http.StatusOK { + return phttp.NewStatusError(status, respBody) + } + + var resp graphQLResponse + if err := json.Unmarshal(respBody, &resp); err != nil { + return fmt.Errorf("decode GraphQL response: %w", err) + } + if len(resp.Errors) > 0 { + return resp.Errors + } + if len(resp.Data) == 0 { + return nil + } + if err := json.Unmarshal(resp.Data, data); err != nil { + return fmt.Errorf("decode GraphQL data: %w", err) + } + return nil +} + +func setGraphQLHeaders(req *http.Request) { + req.Header.Set("Content-Type", "application/json") +} diff --git a/platform/githubapi/githubapi_test.go b/platform/githubapi/githubapi_test.go new file mode 100644 index 000000000..6a5b542dc --- /dev/null +++ b/platform/githubapi/githubapi_test.go @@ -0,0 +1,154 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package githubapi + +import ( + "context" + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + phttp "github.com/uber/submitqueue/platform/http" +) + +func TestSetRESTHeaders(t *testing.T) { + req, err := http.NewRequest(http.MethodGet, "/repos/o/r", nil) + require.NoError(t, err) + + SetRESTHeaders(req) + + assert.Equal(t, "application/vnd.github+json", req.Header.Get("Accept")) + assert.Equal(t, "application/json", req.Header.Get("Content-Type")) + assert.Equal(t, Version, req.Header.Get("X-GitHub-Api-Version")) +} + +type pullRequestData struct { + Repository struct { + PullRequest struct { + HeadRefOid string `json:"headRefOid"` + } `json:"pullRequest"` + } `json:"repository"` +} + +func TestQueryGraphQL(t *testing.T) { + tests := []struct { + name string + status int + body string + wantHead string + wantErr bool + wantStatus int + wantGraphQLErr bool + }{ + { + name: "decodes data", + status: http.StatusOK, + body: `{"data":{"repository":{"pullRequest":{"headRefOid":"abc"}}}}`, + wantHead: "abc", + }, + { + name: "null data leaves the result untouched", + status: http.StatusOK, + body: `{"data":null}`, + }, + { + name: "missing data leaves the result untouched", + status: http.StatusOK, + body: `{}`, + }, + { + name: "non-200 status is a status error", + status: http.StatusBadGateway, + body: `upstream unavailable`, + wantErr: true, + wantStatus: http.StatusBadGateway, + }, + { + name: "401 is a status error", + status: http.StatusUnauthorized, + body: `{"message":"Bad credentials"}`, + wantErr: true, + wantStatus: http.StatusUnauthorized, + }, + { + name: "undecodable body", + status: http.StatusOK, + body: `not json`, + wantErr: true, + }, + { + name: "GraphQL errors", + status: http.StatusOK, + body: `{"errors":[{"type":"NOT_FOUND","message":"Could not resolve to a Repository"},{"type":"FORBIDDEN","message":"Resource not accessible"}]}`, + wantErr: true, + wantGraphQLErr: true, + }, + { + name: "GraphQL errors alongside data", + status: http.StatusOK, + body: `{"data":{"repository":{"pullRequest":null}},"errors":[{"type":"NOT_FOUND","message":"Could not resolve to a PullRequest"}]}`, + wantErr: true, + wantGraphQLErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var gotMethod, gotPath, gotContentType string + var gotReq GraphQLRequest + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotMethod, gotPath, gotContentType = r.Method, r.URL.Path, r.Header.Get("Content-Type") + raw, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(raw, &gotReq) + w.WriteHeader(tt.status) + _, _ = io.WriteString(w, tt.body) + })) + t.Cleanup(server.Close) + client, err := phttp.NewClient(server.URL) + require.NoError(t, err) + + req := GraphQLRequest{Query: "query { viewer { login } }", Variables: map[string]any{"number": float64(7)}} + var data pullRequestData + err = QueryGraphQL(context.Background(), client, req, &data) + + assert.Equal(t, http.MethodPost, gotMethod) + assert.Equal(t, "/graphql", gotPath) + assert.Equal(t, "application/json", gotContentType) + assert.Equal(t, req, gotReq) + + var statusErr *phttp.StatusError + var gqlErr graphQLErrors + if !tt.wantErr { + require.NoError(t, err) + assert.Equal(t, tt.wantHead, data.Repository.PullRequest.HeadRefOid) + return + } + require.Error(t, err) + if tt.wantStatus != 0 { + require.True(t, errors.As(err, &statusErr)) + assert.Equal(t, tt.wantStatus, statusErr.StatusCode) + } else { + assert.False(t, errors.As(err, &statusErr)) + } + assert.Equal(t, tt.wantGraphQLErr, errors.As(err, &gqlErr)) + }) + } +} diff --git a/runway/extension/merger/github/BUILD.bazel b/runway/extension/merger/github/BUILD.bazel index 182759e8b..7581f8bd3 100644 --- a/runway/extension/merger/github/BUILD.bazel +++ b/runway/extension/merger/github/BUILD.bazel @@ -16,6 +16,7 @@ go_library( "//api/runway/messagequeue/protopb:go_default_library", "//platform/base/change/github:go_default_library", "//platform/errs:go_default_library", + "//platform/githubapi:go_default_library", "//platform/http:go_default_library", "//platform/metrics:go_default_library", "//runway/extension/merger:go_default_library", diff --git a/runway/extension/merger/github/client.go b/runway/extension/merger/github/client.go index c46d26679..527858d86 100644 --- a/runway/extension/merger/github/client.go +++ b/runway/extension/merger/github/client.go @@ -22,11 +22,10 @@ import ( "net/url" "strconv" + "github.com/uber/submitqueue/platform/githubapi" phttp "github.com/uber/submitqueue/platform/http" ) -const githubAPIVersion = "2026-03-10" - // pullStateOpen is the REST API state of a pull request that is neither merged // nor closed. const pullStateOpen = "open" @@ -201,7 +200,7 @@ func (c *client) submitMerge(ctx context.Context, number int, req asyncMergeRequ return asyncMergeStatus{}, 0, fmt.Errorf("marshal merge request: %w", err) } path := c.repoPath("pulls", strconv.Itoa(number), "merge-async") - status, respBody, err := phttp.SendRequest(ctx, c.httpClient, http.MethodPut, path, body, setHeaders) + status, respBody, err := phttp.SendRequest(ctx, c.httpClient, http.MethodPut, path, body, githubapi.SetRESTHeaders) if err != nil { return asyncMergeStatus{}, 0, fmt.Errorf("merge pull request #%d: %w", number, err) } @@ -244,7 +243,7 @@ func (c *client) getMergeStatus(ctx context.Context, number int, uuid string) (a } func (c *client) getJSON(ctx context.Context, path string, out any) error { - status, respBody, err := phttp.SendRequest(ctx, c.httpClient, http.MethodGet, path, nil, setHeaders) + status, respBody, err := phttp.SendRequest(ctx, c.httpClient, http.MethodGet, path, nil, githubapi.SetRESTHeaders) if err != nil { return err } @@ -264,9 +263,3 @@ func (c *client) repoPath(segments ...string) string { } return path } - -func setHeaders(req *http.Request) { - req.Header.Set("Accept", "application/vnd.github+json") - req.Header.Set("Content-Type", "application/json") - req.Header.Set("X-GitHub-Api-Version", githubAPIVersion) -} diff --git a/submitqueue/extension/changeprovider/github/BUILD.bazel b/submitqueue/extension/changeprovider/github/BUILD.bazel index fd60c787b..0fc24ac9e 100644 --- a/submitqueue/extension/changeprovider/github/BUILD.bazel +++ b/submitqueue/extension/changeprovider/github/BUILD.bazel @@ -12,7 +12,7 @@ go_library( visibility = ["//visibility:public"], deps = [ "//platform/base/change/github:go_default_library", - "//platform/http:go_default_library", + "//platform/githubapi:go_default_library", "//platform/metrics:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/changeprovider:go_default_library", diff --git a/submitqueue/extension/changeprovider/github/graphql.go b/submitqueue/extension/changeprovider/github/graphql.go index fc1d26957..845cd51e9 100644 --- a/submitqueue/extension/changeprovider/github/graphql.go +++ b/submitqueue/extension/changeprovider/github/graphql.go @@ -1,15 +1,6 @@ package github -import ( - "bytes" - "context" - "encoding/json" - "fmt" - "io" - "net/http" - - phttp "github.com/uber/submitqueue/platform/http" -) +import "github.com/uber/submitqueue/platform/githubapi" // pullRequestQuery is the GraphQL query to fetch pull request information including files, author, and head SHA. const pullRequestQuery = ` @@ -43,26 +34,11 @@ query($owner: String!, $repo: String!, $prNumber: Int!, $filesCursor: String) { } ` -// graphqlRequest represents a GraphQL request. -type graphqlRequest struct { - Query string `json:"query"` - Variables map[string]any `json:"variables"` -} - -// graphqlResponse represents the top-level GraphQL response. -type graphqlResponse struct { - Data struct { - Repository struct { - PullRequest pullRequestData `json:"pullRequest"` - } `json:"repository"` - } `json:"data"` - Errors []graphqlError `json:"errors,omitempty"` -} - -// graphqlError represents a GraphQL error. -type graphqlError struct { - Message string `json:"message"` - Type string `json:"type"` +// pullRequestQueryData is the data of a pullRequestQuery response. +type pullRequestQueryData struct { + Repository struct { + PullRequest pullRequestData `json:"pullRequest"` + } `json:"repository"` } // pullRequestData contains the pull request metadata. @@ -102,8 +78,8 @@ type fileNode struct { } // buildGraphQLRequest builds a GraphQL request for fetching pull request data. -func buildGraphQLRequest(org, repo string, prNumber int, cursor string) graphqlRequest { - return graphqlRequest{ +func buildGraphQLRequest(org, repo string, prNumber int, cursor string) githubapi.GraphQLRequest { + return githubapi.GraphQLRequest{ Query: pullRequestQuery, Variables: map[string]any{ "owner": org, @@ -113,46 +89,3 @@ func buildGraphQLRequest(org, repo string, prNumber int, cursor string) graphqlR }, } } - -// doGraphQLRequest executes a GraphQL HTTP request. -// The path "/graphql" is relative — BaseURLTransport on the client resolves it to the full URL. -func doGraphQLRequest( - ctx context.Context, - bodyBytes []byte, - client *http.Client, -) (*http.Response, error) { - req, err := http.NewRequestWithContext(ctx, http.MethodPost, "/graphql", bytes.NewReader(bodyBytes)) - if err != nil { - return nil, fmt.Errorf("failed to create HTTP request: %w", err) - } - - req.Header.Set("Content-Type", "application/json") - - resp, err := client.Do(req) - if err != nil { - return nil, fmt.Errorf("HTTP request failed: %w", err) - } - - return resp, nil -} - -// parseGraphQLResponse parses and validates a GraphQL response. -func parseGraphQLResponse( - resp *http.Response, -) (*pullRequestData, error) { - if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) - return nil, fmt.Errorf("GitHub API request failed: %w", phttp.NewStatusError(resp.StatusCode, body)) - } - - var gqlResp graphqlResponse - if err := json.NewDecoder(resp.Body).Decode(&gqlResp); err != nil { - return nil, fmt.Errorf("failed to decode GraphQL response: %w", err) - } - - if len(gqlResp.Errors) > 0 { - return nil, fmt.Errorf("GraphQL errors: %+v", gqlResp.Errors) - } - - return &gqlResp.Data.Repository.PullRequest, nil -} diff --git a/submitqueue/extension/changeprovider/github/graphql_test.go b/submitqueue/extension/changeprovider/github/graphql_test.go index 7eab8e45f..2ef8020e0 100644 --- a/submitqueue/extension/changeprovider/github/graphql_test.go +++ b/submitqueue/extension/changeprovider/github/graphql_test.go @@ -1,9 +1,7 @@ package github import ( - "io" - "net/http" - "strings" + "encoding/json" "testing" "github.com/stretchr/testify/assert" @@ -56,34 +54,29 @@ func TestBuildGraphQLRequest(t *testing.T) { } } -func TestParseGraphQLResponse(t *testing.T) { +func TestPullRequestQueryData_DecodesGraphQLData(t *testing.T) { tests := []struct { - name string - statusCode int - body string - wantErr bool - wantData *pullRequestData + name string + data string + want pullRequestData }{ { - name: "success", - statusCode: http.StatusOK, - body: `{ - "data": { - "repository": { - "pullRequest": { - "number": 42, - "headRefOid": "abc123", - "author": {"login": "octocat", "name": "The Octocat", "email": "octocat@example.com"}, - "files": { - "totalCount": 1, - "pageInfo": {"endCursor": "cur1", "hasNextPage": false}, - "nodes": [{"path": "main.go", "additions": 10, "deletions": 2, "changeType": "MODIFIED"}] - } + name: "pull request with author and files", + data: `{ + "repository": { + "pullRequest": { + "number": 42, + "headRefOid": "abc123", + "author": {"login": "octocat", "name": "The Octocat", "email": "octocat@example.com"}, + "files": { + "totalCount": 1, + "pageInfo": {"endCursor": "cur1", "hasNextPage": false}, + "nodes": [{"path": "main.go", "additions": 10, "deletions": 2, "changeType": "MODIFIED"}] } } } }`, - wantData: &pullRequestData{ + want: pullRequestData{ Number: 42, HeadRefOid: "abc123", Author: authorData{Login: "octocat", Name: "The Octocat", Email: "octocat@example.com"}, @@ -94,52 +87,13 @@ func TestParseGraphQLResponse(t *testing.T) { }, }, }, - { - name: "non-200 status", - statusCode: http.StatusInternalServerError, - body: `{"message":"Internal Server Error"}`, - wantErr: true, - }, - { - name: "404 not found", - statusCode: http.StatusNotFound, - body: `{"message":"Not Found"}`, - wantErr: true, - }, - { - name: "invalid JSON", - statusCode: http.StatusOK, - body: `{invalid json`, - wantErr: true, - }, - { - name: "GraphQL errors", - statusCode: http.StatusOK, - body: `{"errors":[{"message":"Field doesn't exist","type":"INVALID_FIELD"}]}`, - wantErr: true, - }, - { - name: "multiple GraphQL errors", - statusCode: http.StatusOK, - body: `{"errors":[{"message":"Not Found","type":"NOT_FOUND"},{"message":"Forbidden","type":"FORBIDDEN"}]}`, - wantErr: true, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - resp := &http.Response{ - StatusCode: tt.statusCode, - Body: io.NopCloser(strings.NewReader(tt.body)), - } - - got, err := parseGraphQLResponse(resp) - if tt.wantErr { - require.Error(t, err) - return - } - require.NoError(t, err) - assert.Equal(t, tt.wantData, got) + var got pullRequestQueryData + require.NoError(t, json.Unmarshal([]byte(tt.data), &got)) + assert.Equal(t, tt.want, got.Repository.PullRequest) }) } } diff --git a/submitqueue/extension/changeprovider/github/provider.go b/submitqueue/extension/changeprovider/github/provider.go index 748e6553f..165544bad 100644 --- a/submitqueue/extension/changeprovider/github/provider.go +++ b/submitqueue/extension/changeprovider/github/provider.go @@ -2,7 +2,6 @@ package github import ( "context" - "encoding/json" "fmt" "net/http" @@ -10,6 +9,7 @@ import ( "go.uber.org/zap" entitygithub "github.com/uber/submitqueue/platform/base/change/github" + "github.com/uber/submitqueue/platform/githubapi" coremetrics "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/changeprovider" @@ -155,18 +155,9 @@ func (p *provider) fetchPullRequest(ctx context.Context, parsed entitygithub.Cha // fetchPullRequestPage fetches a single page of PR data. func (p *provider) fetchPullRequestPage(ctx context.Context, org, repo string, prNumber int, cursor string) (*pullRequestData, error) { - reqBody := buildGraphQLRequest(org, repo, prNumber, cursor) - - bodyBytes, err := json.Marshal(reqBody) - if err != nil { - return nil, fmt.Errorf("failed to marshal GraphQL request: %w", err) + var data pullRequestQueryData + if err := githubapi.QueryGraphQL(ctx, p.httpClient, buildGraphQLRequest(org, repo, prNumber, cursor), &data); err != nil { + return nil, fmt.Errorf("GitHub GraphQL query failed: %w", err) } - - resp, err := doGraphQLRequest(ctx, bodyBytes, p.httpClient) - if err != nil { - return nil, err - } - defer resp.Body.Close() - - return parseGraphQLResponse(resp) + return &data.Repository.PullRequest, nil } diff --git a/submitqueue/extension/changeprovider/github/provider_test.go b/submitqueue/extension/changeprovider/github/provider_test.go index 9ad844931..0f703c74c 100644 --- a/submitqueue/extension/changeprovider/github/provider_test.go +++ b/submitqueue/extension/changeprovider/github/provider_test.go @@ -43,7 +43,9 @@ func newTestProvider(t *testing.T, serverURL string) changeprovider.ChangeProvid func servePR(t *testing.T, w http.ResponseWriter, data pullRequestData) { t.Helper() - var resp graphqlResponse + var resp struct { + Data pullRequestQueryData `json:"data"` + } resp.Data.Repository.PullRequest = data w.Header().Set("Content-Type", "application/json") require.NoError(t, json.NewEncoder(w).Encode(resp)) diff --git a/submitqueue/extension/validator/github/BUILD.bazel b/submitqueue/extension/validator/github/BUILD.bazel index aee86fcef..7bccc3e7d 100644 --- a/submitqueue/extension/validator/github/BUILD.bazel +++ b/submitqueue/extension/validator/github/BUILD.bazel @@ -11,7 +11,7 @@ go_library( visibility = ["//visibility:public"], deps = [ "//platform/base/change/github:go_default_library", - "//platform/http:go_default_library", + "//platform/githubapi:go_default_library", "//platform/metrics:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/validator:go_default_library", @@ -30,6 +30,7 @@ go_test( deps = [ "//platform/base/change:go_default_library", "//platform/base/change/github:go_default_library", + "//platform/githubapi:go_default_library", "//platform/http:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/validator:go_default_library", diff --git a/submitqueue/extension/validator/github/graphql.go b/submitqueue/extension/validator/github/graphql.go index 3067c3f3a..9c5f48dc4 100644 --- a/submitqueue/extension/validator/github/graphql.go +++ b/submitqueue/extension/validator/github/graphql.go @@ -16,12 +16,11 @@ package github import ( "context" - "encoding/json" "fmt" "net/http" entitygithub "github.com/uber/submitqueue/platform/base/change/github" - phttp "github.com/uber/submitqueue/platform/http" + "github.com/uber/submitqueue/platform/githubapi" ) // maxCheckContexts bounds the checks read; one past it still surfaces through @@ -64,23 +63,10 @@ query($owner: String!, $repo: String!, $number: Int!, $maxChecks: Int!) { } ` -type graphqlRequest struct { - Query string `json:"query"` - Variables map[string]any `json:"variables"` -} - -type graphqlResponse struct { - Data struct { - Repository struct { - PullRequest pullRequestNode `json:"pullRequest"` - } `json:"repository"` - } `json:"data"` - Errors []graphqlError `json:"errors"` -} - -type graphqlError struct { - Type string `json:"type"` - Message string `json:"message"` +type pullRequestStateData struct { + Repository struct { + PullRequest pullRequestNode `json:"pullRequest"` + } `json:"repository"` } type pullRequestNode struct { @@ -115,7 +101,7 @@ type checkContextNode struct { // fetchPullRequestState fails, rather than rejects, on a missing pull request: // the change provider resolved the URI before validators run. func fetchPullRequestState(ctx context.Context, client *http.Client, cid entitygithub.ChangeID) (pullRequestState, error) { - body, err := json.Marshal(graphqlRequest{ + req := githubapi.GraphQLRequest{ Query: pullRequestStateQuery, Variables: map[string]any{ "owner": cid.Org, @@ -123,38 +109,18 @@ func fetchPullRequestState(ctx context.Context, client *http.Client, cid entityg "number": cid.PRNumber, "maxChecks": maxCheckContexts, }, - }) - if err != nil { - return pullRequestState{}, fmt.Errorf("marshal GraphQL request: %w", err) } - - // The path is relative: the client's transport roots it at the API. - status, respBody, err := phttp.SendRequest(ctx, client, http.MethodPost, "/graphql", body, setGraphQLHeaders) - if err != nil { + var data pullRequestStateData + if err := githubapi.QueryGraphQL(ctx, client, req, &data); err != nil { return pullRequestState{}, err } - if status != http.StatusOK { - return pullRequestState{}, phttp.NewStatusError(status, respBody) - } - - var resp graphqlResponse - if err := json.Unmarshal(respBody, &resp); err != nil { - return pullRequestState{}, fmt.Errorf("decode GraphQL response: %w", err) - } - if len(resp.Errors) > 0 { - return pullRequestState{}, fmt.Errorf("GraphQL errors: %+v", resp.Errors) - } - pr := resp.Data.Repository.PullRequest + pr := data.Repository.PullRequest if pr.HeadRefOid == "" { return pullRequestState{}, fmt.Errorf("GraphQL response has no pull request") } return toPullRequestState(pr), nil } -func setGraphQLHeaders(req *http.Request) { - req.Header.Set("Content-Type", "application/json") -} - func toPullRequestState(pr pullRequestNode) pullRequestState { state := pullRequestState{ headSHA: pr.HeadRefOid, diff --git a/submitqueue/extension/validator/github/validator_test.go b/submitqueue/extension/validator/github/validator_test.go index af9814ac9..bb1fa3891 100644 --- a/submitqueue/extension/validator/github/validator_test.go +++ b/submitqueue/extension/validator/github/validator_test.go @@ -31,6 +31,7 @@ import ( "go.uber.org/zap/zaptest" "github.com/uber/submitqueue/platform/base/change" + "github.com/uber/submitqueue/platform/githubapi" phttp "github.com/uber/submitqueue/platform/http" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/validator" @@ -80,7 +81,7 @@ type fakeGitHub struct { func (f *fakeGitHub) ServeHTTP(w http.ResponseWriter, r *http.Request) { raw, _ := io.ReadAll(r.Body) - var req graphqlRequest + var req githubapi.GraphQLRequest _ = json.Unmarshal(raw, &req) f.mu.Lock() diff --git a/test/testutil/githubtestrepo/BUILD.bazel b/test/testutil/githubtestrepo/BUILD.bazel index 4b5c8303d..bf42bf46c 100644 --- a/test/testutil/githubtestrepo/BUILD.bazel +++ b/test/testutil/githubtestrepo/BUILD.bazel @@ -6,6 +6,7 @@ go_library( importpath = "github.com/uber/submitqueue/test/testutil/githubtestrepo", visibility = ["//visibility:public"], deps = [ + "//platform/githubapi:go_default_library", "//platform/http:go_default_library", "@com_github_stretchr_testify//require:go_default_library", "@org_golang_x_oauth2//:go_default_library", diff --git a/test/testutil/githubtestrepo/githubtestrepo.go b/test/testutil/githubtestrepo/githubtestrepo.go index 204e4e694..d51f19d2e 100644 --- a/test/testutil/githubtestrepo/githubtestrepo.go +++ b/test/testutil/githubtestrepo/githubtestrepo.go @@ -44,6 +44,7 @@ import ( "github.com/stretchr/testify/require" "golang.org/x/oauth2" + "github.com/uber/submitqueue/platform/githubapi" phttp "github.com/uber/submitqueue/platform/http" ) @@ -61,8 +62,6 @@ const ( Host = "github.com" // APIBaseURL is the REST API root for Host. APIBaseURL = "https://api.github.com" - - apiVersion = "2026-03-10" ) // TestRepo creates fixtures in the test repository and removes them when the @@ -344,11 +343,7 @@ func (r *TestRepo) send(method, path string, body, out any) (int, string) { payload, err = json.Marshal(body) require.NoError(r.t, err) } - code, resp, err := phttp.SendRequest(context.Background(), r.httpClient, method, path, payload, func(r *http.Request) { - r.Header.Set("Accept", "application/vnd.github+json") - r.Header.Set("Content-Type", "application/json") - r.Header.Set("X-GitHub-Api-Version", apiVersion) - }) + code, resp, err := phttp.SendRequest(context.Background(), r.httpClient, method, path, payload, githubapi.SetRESTHeaders) if err != nil { r.t.Logf("%s %s: %v", method, path, err) return 0, ""