From eed51978ca24797b45c97ef197165633bde575e3 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 5 Jan 2022 14:15:42 +0100 Subject: [PATCH] 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/cmd/apitool/main.go | 2 +- .../experiment/webconnectivity/control.go | 4 +- .../engine/experiment/websteps/control.go | 4 +- internal/engine/geolocate/avast.go | 4 +- internal/engine/geolocate/ipconfig.go | 4 +- internal/engine/geolocate/ipinfo.go | 4 +- internal/engine/geolocate/ubuntu.go | 4 +- internal/engine/httpx/httpx.go | 94 +++++++++++++++---- internal/engine/httpx/httpx_test.go | 52 ++++++++-- internal/engine/probeservices/bouncer.go | 2 +- internal/engine/probeservices/checkin.go | 2 +- .../engine/probeservices/checkreportid.go | 4 +- .../probeservices/checkreportid_test.go | 4 +- internal/engine/probeservices/collector.go | 4 +- internal/engine/probeservices/login.go | 3 +- .../engine/probeservices/measurementmeta.go | 4 +- .../probeservices/measurementmeta_test.go | 4 +- .../engine/probeservices/probeservices.go | 6 +- internal/engine/probeservices/psiphon.go | 6 +- internal/engine/probeservices/register.go | 3 +- internal/engine/probeservices/tor.go | 6 +- internal/engine/probeservices/urls.go | 3 +- 22 files changed, 159 insertions(+), 64 deletions(-) diff --git a/internal/cmd/apitool/main.go b/internal/cmd/apitool/main.go index 0c86a7f..ece42d1 100644 --- a/internal/cmd/apitool/main.go +++ b/internal/cmd/apitool/main.go @@ -28,7 +28,7 @@ func newclient() probeservices.Client { txp := netxlite.NewHTTPTransportStdlib(log.Log) ua := fmt.Sprintf("apitool/%s ooniprobe-engine/%s", version.Version, version.Version) return probeservices.Client{ - APIClient: httpx.APIClient{ + APIClientTemplate: httpx.APIClientTemplate{ BaseURL: "https://ams-pg.ooni.org/", HTTPClient: &http.Client{Transport: txp}, Logger: log.Log, diff --git a/internal/engine/experiment/webconnectivity/control.go b/internal/engine/experiment/webconnectivity/control.go index cfb181a..ff5136b 100644 --- a/internal/engine/experiment/webconnectivity/control.go +++ b/internal/engine/experiment/webconnectivity/control.go @@ -53,7 +53,7 @@ type ControlResponse struct { func Control( ctx context.Context, sess model.ExperimentSession, thAddr string, creq ControlRequest) (out ControlResponse, err error) { - clnt := &httpx.APIClient{ + clnt := &httpx.APIClientTemplate{ BaseURL: thAddr, HTTPClient: sess.DefaultHTTPClient(), Logger: sess.Logger(), @@ -62,7 +62,7 @@ func Control( sess.Logger().Infof("control for %s...", creq.HTTPRequest) // make sure error is wrapped err = legacyerrorsx.SafeErrWrapperBuilder{ - Error: clnt.PostJSON(ctx, "/", creq, &out), + Error: clnt.Build().PostJSON(ctx, "/", creq, &out), Operation: netxlite.TopLevelOperation, }.MaybeBuild() sess.Logger().Infof("control for %s... %+v", creq.HTTPRequest, err) diff --git a/internal/engine/experiment/websteps/control.go b/internal/engine/experiment/websteps/control.go index 258a734..2cbe07e 100644 --- a/internal/engine/experiment/websteps/control.go +++ b/internal/engine/experiment/websteps/control.go @@ -13,14 +13,14 @@ import ( func Control( ctx context.Context, sess model.ExperimentSession, thAddr string, resourcePath string, creq CtrlRequest) (out CtrlResponse, err error) { - clnt := &httpx.APIClient{ + clnt := &httpx.APIClientTemplate{ BaseURL: thAddr, HTTPClient: sess.DefaultHTTPClient(), Logger: sess.Logger(), } // make sure error is wrapped err = errorsxlegacy.SafeErrWrapperBuilder{ - Error: clnt.PostJSON(ctx, resourcePath, creq, &out), + Error: clnt.Build().PostJSON(ctx, resourcePath, creq, &out), Operation: netxlite.TopLevelOperation, }.MaybeBuild() return diff --git a/internal/engine/geolocate/avast.go b/internal/engine/geolocate/avast.go index 4cbbd8c..3bb5500 100644 --- a/internal/engine/geolocate/avast.go +++ b/internal/engine/geolocate/avast.go @@ -19,12 +19,12 @@ func avastIPLookup( userAgent string, ) (string, error) { var v avastResponse - err := (&httpx.APIClient{ + err := (&httpx.APIClientTemplate{ BaseURL: "https://ip-info.ff.avast.com", HTTPClient: httpClient, Logger: logger, UserAgent: userAgent, - }).GetJSON(ctx, "/v1/info", &v) + }).Build().GetJSON(ctx, "/v1/info", &v) if err != nil { return DefaultProbeIP, err } diff --git a/internal/engine/geolocate/ipconfig.go b/internal/engine/geolocate/ipconfig.go index acee6d2..ebf7ebe 100644 --- a/internal/engine/geolocate/ipconfig.go +++ b/internal/engine/geolocate/ipconfig.go @@ -16,12 +16,12 @@ func ipConfigIPLookup( logger model.Logger, userAgent string, ) (string, error) { - data, err := (&httpx.APIClient{ + data, err := (&httpx.APIClientTemplate{ BaseURL: "https://ipconfig.io", HTTPClient: httpClient, Logger: logger, UserAgent: httpheader.CLIUserAgent(), - }).FetchResource(ctx, "/") + }).Build().FetchResource(ctx, "/") if err != nil { return DefaultProbeIP, err } diff --git a/internal/engine/geolocate/ipinfo.go b/internal/engine/geolocate/ipinfo.go index ddf738c..9d70764 100644 --- a/internal/engine/geolocate/ipinfo.go +++ b/internal/engine/geolocate/ipinfo.go @@ -20,13 +20,13 @@ func ipInfoIPLookup( userAgent string, ) (string, error) { var v ipInfoResponse - err := (&httpx.APIClient{ + err := (&httpx.APIClientTemplate{ Accept: "application/json", BaseURL: "https://ipinfo.io", HTTPClient: httpClient, Logger: logger, UserAgent: httpheader.CLIUserAgent(), // we must be a CLI client - }).GetJSON(ctx, "/", &v) + }).Build().GetJSON(ctx, "/", &v) if err != nil { return DefaultProbeIP, err } diff --git a/internal/engine/geolocate/ubuntu.go b/internal/engine/geolocate/ubuntu.go index 8c8d8a1..20e42c3 100644 --- a/internal/engine/geolocate/ubuntu.go +++ b/internal/engine/geolocate/ubuntu.go @@ -20,12 +20,12 @@ func ubuntuIPLookup( logger model.Logger, userAgent string, ) (string, error) { - data, err := (&httpx.APIClient{ + data, err := (&httpx.APIClientTemplate{ BaseURL: "https://geoip.ubuntu.com/", HTTPClient: httpClient, Logger: logger, UserAgent: userAgent, - }).FetchResource(ctx, "/lookup") + }).Build().FetchResource(ctx, "/lookup") if err != nil { return DefaultProbeIP, err } diff --git a/internal/engine/httpx/httpx.go b/internal/engine/httpx/httpx.go index c9322da..8ca1d33 100644 --- a/internal/engine/httpx/httpx.go +++ b/internal/engine/httpx/httpx.go @@ -15,13 +15,74 @@ import ( "github.com/ooni/probe-cli/v3/internal/netxlite" ) +// APIClientTemplate is a template for constructing an APIClient. +type APIClientTemplate struct { + // Accept contains the OPTIONAL accept header. + Accept string + + // Authorization contains the OPTIONAL authorization header. + Authorization string + + // BaseURL is the MANDATORY base URL of the API. + BaseURL string + + // HTTPClient is the MANDATORY underlying http client to use. + HTTPClient model.HTTPClient + + // Host allows to OPTIONALLY set a specific host header. This is useful + // to implement, e.g., cloudfronting. + Host string + + // Logger is MANDATORY the logger to use. + Logger model.DebugLogger + + // UserAgent is the OPTIONAL user agent to use. + UserAgent string +} + +// Build creates an APIClient from the APIClientTemplate. +func (tmpl *APIClientTemplate) Build() APIClient { + return tmpl.BuildWithAuthorization("") +} + +// BuildWithAuthorization creates an APIClient from the +// APIClientTemplate and ensures it uses the given authorization +// value for APIClient.Authorization in subsequent API calls. +func (tmpl *APIClientTemplate) BuildWithAuthorization(authorization string) APIClient { + ac := apiClient(*tmpl) + ac.Authorization = authorization + return &ac +} + // DefaultMaxBodySize is the default value for the maximum // body size you can fetch using an APIClient. const DefaultMaxBodySize = 1 << 22 -// APIClient is an extended HTTP client. To construct this APIClient, make +// APIClient is a client configured to call a given API identified +// by a given baseURL and using a given model.HTTPClient. +type APIClient interface { + // GetJSON reads the JSON resource at resourcePath and unmarshals the + // results into output. The request is bounded by the lifetime of the + // context passed as argument. Returns the error that occurred. + GetJSON(ctx context.Context, resourcePath string, output interface{}) error + + // GetJSONWithQuery is like GetJSON but also has a query. + GetJSONWithQuery(ctx context.Context, resourcePath string, + query url.Values, output interface{}) error + + // PostJSON creates a JSON subresource of the resource at resourcePath + // using the JSON document at input and returning the result into the + // JSON document at output. The request is bounded by the context's + // lifetime. Returns the error that occurred. + PostJSON(ctx context.Context, resourcePath string, input, output interface{}) error + + // FetchResource fetches the specified resource and returns it. + FetchResource(ctx context.Context, URLPath string) ([]byte, error) +} + +// apiClient is an extended HTTP client. To construct this struct, make // sure you initialize all fields marked as MANDATORY. -type APIClient struct { +type apiClient struct { // Accept contains the OPTIONAL accept header. Accept string @@ -46,7 +107,7 @@ type APIClient struct { } // newRequestWithJSONBody creates a new request with a JSON body -func (c *APIClient) newRequestWithJSONBody( +func (c *apiClient) newRequestWithJSONBody( ctx context.Context, method, resourcePath string, query url.Values, body interface{}) (*http.Request, error) { data, err := json.Marshal(body) @@ -66,7 +127,7 @@ func (c *APIClient) newRequestWithJSONBody( } // newRequest creates a new request. -func (c *APIClient) newRequest(ctx context.Context, method, resourcePath string, +func (c *apiClient) newRequest(ctx context.Context, method, resourcePath string, query url.Values, body io.Reader) (*http.Request, error) { URL, err := url.Parse(c.BaseURL) if err != nil { @@ -95,7 +156,7 @@ func (c *APIClient) newRequest(ctx context.Context, method, resourcePath string, var ErrRequestFailed = errors.New("httpx: request failed") // do performs the provided request and returns the response body or an error. -func (c *APIClient) do(request *http.Request) ([]byte, error) { +func (c *apiClient) do(request *http.Request) ([]byte, error) { response, err := c.HTTPClient.Do(request) if err != nil { return nil, err @@ -114,7 +175,7 @@ func (c *APIClient) do(request *http.Request) ([]byte, error) { // doJSON performs the provided request and unmarshals the JSON response body // into the provided output variable. -func (c *APIClient) doJSON(request *http.Request, output interface{}) error { +func (c *apiClient) doJSON(request *http.Request, output interface{}) error { data, err := c.do(request) if err != nil { return err @@ -123,15 +184,13 @@ func (c *APIClient) doJSON(request *http.Request, output interface{}) error { return json.Unmarshal(data, output) } -// GetJSON reads the JSON resource at resourcePath and unmarshals the -// results into output. The request is bounded by the lifetime of the -// context passed as argument. Returns the error that occurred. -func (c *APIClient) GetJSON(ctx context.Context, resourcePath string, output interface{}) error { +// GetJSON implements APIClient.GetJSON. +func (c *apiClient) GetJSON(ctx context.Context, resourcePath string, output interface{}) error { return c.GetJSONWithQuery(ctx, resourcePath, nil, output) } -// GetJSONWithQuery is like GetJSON but also has a query. -func (c *APIClient) GetJSONWithQuery( +// GetJSONWithQuery implements APIClient.GetJSONWithQuery. +func (c *apiClient) GetJSONWithQuery( ctx context.Context, resourcePath string, query url.Values, output interface{}) error { request, err := c.newRequest(ctx, "GET", resourcePath, query, nil) @@ -141,11 +200,8 @@ func (c *APIClient) GetJSONWithQuery( return c.doJSON(request, output) } -// PostJSON creates a JSON subresource of the resource at resourcePath -// using the JSON document at input and returning the result into the -// JSON document at output. The request is bounded by the context's -// lifetime. Returns the error that occurred. -func (c *APIClient) PostJSON( +// PostJSON implements APIClient.PostJSON. +func (c *apiClient) PostJSON( ctx context.Context, resourcePath string, input, output interface{}) error { request, err := c.newRequestWithJSONBody(ctx, "POST", resourcePath, nil, input) if err != nil { @@ -154,8 +210,8 @@ func (c *APIClient) PostJSON( return c.doJSON(request, output) } -// FetchResource fetches the specified resource and returns it. -func (c *APIClient) FetchResource(ctx context.Context, URLPath string) ([]byte, error) { +// FetchResource implements APIClient.FetchResource. +func (c *apiClient) FetchResource(ctx context.Context, URLPath string) ([]byte, error) { request, err := c.newRequest(ctx, "GET", URLPath, nil, nil) if err != nil { return nil, err diff --git a/internal/engine/httpx/httpx_test.go b/internal/engine/httpx/httpx_test.go index adc9d16..c2935d5 100644 --- a/internal/engine/httpx/httpx_test.go +++ b/internal/engine/httpx/httpx_test.go @@ -11,12 +11,52 @@ import ( "testing" "github.com/apex/log" + "github.com/ooni/probe-cli/v3/internal/model" ) const userAgent = "miniooni/0.1.0-dev" -func newClient() *APIClient { - return &APIClient{ +func TestAPIClientTemplate(t *testing.T) { + t.Run("normal constructor", func(t *testing.T) { + // TODO(bassosimone): we need to use fakeFiller here + tmpl := &APIClientTemplate{ + Accept: "application/json", + Authorization: "ORIG-TOKEN", + BaseURL: "https://ams-pg.ooni.org/", + HTTPClient: http.DefaultClient, + Host: "ams-pg.ooni.org", + Logger: model.DiscardLogger, + UserAgent: userAgent, + } + ac := tmpl.Build() + if ac == nil { + t.Fatal("expected non-nil Client here") + } + }) + + t.Run("constructor with authorization", func(t *testing.T) { + // TODO(bassosimone): we need to use fakeFiller here + tmpl := &APIClientTemplate{ + Accept: "application/json", + Authorization: "ORIG-TOKEN", + BaseURL: "https://ams-pg.ooni.org/", + HTTPClient: http.DefaultClient, + Host: "ams-pg.ooni.org", + Logger: model.DiscardLogger, + UserAgent: userAgent, + } + ac := tmpl.BuildWithAuthorization("AUTH-TOKEN") + if tmpl.Authorization != "ORIG-TOKEN" { + t.Fatal("invalid template Authorization") + } + if ac.(*apiClient).Authorization != "AUTH-TOKEN" { + t.Fatal("invalid client Authorization") + } + }) +} + +func newClient() *apiClient { + return &apiClient{ BaseURL: "https://httpbin.org", HTTPClient: http.DefaultClient, Logger: log.Log, @@ -271,7 +311,7 @@ func TestCreateJSONFailure(t *testing.T) { func TestFetchResourceIntegration(t *testing.T) { log.SetLevel(log.DebugLevel) ctx := context.Background() - data, err := (&APIClient{ + data, err := (&apiClient{ BaseURL: "http://facebook.com/", HTTPClient: http.DefaultClient, Logger: log.Log, @@ -289,7 +329,7 @@ func TestFetchResourceExpiredContext(t *testing.T) { log.SetLevel(log.DebugLevel) ctx, cancel := context.WithCancel(context.Background()) cancel() - data, err := (&APIClient{ + data, err := (&apiClient{ BaseURL: "http://facebook.com/", HTTPClient: http.DefaultClient, Logger: log.Log, @@ -306,7 +346,7 @@ func TestFetchResourceExpiredContext(t *testing.T) { func TestFetchResourceInvalidURL(t *testing.T) { log.SetLevel(log.DebugLevel) ctx := context.Background() - data, err := (&APIClient{ + data, err := (&apiClient{ BaseURL: "http://\t/", HTTPClient: http.DefaultClient, Logger: log.Log, @@ -329,7 +369,7 @@ func TestFetchResource400(t *testing.T) { defer server.Close() log.SetLevel(log.DebugLevel) ctx := context.Background() - data, err := (&APIClient{ + data, err := (&apiClient{ Authorization: "foobar", BaseURL: server.URL, HTTPClient: http.DefaultClient, diff --git a/internal/engine/probeservices/bouncer.go b/internal/engine/probeservices/bouncer.go index 2a06ba1..9a1f0ff 100644 --- a/internal/engine/probeservices/bouncer.go +++ b/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 } diff --git a/internal/engine/probeservices/checkin.go b/internal/engine/probeservices/checkin.go index 9a2a61d..8c2e4f9 100644 --- a/internal/engine/probeservices/checkin.go +++ b/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 diff --git a/internal/engine/probeservices/checkreportid.go b/internal/engine/probeservices/checkreportid.go index a71a61f..a16755a 100644 --- a/internal/engine/probeservices/checkreportid.go +++ b/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 } diff --git a/internal/engine/probeservices/checkreportid_test.go b/internal/engine/probeservices/checkreportid_test.go index cd78217..107f49e 100644 --- a/internal/engine/probeservices/checkreportid_test.go +++ b/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, diff --git a/internal/engine/probeservices/collector.go b/internal/engine/probeservices/collector.go index dbbfed1..b363360 100644 --- a/internal/engine/probeservices/collector.go +++ b/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, diff --git a/internal/engine/probeservices/login.go b/internal/engine/probeservices/login.go index 13c9561..642c1b8 100644 --- a/internal/engine/probeservices/login.go +++ b/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 diff --git a/internal/engine/probeservices/measurementmeta.go b/internal/engine/probeservices/measurementmeta.go index 3ffacf0..915e7b4 100644 --- a/internal/engine/probeservices/measurementmeta.go +++ b/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 } diff --git a/internal/engine/probeservices/measurementmeta_test.go b/internal/engine/probeservices/measurementmeta_test.go index 9c2eb5c..7079578 100644 --- a/internal/engine/probeservices/measurementmeta_test.go +++ b/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, diff --git a/internal/engine/probeservices/probeservices.go b/internal/engine/probeservices/probeservices.go index 920fc16..c3e1868 100644 --- a/internal/engine/probeservices/probeservices.go +++ b/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 { diff --git a/internal/engine/probeservices/psiphon.go b/internal/engine/probeservices/psiphon.go index 20405f7..a01421d 100644 --- a/internal/engine/probeservices/psiphon.go +++ b/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") } diff --git a/internal/engine/probeservices/register.go b/internal/engine/probeservices/register.go index 8941ed9..c62482c 100644 --- a/internal/engine/probeservices/register.go +++ b/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 diff --git a/internal/engine/probeservices/tor.go b/internal/engine/probeservices/tor.go index cb5d07d..8c5cef7 100644 --- a/internal/engine/probeservices/tor.go +++ b/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( diff --git a/internal/engine/probeservices/urls.go b/internal/engine/probeservices/urls.go index 18c1c77..abdb06a 100644 --- a/internal/engine/probeservices/urls.go +++ b/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 }