refactor(changeprovider): Read Phabricator diffs through platform/conduit

platform/README.md

@@ -19,3 +19,11 @@ Cross-domain packages shared by SubmitQueue, Stovepipe, and other services in th
 - **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/`.
 
 Domain-scoped infrastructure and extensions stay under each domain (for example `submitqueue/core/`, `submitqueue/extension/`).
+
+## Calling a vendor API
+
+GitHub and Phabricator callers split the work across the same three layers:
+
+- **Protocol**: `githubapi/` and `conduit/` hold what every caller of the API would otherwise repeat, which is request encoding and the response envelope with its errors. They have no endpoint methods.
+- **Endpoints**: each extension keeps the calls it makes, with its own typed requests and results, and takes the authenticated connection as a dependency. That dependency is the smallest type that can carry the API's auth: an `*http.Client` for GitHub, whose token rides in a transport, and a `*conduit.Client` for Phabricator, whose token has to be inside the request body.
+- **Wiring**: the service's server reads the credentials, builds each connection once with a timeout, and hands it to the extensions that use it, as `githubClient` and `conduitClient` do in the orchestrator's `profiles.go`.

platform/conduit/README.md

@@ -47,3 +47,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.

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",

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"
@@ -585,26 +586,21 @@ func (b *profileBuilder) newGitHubChangeProvider(cfg githubProviderConfig, where
 
 // newPhabChangeProvider returns a function building the Phabricator change
 // provider for a given queue, or nil when its token is unset. As with GitHub,
-// the HTTP client is built once here.
+// the Conduit client is built once here.
 func (b *profileBuilder) newPhabChangeProvider(cfg phabProviderConfig, where string) (func(changeprovider.Config) changeprovider.ChangeProvider, error) {
 	token, ok := tokenFrom(cfg.TokenEnv)
 	if !ok {
 		return nil, nil
 	}
-	client, err := http.NewClient(cfg.Endpoint)
+	conduitClient, err := b.conduitClient(cfg.Endpoint, cfg.Timeout, token, where)
 	if err != nil {
-		return nil, fmt.Errorf("%s: failed to build Phabricator HTTP client: %w", where, err)
+		return nil, err
 	}
-	client.Timeout = timeoutOr(cfg.Timeout, defaultHTTPTimeout)
-
-	baseTransport := client.Transport.(*http.BaseURLTransport)
-	baseTransport.Next = &apiTokenTransport{token: token, next: baseTransport.Next}
-
 	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,
 		})
@@ -814,17 +810,14 @@ 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)
+// conduitClient builds a Conduit client rooted at a Phabricator host. It is the
+// counterpart of githubClient: Conduit reads its token from the request body,
+// so the token goes to the client rather than to a transport.
+func (b *profileBuilder) conduitClient(endpoint, timeout, token, where string) (*conduit.Client, error) {
+	client, err := http.NewClient(endpoint)
+	if err != nil {
+		return nil, fmt.Errorf("%s: failed to build Phabricator HTTP client: %w", where, err)
+	}
+	client.Timeout = timeoutOr(timeout, defaultHTTPTimeout)
+	return conduit.NewClient(client, token), nil
 }

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",