refactor(probeservices): httpx -> httpclientx (#1576)

This issue replaces httpx with httpclientx for probeservices. Part of ooni/probe#2723. --- Here are some checks to make sure we' not changing API semantics. ## GetTestHelpers Before: ```Go err = c.APIClientTemplate.WithBodyLogging().Build().GetJSON(ctx, "/api/v1/test-helpers", &output) ``` After: ```Go // construct the URL to use URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/test-helpers", "") if err != nil { return nil, err } // get the response return httpclientx.GetJSON[map[string][]model.OOAPIService](ctx, URL, &httpclientx.Config{ Client: c.HTTPClient, Logger: c.Logger, UserAgent: c.UserAgent, }) ``` In both cases: uses the same client and user agent, uses the same URL path, uses logging. ## OpenReport Before: ```Go var cor model.OOAPICollectorOpenResponse if err := c.APIClientTemplate.WithBodyLogging().Build().PostJSON(ctx, "/report", rt, &cor); err != nil { ``` After: ```Go URL, err := urlx.ResolveReference(c.BaseURL, "/report", "") if err != nil { return nil, err } cor, err := httpclientx.PostJSON[model.OOAPIReportTemplate, *model.OOAPICollectorOpenResponse]( ctx, URL, rt, &httpclientx.Config{ Client: c.HTTPClient, Logger: c.Logger, UserAgent: c.UserAgent, }, ) ``` In both cases: uses the same client and user agent, uses the same URL path, uses logging. ## SubmitMeasurement Before: ```Go err := r.client.APIClientTemplate.WithBodyLogging().Build().PostJSON( ctx, fmt.Sprintf("/report/%s", r.ID), model.OOAPICollectorUpdateRequest{ Format: "json", Content: m, }, &updateResponse, ``` After: ```Go URL, err := urlx.ResolveReference(r.client.BaseURL, fmt.Sprintf("/report/%s", r.ID), "") if err != nil { return err } apiReq := model.OOAPICollectorUpdateRequest{ Format: "json", Content: m, } updateResponse, err := httpclientx.PostJSON[ model.OOAPICollectorUpdateRequest, *model.OOAPICollectorUpdateResponse]( ctx, URL, apiReq, &httpclientx.Config{ Client: r.client.HTTPClient, Logger: r.client.Logger, UserAgent: r.client.UserAgent, }, ) ``` In both cases: uses the same client and user agent, uses the same URL path, uses logging. ## Login Before: ```Go var auth model.OOAPILoginAuth if err := c.APIClientTemplate.Build().PostJSON( ctx, "/api/v1/login", *creds, &auth); err != nil { ``` After: ```Go URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/login", "") if err != nil { return err } auth, err := httpclientx.PostJSON[*model.OOAPILoginCredentials, *model.OOAPILoginAuth]( ctx, URL, creds, &httpclientx.Config{ Client: c.HTTPClient, Logger: model.DiscardLogger, UserAgent: c.UserAgent, }, ) ``` In both cases: uses the same client and user agent, uses the same URL path, and we're not using logging. ## MeasurementMeta Before: ```Go var response model.OOAPIMeasurementMeta err := (&httpx.APIClientTemplate{ BaseURL: c.BaseURL, HTTPClient: c.HTTPClient, Logger: c.Logger, UserAgent: c.UserAgent, }).WithBodyLogging().Build().GetJSONWithQuery(ctx, "/api/v1/measurement_meta", query, &response) ``` After: ```Go // construct the URL to use URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/measurement_meta", query.Encode()) if err != nil { return nil, err } return &response, nil // get the response return httpclientx.GetJSON[*model.OOAPIMeasurementMeta](ctx, URL, &httpclientx.Config{ Client: c.HTTPClient, Logger: c.Logger, UserAgent: c.UserAgent, }) ``` In both cases: uses the same client and user agent, uses the same URL path, and logging. ## Psiphon Before: ```Go client := c.APIClientTemplate.BuildWithAuthorization(s) return client.FetchResource(ctx, "/api/v1/test-list/psiphon-config") ``` After: ```Go // construct the URL to use URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/test-list/psiphon-config", "") if err != nil { return nil, err } // get response // // use a model.DiscardLogger to avoid logging config return httpclientx.GetRaw(ctx, URL, &httpclientx.Config{ Authorization: s, Client: c.HTTPClient, Logger: model.DiscardLogger, UserAgent: c.UserAgent, }) ``` In both cases: uses the same client and user agent, uses the same URL path, and we're not logging. ## Register Before: ```Go var resp model.OOAPIRegisterResponse if err := c.APIClientTemplate.Build().PostJSON( ctx, "/api/v1/register", req, &resp); err != nil { ``` After: ```Go // construct the URL to use URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/register", "") if err != nil { return err } resp, err := httpclientx.PostJSON[*model.OOAPIRegisterRequest, *model.OOAPIRegisterResponse]( ctx, URL, req, &httpclientx.Config{ Client: c.HTTPClient, Logger: model.DiscardLogger, UserAgent: c.UserAgent, }, ) ``` In both cases: uses the same client and user agent, uses the same URL path, and we're not logging. ## Tor Before: ```Go client := c.APIClientTemplate.BuildWithAuthorization(s) err = client.GetJSONWithQuery( ctx, "/api/v1/test-list/tor-targets", query, &result) ``` After: ```Go // construct the URL to use URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/test-list/tor-targets", query.Encode()) if err != nil { return nil, err } // get response // // use a model.DiscardLogger to avoid logging bridges return httpclientx.GetJSON[map[string]model.OOAPITorTarget](ctx, URL, &httpclientx.Config{ Authorization: s, Client: c.HTTPClient, Logger: model.DiscardLogger, UserAgent: c.UserAgent, }) ``` In both cases: uses the same client and user agent, uses the same URL path, and we're not logging.
ooni · Apr 30, 2024 · ce62132 · ce62132
1 parent dedbae6
commit ce62132
Show file tree

Hide file tree

Showing 12 changed files with 320 additions and 35 deletions.
diff --git a/internal/probeservices/bouncer.go b/internal/probeservices/bouncer.go
@@ -1,14 +1,29 @@
 package probeservices
 
+//
+// bouncer.go - GET /api/v1/test-helpers
+//
+
 import (
 	"context"
 
+	"github.com/ooni/probe-cli/v3/internal/httpclientx"
 	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/urlx"
 )
 
-// GetTestHelpers is like GetCollectors but for test helpers.
-func (c Client) GetTestHelpers(
-	ctx context.Context) (output map[string][]model.OOAPIService, err error) {
-	err = c.APIClientTemplate.WithBodyLogging().Build().GetJSON(ctx, "/api/v1/test-helpers", &output)
-	return
+// GetTestHelpers queries the /api/v1/test-helpers API.
+func (c *Client) GetTestHelpers(ctx context.Context) (map[string][]model.OOAPIService, error) {
+	// construct the URL to use
+	URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/test-helpers", "")
+	if err != nil {
+		return nil, err
+	}
+
+	// get the response
+	return httpclientx.GetJSON[map[string][]model.OOAPIService](ctx, URL, &httpclientx.Config{
+		Client:    c.HTTPClient,
+		Logger:    c.Logger,
+		UserAgent: c.UserAgent,
+	})
 }
diff --git a/internal/probeservices/bouncer_test.go b/internal/probeservices/bouncer_test.go
@@ -162,4 +162,25 @@ func TestGetTestHelpers(t *testing.T) {
 			t.Fatal("expected result lenght to be zero")
 		}
 	})
