diff --git a/platform/conduit/README.md b/platform/conduit/README.md index 2ce1aefc6..a3c47ee4f 100644 --- a/platform/conduit/README.md +++ b/platform/conduit/README.md @@ -45,3 +45,7 @@ Every error is wrapped with the method name. - **Per-method wrappers.** Each caller keeps its own typed request and result for the methods it calls, and decides what a missing object means for it. A method only one caller uses never lands in this package. - **Retries, logging and metrics.** Retry decisions belong to the consumer's error classifiers, and callers already log and record metrics in their own scope. + +## Who uses it + +- `submitqueue/extension/changeprovider/phabricator` reads diffs with `differential.querydiffs`; the orchestrator's wiring builds its client from the configured endpoint and token. diff --git a/service/submitqueue/orchestrator/server/BUILD.bazel b/service/submitqueue/orchestrator/server/BUILD.bazel index 20fa35e32..9f978fe11 100644 --- a/service/submitqueue/orchestrator/server/BUILD.bazel +++ b/service/submitqueue/orchestrator/server/BUILD.bazel @@ -20,6 +20,7 @@ go_library( "//api/base/hook:go_default_library", "//api/submitqueue/orchestrator/protopb:go_default_library", "//platform/buildkite:go_default_library", + "//platform/conduit:go_default_library", "//platform/errs:go_default_library", "//platform/errs/generic:go_default_library", "//platform/errs/git:go_default_library", diff --git a/service/submitqueue/orchestrator/server/profiles.go b/service/submitqueue/orchestrator/server/profiles.go index e2ad5b718..59637ab0a 100644 --- a/service/submitqueue/orchestrator/server/profiles.go +++ b/service/submitqueue/orchestrator/server/profiles.go @@ -29,6 +29,7 @@ import ( "context" platformbuildkite "github.com/uber/submitqueue/platform/buildkite" + "github.com/uber/submitqueue/platform/conduit" "github.com/uber/submitqueue/platform/extension/gitworkspace" gitworkspacelocal "github.com/uber/submitqueue/platform/extension/gitworkspace/local" gitrepo "github.com/uber/submitqueue/platform/git/repo" @@ -596,15 +597,13 @@ func (b *profileBuilder) newPhabChangeProvider(cfg phabProviderConfig, where str return nil, fmt.Errorf("%s: failed to build Phabricator HTTP client: %w", where, err) } client.Timeout = timeoutOr(cfg.Timeout, defaultHTTPTimeout) - - baseTransport := client.Transport.(*http.BaseURLTransport) - baseTransport.Next = &apiTokenTransport{token: token, next: baseTransport.Next} + conduitClient := conduit.NewClient(client, token) scope := b.scope.SubScope("changeprovider.phabricator") return func(c changeprovider.Config) changeprovider.ChangeProvider { return phabprovider.NewProvider(phabprovider.Params{ Config: c, - HTTPClient: client, + Conduit: conduitClient, Logger: b.logger.Sugar(), MetricsScope: scope, }) @@ -813,18 +812,3 @@ func (b *profileBuilder) githubClient(baseURL, timeout, token, where string) (*n } return client, nil } - -// apiTokenTransport injects a Phabricator API token as a query parameter in -// each request. -type apiTokenTransport struct { - token string - next nethttp.RoundTripper -} - -func (t *apiTokenTransport) RoundTrip(req *nethttp.Request) (*nethttp.Response, error) { - r := req.Clone(req.Context()) - q := r.URL.Query() - q.Set("api.token", t.token) - r.URL.RawQuery = q.Encode() - return t.next.RoundTrip(r) -} diff --git a/submitqueue/extension/changeprovider/phabricator/BUILD.bazel b/submitqueue/extension/changeprovider/phabricator/BUILD.bazel index 84ff2f62c..e91183b3d 100644 --- a/submitqueue/extension/changeprovider/phabricator/BUILD.bazel +++ b/submitqueue/extension/changeprovider/phabricator/BUILD.bazel @@ -3,15 +3,15 @@ load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", srcs = [ - "conduit.go", "convert.go", "provider.go", + "querydiffs.go", ], importpath = "github.com/uber/submitqueue/submitqueue/extension/changeprovider/phabricator", visibility = ["//visibility:public"], deps = [ "//platform/base/change/phabricator:go_default_library", - "//platform/http:go_default_library", + "//platform/conduit:go_default_library", "//platform/metrics:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/changeprovider:go_default_library", @@ -23,14 +23,15 @@ go_library( go_test( name = "go_default_test", srcs = [ - "conduit_test.go", "convert_test.go", "provider_test.go", + "querydiffs_test.go", ], embed = [":go_default_library"], deps = [ "//platform/base/change:go_default_library", "//platform/base/change/phabricator:go_default_library", + "//platform/conduit:go_default_library", "//platform/errs:go_default_library", "//platform/errs/http:go_default_library", "//platform/http:go_default_library", diff --git a/submitqueue/extension/changeprovider/phabricator/conduit.go b/submitqueue/extension/changeprovider/phabricator/conduit.go deleted file mode 100644 index 6b10cf1a7..000000000 --- a/submitqueue/extension/changeprovider/phabricator/conduit.go +++ /dev/null @@ -1,103 +0,0 @@ -package phabricator - -import ( - "context" - "encoding/json" - "fmt" - "io" - "net/http" - "net/url" - "strconv" - - phttp "github.com/uber/submitqueue/platform/http" -) - -// conduitResponse stores a response from the Conduit API. Every Conduit endpoint wraps its -// payload in this shape; a non-nil ErrorCode signals a server-side failure. -type conduitResponse struct { - Result json.RawMessage `json:"result"` - ErrorCode *string `json:"error_code"` - ErrorInfo *string `json:"error_info"` -} - -// diffResult represents a single diff from the differential.querydiffs response. -type diffResult struct { - Changes []fileChange `json:"changes"` - AuthorName string `json:"authorName"` - AuthorEmail string `json:"authorEmail"` -} - -// fileChange represents a single file modification in a Phabricator diff. -type fileChange struct { - CurrentPath string `json:"currentPath"` - AddLines string `json:"addLines"` - DelLines string `json:"delLines"` -} - -// buildQueryDiffsRequest builds query parameters for a differential.querydiffs call. -func buildQueryDiffsRequest(diffIDs []int) url.Values { - form := url.Values{} - for _, id := range diffIDs { - form.Add("ids[]", strconv.Itoa(id)) - } - return form -} - -// doConduitRequest executes a Conduit HTTP request. -// The path is relative — transport layers on the client resolve it to the full URL. -// Authentication (e.g. Bearer token) is handled by the client's transport. -func doConduitRequest(ctx context.Context, httpClient *http.Client, form url.Values) (*http.Response, error) { - req, err := http.NewRequestWithContext(ctx, http.MethodPost, "/api/differential.querydiffs", nil) - if err != nil { - return nil, fmt.Errorf("failed to create HTTP request: %w", err) - } - req.URL.RawQuery = form.Encode() - - resp, err := httpClient.Do(req) - if err != nil { - return nil, fmt.Errorf("HTTP request failed: %w", err) - } - - return resp, nil -} - -// parseConduitResponse reads the HTTP response, unwraps the Conduit response, -// and extracts the diffResults for the requested diff IDs. -func parseConduitResponse(resp *http.Response, diffIDs []int) (map[int]*diffResult, error) { - if resp.StatusCode != http.StatusOK { - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, fmt.Errorf("Conduit API returned status %d and failed to read body: %w", resp.StatusCode, err) - } - return nil, fmt.Errorf("Conduit API request failed: %w", phttp.NewStatusError(resp.StatusCode, body)) - } - - var phabResponse conduitResponse - if err := json.NewDecoder(resp.Body).Decode(&phabResponse); err != nil { - return nil, fmt.Errorf("failed to decode Conduit response: %w", err) - } - - if phabResponse.ErrorCode != nil { - info := "" - if phabResponse.ErrorInfo != nil { - info = *phabResponse.ErrorInfo - } - return nil, fmt.Errorf("Conduit error %s: %s", *phabResponse.ErrorCode, info) - } - - var rawMap map[string]*diffResult - if err := json.Unmarshal(phabResponse.Result, &rawMap); err != nil { - return nil, fmt.Errorf("failed to decode querydiffs result: %w", err) - } - - results := make(map[int]*diffResult, len(diffIDs)) - for _, id := range diffIDs { - diff, ok := rawMap[strconv.Itoa(id)] - if !ok { - return nil, fmt.Errorf("diff %d not found in querydiffs response", id) - } - results[id] = diff - } - - return results, nil -} diff --git a/submitqueue/extension/changeprovider/phabricator/conduit_test.go b/submitqueue/extension/changeprovider/phabricator/conduit_test.go deleted file mode 100644 index bfcdae044..000000000 --- a/submitqueue/extension/changeprovider/phabricator/conduit_test.go +++ /dev/null @@ -1,224 +0,0 @@ -package phabricator - -import ( - "encoding/json" - "fmt" - "io" - "net/http" - "net/http/httptest" - "strings" - "testing" - "testing/iotest" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestBuildQueryDiffsRequest(t *testing.T) { - testCases := []struct { - name string - diffIDs []int - wantAll []string - }{ - { - name: "single ID", - diffIDs: []int{42}, - wantAll: []string{"ids%5B%5D=42"}, - }, - { - name: "multiple IDs", - diffIDs: []int{42, 43, 44}, - wantAll: []string{"ids%5B%5D=42", "ids%5B%5D=43", "ids%5B%5D=44"}, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - form := buildQueryDiffsRequest(tc.diffIDs) - encoded := form.Encode() - for _, want := range tc.wantAll { - assert.Contains(t, encoded, want) - } - }) - } -} - -func TestDoConduitRequest(t *testing.T) { - var capturedPath string - var capturedQuery string - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - capturedPath = r.URL.Path - capturedQuery = r.URL.RawQuery - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`{"result":{}}`)) - })) - defer server.Close() - - client := &http.Client{Transport: &testTransport{baseURL: server.URL}} - form := buildQueryDiffsRequest([]int{100, 200}) - - resp, err := doConduitRequest(t.Context(), client, form) - require.NoError(t, err) - defer resp.Body.Close() - - assert.Equal(t, "/api/differential.querydiffs", capturedPath) - assert.Contains(t, capturedQuery, "ids%5B%5D=100") - assert.Contains(t, capturedQuery, "ids%5B%5D=200") -} - -func TestDoConduitRequest_ConnectionError(t *testing.T) { - client := &http.Client{Transport: &testTransport{baseURL: "http://127.0.0.1:0"}} - form := buildQueryDiffsRequest([]int{1}) - - _, err := doConduitRequest(t.Context(), client, form) - - require.Error(t, err) - assert.Contains(t, err.Error(), "HTTP request failed") -} - -func TestParseConduitResponse(t *testing.T) { - validResult := map[string]*diffResult{ - "100": {AuthorName: "Alice", Changes: []fileChange{{CurrentPath: "a.go"}}}, - "101": {AuthorName: "Bob", Changes: []fileChange{{CurrentPath: "b.go"}}}, - } - - testCases := []struct { - name string - resp *http.Response - diffIDs []int - wantErr string - }{ - { - name: "success", - resp: newConduitHTTPResponse(t, validResult, http.StatusOK), - diffIDs: []int{100, 101}, - }, - { - name: "HTTP error", - resp: newPlainHTTPResponse("server error", http.StatusInternalServerError), - diffIDs: []int{100}, - wantErr: "unexpected status 500", - }, - { - name: "HTTP error with unreadable body", - resp: &http.Response{ - StatusCode: http.StatusBadGateway, - Body: io.NopCloser(&errReader{}), - }, - diffIDs: []int{100}, - wantErr: "failed to read body", - }, - { - name: "conduit error", - resp: newConduitErrorHTTPResponse(t, "ERR-CONDUIT-CORE", "Invalid diff ID"), - diffIDs: []int{100}, - wantErr: "Conduit error", - }, - { - name: "diff not found in response", - resp: newConduitHTTPResponse(t, map[string]*diffResult{"999": {}}, http.StatusOK), - diffIDs: []int{100}, - wantErr: "diff 100 not found", - }, - { - name: "malformed JSON", - resp: newPlainHTTPResponse("{not json", http.StatusOK), - diffIDs: []int{100}, - wantErr: "failed to decode Conduit response", - }, - { - name: "malformed result payload", - resp: newPlainHTTPResponse(`{"result":"not a map"}`, http.StatusOK), - diffIDs: []int{100}, - wantErr: "failed to decode querydiffs result", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - defer tc.resp.Body.Close() - results, err := parseConduitResponse(tc.resp, tc.diffIDs) - - if tc.wantErr != "" { - require.Error(t, err) - assert.Contains(t, err.Error(), tc.wantErr) - return - } - require.NoError(t, err) - assert.Len(t, results, len(tc.diffIDs)) - }) - } -} - -func TestParseConduitResponse_BodyReadError(t *testing.T) { - resp := &http.Response{ - StatusCode: http.StatusBadGateway, - Body: io.NopCloser(iotest.ErrReader(io.ErrUnexpectedEOF)), - } - t.Cleanup(func() { _ = resp.Body.Close() }) - - _, err := parseConduitResponse(resp, []int{100}) - - require.ErrorIs(t, err, io.ErrUnexpectedEOF) -} - -// newConduitHTTPResponse builds an http.Response with a successful Conduit envelope. -func newConduitHTTPResponse(t *testing.T, result any, statusCode int) *http.Response { - t.Helper() - resultBytes, err := json.Marshal(result) - require.NoError(t, err) - envelope := conduitResponse{Result: resultBytes} - body, err := json.Marshal(envelope) - require.NoError(t, err) - return &http.Response{ - StatusCode: statusCode, - Body: io.NopCloser(strings.NewReader(string(body))), - } -} - -// newConduitErrorHTTPResponse builds an http.Response with a Conduit error envelope. -func newConduitErrorHTTPResponse(t *testing.T, code, info string) *http.Response { - t.Helper() - envelope := conduitResponse{ErrorCode: &code, ErrorInfo: &info} - body, err := json.Marshal(envelope) - require.NoError(t, err) - return &http.Response{ - StatusCode: http.StatusOK, - Body: io.NopCloser(strings.NewReader(string(body))), - } -} - -// newPlainHTTPResponse builds an http.Response with a plain string body. -func newPlainHTTPResponse(body string, statusCode int) *http.Response { - return &http.Response{ - StatusCode: statusCode, - Body: io.NopCloser(strings.NewReader(body)), - } -} - -// serveConduit writes a successful Conduit response envelope to an HTTP handler. -func serveConduit(t *testing.T, w http.ResponseWriter, result any) { - t.Helper() - resultBytes, err := json.Marshal(result) - require.NoError(t, err) - resp := conduitResponse{Result: resultBytes} - w.Header().Set("Content-Type", "application/json") - require.NoError(t, json.NewEncoder(w).Encode(resp)) -} - -// testTransport rewrites request URLs to point at the test server. -type testTransport struct { - baseURL string -} - -func (t *testTransport) RoundTrip(req *http.Request) (*http.Response, error) { - req.URL.Scheme = "http" - req.URL.Host = t.baseURL[len("http://"):] - return http.DefaultTransport.RoundTrip(req) -} - -type errReader struct{} - -func (e *errReader) Read([]byte) (int, error) { - return 0, fmt.Errorf("simulated read error") -} diff --git a/submitqueue/extension/changeprovider/phabricator/provider.go b/submitqueue/extension/changeprovider/phabricator/provider.go index cbfa8963c..252cc0511 100644 --- a/submitqueue/extension/changeprovider/phabricator/provider.go +++ b/submitqueue/extension/changeprovider/phabricator/provider.go @@ -3,12 +3,12 @@ package phabricator import ( "context" "fmt" - "net/http" "github.com/uber-go/tally" "go.uber.org/zap" changephab "github.com/uber/submitqueue/platform/base/change/phabricator" + "github.com/uber/submitqueue/platform/conduit" coremetrics "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/changeprovider" @@ -23,10 +23,8 @@ type Params struct { // Config is the per-queue identity handed to the Factory that built // this provider. Config changeprovider.Config - // HTTPClient is a pre-configured HTTP client. The caller is responsible for - // configuring the base URL (e.g. via httpclient.NewClient) and authentication - // (e.g. via a RoundTripper that injects credentials) via transport layers. - HTTPClient *http.Client + // Conduit calls the Phabricator instance the queue's diffs live on. + Conduit *conduit.Client // Logger is the structured logger. Logger *zap.SugaredLogger // MetricsScope is the metrics scope for instrumentation. @@ -36,7 +34,7 @@ type Params struct { type provider struct { // cfg is the per-queue identity this provider was built for. cfg changeprovider.Config - httpClient *http.Client + conduit *conduit.Client logger *zap.SugaredLogger metricsScope tally.Scope } @@ -45,7 +43,7 @@ type provider struct { func NewProvider(params Params) changeprovider.ChangeProvider { return &provider{ cfg: params.Config, - httpClient: params.HTTPClient, + conduit: params.Conduit, logger: params.Logger.Named("phabricator_changeprovider"), metricsScope: params.MetricsScope.SubScope("phabricator_changeprovider"), } @@ -113,7 +111,7 @@ func (p *provider) fetchAllDiffs(ctx context.Context, changeIDs []changephab.Cha end = len(diffIDs) } - batch, err := p.fetchDiffBatch(ctx, diffIDs[start:end]) + batch, err := queryDiffs(ctx, p.conduit, diffIDs[start:end]) if err != nil { return nil, fmt.Errorf("failed to fetch diffs: %w", err) } @@ -124,16 +122,3 @@ func (p *provider) fetchAllDiffs(ctx context.Context, changeIDs []changephab.Cha return results, nil } - -// fetchDiffBatch fetches a single batch of diffs from Phabricator. -func (p *provider) fetchDiffBatch(ctx context.Context, diffIDs []int) (map[int]*diffResult, error) { - form := buildQueryDiffsRequest(diffIDs) - - resp, err := doConduitRequest(ctx, p.httpClient, form) - if err != nil { - return nil, err - } - defer resp.Body.Close() - - return parseConduitResponse(resp, diffIDs) -} diff --git a/submitqueue/extension/changeprovider/phabricator/provider_test.go b/submitqueue/extension/changeprovider/phabricator/provider_test.go index eebafe207..aa5d5c9b9 100644 --- a/submitqueue/extension/changeprovider/phabricator/provider_test.go +++ b/submitqueue/extension/changeprovider/phabricator/provider_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "strconv" "testing" "github.com/stretchr/testify/assert" @@ -14,6 +15,7 @@ import ( "go.uber.org/zap/zaptest" "github.com/uber/submitqueue/platform/base/change" + "github.com/uber/submitqueue/platform/conduit" "github.com/uber/submitqueue/platform/errs" httperrs "github.com/uber/submitqueue/platform/errs/http" phttp "github.com/uber/submitqueue/platform/http" @@ -21,10 +23,12 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/changeprovider" ) -func newTestProvider(t *testing.T, client *http.Client) changeprovider.ChangeProvider { +func newTestProvider(t *testing.T, serverURL string) changeprovider.ChangeProvider { t.Helper() + httpClient, err := phttp.NewClient(serverURL) + require.NoError(t, err) return NewProvider(Params{ - HTTPClient: client, + Conduit: conduit.NewClient(httpClient, "test-token"), Logger: zaptest.NewLogger(t).Sugar(), MetricsScope: tally.NoopScope, }) @@ -62,6 +66,14 @@ func TestProvider_Get(t *testing.T) { uris: []string{"invalid://uri"}, wantErr: "failed to parse Phabricator change ID", }, + { + name: "rejected token returns error", + handler: func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"result":null,"error_code":"ERR-INVALID-SESSION","error_info":"Session key is not present."}`)) + }, + uris: []string{"phab://phab.example.com/D200/100"}, + wantErr: "ERR-INVALID-SESSION", + }, { name: "HTTP error returns error", handler: func(w http.ResponseWriter, r *http.Request) { @@ -74,16 +86,14 @@ func TestProvider_Get(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - var client *http.Client + serverURL := "http://localhost" if tc.handler != nil { server := httptest.NewServer(tc.handler) defer server.Close() - client = &http.Client{Transport: &testTransport{baseURL: server.URL}} - } else { - client = &http.Client{Transport: &testTransport{baseURL: "http://localhost"}} + serverURL = server.URL } - p := newTestProvider(t, client) + p := newTestProvider(t, serverURL) infos, err := p.Get(context.Background(), entity.Request{Change: change.Change{URIs: tc.uris}}) if tc.wantErr != "" { @@ -119,8 +129,7 @@ func TestProvider_Get_MultipleDiffs(t *testing.T) { })) defer server.Close() - client := &http.Client{Transport: &testTransport{baseURL: server.URL}} - p := newTestProvider(t, client) + p := newTestProvider(t, server.URL) infos, err := p.Get(context.Background(), entity.Request{Change: change.Change{ URIs: []string{"phab://phab.example.com/D200/100", "phab://phab.example.com/D201/101"}, }}) @@ -133,8 +142,7 @@ func TestProvider_Get_MultipleDiffs(t *testing.T) { } func TestProvider_Get_ConnectionError(t *testing.T) { - client := &http.Client{Transport: &testTransport{baseURL: "http://127.0.0.1:0"}} - p := newTestProvider(t, client) + p := newTestProvider(t, "http://127.0.0.1:0") _, err := p.Get(context.Background(), entity.Request{Change: change.Change{ URIs: []string{"phab://phab.example.com/D200/100"}, }}) @@ -163,8 +171,7 @@ func TestProvider_Get_HTTPStatusError(t *testing.T) { })) t.Cleanup(server.Close) - client := &http.Client{Transport: &testTransport{baseURL: server.URL}} - p := newTestProvider(t, client) + p := newTestProvider(t, server.URL) _, err := p.Get(t.Context(), entity.Request{Change: change.Change{ URIs: []string{"phab://phab.example.com/D200/100"}, }}) @@ -193,15 +200,11 @@ func TestProvider_Get_FileDetails(t *testing.T) { }, }, } - resultBytes, _ := json.Marshal(result) - resp := conduitResponse{Result: resultBytes} - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(resp) + serveConduit(t, w, result) })) defer server.Close() - client := &http.Client{Transport: &testTransport{baseURL: server.URL}} - p := newTestProvider(t, client) + p := newTestProvider(t, server.URL) infos, err := p.Get(context.Background(), entity.Request{Change: change.Change{ URIs: []string{"phab://phab.example.com/D200/100"}, }}) @@ -232,8 +235,7 @@ func TestProvider_Get_Batching(t *testing.T) { callCount := 0 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { callCount++ - query := r.URL.Query() - ids := query["ids[]"] + ids := requestedDiffIDs(t, r) result := make(map[string]*diffResult, len(ids)) for _, id := range ids { result[id] = &diffResult{ @@ -252,8 +254,7 @@ func TestProvider_Get_Batching(t *testing.T) { uris[i] = fmt.Sprintf("phab://phab.example.com/D%d/%d", revisionID, diffID) } - client := &http.Client{Transport: &testTransport{baseURL: server.URL}} - p := newTestProvider(t, client) + p := newTestProvider(t, server.URL) infos, err := p.Get(context.Background(), entity.Request{Change: change.Change{URIs: uris}}) require.NoError(t, err) @@ -269,8 +270,7 @@ func TestProvider_Get_BatchingStopsOnError(t *testing.T) { w.WriteHeader(http.StatusInternalServerError) return } - query := r.URL.Query() - ids := query["ids[]"] + ids := requestedDiffIDs(t, r) result := make(map[string]*diffResult, len(ids)) for _, id := range ids { result[id] = &diffResult{ @@ -288,10 +288,33 @@ func TestProvider_Get_BatchingStopsOnError(t *testing.T) { uris[i] = fmt.Sprintf("phab://phab.example.com/D%d/%d", revisionID, diffID) } - client := &http.Client{Transport: &testTransport{baseURL: server.URL}} - p := newTestProvider(t, client) + p := newTestProvider(t, server.URL) _, err := p.Get(context.Background(), entity.Request{Change: change.Change{URIs: uris}}) require.Error(t, err) assert.Equal(t, 2, callCount) } + +// serveConduit writes a successful Conduit response envelope. +func serveConduit(t *testing.T, w http.ResponseWriter, result any) { + t.Helper() + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode(map[string]any{"result": result, "error_code": nil, "error_info": nil})) +} + +// requestedDiffIDs returns the diff IDs a querydiffs call asked for, as the +// string keys Conduit answers with. +func requestedDiffIDs(t *testing.T, r *http.Request) []string { + t.Helper() + var params struct { + IDs []int `json:"ids"` + } + if !assert.NoError(t, r.ParseForm()) || !assert.NoError(t, json.Unmarshal([]byte(r.PostForm.Get("params")), ¶ms)) { + return nil + } + ids := make([]string, len(params.IDs)) + for i, id := range params.IDs { + ids[i] = strconv.Itoa(id) + } + return ids +} diff --git a/submitqueue/extension/changeprovider/phabricator/querydiffs.go b/submitqueue/extension/changeprovider/phabricator/querydiffs.go new file mode 100644 index 000000000..11fce8ec2 --- /dev/null +++ b/submitqueue/extension/changeprovider/phabricator/querydiffs.go @@ -0,0 +1,44 @@ +package phabricator + +import ( + "context" + "fmt" + "strconv" + + "github.com/uber/submitqueue/platform/conduit" +) + +const queryDiffsMethod = "differential.querydiffs" + +// diffResult represents a single diff from the differential.querydiffs response. +type diffResult struct { + Changes []fileChange `json:"changes"` + AuthorName string `json:"authorName"` + AuthorEmail string `json:"authorEmail"` +} + +// fileChange represents a single file modification in a Phabricator diff. +type fileChange struct { + CurrentPath string `json:"currentPath"` + AddLines string `json:"addLines"` + DelLines string `json:"delLines"` +} + +// queryDiffs reads diffIDs with one differential.querydiffs call and fails if +// Conduit omits any of them. +func queryDiffs(ctx context.Context, client *conduit.Client, diffIDs []int) (map[int]*diffResult, error) { + var byID map[string]*diffResult + if err := client.Call(ctx, queryDiffsMethod, map[string][]int{"ids": diffIDs}, &byID); err != nil { + return nil, err + } + + results := make(map[int]*diffResult, len(diffIDs)) + for _, id := range diffIDs { + diff, ok := byID[strconv.Itoa(id)] + if !ok { + return nil, fmt.Errorf("diff %d not found in querydiffs response", id) + } + results[id] = diff + } + return results, nil +} diff --git a/submitqueue/extension/changeprovider/phabricator/querydiffs_test.go b/submitqueue/extension/changeprovider/phabricator/querydiffs_test.go new file mode 100644 index 000000000..405f12bb1 --- /dev/null +++ b/submitqueue/extension/changeprovider/phabricator/querydiffs_test.go @@ -0,0 +1,90 @@ +package phabricator + +import ( + "errors" + "net/http" + "net/http/httptest" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/uber/submitqueue/platform/conduit" + phttp "github.com/uber/submitqueue/platform/http" +) + +func TestQueryDiffs(t *testing.T) { + testCases := []struct { + name string + diffIDs []int + reply string + want map[int]*diffResult + wantErr bool + wantErrorCode string + }{ + { + name: "returns every requested diff", + diffIDs: []int{100, 101}, + reply: `{"result":{"100":{"authorName":"Alice","changes":[{"currentPath":"a.go"}]},"101":{"authorName":"Bob","changes":[{"currentPath":"b.go"}]}}}`, + want: map[int]*diffResult{ + 100: {AuthorName: "Alice", Changes: []fileChange{{CurrentPath: "a.go"}}}, + 101: {AuthorName: "Bob", Changes: []fileChange{{CurrentPath: "b.go"}}}, + }, + }, + { + name: "diff missing from the result", + diffIDs: []int{100}, + reply: `{"result":{"999":{}}}`, + wantErr: true, + }, + { + name: "empty result", + diffIDs: []int{100}, + reply: `{"result":[]}`, + wantErr: true, + }, + { + name: "conduit error", + diffIDs: []int{100}, + reply: `{"result":null,"error_code":"ERR-CONDUIT-CORE","error_info":"Invalid diff ID"}`, + wantErr: true, + wantErrorCode: "ERR-CONDUIT-CORE", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + type request struct { + path string + ids []string + } + requests := make(chan request, 1) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests <- request{path: r.URL.Path, ids: requestedDiffIDs(t, r)} + _, _ = w.Write([]byte(tc.reply)) + })) + t.Cleanup(server.Close) + httpClient, err := phttp.NewClient(server.URL) + require.NoError(t, err) + + got, err := queryDiffs(t.Context(), conduit.NewClient(httpClient, "test-token"), tc.diffIDs) + + received := <-requests + assert.Equal(t, "/api/differential.querydiffs", received.path) + wantIDs := make([]string, len(tc.diffIDs)) + for i, id := range tc.diffIDs { + wantIDs[i] = strconv.Itoa(id) + } + assert.Equal(t, wantIDs, received.ids) + if tc.wantErr { + require.Error(t, err) + var conduitErr *conduit.Error + assert.Equal(t, tc.wantErrorCode != "", errors.As(err, &conduitErr)) + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +}