Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(probeservices): use better naming #1628

Merged
merged 8 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion internal/engine/experiment_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package engine
import (
"context"
"encoding/json"
"errors"
"net/http"
"net/http/httptest"
"os"
Expand All @@ -11,6 +12,7 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/probeservices"
"github.com/ooni/probe-cli/v3/internal/registry"
)

Expand Down Expand Up @@ -384,7 +386,7 @@ func TestOpenReportNewClientFailure(t *testing.T) {
Type: "antani",
}
err = exp.OpenReportContext(context.Background())
if err.Error() != "probe services: unsupported endpoint type" {
if !errors.Is(err, probeservices.ErrUnsupportedServiceType) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I upgraded this line to use errors.Is which is the idiomatic way of checking after errors.Is was added to Go.

t.Fatal(err)
}
}
Expand Down
4 changes: 2 additions & 2 deletions internal/engine/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -673,8 +673,8 @@ func (s *Session) MaybeLookupBackendsContext(ctx context.Context) error {
if selected == nil {
return ErrAllProbeServicesFailed
}
s.logger.Infof("session: using probe services: %+v", selected.Endpoint)
s.selectedProbeService = &selected.Endpoint
s.logger.Infof("session: using probe services: %+v", selected.Service)
s.selectedProbeService = &selected.Service
s.availableTestHelpers = selected.TestHelpers
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion internal/engine/session_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ func TestNewOrchestraClientProbeServicesNewClientFailure(t *testing.T) {
svc.Type = "antani" // should really not be supported for a long time
}
client, err := sess.newOrchestraClient(context.Background())
if !errors.Is(err, probeservices.ErrUnsupportedEndpoint) {
if !errors.Is(err, probeservices.ErrUnsupportedServiceType) {
t.Fatal("not the error we expected")
}
if client != nil {
Expand Down
2 changes: 1 addition & 1 deletion internal/enginelocate/cloudflare.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func cloudflareIPLookup(
// get the raw response body
data, err := httpclientx.GetRaw(
ctx,
httpclientx.NewEndpoint("https://www.cloudflare.com/cdn-cgi/trace"),
httpclientx.NewBaseURL("https://www.cloudflare.com/cdn-cgi/trace"),
&httpclientx.Config{
Authorization: "", // not needed
Client: httpClient,
Expand Down
2 changes: 1 addition & 1 deletion internal/enginelocate/ubuntu.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func ubuntuIPLookup(
// read the HTTP response and parse as XML
v, err := httpclientx.GetXML[*ubuntuResponse](
ctx,
httpclientx.NewEndpoint("https://geoip.ubuntu.com/lookup"),
httpclientx.NewBaseURL("https://geoip.ubuntu.com/lookup"),
&httpclientx.Config{
Authorization: "", // not needed
Client: httpClient,
Expand Down
32 changes: 16 additions & 16 deletions internal/httpclientx/DESIGN.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,18 +76,18 @@ type Config struct {
UserAgent string
}

type Endpoint struct {
URL string
Host string // optional for cloudfronting
type BaseURL struct {
Value string // mandatory base-URL value
HostOverride string // optional for cloudfronting
}

func GetJSON[Output any](ctx context.Context, epnt *Endpoint, config *Config) (Output, error)
func GetJSON[Output any](ctx context.Context, base *BaseURL, config *Config) (Output, error)

func GetRaw(ctx context.Context, epnt *Endpoint, config *Config) ([]byte, error)
func GetRaw(ctx context.Context, base *BaseURL, config *Config) ([]byte, error)

func GetXML[Output any](ctx context.Context, epnt *Endpoint, config *Config) (Output, error)
func GetXML[Output any](ctx context.Context, base *BaseURL, config *Config) (Output, error)

func PostJSON[Input, Output any](ctx context.Context, epnt *Endpoint, input Input, config *Config) (Output, error)
func PostJSON[Input, Output any](ctx context.Context, base *BaseURL, input Input, config *Config) (Output, error)
```

(The `*Config` is the last argument because it is handy to create it inline when calling
Expand Down Expand Up @@ -124,9 +124,9 @@ To avoid logging bodies, one just needs to pass `model.DiscardLogger` as the
The code at `./internal/httpapi` performs sequential function calls. This design
does not interact well with the `enginenetx` package and its dial tactics. A better
strategy is to allow calls to be overlapped. This means that, if the `enginenetx`
is busy trying tactics for a given API endpoint, we eventually try to use the
subsequent (semantically-equivalent) endpoint after a given time, without waiting
for the first endpoint to complete.
is busy trying tactics for a given API base URL, we eventually try to use the
subsequent (semantically-equivalent) base URL after a given time, without waiting
for the first base URL to complete.

We allow for overlapped operations by defining these constructors:

Expand All @@ -144,7 +144,7 @@ They all construct the same `*Overlapped` struct, which looks like this:

```Go
type Overlapped[Output any] struct {
RunFunc func(ctx context.Context, epnt *Endpoint) (Output, error)
RunFunc func(ctx context.Context, base *BaseURL) (Output, error)

ScheduleInterval time.Duration
}
Expand All @@ -156,15 +156,15 @@ name (i.e., `NewOverlappedGetXML` configures `RunFunc` to run `GetXML`).
Then, we define the following method:

```Go
func (ovx *Overlapped[Output]) Run(ctx context.Context, epnts ...*Endpoint) (Output, error)
func (ovx *Overlapped[Output]) Run(ctx context.Context, bases ...*BaseURL) (Output, error)
```

This method starts N goroutines to issue the API calls with each endpoint URL. (A classic example
This method starts N goroutines to issue the API calls with each base URL. (A classic example
is for the URLs to be `https://0.th.ooni.org/`, `https://1.th.ooni.org/` and so on.)

By default, `ScheduleInterval` is 15 seconds. If the first endpoint URL does not provide a result
By default, `ScheduleInterval` is 15 seconds. If the first base URL does not provide a result
within 15 seconds, we try the second one. That is, every 15 seconds, we will attempt using
another endpoint URL, until there's a successful response or we run out of URLs.
another base URL, until there's a successful response or we run out of URLs.

As soon as we have a successful response, we cancel all the other pending operations
that may exist. Once all operations have terminated, we return to the caller.
Expand Down Expand Up @@ -329,7 +329,7 @@ that is part of the same package, while in this package we marshal in `PostJSON`
Consider the following code snippet:

```Go
resp, err := httpclientx.GetJSON[*APIResponse](ctx, epnt, config)
resp, err := httpclientx.GetJSON[*APIResponse](ctx, base, config)
runtimex.Assert((resp == nil && err != nil) || (resp != nil && err == nil), "ouch")
```

Expand Down
49 changes: 49 additions & 0 deletions internal/httpclientx/baseurl.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package httpclientx

import "github.com/ooni/probe-cli/v3/internal/model"

// BaseURL is an HTTP-endpoint base URL.
//
// The zero value is invalid; construct using [NewBaseURL].
type BaseURL struct {
// Value is the MANDATORY base-URL Value.
Value string

// HostOverride is the OPTIONAL host header to use for cloudfronting.
HostOverride string
}

// NewBaseURL constructs a new [*BaseURL] instance using the given URL.
func NewBaseURL(URL string) *BaseURL {
return &BaseURL{
Value: URL,
HostOverride: "",
}
}

// WithHostOverride returns a copy of the [*BaseURL] using the given host header override.
func (e *BaseURL) WithHostOverride(host string) *BaseURL {
return &BaseURL{
Value: e.Value,
HostOverride: host,
}
}

// NewBaseURLsFromModelOOAPIServices constructs new [*BaseURL] instances from the
// given [model.OOAPIService] instances, assigning the host header if "cloudfront", and
// skipping all the entries that are neither "https" not "cloudfront".
func NewBaseURLsFromModelOOAPIServices(svcs ...model.OOAPIService) (bases []*BaseURL) {
for _, svc := range svcs {
base := NewBaseURL(svc.Address)
switch svc.Type {
case "cloudfront":
base = base.WithHostOverride(svc.Front)
fallthrough
case "https":
bases = append(bases, base)
default:
// skip entry
}
}
return
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,23 @@ import (
"github.com/ooni/probe-cli/v3/internal/model"
)

func TestEndpoint(t *testing.T) {
func TestBaseURL(t *testing.T) {
t.Run("the constructor only assigns the URL", func(t *testing.T) {
epnt := NewEndpoint("https://www.example.com/")
if epnt.URL != "https://www.example.com/" {
base := NewBaseURL("https://www.example.com/")
if base.Value != "https://www.example.com/" {
t.Fatal("unexpected URL")
}
if epnt.Host != "" {
if base.HostOverride != "" {
t.Fatal("unexpected host")
}
})

t.Run("we can optionally get a copy with an assigned host header", func(t *testing.T) {
epnt := NewEndpoint("https://www.example.com/").WithHostOverride("www.cloudfront.com")
if epnt.URL != "https://www.example.com/" {
base := NewBaseURL("https://www.example.com/").WithHostOverride("www.cloudfront.com")
if base.Value != "https://www.example.com/" {
t.Fatal("unexpected URL")
}
if epnt.Host != "www.cloudfront.com" {
if base.HostOverride != "www.cloudfront.com" {
t.Fatal("unexpected host")
}
})
Expand All @@ -50,14 +50,14 @@ func TestEndpoint(t *testing.T) {
Front: "",
}}

expect := []*Endpoint{{
URL: "https://www.example.com/",
expect := []*BaseURL{{
Value: "https://www.example.com/",
}, {
URL: "https://www.example.com/",
Host: "www.cloudfront.com",
Value: "https://www.example.com/",
HostOverride: "www.cloudfront.com",
}}

got := NewEndpointFromModelOOAPIServices(services...)
got := NewBaseURLsFromModelOOAPIServices(services...)
if diff := cmp.Diff(expect, got); diff != "" {
t.Fatal(diff)
}
Expand Down
49 changes: 0 additions & 49 deletions internal/httpclientx/endpoint.go

This file was deleted.

10 changes: 5 additions & 5 deletions internal/httpclientx/getjson.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,18 @@ import (
//
// - ctx is the cancellable context;
//
// - epnt is the HTTP [*Endpoint] to use;
// - base is the HTTP [*BaseURL] to use;
//
// - config contains the config.
//
// This function either returns an error or a valid Output.
func GetJSON[Output any](ctx context.Context, epnt *Endpoint, config *Config) (Output, error) {
return OverlappedIgnoreIndex(NewOverlappedGetJSON[Output](config).Run(ctx, epnt))
func GetJSON[Output any](ctx context.Context, base *BaseURL, config *Config) (Output, error) {
return OverlappedIgnoreIndex(NewOverlappedGetJSON[Output](config).Run(ctx, base))
}

func getJSON[Output any](ctx context.Context, epnt *Endpoint, config *Config) (Output, error) {
func getJSON[Output any](ctx context.Context, base *BaseURL, config *Config) (Output, error) {
// read the raw body
rawrespbody, err := GetRaw(ctx, epnt, config)
rawrespbody, err := GetRaw(ctx, base, config)

// handle the case of error
if err != nil {
Expand Down
16 changes: 8 additions & 8 deletions internal/httpclientx/getjson_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func TestGetJSON(t *testing.T) {
// invoke the API
resp, err := GetJSON[*apiResponse](
context.Background(),
NewEndpoint(server.URL),
NewBaseURL(server.URL),
&Config{
Client: http.DefaultClient,
Logger: model.DiscardLogger,
Expand Down Expand Up @@ -59,7 +59,7 @@ func TestGetJSON(t *testing.T) {
// invoke the API
resp, err := GetJSON[*apiResponse](
context.Background(),
NewEndpoint(server.URL),
NewBaseURL(server.URL),
&Config{
Client: http.DefaultClient,
Logger: model.DiscardLogger,
Expand Down Expand Up @@ -90,7 +90,7 @@ func TestGetJSON(t *testing.T) {
// invoke the API
resp, err := GetJSON[*apiResponse](
context.Background(),
NewEndpoint(server.URL),
NewBaseURL(server.URL),
&Config{
Client: http.DefaultClient,
Logger: model.DiscardLogger,
Expand Down Expand Up @@ -137,7 +137,7 @@ func TestGetJSONHeadersOkay(t *testing.T) {
// send the request and receive the response
apiresp, err := GetJSON[*apiResponse](
context.Background(),
NewEndpoint(server.URL).WithHostOverride("www.cloudfront.com"),
NewBaseURL(server.URL).WithHostOverride("www.cloudfront.com"),
&Config{
Authorization: "scribai",
Client: http.DefaultClient,
Expand Down Expand Up @@ -194,7 +194,7 @@ func TestGetJSONLoggingOkay(t *testing.T) {
// invoke the API
resp, err := GetJSON[*apiResponse](
context.Background(),
NewEndpoint(server.URL),
NewBaseURL(server.URL),
&Config{
Client: http.DefaultClient,
Logger: logger,
Expand Down Expand Up @@ -238,7 +238,7 @@ func TestGetJSONCorrectlyRejectsNilValues(t *testing.T) {
// invoke the API
resp, err := GetJSON[map[string]string](
context.Background(),
NewEndpoint(server.URL),
NewBaseURL(server.URL),
&Config{
Client: http.DefaultClient,
Logger: model.DiscardLogger,
Expand Down Expand Up @@ -268,7 +268,7 @@ func TestGetJSONCorrectlyRejectsNilValues(t *testing.T) {
// invoke the API
resp, err := GetJSON[*apiResponse](
context.Background(),
NewEndpoint(server.URL),
NewBaseURL(server.URL),
&Config{
Client: http.DefaultClient,
Logger: model.DiscardLogger,
Expand Down Expand Up @@ -298,7 +298,7 @@ func TestGetJSONCorrectlyRejectsNilValues(t *testing.T) {
// invoke the API
resp, err := GetJSON[[]string](
context.Background(),
NewEndpoint(server.URL),
NewBaseURL(server.URL),
&Config{
Client: http.DefaultClient,
Logger: model.DiscardLogger,
Expand Down
Loading
Loading