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.

Sign up for GitHub

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: move engine.InputLoader to targetloading #1616

Merged
merged 5 commits into from
Jun 6, 2024
Merged

refactor: move engine.InputLoader to targetloading #1616

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
960434e
refactor: move engine.InputLoader to inputloading
bassosimone Jun 6, 2024
f05ba4a
x
bassosimone Jun 6, 2024
f20490e
x
bassosimone Jun 6, 2024
a362d36
x
bassosimone Jun 6, 2024
067e043
Update pkg/oonimkall/taskrunner.go
bassosimone Jun 6, 2024
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: 2 additions & 2 deletions cmd/ooniprobe/internal/nettests/dnscheck.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@ package nettests
import (
"context"

engine "github.com/ooni/probe-cli/v3/internal/engine"
"github.com/ooni/probe-cli/v3/internal/inputloading"
"github.com/ooni/probe-cli/v3/internal/model"
)

// DNSCheck nettest implementation.
type DNSCheck struct{}

func (n DNSCheck) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) {
inputloader := &engine.InputLoader{
inputloader := &inputloading.Loader{
CheckInConfig: &model.OOAPICheckInConfig{
// not needed because we have default static input in the engine
},
Expand Down
4 changes: 2 additions & 2 deletions cmd/ooniprobe/internal/nettests/stunreachability.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@ package nettests
import (
"context"

engine "github.com/ooni/probe-cli/v3/internal/engine"
"github.com/ooni/probe-cli/v3/internal/inputloading"
"github.com/ooni/probe-cli/v3/internal/model"
)

// STUNReachability nettest implementation.
type STUNReachability struct{}

func (n STUNReachability) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) {
inputloader := &engine.InputLoader{
inputloader := &inputloading.Loader{
CheckInConfig: &model.OOAPICheckInConfig{
// not needed because we have default static input in the engine
},
Expand Down
4 changes: 2 additions & 2 deletions cmd/ooniprobe/internal/nettests/web_connectivity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ import (
"context"

"github.com/apex/log"
engine "github.com/ooni/probe-cli/v3/internal/engine"
"github.com/ooni/probe-cli/v3/internal/inputloading"
"github.com/ooni/probe-cli/v3/internal/model"
)

func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]model.ExperimentTarget, error) {
inputloader := &engine.InputLoader{
inputloader := &inputloading.Loader{
CheckInConfig: &model.OOAPICheckInConfig{
// Setting Charging and OnWiFi to true causes the CheckIn
// API to return to us as much URL as possible with the
Expand Down
9 changes: 8 additions & 1 deletion internal/engine/inputloader_network_test.go → ...al/engine/inputloader_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,17 @@ import (

"github.com/apex/log"
"github.com/ooni/probe-cli/v3/internal/engine"
"github.com/ooni/probe-cli/v3/internal/inputloading"
"github.com/ooni/probe-cli/v3/internal/kvstore"
"github.com/ooni/probe-cli/v3/internal/model"
)

// This historical integration test ensures that we're able to fetch URLs from
// the dev infrastructure. We say this test's historical because the inputloading.Loader
// belonged to the engine package before we introduced richer input. It kind of feels
// good to keep this integration test here since we want to use a real session and a real
// Loader and double check whether we can get inputs. In a more distant future it would
// kind of make sense to have a broader package with this kind of integration tests.
func TestInputLoaderInputOrQueryBackendWithNoInput(t *testing.T) {
if testing.Short() {
t.Skip("skip test in short mode")
Expand All @@ -29,7 +36,7 @@ func TestInputLoaderInputOrQueryBackendWithNoInput(t *testing.T) {
t.Fatal(err)
}
defer sess.Close()
il := &engine.InputLoader{
il := &inputloading.Loader{
InputPolicy: model.InputOrQueryBackend,
Session: sess,
}
Expand Down
54 changes: 27 additions & 27 deletions internal/engine/inputloader.go → internal/inputloading/inputloading.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
package engine
// Package inputloading contains common code to load input.
package inputloading

import (
"bufio"
Expand All @@ -16,7 +17,7 @@ import (
"github.com/ooni/probe-cli/v3/internal/stuninput"
)

// These errors are returned by the InputLoader.
// These errors are returned by the [*Loader].
var (
ErrNoURLsReturned = errors.New("no URLs returned")
ErrDetectedEmptyFile = errors.New("file did not contain any input")
Expand All @@ -25,21 +26,20 @@ var (
ErrNoStaticInput = errors.New("no static input for this experiment")
)

// InputLoaderSession is the session according to an InputLoader. We
// introduce this abstraction because it helps us with testing.
type InputLoaderSession interface {
// Session is the session according to a [*Loader].
type Session interface {
CheckIn(ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error)
FetchOpenVPNConfig(ctx context.Context,
provider, cc string) (*model.OOAPIVPNProviderConfig, error)
}

// InputLoaderLogger is the logger according to an InputLoader.
type InputLoaderLogger interface {
// Logger is the [model.Logger] according to a [*Loader].
type Logger interface {
// Warnf formats and emits a warning message.
Warnf(format string, v ...interface{})
}

// InputLoader loads input according to the specified policy
// Loader loads input according to the specified policy
// either from command line and input files or from OONI services. The
// behaviour depends on the input policy as described below.
//
Expand Down Expand Up @@ -74,7 +74,7 @@ type InputLoaderLogger interface {
//
// We gather input from StaticInput and SourceFiles. If there is
// input, we return it. Otherwise, we return an error.
type InputLoader struct {
type Loader struct {
// CheckInConfig contains options for the CheckIn API. If
// not set, then we'll create a default config. If set but
// there are fields inside it that are not set, then we
Expand All @@ -91,14 +91,14 @@ type InputLoader struct {
// this field.
InputPolicy model.InputPolicy

// Logger is the optional logger that the InputLoader
// Logger is the optional logger that the [*Loader]
// should be using. If not set, we will use the default
// logger of github.com/apex/log.
Logger InputLoaderLogger
Logger Logger

// Session is the current measurement session. You
// MUST fill in this field.
Session InputLoaderSession
Session Session

// StaticInputs contains optional input to be added
// to the resulting input list if possible.
Expand All @@ -113,7 +113,7 @@ type InputLoader struct {

// Load attempts to load input using the specified input loader. We will
// return a list of URLs because this is the only input we support.
func (il *InputLoader) Load(ctx context.Context) ([]model.ExperimentTarget, error) {
func (il *Loader) Load(ctx context.Context) ([]model.ExperimentTarget, error) {
switch il.InputPolicy {
case model.InputOptional:
return il.loadOptional()
Expand All @@ -129,7 +129,7 @@ func (il *InputLoader) Load(ctx context.Context) ([]model.ExperimentTarget, erro
}

// loadNone implements the InputNone policy.
func (il *InputLoader) loadNone() ([]model.ExperimentTarget, error) {
func (il *Loader) loadNone() ([]model.ExperimentTarget, error) {
if len(il.StaticInputs) > 0 || len(il.SourceFiles) > 0 {
return nil, ErrNoInputExpected
}
Expand All @@ -140,7 +140,7 @@ func (il *InputLoader) loadNone() ([]model.ExperimentTarget, error) {
}

// loadOptional implements the InputOptional policy.
func (il *InputLoader) loadOptional() ([]model.ExperimentTarget, error) {
func (il *Loader) loadOptional() ([]model.ExperimentTarget, error) {
inputs, err := il.loadLocal()
if err == nil && len(inputs) <= 0 {
// Implementation note: the convention for input-less experiments is that
Expand All @@ -151,7 +151,7 @@ func (il *InputLoader) loadOptional() ([]model.ExperimentTarget, error) {
}

// loadStrictlyRequired implements the InputStrictlyRequired policy.
func (il *InputLoader) loadStrictlyRequired(_ context.Context) ([]model.ExperimentTarget, error) {
func (il *Loader) loadStrictlyRequired(_ context.Context) ([]model.ExperimentTarget, error) {
inputs, err := il.loadLocal()
if err != nil || len(inputs) > 0 {
return inputs, err
Expand All @@ -160,7 +160,7 @@ func (il *InputLoader) loadStrictlyRequired(_ context.Context) ([]model.Experime
}

// loadOrQueryBackend implements the InputOrQueryBackend policy.
func (il *InputLoader) loadOrQueryBackend(ctx context.Context) ([]model.ExperimentTarget, error) {
func (il *Loader) loadOrQueryBackend(ctx context.Context) ([]model.ExperimentTarget, error) {
inputs, err := il.loadLocal()
if err != nil || len(inputs) > 0 {
return inputs, err
Expand Down Expand Up @@ -247,7 +247,7 @@ func staticInputForExperiment(name string) ([]model.ExperimentTarget, error) {
}

// loadOrStaticDefault implements the InputOrStaticDefault policy.
func (il *InputLoader) loadOrStaticDefault(_ context.Context) ([]model.ExperimentTarget, error) {
func (il *Loader) loadOrStaticDefault(_ context.Context) ([]model.ExperimentTarget, error) {
inputs, err := il.loadLocal()
if err != nil || len(inputs) > 0 {
return inputs, err
Expand All @@ -256,7 +256,7 @@ func (il *InputLoader) loadOrStaticDefault(_ context.Context) ([]model.Experimen
}

// loadLocal loads inputs from StaticInputs and SourceFiles.
func (il *InputLoader) loadLocal() ([]model.ExperimentTarget, error) {
func (il *Loader) loadLocal() ([]model.ExperimentTarget, error) {
inputs := []model.ExperimentTarget{}
for _, input := range il.StaticInputs {
inputs = append(inputs, model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(input))
Expand All @@ -280,7 +280,7 @@ type inputLoaderOpenFn func(filepath string) (fs.File, error)

// readfile reads inputs from the specified file. The open argument should be
// compatible with stdlib's fs.Open and helps us with unit testing.
func (il *InputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]model.ExperimentTarget, error) {
func (il *Loader) readfile(filepath string, open inputLoaderOpenFn) ([]model.ExperimentTarget, error) {
inputs := []model.ExperimentTarget{}
filep, err := open(filepath)
if err != nil {
Expand All @@ -304,7 +304,7 @@ func (il *InputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]mode
}

// loadRemote loads inputs from a remote source.
func (il *InputLoader) loadRemote(ctx context.Context) ([]model.ExperimentTarget, error) {
func (il *Loader) loadRemote(ctx context.Context) ([]model.ExperimentTarget, error) {
switch registry.CanonicalizeExperimentName(il.ExperimentName) {
case "openvpn":
// TODO(ainghazal): given the semantics of the current API call, in an ideal world we'd need to pass
Expand All @@ -319,7 +319,7 @@ func (il *InputLoader) loadRemote(ctx context.Context) ([]model.ExperimentTarget
}

// loadRemoteWebConnectivity loads webconnectivity inputs from a remote source.
func (il *InputLoader) loadRemoteWebConnectivity(ctx context.Context) ([]model.ExperimentTarget, error) {
func (il *Loader) loadRemoteWebConnectivity(ctx context.Context) ([]model.ExperimentTarget, error) {
config := il.CheckInConfig
if config == nil {
// Note: Session.CheckIn documentation says it will fill in
Expand Down Expand Up @@ -356,7 +356,7 @@ func inputLoaderModelOOAPIURLInfoToModelExperimentTarget(
}

// loadRemoteOpenVPN loads openvpn inputs from a remote source.
func (il *InputLoader) loadRemoteOpenVPN(ctx context.Context) ([]model.ExperimentTarget, error) {
func (il *Loader) loadRemoteOpenVPN(ctx context.Context) ([]model.ExperimentTarget, error) {
// VPN Inputs do not match exactly the semantics expected from [model.OOAPIURLInfo],
// since OOAPIURLInfo is oriented towards webconnectivity,
// but we force VPN targets in the URL and ignore all the other fields.
Expand Down Expand Up @@ -393,7 +393,7 @@ func (il *InputLoader) loadRemoteOpenVPN(ctx context.Context) ([]model.Experimen
// checkIn executes the check-in and filters the returned URLs to exclude
// the URLs that are not part of the requested categories. This is done for
// robustness, just in case we or the API do something wrong.
func (il *InputLoader) checkIn(
func (il *Loader) checkIn(
ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResultNettests, error) {
reply, err := il.Session.CheckIn(ctx, config)
if err != nil {
Expand All @@ -409,7 +409,7 @@ func (il *InputLoader) checkIn(
}

// fetchOpenVPNConfig fetches vpn information for the configured providers
func (il *InputLoader) fetchOpenVPNConfig(ctx context.Context, provider string) (*model.OOAPIVPNProviderConfig, error) {
func (il *Loader) fetchOpenVPNConfig(ctx context.Context, provider string) (*model.OOAPIVPNProviderConfig, error) {
reply, err := il.Session.FetchOpenVPNConfig(ctx, provider, "XX")
if err != nil {
return nil, err
Expand All @@ -420,7 +420,7 @@ func (il *InputLoader) fetchOpenVPNConfig(ctx context.Context, provider string)
// preventMistakes makes the code more robust with respect to any possible
// integration issue where the backend returns to us URLs that don't
// belong to the category codes we requested.
func (il *InputLoader) preventMistakes(input []model.OOAPIURLInfo, categories []string) (output []model.OOAPIURLInfo) {
func (il *Loader) preventMistakes(input []model.OOAPIURLInfo, categories []string) (output []model.OOAPIURLInfo) {
if len(categories) <= 0 {
return input
}
Expand All @@ -442,7 +442,7 @@ func (il *InputLoader) preventMistakes(input []model.OOAPIURLInfo, categories []
}

// logger returns the configured logger or apex/log's default.
func (il *InputLoader) logger() InputLoaderLogger {
func (il *Loader) logger() Logger {
if il.Logger != nil {
return il.Logger
}
Expand Down
Loading
Loading