refactor(httpx): hide the real APIClient (#648)

As mentioned in https://github.com/ooni/probe/issues/1951, one of
the main issues I did see with httpx.APIClient is that in some cases
it's used in a very fragile way by probeservices.Client.

This happens in psiphon.go and tor.go, where we create a copy of
the APIClient and then modify it's Authorization field.

If we ever refactor probeservices.Client to take a pointer to
httpx.Client, we are now mutating the httpx.Client.

Of course, we don't want that to happen.

This diff attempts to address such a problem as follows:

1. we create a new APIClientTemplate type that holds the same
fields of an APIClient and allows to build an APIClient

2. we modify every user of APIClient to use APIClientTemplate

3. when we need an APIClient, we build it from the corresponding
template and, when we need to use a specific Authorization, we
use a build factory that sets APIClient.Authorization

4. we hide APIClient by renaming it apiClient and by defining
an interface called APIClient that allows to use it

So, now the codebase always uses the opaque APIClient interface to
issue API calls and always uses the APIClientTemplate to build an
opaque APIClient.

Boom! We have separated construction from usage and we are not
mutating in weird ways the APIClient anymore.

internal/engine/probeservices/bouncer.go

@@ -9,6 +9,6 @@ import (
 // GetTestHelpers is like GetCollectors but for test helpers.
 func (c Client) GetTestHelpers(
 	ctx context.Context) (output map[string][]model.OOAPIService, err error) {
-	err = c.APIClient.GetJSON(ctx, "/api/v1/test-helpers", &output)
+	err = c.APIClientTemplate.Build().GetJSON(ctx, "/api/v1/test-helpers", &output)
 	return
 }

internal/engine/probeservices/checkin.go

@@ -16,7 +16,7 @@ type checkInResult struct {
 // Returns the list of tests to run and the URLs, on success, or an explanatory error, in case of failure.
 func (c Client) CheckIn(ctx context.Context, config model.OOAPICheckInConfig) (*model.OOAPICheckInInfo, error) {
 	var response checkInResult
-	if err := c.APIClient.PostJSON(ctx, "/api/v1/check-in", config, &response); err != nil {
+	if err := c.APIClientTemplate.Build().PostJSON(ctx, "/api/v1/check-in", config, &response); err != nil {
 		return nil, err
 	}
 	return &response.Tests, nil

internal/engine/probeservices/checkreportid.go

@@ -16,12 +16,12 @@ func (c Client) CheckReportID(ctx context.Context, reportID string) (bool, error
 	query := url.Values{}
 	query.Add("report_id", reportID)
 	var response checkReportIDResponse
-	err := (&httpx.APIClient{
+	err := (&httpx.APIClientTemplate{
 		BaseURL:    c.BaseURL,
 		HTTPClient: c.HTTPClient,
 		Logger:     c.Logger,
 		UserAgent:  c.UserAgent,
-	}).GetJSONWithQuery(ctx, "/api/_/check_report_id", query, &response)
+	}).Build().GetJSONWithQuery(ctx, "/api/_/check_report_id", query, &response)
 	if err != nil {
 		return false, err
 	}

internal/engine/probeservices/checkreportid_test.go

@@ -15,7 +15,7 @@ import (
 
 func TestCheckReportIDWorkingAsIntended(t *testing.T) {
 	client := probeservices.Client{
-		APIClient: httpx.APIClient{
+		APIClientTemplate: httpx.APIClientTemplate{
 			BaseURL:    "https://ams-pg.ooni.org/",
 			HTTPClient: http.DefaultClient,
 			Logger:     log.Log,
@@ -38,7 +38,7 @@ func TestCheckReportIDWorkingAsIntended(t *testing.T) {
 
 func TestCheckReportIDWorkingWithCancelledContext(t *testing.T) {
 	client := probeservices.Client{
-		APIClient: httpx.APIClient{
+		APIClientTemplate: httpx.APIClientTemplate{
 			BaseURL:    "https://ams-pg.ooni.org/",
 			HTTPClient: http.DefaultClient,
 			Logger:     log.Log,

internal/engine/probeservices/collector.go

@@ -105,7 +105,7 @@ func (c Client) OpenReport(ctx context.Context, rt ReportTemplate) (ReportChanne
 		return nil, ErrUnsupportedFormat
 	}
 	var cor collectorOpenResponse
-	if err := c.APIClient.PostJSON(ctx, "/report", rt, &cor); err != nil {
+	if err := c.APIClientTemplate.Build().PostJSON(ctx, "/report", rt, &cor); err != nil {
 		return nil, err
 	}
 	for _, format := range cor.SupportedFormats {
@@ -144,7 +144,7 @@ func (r reportChan) CanSubmit(m *model.Measurement) bool {
 func (r reportChan) SubmitMeasurement(ctx context.Context, m *model.Measurement) error {
 	var updateResponse collectorUpdateResponse
 	m.ReportID = r.ID
-	err := r.client.APIClient.PostJSON(
+	err := r.client.APIClientTemplate.Build().PostJSON(
 		ctx, fmt.Sprintf("/report/%s", r.ID), collectorUpdateRequest{
 			Format:  "json",
 			Content: m,

internal/engine/probeservices/login.go

@@ -29,7 +29,8 @@ func (c Client) MaybeLogin(ctx context.Context) error {
 	}
 	c.LoginCalls.Add(1)
 	var auth LoginAuth
-	if err := c.APIClient.PostJSON(ctx, "/api/v1/login", *creds, &auth); err != nil {
+	if err := c.APIClientTemplate.Build().PostJSON(
+		ctx, "/api/v1/login", *creds, &auth); err != nil {
 		return err
 	}
 	state.Expire = auth.Expire

internal/engine/probeservices/measurementmeta.go

@@ -54,12 +54,12 @@ func (c Client) GetMeasurementMeta(
 		query.Add("full", "true")
 	}
 	var response MeasurementMeta
-	err := (&httpx.APIClient{
+	err := (&httpx.APIClientTemplate{
 		BaseURL:    c.BaseURL,
 		HTTPClient: c.HTTPClient,
 		Logger:     c.Logger,
 		UserAgent:  c.UserAgent,
-	}).GetJSONWithQuery(ctx, "/api/v1/measurement_meta", query, &response)
+	}).Build().GetJSONWithQuery(ctx, "/api/v1/measurement_meta", query, &response)
 	if err != nil {
 		return nil, err
 	}

internal/engine/probeservices/measurementmeta_test.go

@@ -16,7 +16,7 @@ import (
 
 func TestGetMeasurementMetaWorkingAsIntended(t *testing.T) {
 	client := probeservices.Client{
-		APIClient: httpx.APIClient{
+		APIClientTemplate: httpx.APIClientTemplate{
 			BaseURL:    "https://ams-pg.ooni.org/",
 			HTTPClient: http.DefaultClient,
 			Logger:     log.Log,
@@ -84,7 +84,7 @@ func TestGetMeasurementMetaWorkingAsIntended(t *testing.T) {
 
 func TestGetMeasurementMetaWorkingWithCancelledContext(t *testing.T) {
 	client := probeservices.Client{
-		APIClient: httpx.APIClient{
+		APIClientTemplate: httpx.APIClientTemplate{
 			BaseURL:    "https://ams-pg.ooni.org/",
 			HTTPClient: http.DefaultClient,
 			Logger:     log.Log,

internal/engine/probeservices/probeservices.go

@@ -65,7 +65,7 @@ type Session interface {
 
 // Client is a client for the OONI probe services API.
 type Client struct {
-	httpx.APIClient
+	httpx.APIClientTemplate
 	LoginCalls    *atomicx.Int64
 	RegisterCalls *atomicx.Int64
 	StateFile     StateFile
@@ -91,7 +91,7 @@ func (c Client) GetCredsAndAuth() (*LoginCredentials, *LoginAuth, error) {
 // function fails, e.g., we don't support the specified endpoint.
 func NewClient(sess Session, endpoint model.OOAPIService) (*Client, error) {
 	client := &Client{
-		APIClient: httpx.APIClient{
+		APIClientTemplate: httpx.APIClientTemplate{
 			BaseURL:    endpoint.Address,
 			HTTPClient: sess.DefaultHTTPClient(),
 			Logger:     sess.Logger(),
@@ -115,7 +115,7 @@ func NewClient(sess Session, endpoint model.OOAPIService) (*Client, error) {
 		if URL.Scheme != "https" || URL.Host != URL.Hostname() {
 			return nil, ErrUnsupportedCloudFrontAddress
 		}
-		client.APIClient.Host = URL.Hostname()
+		client.APIClientTemplate.Host = URL.Hostname()
 		URL.Host = endpoint.Front
 		client.BaseURL = URL.String()
 		if _, err := url.Parse(client.BaseURL); err != nil {

internal/engine/probeservices/psiphon.go

@@ -11,9 +11,7 @@ func (c Client) FetchPsiphonConfig(ctx context.Context) ([]byte, error) {
 	if err != nil {
 		return nil, err
 	}
-	// Note: the following code is very bad: it copies the original
-	// API client and then overrides one of its fields. Bleah...
-	client := c.APIClient
-	client.Authorization = fmt.Sprintf("Bearer %s", auth.Token)
+	s := fmt.Sprintf("Bearer %s", auth.Token)
+	client := c.APIClientTemplate.BuildWithAuthorization(s)
 	return client.FetchResource(ctx, "/api/v1/test-list/psiphon-config")
 }

internal/engine/probeservices/register.go

@@ -33,7 +33,8 @@ func (c Client) MaybeRegister(ctx context.Context, metadata Metadata) error {
 		Password: pwd,
 	}
 	var resp registerResult
-	if err := c.APIClient.PostJSON(ctx, "/api/v1/register", req, &resp); err != nil {
+	if err := c.APIClientTemplate.Build().PostJSON(
+		ctx, "/api/v1/register", req, &resp); err != nil {
 		return err
 	}
 	state.ClientID = resp.ClientID

internal/engine/probeservices/tor.go

@@ -14,10 +14,8 @@ func (c Client) FetchTorTargets(ctx context.Context, cc string) (result map[stri
 	if err != nil {
 		return nil, err
 	}
-	// Note: the following code is very bad: it copies the original
-	// API client and then overrides one of its fields. Bleah...
-	client := c.APIClient
-	client.Authorization = fmt.Sprintf("Bearer %s", auth.Token)
+	s := fmt.Sprintf("Bearer %s", auth.Token)
+	client := c.APIClientTemplate.BuildWithAuthorization(s)
 	query := url.Values{}
 	query.Add("country_code", cc)
 	err = client.GetJSONWithQuery(

internal/engine/probeservices/urls.go

@@ -28,7 +28,8 @@ func (c Client) FetchURLList(ctx context.Context, config model.OOAPIURLListConfi
 		query.Set("category_codes", strings.Join(config.Categories, ","))
 	}
 	var response urlListResult
-	err := c.APIClient.GetJSONWithQuery(ctx, "/api/v1/test-list/urls", query, &response)
+	err := c.APIClientTemplate.Build().GetJSONWithQuery(ctx,
+		"/api/v1/test-list/urls", query, &response)
 	if err != nil {
 		return nil, err
 	}