refactor(engine): set options from any value (#837)

This diff refactors how we set options for experiments to accept
in input an any value or a map[string]any, depending on which method
we choose to actually set options.

There should be no functional change, except that now we're not
guessing the type and then attempting to set the value of the selected
field: now, instead, we match the provided type and the field's type
as part of the same function (i.e., SetOptionAny).

This diff is functional to ooni/probe#2184,
because it will allow us to load options from a map[string]any,
which will be part of the OONI Run v2 JSON descriptor.

If we didn't apply this change, we would only have been to set options
from a map[string]string, which is good enough as a solution for the
CLI but is definitely clumsy when you have to write stuff like:

```JSON
{
  "options": {
    "HTTP3Enabled": "true"
  }
}
```

when you could instead more naturally write:

```JSON
{
  "options": {
    "HTTP3Enabled": true
  }
}
```

internal/cmd/miniooni/libminiooni.go

@@ -176,7 +176,7 @@ func warnOnError(err error, msg string) {
 	}
 }
 
-func mustMakeMap(input []string) (output map[string]string) {
+func mustMakeMapString(input []string) (output map[string]string) {
 	output = make(map[string]string)
 	for _, opt := range input {
 		key, value, err := split(opt)
@@ -186,6 +186,16 @@ func mustMakeMap(input []string) (output map[string]string) {
 	return
 }
 
+func mustMakeMapAny(input []string) (output map[string]any) {
+	output = make(map[string]any)
+	for _, opt := range input {
+		key, value, err := split(opt)
+		fatalOnError(err, "cannot split key-value pair")
+		output[key] = value
+	}
+	return
+}
+
 func mustParseURL(URL string) *url.URL {
 	rv, err := url.Parse(URL)
 	fatalOnError(err, "cannot parse URL")
@@ -233,15 +243,15 @@ Do you consent to OONI Probe data collection?
 
 OONI Probe collects evidence of internet censorship and measures
 network performance:
- 
+
 - OONI Probe will likely test objectionable sites and services;
- 
+
 - Anyone monitoring your internet activity (such as a government
 or Internet provider) may be able to tell that you are using OONI Probe;
- 
+
 - The network data you collect will be published automatically
 unless you use miniooni's -n command line flag.
- 
+
 To learn more, see https://ooni.org/about/risks/.
 
 If you're onboard, re-run the same command and add the --yes flag, to
@@ -296,8 +306,8 @@ func MainWithConfiguration(experimentName string, currentOptions Options) {
 
 	ctx := context.Background()
 
-	extraOptions := mustMakeMap(currentOptions.ExtraOptions)
-	annotations := mustMakeMap(currentOptions.Annotations)
+	extraOptions := mustMakeMapAny(currentOptions.ExtraOptions)
+	annotations := mustMakeMapString(currentOptions.Annotations)
 
 	logger := &log.Logger{Level: log.InfoLevel, Handler: &logHandler{Writer: os.Stderr}}
 	if currentOptions.Verbose {
@@ -413,7 +423,7 @@ func MainWithConfiguration(experimentName string, currentOptions Options) {
 		})
 	}
 
-	err = builder.SetOptionsGuessType(extraOptions)
+	err = builder.SetOptionsAny(extraOptions)
 	fatalOnError(err, "cannot parse extraOptions")
 
 	experiment := builder.NewExperiment()

internal/engine/experiment_integration_test.go

@@ -157,7 +157,7 @@ func TestSetCallbacks(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if err := builder.SetOptionInt("SleepTime", 0); err != nil {
+	if err := builder.SetOptionAny("SleepTime", 0); err != nil {
 		t.Fatal(err)
 	}
 	register := &registerCallbacksCalled{}
@@ -203,7 +203,7 @@ func TestMeasurementFailure(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if err := builder.SetOptionBool("ReturnError", true); err != nil {
+	if err := builder.SetOptionAny("ReturnError", true); err != nil {
 		t.Fatal(err)
 	}
 	measurement, err := builder.NewExperiment().Measure("")
@@ -279,13 +279,13 @@ func TestUseOptions(t *testing.T) {
 	if !sleepTime {
 		t.Fatal("did not find SleepTime option")
 	}
-	if err := builder.SetOptionBool("ReturnError", true); err != nil {
+	if err := builder.SetOptionAny("ReturnError", true); err != nil {
 		t.Fatal("cannot set ReturnError field")
 	}
-	if err := builder.SetOptionInt("SleepTime", 10); err != nil {
+	if err := builder.SetOptionAny("SleepTime", 10); err != nil {
 		t.Fatal("cannot set SleepTime field")
 	}
-	if err := builder.SetOptionString("Message", "antani"); err != nil {
+	if err := builder.SetOptionAny("Message", "antani"); err != nil {
 		t.Fatal("cannot set Message field")
 	}
 	config := builder.config.(*example.Config)

internal/engine/experimentbuilder.go

@@ -4,7 +4,6 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
-	"regexp"
 	"strconv"
 
 	"github.com/iancoleman/strcase"
@@ -63,22 +62,50 @@ func (b *ExperimentBuilder) InputPolicy() InputPolicy {
 	return b.inputPolicy
 }
 
-// OptionInfo contains info about an option
+// OptionInfo contains info about an option.
 type OptionInfo struct {
-	Doc  string
+	// Doc contains the documentation.
+	Doc string
+
+	// Type contains the type.
 	Type string
 }
 
+var (
+	// ErrConfigIsNotAStructPointer indicates we expected a pointer to struct.
+	ErrConfigIsNotAStructPointer = errors.New("config is not a struct pointer")
+
+	// ErrNoSuchField indicates there's no field with the given name.
+	ErrNoSuchField = errors.New("no such field")
+
+	// ErrCannotSetIntegerOption means SetOptionAny couldn't set an integer option.
+	ErrCannotSetIntegerOption = errors.New("cannot set integer option")
+
+	// ErrInvalidStringRepresentationOfBool indicates the string you passed
+	// to SetOptionaAny is not a valid string representation of a bool.
+	ErrInvalidStringRepresentationOfBool = errors.New("invalid string representation of bool")
+
+	// ErrCannotSetBoolOption means SetOptionAny couldn't set a bool option.
+	ErrCannotSetBoolOption = errors.New("cannot set bool option")
+
+	// ErrCannotSetStringOption means SetOptionAny couldn't set a string option.
+	ErrCannotSetStringOption = errors.New("cannot set string option")
+
+	// ErrUnsupportedOptionType means we don't support the type passed to
+	// the SetOptionAny method as an opaque any type.
+	ErrUnsupportedOptionType = errors.New("unsupported option type")
+)
+
 // Options returns info about all options
 func (b *ExperimentBuilder) Options() (map[string]OptionInfo, error) {
 	result := make(map[string]OptionInfo)
 	ptrinfo := reflect.ValueOf(b.config)
 	if ptrinfo.Kind() != reflect.Ptr {
-		return nil, errors.New("config is not a pointer")
+		return nil, ErrConfigIsNotAStructPointer
 	}
 	structinfo := ptrinfo.Elem().Type()
 	if structinfo.Kind() != reflect.Struct {
-		return nil, errors.New("config is not a struct")
+		return nil, ErrConfigIsNotAStructPointer
 	}
 	for i := 0; i < structinfo.NumField(); i++ {
 		field := structinfo.Field(i)
@@ -90,67 +117,90 @@ func (b *ExperimentBuilder) Options() (map[string]OptionInfo, error) {
 	return result, nil
 }
 
-// SetOptionBool sets a bool option
-func (b *ExperimentBuilder) SetOptionBool(key string, value bool) error {
-	field, err := fieldbyname(b.config, key)
-	if err != nil {
-		return err
-	}
-	if field.Kind() != reflect.Bool {
-		return errors.New("field is not a bool")
+// setOptionBool sets a bool option.
+func (b *ExperimentBuilder) setOptionBool(field reflect.Value, value any) error {
+	switch v := value.(type) {
+	case bool:
+		field.SetBool(v)
+		return nil
+	case string:
+		if v != "true" && v != "false" {
+			return fmt.Errorf("%w: %s", ErrInvalidStringRepresentationOfBool, v)
+		}
+		field.SetBool(v == "true")
+		return nil
+	default:
+		return fmt.Errorf("%w from a value of type %T", ErrCannotSetBoolOption, value)
 	}
-	field.SetBool(value)
-	return nil
 }
 
-// SetOptionInt sets an int option
-func (b *ExperimentBuilder) SetOptionInt(key string, value int64) error {
-	field, err := fieldbyname(b.config, key)
-	if err != nil {
-		return err
-	}
-	if field.Kind() != reflect.Int64 {
-		return errors.New("field is not an int64")
+// setOptionInt sets an int option
+func (b *ExperimentBuilder) setOptionInt(field reflect.Value, value any) error {
+	switch v := value.(type) {
+	case int64:
+		field.SetInt(v)
+		return nil
+	case int32:
+		field.SetInt(int64(v))
+		return nil
+	case int16:
+		field.SetInt(int64(v))
+		return nil
+	case int8:
+		field.SetInt(int64(v))
+		return nil
+	case int:
+		field.SetInt(int64(v))
+		return nil
+	case string:
+		number, err := strconv.ParseInt(v, 10, 64)
+		if err != nil {
+			return fmt.Errorf("%w: %s", ErrCannotSetIntegerOption, err.Error())
+		}
+		field.SetInt(number)
+		return nil
+	default:
+		return fmt.Errorf("%w from a value of type %T", ErrCannotSetIntegerOption, value)
 	}
-	field.SetInt(value)
-	return nil
 }
 
-// SetOptionString sets a string option
-func (b *ExperimentBuilder) SetOptionString(key, value string) error {
-	field, err := fieldbyname(b.config, key)
-	if err != nil {
-		return err
-	}
-	if field.Kind() != reflect.String {
-		return errors.New("field is not a string")
+// setOptionString sets a string option
+func (b *ExperimentBuilder) setOptionString(field reflect.Value, value any) error {
+	switch v := value.(type) {
+	case string:
+		field.SetString(v)
+		return nil
+	default:
+		return fmt.Errorf("%w from a value of type %T", ErrCannotSetStringOption, value)
 	}
-	field.SetString(value)
-	return nil
 }
 
-var intregexp = regexp.MustCompile("^[0-9]+$")
-
-// SetOptionGuessType sets an option whose type depends on the
-// option value. If the value is `"true"` or `"false"` we
-// assume the option is boolean. If the value is numeric, then we
-// set an integer option. Otherwise we set a string option.
-func (b *ExperimentBuilder) SetOptionGuessType(key, value string) error {
-	if value == "true" || value == "false" {
-		return b.SetOptionBool(key, value == "true")
+// SetOptionAny sets an option whose value is an any value. We will use reasonable
+// heuristics to convert the any value to the proper type of the field whose name is
+// contained by the key variable. If we cannot convert the provided any value to
+// the proper type, then this function returns an error.
+func (b *ExperimentBuilder) SetOptionAny(key string, value any) error {
+	field, err := b.fieldbyname(b.config, key)
+	if err != nil {
+		return err
 	}
-	if !intregexp.MatchString(value) {
-		return b.SetOptionString(key, value)
+	switch field.Kind() {
+	case reflect.Int64:
+		return b.setOptionInt(field, value)
+	case reflect.Bool:
+		return b.setOptionBool(field, value)
+	case reflect.String:
+		return b.setOptionString(field, value)
+	default:
+		return fmt.Errorf("%w: %T", ErrUnsupportedOptionType, value)
 	}
-	number, _ := strconv.ParseInt(value, 10, 64)
-	return b.SetOptionInt(key, number)
 }
 
-// SetOptionsGuessType calls the SetOptionGuessType method for every
-// key, value pair contained by the opts input map.
-func (b *ExperimentBuilder) SetOptionsGuessType(opts map[string]string) error {
-	for k, v := range opts {
-		if err := b.SetOptionGuessType(k, v); err != nil {
+// SetOptionsAny sets options from a map[string]any. See the documentation of
+// the SetOptionAny function for more information.
+func (b *ExperimentBuilder) SetOptionsAny(options map[string]any) error {
+	for key, value := range options {
+		if err := b.SetOptionAny(key, value); err != nil {
 			return err
 		}
 	}
@@ -162,19 +212,19 @@ func (b *ExperimentBuilder) SetCallbacks(callbacks model.ExperimentCallbacks) {
 	b.callbacks = callbacks
 }
 
-func fieldbyname(v interface{}, key string) (reflect.Value, error) {
+func (b *ExperimentBuilder) fieldbyname(v interface{}, key string) (reflect.Value, error) {
 	// See https://stackoverflow.com/a/6396678/4354461
 	ptrinfo := reflect.ValueOf(v)
 	if ptrinfo.Kind() != reflect.Ptr {
-		return reflect.Value{}, errors.New("value is not a pointer")
+		return reflect.Value{}, fmt.Errorf("%w but a %T", ErrConfigIsNotAStructPointer, v)
 	}
 	structinfo := ptrinfo.Elem()
 	if structinfo.Kind() != reflect.Struct {
-		return reflect.Value{}, errors.New("value is not a pointer to struct")
+		return reflect.Value{}, fmt.Errorf("%w but a %T", ErrConfigIsNotAStructPointer, v)
 	}
 	field := structinfo.FieldByName(key)
 	if !field.IsValid() || !field.CanSet() {
-		return reflect.Value{}, errors.New("no such field")
+		return reflect.Value{}, fmt.Errorf("%w: %s", ErrNoSuchField, key)
 	}
 	return field, nil
 }