Skip to content

Commit

Permalink
refactor(engine): set options from any value
Browse files Browse the repository at this point in the history 
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
  }
}
```
  • Loading branch information
@bassosimone
bassosimone committed Jul 8, 2022
1 parent 6019b25 commit 2bb58e3
Show file tree
Hide file tree
Showing 4 changed files with 401 additions and 199 deletions.
26 changes: 18 additions & 8 deletions internal/cmd/miniooni/libminiooni.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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")
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand Down
10 changes: 5 additions & 5 deletions internal/engine/experiment_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down Expand Up @@ -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("")
Expand Down Expand Up @@ -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)
Expand Down
163 changes: 99 additions & 64 deletions internal/engine/experimentbuilder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"errors"
"fmt"
"reflect"
"regexp"
"strconv"

"github.com/iancoleman/strcase"
Expand Down Expand Up @@ -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 could 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 could set a bool option.
ErrCannotSetBoolOption = errors.New("cannot set bool option")

// ErrCannotSetStringOption means SetOptionAny could 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)
Expand All @@ -90,67 +117,75 @@ 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)
// 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 field.Kind() != reflect.Bool {
return errors.New("field is not a bool")
}
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")
}
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")
}
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")
}
if !intregexp.MatchString(value) {
return b.SetOptionString(key, value)
switch field.Kind() {
case reflect.Int64:
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)
}
case reflect.Bool:
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)
}
case reflect.String:
switch v := value.(type) {
case string:
field.SetString(v)
return nil
default:
return fmt.Errorf("%w from a value of type %T", ErrCannotSetStringOption, 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
}
}
Expand All @@ -162,19 +197,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
}
Expand Down
Loading

0 comments on commit 2bb58e3

Please sign in to comment.