+
+	t.Run("correctly handles the case where the URL is unparseable", func(t *testing.T) {
+		// create a probeservices client
+		client := newclient()
+
+		// override the URL to be unparseable
+		client.BaseURL = "\t\t\t"
+
+		// issue the GET request
+		testhelpers, err := client.GetTestHelpers(context.Background())
+
+		// we do expect an error
+		if err == nil || err.Error() != `parse "\t\t\t": net/url: invalid control character in URL` {
+			t.Fatal("unexpected error", err)
+		}
+
+		// we expect to see a zero-length / nil map
+		if len(testhelpers) != 0 {
+			t.Fatal("expected result lenght to be zero")
+		}
+	})
 }
diff --git a/internal/probeservices/collector.go b/internal/probeservices/collector.go
@@ -8,7 +8,9 @@ import (
 	"reflect"
 	"sync"
 
+	"github.com/ooni/probe-cli/v3/internal/httpclientx"
 	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/urlx"
 )
 
 var (
@@ -58,10 +60,24 @@ func (c Client) OpenReport(ctx context.Context, rt model.OOAPIReportTemplate) (R
 	if rt.Format != model.OOAPIReportDefaultFormat {
 		return nil, ErrUnsupportedFormat
 	}
-	var cor model.OOAPICollectorOpenResponse
-	if err := c.APIClientTemplate.WithBodyLogging().Build().PostJSON(ctx, "/report", rt, &cor); err != nil {
+
+	URL, err := urlx.ResolveReference(c.BaseURL, "/report", "")
+	if err != nil {
+		return nil, err
+	}
+
+	cor, err := httpclientx.PostJSON[model.OOAPIReportTemplate, *model.OOAPICollectorOpenResponse](
+		ctx, URL, rt, &httpclientx.Config{
+			Client:    c.HTTPClient,
+			Logger:    c.Logger,
+			UserAgent: c.UserAgent,
+		},
+	)
+
+	if err != nil {
 		return nil, err
 	}
+
 	for _, format := range cor.SupportedFormats {
 		if format == "json" {
 			return &reportChan{ID: cor.ReportID, client: c, tmpl: rt}, nil
@@ -83,18 +99,35 @@ func (r reportChan) CanSubmit(m *model.Measurement) bool {
 // submitted. Otherwise, we'll set the report ID to the empty
 // string, so that you know which measurements weren't submitted.
 func (r reportChan) SubmitMeasurement(ctx context.Context, m *model.Measurement) error {
-	var updateResponse model.OOAPICollectorUpdateResponse
+	// TODO(bassosimone): do we need to prevent measurement submission
+	// if the measurement isn't consistent with the orig template?
+
 	m.ReportID = r.ID
-	err := r.client.APIClientTemplate.WithBodyLogging().Build().PostJSON(
-		ctx, fmt.Sprintf("/report/%s", r.ID), model.OOAPICollectorUpdateRequest{
-			Format:  "json",
-			Content: m,
-		}, &updateResponse,
+
+	URL, err := urlx.ResolveReference(r.client.BaseURL, fmt.Sprintf("/report/%s", r.ID), "")
+	if err != nil {
+		return err
+	}
+
+	apiReq := model.OOAPICollectorUpdateRequest{
+		Format:  "json",
+		Content: m,
+	}
+
+	updateResponse, err := httpclientx.PostJSON[
+		model.OOAPICollectorUpdateRequest, *model.OOAPICollectorUpdateResponse](
+		ctx, URL, apiReq, &httpclientx.Config{
+			Client:    r.client.HTTPClient,
+			Logger:    r.client.Logger,
+			UserAgent: r.client.UserAgent,
+		},
 	)
+
 	if err != nil {
 		m.ReportID = ""
 		return err
 	}
+
 	// TODO(bassosimone): we should use the session logger here but for now this stopgap
 	// solution will allow observing the measurement URL for CLI users.
 	log.Printf("Measurement URL: https://explorer.ooni.org/m/%s", updateResponse.MeasurementUID)

diff --git a/internal/probeservices/login.go b/internal/probeservices/login.go
@@ -3,7 +3,9 @@ package probeservices
 import (
 	"context"
 
+	"github.com/ooni/probe-cli/v3/internal/httpclientx"
 	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/urlx"
 )
 
 // MaybeLogin performs login if necessary
@@ -17,11 +19,24 @@ func (c Client) MaybeLogin(ctx context.Context) error {
 		return ErrNotRegistered
 	}
 	c.LoginCalls.Add(1)
-	var auth model.OOAPILoginAuth
-	if err := c.APIClientTemplate.Build().PostJSON(
-		ctx, "/api/v1/login", *creds, &auth); err != nil {
+
+	URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/login", "")
+	if err != nil {
+		return err
+	}
+
+	auth, err := httpclientx.PostJSON[*model.OOAPILoginCredentials, *model.OOAPILoginAuth](
+		ctx, URL, creds, &httpclientx.Config{
+			Client:    c.HTTPClient,
+			Logger:    model.DiscardLogger,
+			UserAgent: c.UserAgent,
+		},
+	)
+
+	if err != nil {
 		return err
 	}
+
 	state.Expire = auth.Expire
 	state.Token = auth.Token
 	return c.StateFile.Set(state)

diff --git a/internal/probeservices/measurementmeta.go b/internal/probeservices/measurementmeta.go
@@ -1,16 +1,22 @@
 package probeservices
 
+//
+// measurementmeta.go - GET /api/v1/measurement_meta
+//
+
 import (
 	"context"
 	"net/url"
 
-	"github.com/ooni/probe-cli/v3/internal/httpx"
+	"github.com/ooni/probe-cli/v3/internal/httpclientx"
 	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/urlx"
 )
 
 // GetMeasurementMeta returns meta information about a measurement.
 func (c Client) GetMeasurementMeta(
 	ctx context.Context, config model.OOAPIMeasurementMetaConfig) (*model.OOAPIMeasurementMeta, error) {
+	// construct the query to use
 	query := url.Values{}
 	query.Add("report_id", config.ReportID)
 	if config.Input != "" {
@@ -19,15 +25,17 @@ func (c Client) GetMeasurementMeta(
 	if config.Full {
 		query.Add("full", "true")
 	}
-	var response model.OOAPIMeasurementMeta
-	err := (&httpx.APIClientTemplate{
-		BaseURL:    c.BaseURL,
-		HTTPClient: c.HTTPClient,
-		Logger:     c.Logger,
-		UserAgent:  c.UserAgent,
-	}).WithBodyLogging().Build().GetJSONWithQuery(ctx, "/api/v1/measurement_meta", query, &response)
+
+	// construct the URL to use
+	URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/measurement_meta", query.Encode())
 	if err != nil {
 		return nil, err
 	}
-	return &response, nil
+
+	// get the response
+	return httpclientx.GetJSON[*model.OOAPIMeasurementMeta](ctx, URL, &httpclientx.Config{
+		Client:    c.HTTPClient,
+		Logger:    c.Logger,
+		UserAgent: c.UserAgent,
+	})
 }
diff --git a/internal/probeservices/psiphon.go b/internal/probeservices/psiphon.go
@@ -3,15 +3,36 @@ package probeservices
 import (
 	"context"
 	"fmt"
+
+	"github.com/ooni/probe-cli/v3/internal/httpclientx"
+	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/urlx"
 )
 
 // FetchPsiphonConfig fetches psiphon config from authenticated OONI orchestra.
 func (c Client) FetchPsiphonConfig(ctx context.Context) ([]byte, error) {
+	// get credentials and authentication token
 	_, auth, err := c.GetCredsAndAuth()
 	if err != nil {
 		return nil, err
 	}
+
+	// format Authorization header value
 	s := fmt.Sprintf("Bearer %s", auth.Token)
-	client := c.APIClientTemplate.BuildWithAuthorization(s)
-	return client.FetchResource(ctx, "/api/v1/test-list/psiphon-config")
+
+	// construct the URL to use
+	URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/test-list/psiphon-config", "")
+	if err != nil {
+		return nil, err
+	}
+
+	// get response
+	//
+	// use a model.DiscardLogger to avoid logging config
+	return httpclientx.GetRaw(ctx, URL, &httpclientx.Config{
+		Authorization: s,
+		Client:        c.HTTPClient,
+		Logger:        model.DiscardLogger,
+		UserAgent:     c.UserAgent,
+	})
 }
diff --git a/internal/probeservices/psiphon_test.go b/internal/probeservices/psiphon_test.go
@@ -9,6 +9,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/ooni/probe-cli/v3/internal/mocks"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
 	"github.com/ooni/probe-cli/v3/internal/runtimex"
@@ -198,4 +199,57 @@ func TestFetchPsiphonConfig(t *testing.T) {
 			t.Fatal("expected zero length data")
 		}
 	})
+
+	t.Run("is not logging the response body", func(t *testing.T) {
+		// create state for emulating the OONI backend
+		state := &testingx.OONIBackendWithLoginFlow{}
+
+		// make sure we return something that is JSON parseable
+		state.SetPsiphonConfig([]byte(`{}`))
+
+		// expose the state via HTTP
+		srv := testingx.MustNewHTTPServer(state.NewMux())
+		defer srv.Close()
+
+		// create a probeservices client
+		client := newclient()
+
+		// create and use a logger for collecting logs
+		logger := &testingx.Logger{}
+		client.Logger = logger
+
+		// override the HTTP client so we speak with our local server rather than the true backend
+		client.HTTPClient = &mocks.HTTPClient{
+			MockDo: func(req *http.Request) (*http.Response, error) {
+				URL := runtimex.Try1(url.Parse(srv.URL))
+				req.URL.Scheme = URL.Scheme
+				req.URL.Host = URL.Host
+				return http.DefaultClient.Do(req)
+			},
+			MockCloseIdleConnections: func() {
+				http.DefaultClient.CloseIdleConnections()
+			},
+		}
+
+		// then we can try to fetch the config
+		data, err := psiphonflow(t, client)
+
+		// we do not expect an error here
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		// the config is bytes but we want to make sure we can parse it
+		var config interface{}
+		if err := json.Unmarshal(data, &config); err != nil {
+			t.Fatal(err)
+		}
+
+		// assert that there are no logs
+		//
+		// the register, login, and psiphon API should not log their bodies
+		if diff := cmp.Diff([]string{}, logger.AllLines()); diff != "" {
+			t.Fatal(diff)
+		}
+	})
 }
diff --git a/internal/probeservices/register.go b/internal/probeservices/register.go
@@ -1,10 +1,16 @@
 package probeservices
 
+//
+// register.go - POST /api/v1/register
+//
+
 import (
 	"context"
 
+	"github.com/ooni/probe-cli/v3/internal/httpclientx"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/randx"
+	"github.com/ooni/probe-cli/v3/internal/urlx"
 )
 
 // MaybeRegister registers this client if not already registered
@@ -24,11 +30,24 @@ func (c Client) MaybeRegister(ctx context.Context, metadata model.OOAPIProbeMeta
 		OOAPIProbeMetadata: metadata,
 		Password:           pwd,
 	}
-	var resp model.OOAPIRegisterResponse
-	if err := c.APIClientTemplate.Build().PostJSON(
-		ctx, "/api/v1/register", req, &resp); err != nil {
+
+	// construct the URL to use
+	URL, err := urlx.ResolveReference(c.BaseURL, "/api/v1/register", "")
+	if err != nil {
+		return err
+	}
+
+	resp, err := httpclientx.PostJSON[*model.OOAPIRegisterRequest, *model.OOAPIRegisterResponse](
+		ctx, URL, req, &httpclientx.Config{
+			Client:    c.HTTPClient,
+			Logger:    model.DiscardLogger,
+			UserAgent: c.UserAgent,
+		},
+	)
+	if err != nil {
 		return err
 	}
+
 	state.ClientID = resp.ClientID
 	state.Password = pwd
 	return c.StateFile.Set(state)