Adding Phabricator ChangeProvider

submitqueue/extension/changeprovider/phabricator/BUILD.bazel

@@ -0,0 +1,42 @@
+load("@rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+    name = "phabricator",
+    srcs = [
+        "conduit.go",
+        "convert.go",
+        "provider.go",
+        "validate.go",
+    ],
+    importpath = "github.com/uber/submitqueue/submitqueue/extension/changeprovider/phabricator",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//core/metrics",
+        "//entity/change/phabricator",
+        "//submitqueue/entity",
+        "//submitqueue/extension/changeprovider",
+        "@com_github_uber_go_tally//:tally",
+        "@org_uber_go_zap//:zap",
+    ],
+)
+
+go_test(
+    name = "phabricator_test",
+    srcs = [
+        "conduit_test.go",
+        "convert_test.go",
+        "provider_test.go",
+        "validate_test.go",
+    ],
+    embed = [":phabricator"],
+    deps = [
+        "//entity/change",
+        "//entity/change/phabricator",
+        "//submitqueue/entity",
+        "//submitqueue/extension/changeprovider",
+        "@com_github_stretchr_testify//assert",
+        "@com_github_stretchr_testify//require",
+        "@com_github_uber_go_tally//:tally",
+        "@org_uber_go_zap//zaptest",
+    ],
+)

submitqueue/extension/changeprovider/phabricator/conduit.go

@@ -0,0 +1,101 @@
+package phabricator
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"io"
+	"net/http"
+	"net/url"
+	"strconv"
+)
+
+// 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, apiToken string) url.Values {
+	form := url.Values{}
+	for _, id := range diffIDs {
+		form.Add("ids[]", strconv.Itoa(id))
+	}
+	if apiToken != "" {
+		form.Set("api.token", apiToken)
+	}
+	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, _ := io.ReadAll(resp.Body)
+		return nil, fmt.Errorf("Conduit API returned status %d: %s", resp.StatusCode, string(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
+}

submitqueue/extension/changeprovider/phabricator/conduit_test.go

@@ -0,0 +1,202 @@
+package phabricator
+
+import (
+	"encoding/json"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestBuildQueryDiffsRequest(t *testing.T) {
+	testCases := []struct {
+		name     string
+		diffIDs  []int
+		apiToken string
+		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"},
+		},
+		{
+			name:     "includes api.token when set",
+			diffIDs:  []int{42},
+			apiToken: "my-secret",
+			wantAll:  []string{"ids%5B%5D=42", "api.token=my-secret"},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			form := buildQueryDiffsRequest(tc.diffIDs, tc.apiToken)
+			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: "Conduit API returned status 500",
+		},
+		{
+			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))
+		})
+	}
+}
+
+// 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)
+}

submitqueue/extension/changeprovider/phabricator/convert.go

@@ -0,0 +1,41 @@
+package phabricator
+
+import (
+	"strconv"
+
+	entityphab "github.com/uber/submitqueue/entity/change/phabricator"
+	"github.com/uber/submitqueue/submitqueue/entity"
+)
+
+// convertToChangeInfo converts a Phabricator diff result to an entity.ChangeInfo.
+func convertToChangeInfo(parsed entityphab.ChangeID, diff *diffResult) entity.ChangeInfo {
+	return entity.ChangeInfo{
+		URI: parsed.String(),
+		Details: entity.ChangeDetails{
+			Author: entity.Author{
+				Name:  diff.AuthorName,
+				Email: diff.AuthorEmail,
+			},
+			ChangedFiles: convertFiles(diff.Changes),
+		},
+	}
+}
+
+// convertFiles converts Phabricator file changes to entity.ChangedFile structs.
+// Phabricator reports addLines and delLines as strings; parsing failures default
+// to zero. Unlike GitHub, Phabricator reports both additions and deletions, so
+// LinesModified is set to their sum.
+func convertFiles(changes []fileChange) []entity.ChangedFile {
+	files := make([]entity.ChangedFile, 0, len(changes))
+	for _, c := range changes {
+		added, _ := strconv.Atoi(c.AddLines)
+		deleted, _ := strconv.Atoi(c.DelLines)
+		files = append(files, entity.ChangedFile{
+			Path:          c.CurrentPath,
+			LinesAdded:    added,
+			LinesDeleted:  deleted,
+			LinesModified: added + deleted,
+		})
+	}
+	return files
+}