From 7b7df2c6af070bee20a14b46a81358d09b043f64 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 5 Jan 2022 12:48:32 +0100 Subject: [PATCH] refactor(httpx): improve and modernize (1/n) (#647) This PR starts to implement the refactoring described at https://github.com/ooni/probe/issues/1951. I originally wrote more patches than the ones in this PR, but overall they were not readable. Since I want to squash and merge, here's a reasonable subset of the original patches that will still be readable and understandable in the future. --- internal/cmd/apitool/main.go | 2 +- .../experiment/webconnectivity/control.go | 2 +- .../engine/experiment/websteps/control.go | 2 +- internal/engine/geolocate/avast.go | 2 +- internal/engine/geolocate/ipconfig.go | 2 +- internal/engine/geolocate/ipinfo.go | 2 +- internal/engine/geolocate/ubuntu.go | 2 +- internal/engine/httpx/fetch.go | 31 ---- internal/engine/httpx/fetch_test.go | 154 ------------------ .../engine/httpx/{jsonapi.go => httpx.go} | 86 +++++----- .../httpx/{jsonapi_test.go => httpx_test.go} | 140 +++++++++++----- internal/engine/probeservices/bouncer.go | 2 +- internal/engine/probeservices/checkin.go | 2 +- .../engine/probeservices/checkreportid.go | 2 +- .../probeservices/checkreportid_test.go | 4 +- internal/engine/probeservices/collector.go | 4 +- internal/engine/probeservices/login.go | 2 +- .../engine/probeservices/measurementmeta.go | 2 +- .../probeservices/measurementmeta_test.go | 4 +- .../engine/probeservices/probeservices.go | 6 +- internal/engine/probeservices/psiphon.go | 4 +- internal/engine/probeservices/register.go | 2 +- internal/engine/probeservices/tor.go | 4 +- internal/engine/probeservices/tor_test.go | 2 +- internal/engine/probeservices/urls.go | 2 +- 25 files changed, 173 insertions(+), 294 deletions(-) delete mode 100644 internal/engine/httpx/fetch.go delete mode 100644 internal/engine/httpx/fetch_test.go rename internal/engine/httpx/{jsonapi.go => httpx.go} (54%) rename internal/engine/httpx/{jsonapi_test.go => httpx_test.go} (67%) diff --git a/internal/cmd/apitool/main.go b/internal/cmd/apitool/main.go index 29e9857..0c86a7f 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{ - Client: httpx.Client{ + APIClient: httpx.APIClient{ 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 f6a990e..cfb181a 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.Client{ + clnt := &httpx.APIClient{ BaseURL: thAddr, HTTPClient: sess.DefaultHTTPClient(), Logger: sess.Logger(), diff --git a/internal/engine/experiment/websteps/control.go b/internal/engine/experiment/websteps/control.go index 4c68b8e..258a734 100644 --- a/internal/engine/experiment/websteps/control.go +++ b/internal/engine/experiment/websteps/control.go @@ -13,7 +13,7 @@ import ( func Control( ctx context.Context, sess model.ExperimentSession, thAddr string, resourcePath string, creq CtrlRequest) (out CtrlResponse, err error) { - clnt := httpx.Client{ + clnt := &httpx.APIClient{ BaseURL: thAddr, HTTPClient: sess.DefaultHTTPClient(), Logger: sess.Logger(), diff --git a/internal/engine/geolocate/avast.go b/internal/engine/geolocate/avast.go index cf7986c..4cbbd8c 100644 --- a/internal/engine/geolocate/avast.go +++ b/internal/engine/geolocate/avast.go @@ -19,7 +19,7 @@ func avastIPLookup( userAgent string, ) (string, error) { var v avastResponse - err := (httpx.Client{ + err := (&httpx.APIClient{ BaseURL: "https://ip-info.ff.avast.com", HTTPClient: httpClient, Logger: logger, diff --git a/internal/engine/geolocate/ipconfig.go b/internal/engine/geolocate/ipconfig.go index 057ad68..acee6d2 100644 --- a/internal/engine/geolocate/ipconfig.go +++ b/internal/engine/geolocate/ipconfig.go @@ -16,7 +16,7 @@ func ipConfigIPLookup( logger model.Logger, userAgent string, ) (string, error) { - data, err := (httpx.Client{ + data, err := (&httpx.APIClient{ BaseURL: "https://ipconfig.io", HTTPClient: httpClient, Logger: logger, diff --git a/internal/engine/geolocate/ipinfo.go b/internal/engine/geolocate/ipinfo.go index 141c1ff..ddf738c 100644 --- a/internal/engine/geolocate/ipinfo.go +++ b/internal/engine/geolocate/ipinfo.go @@ -20,7 +20,7 @@ func ipInfoIPLookup( userAgent string, ) (string, error) { var v ipInfoResponse - err := (httpx.Client{ + err := (&httpx.APIClient{ Accept: "application/json", BaseURL: "https://ipinfo.io", HTTPClient: httpClient, diff --git a/internal/engine/geolocate/ubuntu.go b/internal/engine/geolocate/ubuntu.go index b820fc4..8c8d8a1 100644 --- a/internal/engine/geolocate/ubuntu.go +++ b/internal/engine/geolocate/ubuntu.go @@ -20,7 +20,7 @@ func ubuntuIPLookup( logger model.Logger, userAgent string, ) (string, error) { - data, err := (httpx.Client{ + data, err := (&httpx.APIClient{ BaseURL: "https://geoip.ubuntu.com/", HTTPClient: httpClient, Logger: logger, diff --git a/internal/engine/httpx/fetch.go b/internal/engine/httpx/fetch.go deleted file mode 100644 index 8db6502..0000000 --- a/internal/engine/httpx/fetch.go +++ /dev/null @@ -1,31 +0,0 @@ -package httpx - -import ( - "context" - "crypto/sha256" - "fmt" -) - -// FetchResource fetches the specified resource and returns it. -func (c Client) FetchResource(ctx context.Context, URLPath string) ([]byte, error) { - request, err := c.NewRequest(ctx, "GET", URLPath, nil, nil) - if err != nil { - return nil, err - } - return c.Do(request) -} - -// FetchResourceAndVerify fetches and verifies a specific resource. -func (c Client) FetchResourceAndVerify(ctx context.Context, URL, SHA256Sum string) ([]byte, error) { - c.Logger.Debugf("httpx: expected SHA256: %s", SHA256Sum) - data, err := c.FetchResource(ctx, URL) - if err != nil { - return nil, err - } - s := fmt.Sprintf("%x", sha256.Sum256(data)) - c.Logger.Debugf("httpx: real SHA256: %s", s) - if SHA256Sum != s { - return nil, fmt.Errorf("httpx: SHA256 mismatch: got %s and expected %s", s, SHA256Sum) - } - return data, nil -} diff --git a/internal/engine/httpx/fetch_test.go b/internal/engine/httpx/fetch_test.go deleted file mode 100644 index df55874..0000000 --- a/internal/engine/httpx/fetch_test.go +++ /dev/null @@ -1,154 +0,0 @@ -package httpx_test - -import ( - "context" - "errors" - "net/http" - "net/http/httptest" - "strings" - "testing" - - "github.com/apex/log" - "github.com/ooni/probe-cli/v3/internal/engine/httpx" -) - -func TestFetchResourceIntegration(t *testing.T) { - log.SetLevel(log.DebugLevel) - ctx := context.Background() - data, err := (httpx.Client{ - BaseURL: "http://facebook.com/", - HTTPClient: http.DefaultClient, - Logger: log.Log, - UserAgent: "ooniprobe-engine/0.1.0", - }).FetchResource(ctx, "/robots.txt") - if err != nil { - t.Fatal(err) - } - if len(data) <= 0 { - t.Fatal("Did not expect an empty resource") - } -} - -func TestFetchResourceExpiredContext(t *testing.T) { - log.SetLevel(log.DebugLevel) - ctx, cancel := context.WithCancel(context.Background()) - cancel() - data, err := (httpx.Client{ - BaseURL: "http://facebook.com/", - HTTPClient: http.DefaultClient, - Logger: log.Log, - UserAgent: "ooniprobe-engine/0.1.0", - }).FetchResource(ctx, "/robots.txt") - if !errors.Is(err, context.Canceled) { - t.Fatal("not the error we expected") - } - if len(data) != 0 { - t.Fatal("expected an empty resource") - } -} - -func TestFetchResourceAndVerifyIntegration(t *testing.T) { - log.SetLevel(log.DebugLevel) - ctx := context.Background() - data, err := (httpx.Client{ - BaseURL: "https://github.com/", - HTTPClient: http.DefaultClient, - Logger: log.Log, - UserAgent: "ooniprobe-engine/0.1.0", - }).FetchResourceAndVerify( - ctx, - "/measurement-kit/generic-assets/releases/download/20190426155936/generic-assets-20190426155936.tar.gz", - "34d8a9c8ab30c242469482dc280be832d8a06b4400f8927604dd361bf979b795", - ) - if err != nil { - t.Fatal(err) - } - if len(data) <= 0 { - t.Fatal("Did not expect an empty resource") - } -} - -func TestFetchResourceInvalidURL(t *testing.T) { - log.SetLevel(log.DebugLevel) - ctx := context.Background() - data, err := (httpx.Client{ - BaseURL: "http://\t/", - HTTPClient: http.DefaultClient, - Logger: log.Log, - UserAgent: "ooniprobe-engine/0.1.0", - }).FetchResource(ctx, "/robots.txt") - if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") { - t.Fatal("not the error we expected") - } - if len(data) != 0 { - t.Fatal("expected an empty resource") - } -} - -func TestFetchResource400(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc( - func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(400) - }, - )) - defer server.Close() - log.SetLevel(log.DebugLevel) - ctx := context.Background() - data, err := (httpx.Client{ - Authorization: "foobar", - BaseURL: server.URL, - HTTPClient: http.DefaultClient, - Logger: log.Log, - UserAgent: "ooniprobe-engine/0.1.0", - }).FetchResource(ctx, "") - if err == nil || !strings.HasSuffix(err.Error(), "400 Bad Request") { - t.Fatal("not the error we expected") - } - if len(data) != 0 { - t.Fatal("expected an empty resource") - } -} - -func TestFetchResourceAndVerify400(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc( - func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(400) - }, - )) - defer server.Close() - log.SetLevel(log.DebugLevel) - ctx := context.Background() - data, err := (httpx.Client{ - BaseURL: server.URL, - HTTPClient: http.DefaultClient, - Logger: log.Log, - UserAgent: "ooniprobe-engine/0.1.0", - }).FetchResourceAndVerify(ctx, "", "abcde") - if err == nil || !strings.HasSuffix(err.Error(), "400 Bad Request") { - t.Fatal("not the error we expected") - } - if len(data) != 0 { - t.Fatal("expected an empty resource") - } -} - -func TestFetchResourceAndVerifyInvalidSHA256(t *testing.T) { - log.SetLevel(log.DebugLevel) - ctx := context.Background() - data, err := (httpx.Client{ - BaseURL: "https://github.com/", - HTTPClient: http.DefaultClient, - Logger: log.Log, - UserAgent: "ooniprobe-engine/0.1.0", - }).FetchResourceAndVerify( - ctx, - "/measurement-kit/generic-assets/releases/download/20190426155936/generic-assets-20190426155936.tar.gz", - "34d8a9ceeb30c242469482dc280be832d8a06b4400f8927604dd361bf979b795", - ) - if err == nil || !strings.HasPrefix(err.Error(), "httpx: SHA256 mismatch:") { - t.Fatal("not the error we expected") - } - if len(data) != 0 { - t.Fatal("expected an empty resource") - } -} diff --git a/internal/engine/httpx/jsonapi.go b/internal/engine/httpx/httpx.go similarity index 54% rename from internal/engine/httpx/jsonapi.go rename to internal/engine/httpx/httpx.go index ec52f48..c9322da 100644 --- a/internal/engine/httpx/jsonapi.go +++ b/internal/engine/httpx/httpx.go @@ -5,6 +5,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -14,33 +15,38 @@ import ( "github.com/ooni/probe-cli/v3/internal/netxlite" ) -// Client is an extended client. -type Client struct { - // Accept contains the accept header. +// 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 +// sure you initialize all fields marked as MANDATORY. +type APIClient struct { + // Accept contains the OPTIONAL accept header. Accept string - // Authorization contains the authorization header. + // Authorization contains the OPTIONAL authorization header. Authorization string - // BaseURL is the base URL of the API. + // BaseURL is the MANDATORY base URL of the API. BaseURL string - // HTTPClient is the real http client to use. - HTTPClient *http.Client + // HTTPClient is the MANDATORY underlying http client to use. + HTTPClient model.HTTPClient - // Host allows to set a specific host header. This is useful + // Host allows to OPTIONALLY set a specific host header. This is useful // to implement, e.g., cloudfronting. Host string - // Logger is the logger to use. + // Logger is MANDATORY the logger to use. Logger model.DebugLogger - // UserAgent is the user agent to use. + // UserAgent is the OPTIONAL user agent to use. UserAgent string } -// NewRequestWithJSONBody creates a new request with a JSON body -func (c Client) NewRequestWithJSONBody( +// newRequestWithJSONBody creates a new request with a JSON body +func (c *APIClient) newRequestWithJSONBody( ctx context.Context, method, resourcePath string, query url.Values, body interface{}) (*http.Request, error) { data, err := json.Marshal(body) @@ -48,7 +54,7 @@ func (c Client) NewRequestWithJSONBody( return nil, err } c.Logger.Debugf("httpx: request body: %d bytes", len(data)) - request, err := c.NewRequest( + request, err := c.newRequest( ctx, method, resourcePath, query, bytes.NewReader(data)) if err != nil { return nil, err @@ -59,8 +65,8 @@ func (c Client) NewRequestWithJSONBody( return request, nil } -// NewRequest creates a new request. -func (c Client) NewRequest(ctx context.Context, method, resourcePath string, +// newRequest creates a new request. +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 { @@ -70,8 +76,6 @@ func (c Client) NewRequest(ctx context.Context, method, resourcePath string, if query != nil { URL.RawQuery = query.Encode() } - c.Logger.Debugf("httpx: method: %s", method) - c.Logger.Debugf("httpx: URL: %s", URL.String()) request, err := http.NewRequest(method, URL.String(), body) if err != nil { return nil, err @@ -87,23 +91,31 @@ func (c Client) NewRequest(ctx context.Context, method, resourcePath string, return request.WithContext(ctx), nil } -// Do performs the provided request and returns the response body or an error. -func (c Client) Do(request *http.Request) ([]byte, error) { +// ErrRequestFailed indicates that the server returned >= 400. +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) { response, err := c.HTTPClient.Do(request) if err != nil { return nil, err } defer response.Body.Close() if response.StatusCode >= 400 { - return nil, fmt.Errorf("httpx: request failed: %s", response.Status) + return nil, fmt.Errorf("%w: %s", ErrRequestFailed, response.Status) } - return netxlite.ReadAllContext(request.Context(), response.Body) + r := io.LimitReader(response.Body, DefaultMaxBodySize) + data, err := netxlite.ReadAllContext(request.Context(), r) + if err != nil { + return nil, err + } + return data, nil } -// DoJSON performs the provided request and unmarshals the JSON response body +// doJSON performs the provided request and unmarshals the JSON response body // into the provided output variable. -func (c Client) DoJSON(request *http.Request, output interface{}) error { - data, err := c.Do(request) +func (c *APIClient) doJSON(request *http.Request, output interface{}) error { + data, err := c.do(request) if err != nil { return err } @@ -114,41 +126,39 @@ func (c Client) DoJSON(request *http.Request, output interface{}) error { // 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 Client) GetJSON(ctx context.Context, resourcePath string, output interface{}) error { +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 Client) 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) + request, err := c.newRequest(ctx, "GET", resourcePath, query, nil) if err != nil { return err } - return c.DoJSON(request, output) + 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 Client) PostJSON( +func (c *APIClient) PostJSON( ctx context.Context, resourcePath string, input, output interface{}) error { - request, err := c.NewRequestWithJSONBody(ctx, "POST", resourcePath, nil, input) + request, err := c.newRequestWithJSONBody(ctx, "POST", resourcePath, nil, input) if err != nil { return err } - return c.DoJSON(request, output) + return c.doJSON(request, output) } -// PutJSON updates a JSON resource at a specific path and returns -// the error that occurred and possibly an output document -func (c Client) PutJSON( - ctx context.Context, resourcePath string, input, output interface{}) error { - request, err := c.NewRequestWithJSONBody(ctx, "PUT", resourcePath, nil, input) +// FetchResource fetches the specified resource and returns it. +func (c *APIClient) FetchResource(ctx context.Context, URLPath string) ([]byte, error) { + request, err := c.newRequest(ctx, "GET", URLPath, nil, nil) if err != nil { - return err + return nil, err } - return c.DoJSON(request, output) + return c.do(request) } diff --git a/internal/engine/httpx/jsonapi_test.go b/internal/engine/httpx/httpx_test.go similarity index 67% rename from internal/engine/httpx/jsonapi_test.go rename to internal/engine/httpx/httpx_test.go index 68f962f..adc9d16 100644 --- a/internal/engine/httpx/jsonapi_test.go +++ b/internal/engine/httpx/httpx_test.go @@ -1,22 +1,22 @@ -package httpx_test +package httpx import ( "context" "errors" "io" "net/http" + "net/http/httptest" "net/url" "strings" "testing" "github.com/apex/log" - "github.com/ooni/probe-cli/v3/internal/engine/httpx" ) const userAgent = "miniooni/0.1.0-dev" -func newClient() httpx.Client { - return httpx.Client{ +func newClient() *APIClient { + return &APIClient{ BaseURL: "https://httpbin.org", HTTPClient: http.DefaultClient, Logger: log.Log, @@ -26,7 +26,7 @@ func newClient() httpx.Client { func TestNewRequestWithJSONBodyJSONMarshalFailure(t *testing.T) { client := newClient() - req, err := client.NewRequestWithJSONBody( + req, err := client.newRequestWithJSONBody( context.Background(), "GET", "/", nil, make(chan interface{}), ) if err == nil || !strings.HasPrefix(err.Error(), "json: unsupported type") { @@ -40,7 +40,7 @@ func TestNewRequestWithJSONBodyJSONMarshalFailure(t *testing.T) { func TestNewRequestWithJSONBodyNewRequestFailure(t *testing.T) { client := newClient() client.BaseURL = "\t\t\t" // cause URL parse error - req, err := client.NewRequestWithJSONBody( + req, err := client.newRequestWithJSONBody( context.Background(), "GET", "/", nil, nil, ) if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") { @@ -56,7 +56,7 @@ func TestNewRequestWithQuery(t *testing.T) { q := url.Values{} q.Add("antani", "mascetti") q.Add("melandri", "conte") - req, err := client.NewRequest( + req, err := client.newRequest( context.Background(), "GET", "/", q, nil, ) if err != nil { @@ -72,7 +72,7 @@ func TestNewRequestWithQuery(t *testing.T) { func TestNewRequestNewRequestFailure(t *testing.T) { client := newClient() - req, err := client.NewRequest( + req, err := client.newRequest( context.Background(), "\t\t\t", "/", nil, nil, ) if err == nil || !strings.HasPrefix(err.Error(), "net/http: invalid method") { @@ -86,7 +86,7 @@ func TestNewRequestNewRequestFailure(t *testing.T) { func TestNewRequestCloudfronting(t *testing.T) { client := newClient() client.Host = "www.x.org" - req, err := client.NewRequest( + req, err := client.newRequest( context.Background(), "GET", "/", nil, nil, ) if err != nil { @@ -100,7 +100,7 @@ func TestNewRequestCloudfronting(t *testing.T) { func TestNewRequestAcceptIsSet(t *testing.T) { client := newClient() client.Accept = "application/xml" - req, err := client.NewRequestWithJSONBody( + req, err := client.newRequestWithJSONBody( context.Background(), "GET", "/", nil, []string{}, ) if err != nil { @@ -113,7 +113,7 @@ func TestNewRequestAcceptIsSet(t *testing.T) { func TestNewRequestContentTypeIsSet(t *testing.T) { client := newClient() - req, err := client.NewRequestWithJSONBody( + req, err := client.newRequestWithJSONBody( context.Background(), "GET", "/", nil, []string{}, ) if err != nil { @@ -127,7 +127,7 @@ func TestNewRequestContentTypeIsSet(t *testing.T) { func TestNewRequestAuthorizationHeader(t *testing.T) { client := newClient() client.Authorization = "deadbeef" - req, err := client.NewRequest( + req, err := client.newRequest( context.Background(), "GET", "/", nil, nil, ) if err != nil { @@ -140,7 +140,7 @@ func TestNewRequestAuthorizationHeader(t *testing.T) { func TestNewRequestUserAgentIsSet(t *testing.T) { client := newClient() - req, err := client.NewRequest( + req, err := client.newRequest( context.Background(), "GET", "/", nil, nil, ) if err != nil { @@ -154,10 +154,10 @@ func TestNewRequestUserAgentIsSet(t *testing.T) { func TestClientDoJSONClientDoFailure(t *testing.T) { expected := errors.New("mocked error") client := newClient() - client.HTTPClient = &http.Client{Transport: httpx.FakeTransport{ + client.HTTPClient = &http.Client{Transport: FakeTransport{ Err: expected, }} - err := client.DoJSON(&http.Request{URL: &url.URL{Scheme: "https", Host: "x.org"}}, nil) + err := client.doJSON(&http.Request{URL: &url.URL{Scheme: "https", Host: "x.org"}}, nil) if !errors.Is(err, expected) { t.Fatal("not the error we expected") } @@ -165,13 +165,13 @@ func TestClientDoJSONClientDoFailure(t *testing.T) { func TestClientDoJSONResponseNotSuccessful(t *testing.T) { client := newClient() - client.HTTPClient = &http.Client{Transport: httpx.FakeTransport{ + client.HTTPClient = &http.Client{Transport: FakeTransport{ Resp: &http.Response{ StatusCode: 401, - Body: httpx.FakeBody{}, + Body: FakeBody{}, }, }} - err := client.DoJSON(&http.Request{URL: &url.URL{Scheme: "https", Host: "x.org"}}, nil) + err := client.doJSON(&http.Request{URL: &url.URL{Scheme: "https", Host: "x.org"}}, nil) if err == nil || !strings.HasPrefix(err.Error(), "httpx: request failed") { t.Fatal("not the error we expected") } @@ -180,15 +180,15 @@ func TestClientDoJSONResponseNotSuccessful(t *testing.T) { func TestClientDoJSONResponseReadingBodyError(t *testing.T) { expected := errors.New("mocked error") client := newClient() - client.HTTPClient = &http.Client{Transport: httpx.FakeTransport{ + client.HTTPClient = &http.Client{Transport: FakeTransport{ Resp: &http.Response{ StatusCode: 200, - Body: httpx.FakeBody{ + Body: FakeBody{ Err: expected, }, }, }} - err := client.DoJSON(&http.Request{URL: &url.URL{Scheme: "https", Host: "x.org"}}, nil) + err := client.doJSON(&http.Request{URL: &url.URL{Scheme: "https", Host: "x.org"}}, nil) if !errors.Is(err, expected) { t.Fatal("not the error we expected") } @@ -196,15 +196,15 @@ func TestClientDoJSONResponseReadingBodyError(t *testing.T) { func TestClientDoJSONResponseIsNotJSON(t *testing.T) { client := newClient() - client.HTTPClient = &http.Client{Transport: httpx.FakeTransport{ + client.HTTPClient = &http.Client{Transport: FakeTransport{ Resp: &http.Response{ StatusCode: 200, - Body: httpx.FakeBody{ + Body: FakeBody{ Err: io.EOF, }, }, }} - err := client.DoJSON(&http.Request{URL: &url.URL{Scheme: "https", Host: "x.org"}}, nil) + err := client.doJSON(&http.Request{URL: &url.URL{Scheme: "https", Host: "x.org"}}, nil) if err == nil || err.Error() != "unexpected end of JSON input" { t.Fatal("not the error we expected") } @@ -248,22 +248,6 @@ func TestCreateJSONSuccess(t *testing.T) { } } -func TestUpdateJSONSuccess(t *testing.T) { - headers := httpbinheaders{ - Headers: map[string]string{ - "Foo": "bar", - }, - } - var response httpbinpost - err := newClient().PutJSON(context.Background(), "/put", &headers, &response) - if err != nil { - t.Fatal(err) - } - if response.Data != `{"headers":{"Foo":"bar"}}` { - t.Fatal(response.Data) - } -} - func TestReadJSONFailure(t *testing.T) { var headers httpbinheaders client := newClient() @@ -284,12 +268,78 @@ func TestCreateJSONFailure(t *testing.T) { } } -func TestUpdateJSONFailure(t *testing.T) { - var headers httpbinheaders - client := newClient() - client.BaseURL = "\t\t\t\t" - err := client.PutJSON(context.Background(), "/headers", &headers, &headers) +func TestFetchResourceIntegration(t *testing.T) { + log.SetLevel(log.DebugLevel) + ctx := context.Background() + data, err := (&APIClient{ + BaseURL: "http://facebook.com/", + HTTPClient: http.DefaultClient, + Logger: log.Log, + UserAgent: "ooniprobe-engine/0.1.0", + }).FetchResource(ctx, "/robots.txt") + if err != nil { + t.Fatal(err) + } + if len(data) <= 0 { + t.Fatal("Did not expect an empty resource") + } +} + +func TestFetchResourceExpiredContext(t *testing.T) { + log.SetLevel(log.DebugLevel) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + data, err := (&APIClient{ + BaseURL: "http://facebook.com/", + HTTPClient: http.DefaultClient, + Logger: log.Log, + UserAgent: "ooniprobe-engine/0.1.0", + }).FetchResource(ctx, "/robots.txt") + if !errors.Is(err, context.Canceled) { + t.Fatal("not the error we expected") + } + if len(data) != 0 { + t.Fatal("expected an empty resource") + } +} + +func TestFetchResourceInvalidURL(t *testing.T) { + log.SetLevel(log.DebugLevel) + ctx := context.Background() + data, err := (&APIClient{ + BaseURL: "http://\t/", + HTTPClient: http.DefaultClient, + Logger: log.Log, + UserAgent: "ooniprobe-engine/0.1.0", + }).FetchResource(ctx, "/robots.txt") if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") { t.Fatal("not the error we expected") } + if len(data) != 0 { + t.Fatal("expected an empty resource") + } +} + +func TestFetchResource400(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(400) + }, + )) + defer server.Close() + log.SetLevel(log.DebugLevel) + ctx := context.Background() + data, err := (&APIClient{ + Authorization: "foobar", + BaseURL: server.URL, + HTTPClient: http.DefaultClient, + Logger: log.Log, + UserAgent: "ooniprobe-engine/0.1.0", + }).FetchResource(ctx, "") + if err == nil || !strings.HasSuffix(err.Error(), "400 Bad Request") { + t.Fatal("not the error we expected") + } + if len(data) != 0 { + t.Fatal("expected an empty resource") + } } diff --git a/internal/engine/probeservices/bouncer.go b/internal/engine/probeservices/bouncer.go index 6c739cc..2a06ba1 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.Client.GetJSON(ctx, "/api/v1/test-helpers", &output) + err = c.APIClient.GetJSON(ctx, "/api/v1/test-helpers", &output) return } diff --git a/internal/engine/probeservices/checkin.go b/internal/engine/probeservices/checkin.go index 988e558..9a2a61d 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.Client.PostJSON(ctx, "/api/v1/check-in", config, &response); err != nil { + if err := c.APIClient.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 262f236..a71a61f 100644 --- a/internal/engine/probeservices/checkreportid.go +++ b/internal/engine/probeservices/checkreportid.go @@ -16,7 +16,7 @@ func (c Client) CheckReportID(ctx context.Context, reportID string) (bool, error query := url.Values{} query.Add("report_id", reportID) var response checkReportIDResponse - err := (httpx.Client{ + err := (&httpx.APIClient{ BaseURL: c.BaseURL, HTTPClient: c.HTTPClient, Logger: c.Logger, diff --git a/internal/engine/probeservices/checkreportid_test.go b/internal/engine/probeservices/checkreportid_test.go index 631713d..cd78217 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{ - Client: httpx.Client{ + APIClient: httpx.APIClient{ 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{ - Client: httpx.Client{ + APIClient: httpx.APIClient{ 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 9af46d6..dbbfed1 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.Client.PostJSON(ctx, "/report", rt, &cor); err != nil { + if err := c.APIClient.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.Client.PostJSON( + err := r.client.APIClient.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 4d0c2d0..13c9561 100644 --- a/internal/engine/probeservices/login.go +++ b/internal/engine/probeservices/login.go @@ -29,7 +29,7 @@ func (c Client) MaybeLogin(ctx context.Context) error { } c.LoginCalls.Add(1) var auth LoginAuth - if err := c.Client.PostJSON(ctx, "/api/v1/login", *creds, &auth); err != nil { + if err := c.APIClient.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 5024b87..3ffacf0 100644 --- a/internal/engine/probeservices/measurementmeta.go +++ b/internal/engine/probeservices/measurementmeta.go @@ -54,7 +54,7 @@ func (c Client) GetMeasurementMeta( query.Add("full", "true") } var response MeasurementMeta - err := (httpx.Client{ + err := (&httpx.APIClient{ BaseURL: c.BaseURL, HTTPClient: c.HTTPClient, Logger: c.Logger, diff --git a/internal/engine/probeservices/measurementmeta_test.go b/internal/engine/probeservices/measurementmeta_test.go index 6613bf0..9c2eb5c 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{ - Client: httpx.Client{ + APIClient: httpx.APIClient{ 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{ - Client: httpx.Client{ + APIClient: httpx.APIClient{ 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 1c8b029..920fc16 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.Client + httpx.APIClient 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{ - Client: httpx.Client{ + APIClient: httpx.APIClient{ 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.Client.Host = URL.Hostname() + client.APIClient.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 e208491..20405f7 100644 --- a/internal/engine/probeservices/psiphon.go +++ b/internal/engine/probeservices/psiphon.go @@ -11,7 +11,9 @@ func (c Client) FetchPsiphonConfig(ctx context.Context) ([]byte, error) { if err != nil { return nil, err } - client := c.Client + // 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) 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 288e777..8941ed9 100644 --- a/internal/engine/probeservices/register.go +++ b/internal/engine/probeservices/register.go @@ -33,7 +33,7 @@ func (c Client) MaybeRegister(ctx context.Context, metadata Metadata) error { Password: pwd, } var resp registerResult - if err := c.Client.PostJSON(ctx, "/api/v1/register", req, &resp); err != nil { + if err := c.APIClient.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 33cae71..cb5d07d 100644 --- a/internal/engine/probeservices/tor.go +++ b/internal/engine/probeservices/tor.go @@ -14,7 +14,9 @@ func (c Client) FetchTorTargets(ctx context.Context, cc string) (result map[stri if err != nil { return nil, err } - client := c.Client + // 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) query := url.Values{} query.Add("country_code", cc) diff --git a/internal/engine/probeservices/tor_test.go b/internal/engine/probeservices/tor_test.go index 127e87c..6b4571a 100644 --- a/internal/engine/probeservices/tor_test.go +++ b/internal/engine/probeservices/tor_test.go @@ -61,7 +61,7 @@ func (clnt *FetchTorTargetsHTTPTransport) RoundTrip(req *http.Request) (*http.Re func TestFetchTorTargetsSetsQueryString(t *testing.T) { clnt := newclient() txp := new(FetchTorTargetsHTTPTransport) - clnt.HTTPClient.Transport = txp + clnt.HTTPClient = &http.Client{Transport: txp} if err := clnt.MaybeRegister(context.Background(), testorchestra.MetadataFixture()); err != nil { t.Fatal(err) } diff --git a/internal/engine/probeservices/urls.go b/internal/engine/probeservices/urls.go index d99ec96..18c1c77 100644 --- a/internal/engine/probeservices/urls.go +++ b/internal/engine/probeservices/urls.go @@ -28,7 +28,7 @@ func (c Client) FetchURLList(ctx context.Context, config model.OOAPIURLListConfi query.Set("category_codes", strings.Join(config.Categories, ",")) } var response urlListResult - err := c.Client.GetJSONWithQuery(ctx, "/api/v1/test-list/urls", query, &response) + err := c.APIClient.GetJSONWithQuery(ctx, "/api/v1/test-list/urls", query, &response) if err != nil { return nil, err }