refactor(inputloader): remove unnecessary javisms (#271)

Part of https://github.com/ooni/probe/issues/1299.

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)

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")
 

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 {
+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.
-	Load(ctx context.Context) ([]model.URLInfo, error)
-}
-
-// InputLoaderConfig contains config for InputLoader.
-type InputLoaderConfig 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
 	}

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 {

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{}
+	il := &InputLoader{
-	lrc := inputLoaderLoadRemoteConfig{
+		Session: &InputLoaderMockableSession{
-		ctx: context.Background(),
-		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{}
+	il := &InputLoader{
-	lrc := inputLoaderLoadRemoteConfig{
+		Session: &InputLoaderMockableSession{
-		ctx: context.Background(),
-		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{}
+	il := &InputLoader{
-	lrc := inputLoaderLoadRemoteConfig{
+		Session: &InputLoaderMockableSession{
-		ctx: context.Background(),
-		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{}
+	il := &InputLoader{
-	lrc := inputLoaderLoadRemoteConfig{
+		Session: &InputLoaderMockableSession{
-		ctx: context.Background(),
-		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)
 	}