diff --git a/cmd/ooniprobe/internal/nettests/web_connectivity.go b/cmd/ooniprobe/internal/nettests/web_connectivity.go index 923fab4..1286845 100644 --- a/cmd/ooniprobe/internal/nettests/web_connectivity.go +++ b/cmd/ooniprobe/internal/nettests/web_connectivity.go @@ -10,7 +10,7 @@ import ( func lookupURLs(ctl *Controller, limit int64, categories []string) ([]string, map[int64]int64, error) { inputloader := engine.NewInputLoader(engine.InputLoaderConfig{ - InputPolicy: engine.InputOrQueryTestLists, + InputPolicy: engine.InputOrQueryBackend, Session: ctl.Session, SourceFiles: ctl.InputFiles, StaticInputs: ctl.Inputs, diff --git a/internal/engine/allexperiments.go b/internal/engine/allexperiments.go index 5b6ebfb..2178269 100644 --- a/internal/engine/allexperiments.go +++ b/internal/engine/allexperiments.go @@ -152,7 +152,7 @@ var experimentsByName = map[string]func(*Session) *ExperimentBuilder{ )) }, config: &httphostheader.Config{}, - inputPolicy: InputOrQueryTestLists, + inputPolicy: InputOrQueryBackend, } }, @@ -237,7 +237,7 @@ var experimentsByName = map[string]func(*Session) *ExperimentBuilder{ )) }, config: &sniblocking.Config{}, - inputPolicy: InputOrQueryTestLists, + inputPolicy: InputOrQueryBackend, } }, @@ -273,7 +273,7 @@ var experimentsByName = map[string]func(*Session) *ExperimentBuilder{ )) }, config: &tlstool.Config{}, - inputPolicy: InputOrQueryTestLists, + inputPolicy: InputOrQueryBackend, } }, @@ -309,7 +309,7 @@ var experimentsByName = map[string]func(*Session) *ExperimentBuilder{ )) }, config: &webconnectivity.Config{}, - inputPolicy: InputOrQueryTestLists, + inputPolicy: InputOrQueryBackend, } }, diff --git a/internal/engine/experiment_integration_test.go b/internal/engine/experiment_integration_test.go index e689b00..f7b8a68 100644 --- a/internal/engine/experiment_integration_test.go +++ b/internal/engine/experiment_integration_test.go @@ -142,7 +142,7 @@ func TestNeedsInput(t *testing.T) { if err != nil { t.Fatal(err) } - if builder.InputPolicy() != InputOrQueryTestLists { + if builder.InputPolicy() != InputOrQueryBackend { t.Fatal("web_connectivity certainly needs input") } } diff --git a/internal/engine/experimentbuilder.go b/internal/engine/experimentbuilder.go index 332e079..97d8299 100644 --- a/internal/engine/experimentbuilder.go +++ b/internal/engine/experimentbuilder.go @@ -16,12 +16,12 @@ import ( type InputPolicy string const ( - // InputOrQueryTestLists indicates that the experiment requires + // InputOrQueryBackend indicates that the experiment requires // external input to run and that this kind of input is URLs // from the citizenlab/test-lists repository. If this input // not provided to the experiment, then the code that runs the // experiment is supposed to fetch from URLs from OONI's backends. - InputOrQueryTestLists = InputPolicy("or_query_test_lists") + InputOrQueryBackend = InputPolicy("or_query_backend") // InputStrictlyRequired indicates that the experiment // requires input and we currently don't have an API for diff --git a/internal/engine/inputloader.go b/internal/engine/inputloader.go index d606d22..ccd1583 100644 --- a/internal/engine/inputloader.go +++ b/internal/engine/inputloader.go @@ -10,14 +10,15 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine/model" ) -// The following errors are returned by the InputLoader. +// These errors are returned by the InputLoader. var ( - ErrNoInputExpected = errors.New("we did not expect any input") - ErrInputRequired = errors.New("no input provided") ErrDetectedEmptyFile = errors.New("file did not contain any input") + ErrInputRequired = errors.New("no input provided") + ErrNoInputExpected = errors.New("we did not expect any input") ) -// InputLoaderSession is the session according to an InputLoader. +// InputLoaderSession is the session according to an InputLoader. We +// introduce this abstraction because it helps us with testing. type InputLoaderSession interface { MaybeLookupLocationContext(ctx context.Context) error NewOrchestraClient(ctx context.Context) (model.ExperimentOrchestraClient, error) @@ -25,8 +26,8 @@ type InputLoaderSession interface { } // InputLoader loads input according to the specified policy -// from the specified sources and OONI services. The behaviour -// depends on the input policy as described below. +// either from command line and input files or from OONI services. The +// behaviour depends on the input policy as described below. // // InputNone // @@ -40,16 +41,16 @@ type InputLoaderSession interface { // input, we return it. Otherwise we return a single, empty entry // that causes experiments that don't require input to run once. // -// InputOrQueryTestLists +// InputOrQueryBackend // // We gather input from StaticInput and SourceFiles. If there is // input, we return it. Otherwise, we use OONI's probe services -// to gather input using the test lists API. +// to gather input using the best API for the task. // // InputStrictlyRequired // -// Like InputOrQueryTestLists but, if there is no input, it's an -// user error and we just abort running the experiment. +// We gather input from StaticInput and SourceFiles. If there is +// input, we return it. Otherwise, we return an error. type InputLoader interface { // 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. @@ -64,7 +65,8 @@ type InputLoaderConfig struct { // SourceFiles contains optional files to read input // from. Each file should contain a single input string - // per line. We will fail if any file is unreadable. + // per line. We will fail if any file is unreadable + // as well as if any file is empty. SourceFiles []string // InputPolicy specifies the input policy for the @@ -94,10 +96,17 @@ func NewInputLoader(config InputLoaderConfig) InputLoader { return inputLoader{InputLoaderConfig: config} } +// TODO(bassosimone): it seems there's no reason to return an +// interface from the constructor. Generally, "Effective Go" +// recommends that an interface is used by the receiver rather +// than by the sender. We should follow that rule of thumb. + +// inputLoader is the concrete implementation of InputLoader. type inputLoader struct { InputLoaderConfig } +// verifies that inputLoader is an InputLoader. var _ InputLoader = inputLoader{} // Load attempts to load input using the specified input loader. We will @@ -106,8 +115,8 @@ func (il inputLoader) Load(ctx context.Context) ([]model.URLInfo, error) { switch il.InputPolicy { case InputOptional: return il.loadOptional() - case InputOrQueryTestLists: - return il.loadOrQueryTestList(ctx) + case InputOrQueryBackend: + return il.loadOrQueryBackend(ctx) case InputStrictlyRequired: return il.loadStrictlyRequired(ctx) default: @@ -115,21 +124,26 @@ func (il inputLoader) Load(ctx context.Context) ([]model.URLInfo, error) { } } +// loadNone implements the InputNone policy. func (il inputLoader) loadNone() ([]model.URLInfo, error) { if len(il.StaticInputs) > 0 || len(il.SourceFiles) > 0 { return nil, ErrNoInputExpected } + // Note that we need to return a single empty entry. return []model.URLInfo{{}}, nil } +// loadOptional implements the InputOptional policy. func (il inputLoader) loadOptional() ([]model.URLInfo, error) { inputs, err := il.loadLocal() if err == nil && len(inputs) <= 0 { + // Note that we need to return a single empty entry. inputs = []model.URLInfo{{}} } return inputs, err } +// loadStrictlyRequired implements the InputStrictlyRequired policy. func (il inputLoader) loadStrictlyRequired(ctx context.Context) ([]model.URLInfo, error) { inputs, err := il.loadLocal() if err != nil || len(inputs) > 0 { @@ -138,14 +152,16 @@ func (il inputLoader) loadStrictlyRequired(ctx context.Context) ([]model.URLInfo return nil, ErrInputRequired } -func (il inputLoader) loadOrQueryTestList(ctx context.Context) ([]model.URLInfo, error) { +// loadOrQueryBackend implements the InputOrQueryBackend policy. +func (il inputLoader) loadOrQueryBackend(ctx context.Context) ([]model.URLInfo, error) { inputs, err := il.loadLocal() if err != nil || len(inputs) > 0 { return inputs, err } - return il.loadRemote(loadRemoteConfig{ctx: ctx, session: il.Session}) + return il.loadRemote(inputLoaderLoadRemoteConfig{ctx: ctx, session: il.Session}) } +// loadLocal loads inputs from StaticInputs and SourceFiles. func (il inputLoader) loadLocal() ([]model.URLInfo, error) { inputs := []model.URLInfo{} for _, input := range il.StaticInputs { @@ -165,7 +181,12 @@ func (il inputLoader) loadLocal() ([]model.URLInfo, error) { return inputs, nil } -func (il inputLoader) readfile(filepath string, open func(string) (fsx.File, error)) ([]model.URLInfo, error) { +// inputLoaderOpenFn is the type of the function to open a file. +type inputLoaderOpenFn func(filepath string) (fsx.File, error) + +// readfile reads inputs from the specified file. The open argument should be +// compatibile with stdlib's fs.Open and helps us with unit testing. +func (il inputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]model.URLInfo, error) { inputs := []model.URLInfo{} filep, err := open(filepath) if err != nil { @@ -188,12 +209,15 @@ func (il inputLoader) readfile(filepath string, open func(string) (fsx.File, err return inputs, nil } -type loadRemoteConfig struct { +// inputLoaderLoadRemoteConfig contains configuration for loading the input from +// a remote source (which currently is _only_ the OONI backend). +type inputLoaderLoadRemoteConfig struct { ctx context.Context session InputLoaderSession } -func (il inputLoader) loadRemote(conf loadRemoteConfig) ([]model.URLInfo, error) { +// loadRemote loads inputs from a remote source. +func (il inputLoader) loadRemote(conf inputLoaderLoadRemoteConfig) ([]model.URLInfo, error) { if err := conf.session.MaybeLookupLocationContext(conf.ctx); err != nil { return nil, err } diff --git a/internal/engine/inputloader_integration_test.go b/internal/engine/inputloader_integration_test.go index c289256..744157a 100644 --- a/internal/engine/inputloader_integration_test.go +++ b/internal/engine/inputloader_integration_test.go @@ -211,7 +211,7 @@ func TestInputLoaderInputOrQueryTestListsWithInput(t *testing.T) { "testdata/inputloader1.txt", "testdata/inputloader2.txt", }, - InputPolicy: engine.InputOrQueryTestLists, + InputPolicy: engine.InputOrQueryBackend, }) ctx := context.Background() out, err := il.Load(ctx) @@ -247,7 +247,7 @@ func TestInputLoaderInputOrQueryTestListsWithNoInputAndCancelledContext(t *testi } defer sess.Close() il := engine.NewInputLoader(engine.InputLoaderConfig{ - InputPolicy: engine.InputOrQueryTestLists, + InputPolicy: engine.InputOrQueryBackend, Session: sess, }) ctx, cancel := context.WithCancel(context.Background()) @@ -282,7 +282,7 @@ func TestInputLoaderInputOrQueryTestListsWithNoInput(t *testing.T) { } defer sess.Close() il := engine.NewInputLoader(engine.InputLoaderConfig{ - InputPolicy: engine.InputOrQueryTestLists, + InputPolicy: engine.InputOrQueryBackend, Session: sess, URLLimit: 30, }) @@ -298,7 +298,7 @@ func TestInputLoaderInputOrQueryTestListsWithNoInput(t *testing.T) { func TestInputLoaderInputOrQueryTestListsWithEmptyFile(t *testing.T) { il := engine.NewInputLoader(engine.InputLoaderConfig{ - InputPolicy: engine.InputOrQueryTestLists, + InputPolicy: engine.InputOrQueryBackend, SourceFiles: []string{ "testdata/inputloader1.txt", "testdata/inputloader3.txt", // we want it before inputloader2.txt diff --git a/internal/engine/inputloader_test.go b/internal/engine/inputloader_test.go index 9e231ef..d40483e 100644 --- a/internal/engine/inputloader_test.go +++ b/internal/engine/inputloader_test.go @@ -65,7 +65,7 @@ func (InputLoaderBrokenSession) ProbeCC() string { func TestInputLoaderNewOrchestraClientFailure(t *testing.T) { il := inputLoader{} - lrc := loadRemoteConfig{ + lrc := inputLoaderLoadRemoteConfig{ ctx: context.Background(), session: InputLoaderBrokenSession{}, } @@ -98,7 +98,7 @@ func (InputLoaderBrokenOrchestraClient) FetchURLList(ctx context.Context, config func TestInputLoaderFetchURLListFailure(t *testing.T) { il := inputLoader{} - lrc := loadRemoteConfig{ + lrc := inputLoaderLoadRemoteConfig{ ctx: context.Background(), session: InputLoaderBrokenSession{ OrchestraClient: InputLoaderBrokenOrchestraClient{}, diff --git a/internal/engine/internal/sessionresolver/resolvermaker.go b/internal/engine/internal/sessionresolver/resolvermaker.go index 0874d7b..2452de0 100644 --- a/internal/engine/internal/sessionresolver/resolvermaker.go +++ b/internal/engine/internal/sessionresolver/resolvermaker.go @@ -21,7 +21,7 @@ const systemResolverURL = "system:///" // allmakers contains all the makers in a list. We use the http3 // prefix to indicate we wanna use http3. The code will translate -// this to https and set the proper next options. +// this to https and set the proper netx options. var allmakers = []*resolvermaker{{ url: "https://cloudflare-dns.com/dns-query", }, { diff --git a/pkg/oonimkall/internal/tasks/runner.go b/pkg/oonimkall/internal/tasks/runner.go index cc106c8..34345e2 100644 --- a/pkg/oonimkall/internal/tasks/runner.go +++ b/pkg/oonimkall/internal/tasks/runner.go @@ -176,7 +176,7 @@ func (r *Runner) Run(ctx context.Context) { builder.SetCallbacks(&runnerCallbacks{emitter: r.emitter}) if len(r.settings.Inputs) <= 0 { switch builder.InputPolicy() { - case engine.InputOrQueryTestLists, engine.InputStrictlyRequired: + case engine.InputOrQueryBackend, engine.InputStrictlyRequired: r.emitter.EmitFailureStartup("no input provided") return } @@ -209,7 +209,7 @@ func (r *Runner) Run(ctx context.Context) { // this policy in the future, but for now this covers in a // reasonable way web connectivity, so we should be ok. switch builder.InputPolicy() { - case engine.InputOrQueryTestLists, engine.InputStrictlyRequired: + case engine.InputOrQueryBackend, engine.InputStrictlyRequired: var cancel context.CancelFunc ctx, cancel = context.WithTimeout( ctx, time.Duration(r.settings.Options.MaxRuntime)*time.Second,