c/ooni-probe-cli
Watch
1
0
Fork
You've already forked ooni-probe-cli
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

refactor(httpx): improve and modernize (1/n) (#647)

Browse source 
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.
This commit is contained in:
Simone Basso 2022-01-05 12:48:32 +01:00 • committed by GitHub
commit 7b7df2c6af
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
25 changed files with 173 additions and 294 deletions
Download patch file Download diff file Expand all files Collapse all files

31
internal/engine/httpx/fetch.go
View file

@ -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
}

154
internal/engine/httpx/fetch_test.go
View file

@ -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")
}
}

86
internal/engine/httpx/jsonapi.go → internal/engine/httpx/httpx.go
View file

@ -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)
}

140
internal/engine/httpx/jsonapi_test.go → internal/engine/httpx/httpx_test.go
View file

@ -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")
}
}