diff --git a/internal/httpapi/call.go b/internal/httpapi/call.go index 0203327d6e..4b06ea6f13 100644 --- a/internal/httpapi/call.go +++ b/internal/httpapi/call.go @@ -19,7 +19,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/netxlite" ) -// joinURLPath appends |resourcePath| to |urlPath|. +// joinURLPath appends resourcePath to urlPath. func joinURLPath(urlPath, resourcePath string) string { if resourcePath == "" { if urlPath == "" { @@ -34,7 +34,7 @@ func joinURLPath(urlPath, resourcePath string) string { return urlPath + resourcePath } -// newRequest creates a new http.Request from the given |ctx|, |endpoint|, and |desc|. +// newRequest creates a new http.Request from the given ctx, endpoint, and desc. func newRequest(ctx context.Context, endpoint *Endpoint, desc *Descriptor) (*http.Request, error) { URL, err := url.Parse(endpoint.BaseURL) if err != nil { @@ -111,7 +111,7 @@ func (err *errMaybeCensorship) Unwrap() error { // ErrTruncated indicates we truncated the response body. var ErrTruncated = errors.New("httpapi: truncated response body") -// docall calls the API represented by the given request |req| on the given |endpoint| +// docall calls the API represented by the given request req on the given endpoint // and returns the response and its body or an error. func docall(endpoint *Endpoint, desc *Descriptor, request *http.Request) (*http.Response, []byte, error) { // Implementation note: remember to mark errors for which you want @@ -178,7 +178,7 @@ func call(ctx context.Context, desc *Descriptor, endpoint *Endpoint) (*http.Resp return docall(endpoint, desc, request) } -// Call invokes the API described by |desc| on the given HTTP |endpoint| and +// Call invokes the API described by desc on the given HTTP endpoint and // returns the response body (as a slice of bytes) or an error. // // Note: this function returns ErrHTTPRequestFailed if the HTTP status code is @@ -189,26 +189,16 @@ func Call(ctx context.Context, desc *Descriptor, endpoint *Endpoint) ([]byte, er return rawResponseBody, err } -// goodContentTypeForJSON tracks known-good content-types for JSON. If the content-type -// is not in this map, |CallWithJSONResponse| emits a warning message. -var goodContentTypeForJSON = map[string]bool{ - applicationJSON: true, -} - // CallWithJSONResponse is like Call but also assumes that the response is a -// JSON body and attempts to parse it into the |response| field. +// JSON body and attempts to parse it into the response field. // // Note: this function returns ErrHTTPRequestFailed if the HTTP status code is // greater or equal than 400. You could use errors.As to obtain a copy of the // error that was returned and see for yourself the actual status code. func CallWithJSONResponse(ctx context.Context, desc *Descriptor, endpoint *Endpoint, response any) error { - httpResp, rawRespBody, err := call(ctx, desc, endpoint) + _, rawRespBody, err := call(ctx, desc, endpoint) if err != nil { return err } - if ctype := httpResp.Header.Get("Content-Type"); !goodContentTypeForJSON[ctype] { - endpoint.Logger.Warnf("httpapi: unexpected content-type: %s", ctype) - // fallthrough - } return json.Unmarshal(rawRespBody, response) } diff --git a/internal/httpapi/call_test.go b/internal/httpapi/call_test.go index 8fbcb4fd6a..38347a9ba1 100644 --- a/internal/httpapi/call_test.go +++ b/internal/httpapi/call_test.go @@ -5,10 +5,8 @@ import ( "compress/gzip" "context" "errors" - "fmt" "io" "net/http" - "net/http/httptest" "net/url" "reflect" "strings" @@ -19,7 +17,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/model/mocks" "github.com/ooni/probe-cli/v3/internal/netxlite" - "github.com/ooni/probe-cli/v3/internal/runtimex" ) func Test_joinURLPath(t *testing.T) { @@ -191,7 +188,7 @@ func Test_newRequest(t *testing.T) { Accept: "", Authorization: "", ContentType: "", - LogBody: false, + LogBody: true, // just to exercise the code path MaxBodySize: 0, Method: http.MethodPost, RequestBody: []byte("deadbeef"), @@ -1227,297 +1224,6 @@ func TestCallWithJSONResponseHonoursContext(t *testing.T) { } } -func TestLogging(t *testing.T) { - - // This test was originally written for the httpx package and we have adapted it - // by keeping the ~same implementation with a custom callx function that converts - // the previous semantics of httpx to the new semantics of httpapi. - callx := func(baseURL string, logBody bool, logger model.Logger, request, response any) error { - desc := MustNewPOSTJSONWithJSONResponseDescriptor("/", request).WithBodyLogging(logBody) - runtimex.Assert(desc.LogBody == logBody, "desc.LogBody should be equal to logBody here") - endpoint := &Endpoint{ - BaseURL: baseURL, - HTTPClient: http.DefaultClient, - Logger: logger, - } - return CallWithJSONResponse(context.Background(), desc, endpoint, response) - } - - // we also needed to create a constructor for the logger - newlogger := func(logs chan string) model.Logger { - return &mocks.Logger{ - MockDebugf: func(format string, v ...interface{}) { - logs <- fmt.Sprintf(format, v...) - }, - MockWarnf: func(format string, v ...interface{}) { - logs <- fmt.Sprintf(format, v...) - }, - } - } - - t.Run("body logging enabled, 200 Ok, and without content-type", func(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc( - func(w http.ResponseWriter, r *http.Request) { - w.Write([]byte("[]")) - }, - )) - logs := make(chan string, 1024) - defer server.Close() - var ( - input []string - output []string - ) - logger := newlogger(logs) - err := callx(server.URL, true, logger, input, &output) - var found int - close(logs) - for entry := range logs { - if strings.HasPrefix(entry, "httpapi: request body: ") { - // we expect this because body logging is enabled - found |= 1 << 0 - continue - } - if strings.HasPrefix(entry, "httpapi: response body: ") { - // we expect this because body logging is enabled - found |= 1 << 1 - continue - } - if strings.HasPrefix(entry, "httpapi: unexpected content-type: ") { - // we would expect this because the server does not send us any content-type - found |= 1 << 2 - continue - } - if strings.HasPrefix(entry, "httpapi: request body length: ") { - // we should see this because we sent a body - found |= 1 << 3 - continue - } - if strings.HasPrefix(entry, "httpapi: response body length: ") { - // we should see this because we receive a body - found |= 1 << 4 - continue - } - } - if found != (1<<0 | 1<<1 | 1<<2 | 1<<3 | 1<<4) { - t.Fatal("did not find the expected logs") - } - if err != nil { - t.Fatal(err) - } - }) - - t.Run("body logging enabled, 200 Ok, and with content-type", func(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc( - func(w http.ResponseWriter, r *http.Request) { - w.Header().Add("content-type", "application/json") - w.Write([]byte("[]")) - }, - )) - logs := make(chan string, 1024) - defer server.Close() - var ( - input []string - output []string - ) - logger := newlogger(logs) - err := callx(server.URL, true, logger, input, &output) - var found int - close(logs) - for entry := range logs { - if strings.HasPrefix(entry, "httpapi: request body: ") { - // we expect this because body logging is enabled - found |= 1 << 0 - continue - } - if strings.HasPrefix(entry, "httpapi: response body: ") { - // we expect this because body logging is enabled - found |= 1 << 1 - continue - } - if strings.HasPrefix(entry, "httpapi: unexpected content-type: ") { - // we do not expect this because the server sends us a content-type - found |= 1 << 2 - continue - } - if strings.HasPrefix(entry, "httpapi: request body length: ") { - // we should see this because we sent a body - found |= 1 << 3 - continue - } - if strings.HasPrefix(entry, "httpapi: response body length: ") { - // we should see this because we receive a body - found |= 1 << 4 - continue - } - } - if found != (1<<0 | 1<<1 | 1<<3 | 1<<4) { - t.Fatal("did not find the expected logs") - } - if err != nil { - t.Fatal(err) - } - }) - - t.Run("body logging enabled and 401 Unauthorized", func(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc( - func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(401) - w.Write([]byte("[]")) - }, - )) - logs := make(chan string, 1024) - defer server.Close() - var ( - input []string - output []string - ) - logger := newlogger(logs) - err := callx(server.URL, true, logger, input, &output) - var found int - close(logs) - for entry := range logs { - if strings.HasPrefix(entry, "httpapi: request body: ") { - // should occur because body logging is enabled - found |= 1 << 0 - continue - } - if strings.HasPrefix(entry, "httpapi: response body: ") { - // should occur because body logging is enabled - found |= 1 << 1 - continue - } - if strings.HasPrefix(entry, "httpapi: unexpected content-type: ") { - // note: this one should not occur because the code is 401 so we're not - // actually going to parse the JSON document - found |= 1 << 2 - continue - } - if strings.HasPrefix(entry, "httpapi: request body length: ") { - // we should see this because we send a body - found |= 1 << 3 - continue - } - if strings.HasPrefix(entry, "httpapi: response body length: ") { - // we should see this because we receive a body - found |= 1 << 4 - continue - } - } - if found != (1<<0 | 1<<1 | 1<<3 | 1<<4) { - t.Fatal("did not find the expected logs") - } - var failure *ErrHTTPRequestFailed - if !errors.As(err, &failure) || failure.StatusCode != 401 { - t.Fatal("unexpected err", err) - } - }) - - t.Run("body logging NOT enabled and 200 Ok", func(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc( - func(w http.ResponseWriter, r *http.Request) { - w.Write([]byte("[]")) - }, - )) - logs := make(chan string, 1024) - defer server.Close() - var ( - input []string - output []string - ) - logger := newlogger(logs) - err := callx(server.URL, false, logger, input, &output) // no logging - var found int - close(logs) - for entry := range logs { - if strings.HasPrefix(entry, "httpapi: request body: ") { - // should not see it: body logging is disabled - found |= 1 << 0 - continue - } - if strings.HasPrefix(entry, "httpapi: response body: ") { - // should not see it: body logging is disabled - found |= 1 << 1 - continue - } - if strings.HasPrefix(entry, "httpapi: unexpected content-type: ") { - // this one should be logged ANYWAY because it's orthogonal to the - // body logging so we should see it also in this case. - found |= 1 << 2 - continue - } - if strings.HasPrefix(entry, "httpapi: request body length: ") { - // should see this because we send a body - found |= 1 << 3 - continue - } - if strings.HasPrefix(entry, "httpapi: response body length: ") { - // should see this because we're receiving a body - found |= 1 << 4 - continue - } - } - if found != (1<<2 | 1<<3 | 1<<4) { - t.Fatal("did not find the expected logs") - } - if err != nil { - t.Fatal(err) - } - }) - - t.Run("body logging NOT enabled and 401 Unauthorized", func(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc( - func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(401) - w.Write([]byte("[]")) - }, - )) - logs := make(chan string, 1024) - defer server.Close() - var ( - input []string - output []string - ) - logger := newlogger(logs) - err := callx(server.URL, false, logger, input, &output) // no logging - var found int - close(logs) - for entry := range logs { - if strings.HasPrefix(entry, "httpapi: request body: ") { - // should not see it: body logging is disabled - found |= 1 << 0 - continue - } - if strings.HasPrefix(entry, "httpapi: response body: ") { - // should not see it: body logging is disabled - found |= 1 << 1 - continue - } - if strings.HasPrefix(entry, "httpapi: unexpected content-type: ") { - // should not see it because we don't parse the body on 401 errors - found |= 1 << 2 - continue - } - if strings.HasPrefix(entry, "httpapi: request body length: ") { - // we send a body so we should see it - found |= 1 << 3 - continue - } - if strings.HasPrefix(entry, "httpapi: response body length: ") { - // we receive a body so we should see it - found |= 1 << 4 - continue - } - } - if found != (1<<3 | 1<<4) { - t.Fatal("did not find the expected logs") - } - var failure *ErrHTTPRequestFailed - if !errors.As(err, &failure) || failure.StatusCode != 401 { - t.Fatal("unexpected err", err) - } - }) -} - func Test_errMaybeCensorship_Unwrap(t *testing.T) { t.Run("for errors.Is", func(t *testing.T) { var err error = &errMaybeCensorship{io.EOF} diff --git a/internal/httpapi/descriptor.go b/internal/httpapi/descriptor.go index 99e020171d..d9f5050169 100644 --- a/internal/httpapi/descriptor.go +++ b/internal/httpapi/descriptor.go @@ -35,7 +35,7 @@ type Descriptor struct { LogBody bool // MaxBodySize is the OPTIONAL maximum response body size. If - // not set, we use the |DefaultMaxBodySize| constant. + // not set, we use the [DefaultMaxBodySize] constant. MaxBodySize int64 // Method is the MANDATORY request method. @@ -45,7 +45,7 @@ type Descriptor struct { RequestBody []byte // Timeout is the OPTIONAL timeout for this call. If no timeout - // is specified we will use the |DefaultCallTimeout| const. + // is specified we will use the [DefaultCallTimeout] const. Timeout time.Duration // URLPath is the MANDATORY URL path. @@ -55,7 +55,7 @@ type Descriptor struct { URLQuery url.Values } -// WithBodyLogging returns a SHALLOW COPY of |Descriptor| with LogBody set to |value|. You SHOULD +// WithBodyLogging returns a SHALLOW COPY of [Descriptor] with LogBody set to value. You SHOULD // only use this method when initializing the descriptor you want to use. func (desc *Descriptor) WithBodyLogging(value bool) *Descriptor { out := &Descriptor{} @@ -81,7 +81,7 @@ func NewGETJSONDescriptor(urlPath string) *Descriptor { const applicationJSON = "application/json" // NewGETJSONWithQueryDescriptor is like NewGETJSONDescriptor but it also -// allows you to provide |query| arguments. Leaving |query| nil or empty +// allows you to provide query arguments. Leaving query nil or empty // is equivalent to calling NewGETJSONDescriptor directly. func NewGETJSONWithQueryDescriptor(urlPath string, query url.Values) *Descriptor { return &Descriptor{ @@ -102,8 +102,8 @@ func NewGETJSONWithQueryDescriptor(urlPath string, query url.Values) *Descriptor // NewPOSTJSONWithJSONResponseDescriptor creates a descriptor that POSTs a JSON document // and expects to receive back a JSON document from the API. // -// This function ONLY fails if we cannot serialize the |request| to JSON. So, if you know -// that |request| is JSON-serializable, you can safely call MustNewPostJSONWithJSONResponseDescriptor instead. +// This function ONLY fails if we cannot serialize the request to JSON. So, if you know +// that request is JSON-serializable, you can safely call MustNewPostJSONWithJSONResponseDescriptor instead. func NewPOSTJSONWithJSONResponseDescriptor(urlPath string, request any) (*Descriptor, error) { rawRequest, err := json.Marshal(request) if err != nil { @@ -126,7 +126,7 @@ func NewPOSTJSONWithJSONResponseDescriptor(urlPath string, request any) (*Descri } // MustNewPOSTJSONWithJSONResponseDescriptor is like NewPOSTJSONWithJSONResponseDescriptor except that -// it panics in case it's not possible to JSON serialize the |request|. +// it panics in case it's not possible to JSON serialize the request. func MustNewPOSTJSONWithJSONResponseDescriptor(urlPath string, request any) *Descriptor { desc, err := NewPOSTJSONWithJSONResponseDescriptor(urlPath, request) runtimex.PanicOnError(err, "NewPOSTJSONWithJSONResponseDescriptor failed") @@ -134,7 +134,7 @@ func MustNewPOSTJSONWithJSONResponseDescriptor(urlPath string, request any) *Des } // NewGETResourceDescriptor creates a generic descriptor for GETting a -// resource of unspecified type using the given |urlPath|. +// resource of unspecified type using the given urlPath. func NewGETResourceDescriptor(urlPath string) *Descriptor { return &Descriptor{ Accept: "", diff --git a/internal/httpapi/doc.go b/internal/httpapi/doc.go index 0c1361cbca..c909322748 100644 --- a/internal/httpapi/doc.go +++ b/internal/httpapi/doc.go @@ -2,14 +2,14 @@ // // We model HTTP APIs as follows: // -// 1. |Endpoint| is an API endpoint (e.g., https://api.ooni.io); +// 1. [Endpoint] is an API endpoint (e.g., https://api.ooni.io); // -// 2. |Descriptor| describes the specific API you want to use (e.g., +// 2. [Descriptor] describes the specific API you want to use (e.g., // GET /api/v1/test-list/urls with JSON response body). // -// Generally, you use |Call| to call the API identified by a |Descriptor| -// on the specified |Endpoint|. However, there are cases where you -// need more complex calling patterns. For example, with |SequenceCaller| -// you can invoke the same API |Descriptor| with multiple equivalent -// API |Endpoint|s until one of them succeeds or all fail. +// Generally, you use [Call] to call the API identified by a [Descriptor] +// on the specified [Endpoint]. However, there are cases where you +// need more complex calling patterns. For example, with [SequenceCaller] +// you can invoke the same API [Descriptor] with multiple equivalent +// API [Endpoint]s until one of them succeeds or all fail. package httpapi diff --git a/internal/httpapi/endpoint.go b/internal/httpapi/endpoint.go index cc3a8d4ede..c0b8b89fb0 100644 --- a/internal/httpapi/endpoint.go +++ b/internal/httpapi/endpoint.go @@ -16,9 +16,9 @@ import "github.com/ooni/probe-cli/v3/internal/model" type Endpoint struct { // BaseURL is the MANDATORY endpoint base URL. We will honour the // path of this URL and prepend it to the actual path specified inside - // a |Descriptor.URLPath|. However, we will always discard any query + // a [Descriptor] URLPath. However, we will always discard any query // that may have been set inside the BaseURL. The only query string - // will be composed from the |Descriptor.URLQuery| values. + // will be composed from the [Descriptor] URLQuery values. // // For example, https://api.ooni.io. BaseURL string @@ -45,7 +45,7 @@ type Endpoint struct { UserAgent string } -// NewEndpointList constructs a list of API endpoints from |services| +// NewEndpointList constructs a list of API endpoints from services // returned by the OONI backend (or known in advance). // // Arguments: diff --git a/internal/httpapi/sequence.go b/internal/httpapi/sequence.go index da1f11d0ba..015618f90f 100644 --- a/internal/httpapi/sequence.go +++ b/internal/httpapi/sequence.go @@ -15,20 +15,20 @@ import ( "github.com/ooni/probe-cli/v3/internal/multierror" ) -// SequenceCaller calls the API specified by |Descriptor| once for each of -// the available |Endpoints| until one of them succeeds. +// SequenceCaller calls the API specified by [Descriptor] once for each of +// the available [Endpoint]s until one of them succeeds. // // CAVEAT: this code will ONLY retry API calls with subsequent endpoints when // the error originates in the HTTP round trip or while reading the body. type SequenceCaller struct { - // Descriptor is the API |Descriptor|. + // Descriptor is the API [Descriptor]. Descriptor *Descriptor - // Endpoints is the list of |Endpoint| to use. + // Endpoints is the list of [Endpoint] to use. Endpoints []*Endpoint } -// NewSequenceCaller is a factory for creating a |SequenceCaller|. +// NewSequenceCaller is a factory for creating a [SequenceCaller]. func NewSequenceCaller(desc *Descriptor, endpoints ...*Endpoint) *SequenceCaller { return &SequenceCaller{ Descriptor: desc, @@ -40,14 +40,14 @@ func NewSequenceCaller(desc *Descriptor, endpoints ...*Endpoint) *SequenceCaller var ErrAllEndpointsFailed = errors.New("httpapi: all endpoints failed") // shouldRetry returns true when we should try with another endpoint given the -// value of |err| which could (obviously) be nil in case of success. +// value of err which could (obviously) be nil in case of success. func (sc *SequenceCaller) shouldRetry(err error) bool { var kind *errMaybeCensorship belongs := errors.As(err, &kind) return belongs } -// Call calls |Call| for each |Endpoint| and |Descriptor| until one endpoint succeeds. The +// Call calls [Call] for each [Endpoint] and [Descriptor] until one endpoint succeeds. The // return value is the response body and the selected endpoint index or the error. // // CAVEAT: this code will ONLY retry API calls with subsequent endpoints when @@ -69,8 +69,8 @@ func (sc *SequenceCaller) Call(ctx context.Context) ([]byte, int, error) { return nil, -1, merr } -// CallWithJSONResponse is like |SequenceCaller.Call| except that it invokes the -// underlying |CallWithJSONResponse| rather than invoking |Call|. +// CallWithJSONResponse is like [SequenceCaller.Call] except that it invokes the +// underlying [CallWithJSONResponse] rather than invoking [Call]. // // CAVEAT: this code will ONLY retry API calls with subsequent endpoints when // the error originates in the HTTP round trip or while reading the body.