-
Notifications
You must be signed in to change notification settings - Fork 54
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
feat: introduce the httpclientx package (#1569)
This package tries to replace both `httpx` and `httpapi`. This commit includes a design document explaining the rationale and the process for performing this replacement. I have been using this package for a while at #1560. It seems clear to me that, at this point, we want to merge. So, I have extracted just the package from #1560 to reduce the size of the diff. The TL;DR of this new package is to ensure we have the same functionality of `httpx` and `httpapi` but we're also simplifying the codebase by (a) replacing two other packages and (b) dropping the requirement that we wanted to generate a swagger from the API implementation, which lead to overly complex code, and, what is possibly worst, to code that at the end of the day we were not using for comparing to the backend's swagger. Part of ooni/probe#2700.
- Loading branch information
1 parent
beb1fc7
commit 3a4652e
Showing
17 changed files
with
2,529 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
// Package erroror contains code to represent an error or a value. | ||
package erroror | ||
|
||
// Value represents an error or a value. | ||
type Value[Type any] struct { | ||
Err error | ||
Value Type | ||
} |
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
package httpclientx | ||
|
||
import "github.com/ooni/probe-cli/v3/internal/model" | ||
|
||
// Config contains configuration shared by [GetJSON], [GetXML], [GetRaw], and [PostJSON]. | ||
// | ||
// The zero value is invalid; initialize the MANDATORY fields. | ||
type Config struct { | ||
// Authorization contains the OPTIONAL Authorization header value to use. | ||
Authorization string | ||
|
||
// Client is the MANDATORY [model.HTTPClient] to use. | ||
Client model.HTTPClient | ||
|
||
// Logger is the MANDATORY [model.Logger] to use. | ||
Logger model.Logger | ||
|
||
// UserAgent is the MANDATORY User-Agent header value to use. | ||
UserAgent string | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
package httpclientx | ||
|
||
// | ||
// getjson.go - GET a JSON response. | ||
// | ||
|
||
import ( | ||
"context" | ||
"encoding/json" | ||
) | ||
|
||
// GetJSON sends a GET request and reads a JSON response. | ||
// | ||
// Arguments: | ||
// | ||
// - ctx is the cancellable context; | ||
// | ||
// - URL is the URL to use; | ||
// | ||
// - config contains the config. | ||
// | ||
// This function either returns an error or a valid Output. | ||
func GetJSON[Output any](ctx context.Context, URL string, config *Config) (Output, error) { | ||
return NewOverlappedGetJSON[Output](config).Run(ctx, URL) | ||
} | ||
|
||
func getJSON[Output any](ctx context.Context, URL string, config *Config) (Output, error) { | ||
// read the raw body | ||
rawrespbody, err := GetRaw(ctx, URL, config) | ||
|
||
// handle the case of error | ||
if err != nil { | ||
return zeroValue[Output](), err | ||
} | ||
|
||
// parse the response body as JSON | ||
var output Output | ||
if err := json.Unmarshal(rawrespbody, &output); err != nil { | ||
return zeroValue[Output](), err | ||
} | ||
|
||
// avoid returning nil pointers, maps, slices | ||
return NilSafetyErrorIfNil(output) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,290 @@ | ||
package httpclientx | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
"net/http" | ||
"strings" | ||
"sync" | ||
"testing" | ||
|
||
"github.com/google/go-cmp/cmp" | ||
"github.com/ooni/probe-cli/v3/internal/model" | ||
"github.com/ooni/probe-cli/v3/internal/netxlite" | ||
"github.com/ooni/probe-cli/v3/internal/testingx" | ||
) | ||
|
||
type apiResponse struct { | ||
Age int | ||
Name string | ||
} | ||
|
||
func TestGetJSON(t *testing.T) { | ||
t.Run("when GetRaw fails", func(t *testing.T) { | ||
// create a server that RST connections | ||
server := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset()) | ||
defer server.Close() | ||
|
||
// invoke the API | ||
resp, err := GetJSON[*apiResponse](context.Background(), server.URL, &Config{ | ||
Client: http.DefaultClient, | ||
Logger: model.DiscardLogger, | ||
UserAgent: model.HTTPHeaderUserAgent, | ||
}) | ||
|
||
t.Log(resp) | ||
t.Log(err) | ||
|
||
// make sure that the error is the expected one | ||
if !errors.Is(err, netxlite.ECONNRESET) { | ||
t.Fatal("unexpected error", err) | ||
} | ||
|
||
// make sure the response is nil. | ||
if resp != nil { | ||
t.Fatal("expected nil response") | ||
} | ||
}) | ||
|
||
t.Run("when JSON parsing fails", func(t *testing.T) { | ||
// create a server that returns an invalid JSON type | ||
server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
w.Write([]byte("[]")) | ||
})) | ||
defer server.Close() | ||
|
||
// invoke the API | ||
resp, err := GetJSON[*apiResponse](context.Background(), server.URL, &Config{ | ||
Client: http.DefaultClient, | ||
Logger: model.DiscardLogger, | ||
UserAgent: model.HTTPHeaderUserAgent, | ||
}) | ||
|
||
t.Log(resp) | ||
t.Log(err) | ||
|
||
// make sure that the error is the expected one | ||
if err.Error() != "json: cannot unmarshal array into Go value of type httpclientx.apiResponse" { | ||
t.Fatal("unexpected error", err) | ||
} | ||
|
||
// make sure the response is nil. | ||
if resp != nil { | ||
t.Fatal("expected nil response") | ||
} | ||
}) | ||
|
||
t.Run("on success", func(t *testing.T) { | ||
// create a server that returns a legit response | ||
server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
w.Write([]byte(`{"Name": "simone", "Age": 41}`)) | ||
})) | ||
defer server.Close() | ||
|
||
// invoke the API | ||
resp, err := GetJSON[*apiResponse](context.Background(), server.URL, &Config{ | ||
Client: http.DefaultClient, | ||
Logger: model.DiscardLogger, | ||
UserAgent: model.HTTPHeaderUserAgent, | ||
}) | ||
|
||
t.Log(resp) | ||
t.Log(err) | ||
|
||
// make sure that the error is the expected one | ||
if err != nil { | ||
t.Fatal("unexpected error", err) | ||
} | ||
|
||
// make sure the response is OK | ||
expect := &apiResponse{Name: "simone", Age: 41} | ||
if diff := cmp.Diff(expect, resp); diff != "" { | ||
t.Fatal(diff) | ||
} | ||
}) | ||
} | ||
|
||
// This test ensures that GetJSON sets correct HTTP headers | ||
func TestGetJSONHeadersOkay(t *testing.T) { | ||
var ( | ||
gotheaders http.Header | ||
gotmu sync.Mutex | ||
) | ||
|
||
server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
// save the headers | ||
gotmu.Lock() | ||
gotheaders = r.Header | ||
gotmu.Unlock() | ||
|
||
// send a minimal 200 Ok response | ||
w.WriteHeader(200) | ||
w.Write([]byte(`{}`)) | ||
})) | ||
defer server.Close() | ||
|
||
// send the request and receive the response | ||
apiresp, err := GetJSON[*apiResponse](context.Background(), server.URL, &Config{ | ||
Authorization: "scribai", | ||
Client: http.DefaultClient, | ||
Logger: model.DiscardLogger, | ||
UserAgent: model.HTTPHeaderUserAgent, | ||
}) | ||
|
||
// we do not expect to see an error here | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
// given the handler, we expect to see an empty structure here | ||
if apiresp.Age != 0 || apiresp.Name != "" { | ||
t.Fatal("expected empty response") | ||
} | ||
|
||
// make sure there are no data races | ||
defer gotmu.Unlock() | ||
gotmu.Lock() | ||
|
||
// make sure we have sent the authorization header | ||
if value := gotheaders.Get("Authorization"); value != "scribai" { | ||
t.Fatal("unexpected Authorization value", value) | ||
} | ||
|
||
// now make sure we have sent user-agent | ||
if value := gotheaders.Get("User-Agent"); value != model.HTTPHeaderUserAgent { | ||
t.Fatal("unexpected User-Agent value", value) | ||
} | ||
|
||
// now make sure we have sent accept-encoding | ||
if value := gotheaders.Get("Accept-Encoding"); value != "gzip" { | ||
t.Fatal("unexpected Accept-Encoding value", value) | ||
} | ||
} | ||
|
||
// This test ensures GetJSON logs the response body at Debug level. | ||
func TestGetJSONLoggingOkay(t *testing.T) { | ||
// create a server that returns a legit response | ||
server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
w.Write([]byte(`{"Name": "simone", "Age": 41}`)) | ||
})) | ||
defer server.Close() | ||
|
||
// instantiate a logger that collects logs | ||
logger := &testingx.Logger{} | ||
|
||
// invoke the API | ||
resp, err := GetJSON[*apiResponse](context.Background(), server.URL, &Config{ | ||
Client: http.DefaultClient, | ||
Logger: logger, | ||
UserAgent: model.HTTPHeaderUserAgent, | ||
}) | ||
|
||
t.Log(resp) | ||
t.Log(err) | ||
|
||
// make sure that the error is the expected one | ||
if err != nil { | ||
t.Fatal("unexpected error", err) | ||
} | ||
|
||
// make sure the response is OK | ||
expect := &apiResponse{Name: "simone", Age: 41} | ||
if diff := cmp.Diff(expect, resp); diff != "" { | ||
t.Fatal(diff) | ||
} | ||
|
||
// collect and verify the debug lines | ||
debuglines := logger.DebugLines() | ||
t.Log(debuglines) | ||
if len(debuglines) != 1 { | ||
t.Fatal("expected to see a single debug line") | ||
} | ||
if !strings.Contains(debuglines[0], "raw response body:") { | ||
t.Fatal("did not see raw response body log line") | ||
} | ||
} | ||
|
||
// TestGetJSONCorrectlyRejectsNilValues ensures we correctly reject nil values. | ||
func TestGetJSONCorrectlyRejectsNilValues(t *testing.T) { | ||
|
||
t.Run("when unmarshaling into a map", func(t *testing.T) { | ||
server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
w.Write([]byte(`null`)) | ||
})) | ||
defer server.Close() | ||
|
||
// invoke the API | ||
resp, err := GetJSON[map[string]string](context.Background(), server.URL, &Config{ | ||
Client: http.DefaultClient, | ||
Logger: model.DiscardLogger, | ||
UserAgent: model.HTTPHeaderUserAgent, | ||
}) | ||
|
||
t.Log(resp) | ||
t.Log(err) | ||
|
||
// make sure that the error is the expected one | ||
if !errors.Is(err, ErrIsNil) { | ||
t.Fatal("unexpected error", err) | ||
} | ||
|
||
// make sure resp is nil | ||
if resp != nil { | ||
t.Fatal("expected nil resp") | ||
} | ||
}) | ||
|
||
t.Run("when unmarshaling into a struct pointer", func(t *testing.T) { | ||
server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
w.Write([]byte(`null`)) | ||
})) | ||
defer server.Close() | ||
|
||
// invoke the API | ||
resp, err := GetJSON[*apiResponse](context.Background(), server.URL, &Config{ | ||
Client: http.DefaultClient, | ||
Logger: model.DiscardLogger, | ||
UserAgent: model.HTTPHeaderUserAgent, | ||
}) | ||
|
||
t.Log(resp) | ||
t.Log(err) | ||
|
||
// make sure that the error is the expected one | ||
if !errors.Is(err, ErrIsNil) { | ||
t.Fatal("unexpected error", err) | ||
} | ||
|
||
// make sure resp is nil | ||
if resp != nil { | ||
t.Fatal("expected nil resp") | ||
} | ||
}) | ||
|
||
t.Run("when unmarshaling into a slice", func(t *testing.T) { | ||
server := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
w.Write([]byte(`null`)) | ||
})) | ||
defer server.Close() | ||
|
||
// invoke the API | ||
resp, err := GetJSON[[]string](context.Background(), server.URL, &Config{ | ||
Client: http.DefaultClient, | ||
Logger: model.DiscardLogger, | ||
UserAgent: model.HTTPHeaderUserAgent, | ||
}) | ||
|
||
t.Log(resp) | ||
t.Log(err) | ||
|
||
// make sure that the error is the expected one | ||
if !errors.Is(err, ErrIsNil) { | ||
t.Fatal("unexpected error", err) | ||
} | ||
|
||
// make sure resp is nil | ||
if resp != nil { | ||
t.Fatal("expected nil resp") | ||
} | ||
}) | ||
} |
Oops, something went wrong.