diff --git a/cmd/ooniprobe/internal/nettests/web_connectivity.go b/cmd/ooniprobe/internal/nettests/web_connectivity.go index c4752ad..3e15472 100644 --- a/cmd/ooniprobe/internal/nettests/web_connectivity.go +++ b/cmd/ooniprobe/internal/nettests/web_connectivity.go @@ -19,7 +19,7 @@ import ( // someone choose the number of URLs explicitly via the config. func lookupURLs(ctl *Controller, limit int64, categories []string) ([]string, map[int64]int64, error) { - inputloader := engine.NewInputLoader(engine.InputLoaderConfig{ + inputloader := &engine.InputLoader{ CheckInConfig: &model.CheckInConfig{ WebConnectivity: model.CheckInConfigWebConnectivity{ CategoryCodes: categories, @@ -29,7 +29,7 @@ func lookupURLs(ctl *Controller, limit int64, categories []string) ([]string, ma Session: ctl.Session, SourceFiles: ctl.InputFiles, StaticInputs: ctl.Inputs, - }) + } testlist, err := inputloader.Load(context.Background()) var urls []string urlIDMap := make(map[int64]int64) diff --git a/internal/cmd/miniooni/libminiooni.go b/internal/cmd/miniooni/libminiooni.go index 51fc51c..765cc1a 100644 --- a/internal/cmd/miniooni/libminiooni.go +++ b/internal/cmd/miniooni/libminiooni.go @@ -356,7 +356,7 @@ func MainWithConfiguration(experimentName string, currentOptions Options) { builder, err := sess.NewExperimentBuilder(experimentName) fatalOnError(err, "cannot create experiment builder") - inputLoader := engine.NewInputLoader(engine.InputLoaderConfig{ + inputLoader := &engine.InputLoader{ CheckInConfig: &model.CheckInConfig{ RunType: "manual", OnWiFi: true, // meaning: not on 4G @@ -366,7 +366,7 @@ func MainWithConfiguration(experimentName string, currentOptions Options) { StaticInputs: currentOptions.Inputs, SourceFiles: currentOptions.InputFilePaths, Session: sess, - }) + } inputs, err := inputLoader.Load(context.Background()) fatalOnError(err, "cannot load inputs") diff --git a/internal/engine/inputloader.go b/internal/engine/inputloader.go index 5561693..2dc1f87 100644 --- a/internal/engine/inputloader.go +++ b/internal/engine/inputloader.go @@ -30,6 +30,9 @@ type InputLoaderSession interface { // either from command line and input files or from OONI services. The // behaviour depends on the input policy as described below. // +// You MUST NOT change any public field of this structure when +// in use, because that MAY lead to data races. +// // InputNone // // We fail if there is any StaticInput or any SourceFiles. If @@ -52,14 +55,7 @@ type InputLoaderSession interface { // // 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. - Load(ctx context.Context) ([]model.URLInfo, error) -} - -// InputLoaderConfig contains config for InputLoader. -type InputLoaderConfig struct { +type InputLoader 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 @@ -87,32 +83,9 @@ type InputLoaderConfig struct { SourceFiles []string } -// NewInputLoader creates a new InputLoader. -func NewInputLoader(config InputLoaderConfig) InputLoader { - // TODO(bassosimone): the current implementation stems from a - // simple refactoring from a previous implementation where - // we weren't using interfaces. Because now we're using interfaces, - // there is the opportunity to select behaviour here depending - // on the specified policy rather than later inside Load. - 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 // return a list of URLs because this is the only input we support. -func (il inputLoader) Load(ctx context.Context) ([]model.URLInfo, error) { +func (il *InputLoader) Load(ctx context.Context) ([]model.URLInfo, error) { switch il.InputPolicy { case InputOptional: return il.loadOptional() @@ -126,7 +99,7 @@ func (il inputLoader) Load(ctx context.Context) ([]model.URLInfo, error) { } // loadNone implements the InputNone policy. -func (il inputLoader) loadNone() ([]model.URLInfo, error) { +func (il *InputLoader) loadNone() ([]model.URLInfo, error) { if len(il.StaticInputs) > 0 || len(il.SourceFiles) > 0 { return nil, ErrNoInputExpected } @@ -135,7 +108,7 @@ func (il inputLoader) loadNone() ([]model.URLInfo, error) { } // loadOptional implements the InputOptional policy. -func (il inputLoader) loadOptional() ([]model.URLInfo, error) { +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. @@ -145,7 +118,7 @@ func (il inputLoader) loadOptional() ([]model.URLInfo, error) { } // loadStrictlyRequired implements the InputStrictlyRequired policy. -func (il inputLoader) loadStrictlyRequired(ctx context.Context) ([]model.URLInfo, error) { +func (il *InputLoader) loadStrictlyRequired(ctx context.Context) ([]model.URLInfo, error) { inputs, err := il.loadLocal() if err != nil || len(inputs) > 0 { return inputs, err @@ -154,16 +127,16 @@ func (il inputLoader) loadStrictlyRequired(ctx context.Context) ([]model.URLInfo } // loadOrQueryBackend implements the InputOrQueryBackend policy. -func (il inputLoader) loadOrQueryBackend(ctx context.Context) ([]model.URLInfo, error) { +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(inputLoaderLoadRemoteConfig{ctx: ctx, session: il.Session}) + return il.loadRemote(ctx) } // loadLocal loads inputs from StaticInputs and SourceFiles. -func (il inputLoader) loadLocal() ([]model.URLInfo, error) { +func (il *InputLoader) loadLocal() ([]model.URLInfo, error) { inputs := []model.URLInfo{} for _, input := range il.StaticInputs { inputs = append(inputs, model.URLInfo{URL: input}) @@ -187,7 +160,7 @@ type inputLoaderOpenFn func(filepath string) (fs.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) { +func (il *InputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]model.URLInfo, error) { inputs := []model.URLInfo{} filep, err := open(filepath) if err != nil { @@ -210,15 +183,8 @@ func (il inputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]model return inputs, nil } -// 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 -} - // loadRemote loads inputs from a remote source. -func (il inputLoader) loadRemote(conf inputLoaderLoadRemoteConfig) ([]model.URLInfo, error) { +func (il *InputLoader) loadRemote(ctx context.Context) ([]model.URLInfo, error) { config := il.CheckInConfig if config == nil { // Note: Session.CheckIn documentation says it will fill in @@ -227,7 +193,7 @@ func (il inputLoader) loadRemote(conf inputLoaderLoadRemoteConfig) ([]model.URLI // concerned about NOT passing it a NULL pointer. config = &model.CheckInConfig{} } - reply, err := conf.session.CheckIn(conf.ctx, config) + reply, err := il.Session.CheckIn(ctx, config) if err != nil { return nil, err } diff --git a/internal/engine/inputloader_network_test.go b/internal/engine/inputloader_network_test.go index 7155d6d..3d10957 100644 --- a/internal/engine/inputloader_network_test.go +++ b/internal/engine/inputloader_network_test.go @@ -30,10 +30,10 @@ func TestInputLoaderInputOrQueryBackendWithNoInput(t *testing.T) { t.Fatal(err) } defer sess.Close() - il := engine.NewInputLoader(engine.InputLoaderConfig{ + il := &engine.InputLoader{ InputPolicy: engine.InputOrQueryBackend, Session: sess, - }) + } ctx := context.Background() out, err := il.Load(ctx) if err != nil { diff --git a/internal/engine/inputloader_test.go b/internal/engine/inputloader_test.go index e722944..7287c46 100644 --- a/internal/engine/inputloader_test.go +++ b/internal/engine/inputloader_test.go @@ -16,10 +16,10 @@ import ( ) func TestInputLoaderInputNoneWithStaticInputs(t *testing.T) { - il := NewInputLoader(InputLoaderConfig{ + il := &InputLoader{ StaticInputs: []string{"https://www.google.com/"}, InputPolicy: InputNone, - }) + } ctx := context.Background() out, err := il.Load(ctx) if !errors.Is(err, ErrNoInputExpected) { @@ -31,13 +31,13 @@ func TestInputLoaderInputNoneWithStaticInputs(t *testing.T) { } func TestInputLoaderInputNoneWithFilesInputs(t *testing.T) { - il := NewInputLoader(InputLoaderConfig{ + il := &InputLoader{ SourceFiles: []string{ "testdata/inputloader1.txt", "testdata/inputloader2.txt", }, InputPolicy: InputNone, - }) + } ctx := context.Background() out, err := il.Load(ctx) if !errors.Is(err, ErrNoInputExpected) { @@ -49,14 +49,14 @@ func TestInputLoaderInputNoneWithFilesInputs(t *testing.T) { } func TestInputLoaderInputNoneWithBothInputs(t *testing.T) { - il := NewInputLoader(InputLoaderConfig{ + il := &InputLoader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ "testdata/inputloader1.txt", "testdata/inputloader2.txt", }, InputPolicy: InputNone, - }) + } ctx := context.Background() out, err := il.Load(ctx) if !errors.Is(err, ErrNoInputExpected) { @@ -68,9 +68,9 @@ func TestInputLoaderInputNoneWithBothInputs(t *testing.T) { } func TestInputLoaderInputNoneWithNoInput(t *testing.T) { - il := NewInputLoader(InputLoaderConfig{ + il := &InputLoader{ InputPolicy: InputNone, - }) + } ctx := context.Background() out, err := il.Load(ctx) if err != nil { @@ -82,9 +82,9 @@ func TestInputLoaderInputNoneWithNoInput(t *testing.T) { } func TestInputLoaderInputOptionalWithNoInput(t *testing.T) { - il := NewInputLoader(InputLoaderConfig{ + il := &InputLoader{ InputPolicy: InputOptional, - }) + } ctx := context.Background() out, err := il.Load(ctx) if err != nil { @@ -96,14 +96,14 @@ func TestInputLoaderInputOptionalWithNoInput(t *testing.T) { } func TestInputLoaderInputOptionalWithInput(t *testing.T) { - il := NewInputLoader(InputLoaderConfig{ + il := &InputLoader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ "testdata/inputloader1.txt", "testdata/inputloader2.txt", }, InputPolicy: InputOptional, - }) + } ctx := context.Background() out, err := il.Load(ctx) if err != nil { @@ -125,7 +125,7 @@ func TestInputLoaderInputOptionalWithInput(t *testing.T) { } func TestInputLoaderInputOptionalNonexistentFile(t *testing.T) { - il := NewInputLoader(InputLoaderConfig{ + il := &InputLoader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ "testdata/inputloader1.txt", @@ -133,7 +133,7 @@ func TestInputLoaderInputOptionalNonexistentFile(t *testing.T) { "testdata/inputloader2.txt", }, InputPolicy: InputOptional, - }) + } ctx := context.Background() out, err := il.Load(ctx) if !errors.Is(err, syscall.ENOENT) { @@ -145,14 +145,14 @@ func TestInputLoaderInputOptionalNonexistentFile(t *testing.T) { } func TestInputLoaderInputStrictlyRequiredWithInput(t *testing.T) { - il := NewInputLoader(InputLoaderConfig{ + il := &InputLoader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ "testdata/inputloader1.txt", "testdata/inputloader2.txt", }, InputPolicy: InputStrictlyRequired, - }) + } ctx := context.Background() out, err := il.Load(ctx) if err != nil { @@ -174,9 +174,9 @@ func TestInputLoaderInputStrictlyRequiredWithInput(t *testing.T) { } func TestInputLoaderInputStrictlyRequiredWithoutInput(t *testing.T) { - il := NewInputLoader(InputLoaderConfig{ + il := &InputLoader{ InputPolicy: InputStrictlyRequired, - }) + } ctx := context.Background() out, err := il.Load(ctx) if !errors.Is(err, ErrInputRequired) { @@ -188,14 +188,14 @@ func TestInputLoaderInputStrictlyRequiredWithoutInput(t *testing.T) { } func TestInputLoaderInputStrictlyRequiredWithEmptyFile(t *testing.T) { - il := NewInputLoader(InputLoaderConfig{ + il := &InputLoader{ InputPolicy: InputStrictlyRequired, SourceFiles: []string{ "testdata/inputloader1.txt", "testdata/inputloader3.txt", // we want it before inputloader2.txt "testdata/inputloader2.txt", }, - }) + } ctx := context.Background() out, err := il.Load(ctx) if !errors.Is(err, ErrDetectedEmptyFile) { @@ -207,14 +207,14 @@ func TestInputLoaderInputStrictlyRequiredWithEmptyFile(t *testing.T) { } func TestInputLoaderInputOrQueryBackendWithInput(t *testing.T) { - il := NewInputLoader(InputLoaderConfig{ + il := &InputLoader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ "testdata/inputloader1.txt", "testdata/inputloader2.txt", }, InputPolicy: InputOrQueryBackend, - }) + } ctx := context.Background() out, err := il.Load(ctx) if err != nil { @@ -248,10 +248,10 @@ func TestInputLoaderInputOrQueryBackendWithNoInputAndCancelledContext(t *testing t.Fatal(err) } defer sess.Close() - il := NewInputLoader(InputLoaderConfig{ + il := &InputLoader{ InputPolicy: InputOrQueryBackend, Session: sess, - }) + } ctx, cancel := context.WithCancel(context.Background()) cancel() // fail immediately out, err := il.Load(ctx) @@ -264,14 +264,14 @@ func TestInputLoaderInputOrQueryBackendWithNoInputAndCancelledContext(t *testing } func TestInputLoaderInputOrQueryBackendWithEmptyFile(t *testing.T) { - il := NewInputLoader(InputLoaderConfig{ + il := &InputLoader{ InputPolicy: InputOrQueryBackend, SourceFiles: []string{ "testdata/inputloader1.txt", "testdata/inputloader3.txt", // we want it before inputloader2.txt "testdata/inputloader2.txt", }, - }) + } ctx := context.Background() out, err := il.Load(ctx) if !errors.Is(err, ErrDetectedEmptyFile) { @@ -303,7 +303,7 @@ func (InputLoaderBrokenFile) Close() error { } func TestInputLoaderReadfileScannerFailure(t *testing.T) { - il := inputLoader{} + il := &InputLoader{} out, err := il.readfile("", InputLoaderBrokenFS{}.Open) if !errors.Is(err, syscall.EFAULT) { t.Fatal("not the error we expected") @@ -335,14 +335,12 @@ func (sess *InputLoaderMockableSession) CheckIn( } func TestInputLoaderCheckInFailure(t *testing.T) { - il := inputLoader{} - lrc := inputLoaderLoadRemoteConfig{ - ctx: context.Background(), - session: &InputLoaderMockableSession{ + il := &InputLoader{ + Session: &InputLoaderMockableSession{ Error: io.EOF, }, } - out, err := il.loadRemote(lrc) + out, err := il.loadRemote(context.Background()) if !errors.Is(err, io.EOF) { t.Fatalf("not the error we expected: %+v", err) } @@ -352,14 +350,12 @@ func TestInputLoaderCheckInFailure(t *testing.T) { } func TestInputLoaderCheckInSuccessWithNilWebConnectivity(t *testing.T) { - il := inputLoader{} - lrc := inputLoaderLoadRemoteConfig{ - ctx: context.Background(), - session: &InputLoaderMockableSession{ + il := &InputLoader{ + Session: &InputLoaderMockableSession{ Output: &model.CheckInInfo{}, }, } - out, err := il.loadRemote(lrc) + out, err := il.loadRemote(context.Background()) if !errors.Is(err, ErrNoURLsReturned) { t.Fatalf("not the error we expected: %+v", err) } @@ -369,16 +365,14 @@ func TestInputLoaderCheckInSuccessWithNilWebConnectivity(t *testing.T) { } func TestInputLoaderCheckInSuccessWithNoURLs(t *testing.T) { - il := inputLoader{} - lrc := inputLoaderLoadRemoteConfig{ - ctx: context.Background(), - session: &InputLoaderMockableSession{ + il := &InputLoader{ + Session: &InputLoaderMockableSession{ Output: &model.CheckInInfo{ WebConnectivity: &model.CheckInInfoWebConnectivity{}, }, }, } - out, err := il.loadRemote(lrc) + out, err := il.loadRemote(context.Background()) if !errors.Is(err, ErrNoURLsReturned) { t.Fatalf("not the error we expected: %+v", err) } @@ -397,10 +391,8 @@ func TestInputLoaderCheckInSuccessWithSomeURLs(t *testing.T) { CountryCode: "IT", URL: "https://corriere.it", }} - il := inputLoader{} - lrc := inputLoaderLoadRemoteConfig{ - ctx: context.Background(), - session: &InputLoaderMockableSession{ + il := &InputLoader{ + Session: &InputLoaderMockableSession{ Output: &model.CheckInInfo{ WebConnectivity: &model.CheckInInfoWebConnectivity{ URLs: expect, @@ -408,7 +400,7 @@ func TestInputLoaderCheckInSuccessWithSomeURLs(t *testing.T) { }, }, } - out, err := il.loadRemote(lrc) + out, err := il.loadRemote(context.Background()) if err != nil { t.Fatal(err) }