From f3c41bba67b0634a115d8e5faebe72e3c987f9ba Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 26 Jun 2024 07:02:59 +0200 Subject: [PATCH] feat(oonirun): allow true JSON richer input --- internal/engine/experimentbuilder.go | 25 ++++++++++++++-------- internal/mocks/experimentbuilder.go | 13 +++++++++++- internal/model/experiment.go | 6 ++++++ internal/oonirun/experiment.go | 27 +++++++++++++++++++----- internal/oonirun/experiment_test.go | 27 +++++++++++++++++++++++- internal/oonirun/v1_test.go | 4 ++++ internal/oonirun/v2.go | 5 +++-- internal/oonirun/v2_test.go | 31 ++++++++++++++-------------- internal/registry/factory.go | 15 ++++++++++++++ internal/registry/portfiltering.go | 4 ++-- internal/registry/telegram.go | 4 ++-- 11 files changed, 124 insertions(+), 37 deletions(-) diff --git a/internal/engine/experimentbuilder.go b/internal/engine/experimentbuilder.go index 330777957..591197f0c 100644 --- a/internal/engine/experimentbuilder.go +++ b/internal/engine/experimentbuilder.go @@ -5,11 +5,13 @@ package engine // import ( + "encoding/json" + "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/registry" ) -// experimentBuilder implements ExperimentBuilder. +// experimentBuilder implements [model.ExperimentBuilder]. // // This type is now just a tiny wrapper around registry.Factory. type experimentBuilder struct { @@ -24,37 +26,42 @@ type experimentBuilder struct { var _ model.ExperimentBuilder = &experimentBuilder{} -// Interruptible implements ExperimentBuilder.Interruptible. +// Interruptible implements [model.ExperimentBuilder]. func (b *experimentBuilder) Interruptible() bool { return b.factory.Interruptible() } -// InputPolicy implements ExperimentBuilder.InputPolicy. +// InputPolicy implements [model.ExperimentBuilder]. func (b *experimentBuilder) InputPolicy() model.InputPolicy { return b.factory.InputPolicy() } -// Options implements ExperimentBuilder.Options. +// Options implements [model.ExperimentBuilder]. func (b *experimentBuilder) Options() (map[string]model.ExperimentOptionInfo, error) { return b.factory.Options() } -// SetOptionAny implements ExperimentBuilder.SetOptionAny. +// SetOptionAny implements [model.ExperimentBuilder]. func (b *experimentBuilder) SetOptionAny(key string, value any) error { return b.factory.SetOptionAny(key, value) } -// SetOptionsAny implements ExperimentBuilder.SetOptionsAny. +// SetOptionsAny implements [model.ExperimentBuilder]. func (b *experimentBuilder) SetOptionsAny(options map[string]any) error { return b.factory.SetOptionsAny(options) } -// SetCallbacks implements ExperimentBuilder.SetCallbacks. +// SetOptionsJSON implements [model.ExperimentBuilder]. +func (b *experimentBuilder) SetOptionsJSON(value json.RawMessage) error { + return b.factory.SetOptionsJSON(value) +} + +// SetCallbacks implements [model.ExperimentBuilder]. func (b *experimentBuilder) SetCallbacks(callbacks model.ExperimentCallbacks) { b.callbacks = callbacks } -// NewExperiment creates the experiment +// NewExperiment creates a new [model.Experiment] instance. func (b *experimentBuilder) NewExperiment() model.Experiment { measurer := b.factory.NewExperimentMeasurer() experiment := newExperiment(b.session, measurer) @@ -67,7 +74,7 @@ func (b *experimentBuilder) NewTargetLoader(config *model.ExperimentTargetLoader return b.factory.NewTargetLoader(config) } -// newExperimentBuilder creates a new experimentBuilder instance. +// newExperimentBuilder creates a new [*experimentBuilder] instance. func newExperimentBuilder(session *Session, name string) (*experimentBuilder, error) { factory, err := registry.NewFactory(name, session.kvStore, session.logger) if err != nil { diff --git a/internal/mocks/experimentbuilder.go b/internal/mocks/experimentbuilder.go index 1f6a27187..a9cd880ba 100644 --- a/internal/mocks/experimentbuilder.go +++ b/internal/mocks/experimentbuilder.go @@ -1,6 +1,10 @@ package mocks -import "github.com/ooni/probe-cli/v3/internal/model" +import ( + "encoding/json" + + "github.com/ooni/probe-cli/v3/internal/model" +) // ExperimentBuilder mocks model.ExperimentBuilder. type ExperimentBuilder struct { @@ -14,6 +18,8 @@ type ExperimentBuilder struct { MockSetOptionsAny func(options map[string]any) error + MockSetOptionsJSON func(value json.RawMessage) error + MockSetCallbacks func(callbacks model.ExperimentCallbacks) MockNewExperiment func() model.Experiment @@ -43,6 +49,11 @@ func (eb *ExperimentBuilder) SetOptionsAny(options map[string]any) error { return eb.MockSetOptionsAny(options) } +func (eb *ExperimentBuilder) SetOptionsJSON(value json.RawMessage) error { + // TODO(bassosimone): write unit tests for this method + return eb.MockSetOptionsJSON(value) +} + func (eb *ExperimentBuilder) SetCallbacks(callbacks model.ExperimentCallbacks) { eb.MockSetCallbacks(callbacks) } diff --git a/internal/model/experiment.go b/internal/model/experiment.go index 5aada21ff..95848242c 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -7,6 +7,7 @@ package model import ( "context" + "encoding/json" "errors" "fmt" ) @@ -235,6 +236,11 @@ type ExperimentBuilder interface { // the SetOptionAny method for more information. SetOptionsAny(options map[string]any) error + // SetOptionsJSON uses the given [json.RawMessage] to initialize fields + // of the configuration for running the experiment. The [json.RawMessage] + // MUST contain a serialization of the experiment config's type. + SetOptionsJSON(value json.RawMessage) error + // SetCallbacks sets the experiment's interactive callbacks. SetCallbacks(callbacks ExperimentCallbacks) diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index c4613107d..a66093524 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -6,6 +6,7 @@ package oonirun import ( "context" + "encoding/json" "fmt" "math/rand" "strings" @@ -25,9 +26,18 @@ type Experiment struct { // Annotations contains OPTIONAL Annotations for the experiment. Annotations map[string]string - // ExtraOptions contains OPTIONAL extra options for the experiment. + // ExtraOptions contains OPTIONAL extra options that modify the + // default-empty experiment-specific configuration. We apply + // the changes described by this field after using the InitialOptions + // field to initialize the experiment-specific configuration. ExtraOptions map[string]any + // InitialOptions contains an OPTIONAL [json.RawMessage] object + // used to initialize the default-empty experiment-specific + // configuration. After we have initialized the configuration + // as such, we then apply the changes described by the ExtraOptions. + InitialOptions json.RawMessage + // Inputs contains the OPTIONAL experiment Inputs Inputs []string @@ -82,15 +92,22 @@ func (ed *Experiment) Run(ctx context.Context) error { return err } - // TODO(bassosimone,DecFox): when we're executed by OONI Run v2, it probably makes - // slightly more sense to set options from a json.RawMessage because the current - // command line limitation is that it's hard to set non scalar parameters and instead - // with using OONI Run v2 we can completely bypass such a limitation. + // TODO(bassosimone): we need another patch after the current one + // to correctly serialize the options as configured using InitialOptions + // and ExtraOptions otherwise the Measurement.Options field turns out + // to always be empty and this is highly suboptimal for us. // 2. configure experiment's options // + // We first unmarshal the InitialOptions into the experiment + // configuration and afterwards we modify the configuration using + // the values contained inside the ExtraOptions field. + // // This MUST happen before loading targets because the options will // possibly be used to produce richer input targets. + if err := builder.SetOptionsJSON(ed.InitialOptions); err != nil { + return err + } if err := builder.SetOptionsAny(ed.ExtraOptions); err != nil { return err } diff --git a/internal/oonirun/experiment_test.go b/internal/oonirun/experiment_test.go index 1fd38abb0..2fe651119 100644 --- a/internal/oonirun/experiment_test.go +++ b/internal/oonirun/experiment_test.go @@ -2,6 +2,7 @@ package oonirun import ( "context" + "encoding/json" "errors" "reflect" "sort" @@ -16,6 +17,7 @@ import ( func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { shuffledInputsPrev := experimentShuffledInputs.Load() var calledSetOptionsAny int + var calledSetOptionsJSON int var failedToSubmit int var calledKibiBytesReceived int var calledKibiBytesSent int @@ -44,6 +46,10 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { calledSetOptionsAny++ return nil }, + MockSetOptionsJSON: func(value json.RawMessage) error { + calledSetOptionsJSON++ + return nil + }, MockNewExperiment: func() model.Experiment { exp := &mocks.Experiment{ MockMeasureWithContext: func( @@ -109,6 +115,9 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { if calledSetOptionsAny < 1 { t.Fatal("should have called SetOptionsAny") } + if calledSetOptionsJSON < 1 { + t.Fatal("should have called SetOptionsJSON") + } if calledKibiBytesReceived < 1 { t.Fatal("did not call KibiBytesReceived") } @@ -198,10 +207,14 @@ func TestExperimentRun(t *testing.T) { args: args{}, expectErr: errMocked, }, { - name: "cannot set options", + name: "cannot set ExtraOptions", fields: fields{ newExperimentBuilderFn: func(experimentName string) (model.ExperimentBuilder, error) { eb := &mocks.ExperimentBuilder{ + MockSetOptionsJSON: func(value json.RawMessage) error { + // TODO(bassosimone): need a test case before this one + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return errMocked }, @@ -226,6 +239,9 @@ func TestExperimentRun(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputOptional }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, @@ -255,6 +271,9 @@ func TestExperimentRun(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputOptional }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, @@ -298,6 +317,9 @@ func TestExperimentRun(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputOptional }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, @@ -344,6 +366,9 @@ func TestExperimentRun(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputOptional }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, diff --git a/internal/oonirun/v1_test.go b/internal/oonirun/v1_test.go index 910d910b0..8ea9b0513 100644 --- a/internal/oonirun/v1_test.go +++ b/internal/oonirun/v1_test.go @@ -2,6 +2,7 @@ package oonirun import ( "context" + "encoding/json" "errors" "net/http" "strings" @@ -26,6 +27,9 @@ func newMinimalFakeSession() *mocks.Session { MockSetOptionsAny: func(options map[string]any) error { return nil }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockNewExperiment: func() model.Experiment { exp := &mocks.Experiment{ MockMeasureWithContext: func( diff --git a/internal/oonirun/v2.go b/internal/oonirun/v2.go index 4330b07b7..6552ad582 100644 --- a/internal/oonirun/v2.go +++ b/internal/oonirun/v2.go @@ -54,7 +54,7 @@ type V2Nettest struct { // `Safe` will be available for the experiment run, but omitted from // the serialized Measurement that the experiment builder will submit // to the OONI backend. - Options map[string]any `json:"options"` + Options json.RawMessage `json:"options"` // TestName contains the nettest name. TestName string `json:"test_name"` @@ -183,7 +183,8 @@ func V2MeasureDescriptor(ctx context.Context, config *LinkConfig, desc *V2Descri // construct an experiment from the current nettest exp := &Experiment{ Annotations: config.Annotations, - ExtraOptions: nettest.Options, + ExtraOptions: make(map[string]any), + InitialOptions: nettest.Options, Inputs: nettest.Inputs, InputFilePaths: nil, MaxRuntime: config.MaxRuntime, diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go index d849c6d08..b77b9b7bf 100644 --- a/internal/oonirun/v2_test.go +++ b/internal/oonirun/v2_test.go @@ -7,7 +7,6 @@ import ( "net/http" "net/http/httptest" "testing" - "time" "github.com/ooni/probe-cli/v3/internal/httpclientx" "github.com/ooni/probe-cli/v3/internal/kvstore" @@ -27,9 +26,9 @@ func TestOONIRunV2LinkCommonCase(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{ - "SleepTime": int64(10 * time.Millisecond), - }, + Options: json.RawMessage(`{ + "SleepTime": 10000000 + }`), TestName: "example", }}, } @@ -73,9 +72,9 @@ func TestOONIRunV2LinkCannotUpdateCache(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{ - "SleepTime": int64(10 * time.Millisecond), - }, + Options: json.RawMessage(`{ + "SleepTime": 10000000 + }`), TestName: "example", }}, } @@ -132,9 +131,9 @@ func TestOONIRunV2LinkWithoutAcceptChanges(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{ - "SleepTime": int64(10 * time.Millisecond), - }, + Options: json.RawMessage(`{ + "SleepTime": 10000000 + }`), TestName: "example", }}, } @@ -220,9 +219,9 @@ func TestOONIRunV2LinkEmptyTestName(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{ - "SleepTime": int64(10 * time.Millisecond), - }, + Options: json.RawMessage(`{ + "SleepTime": 10000000 + }`), TestName: "", // empty! }}, } @@ -374,6 +373,9 @@ func TestV2MeasureDescriptor(t *testing.T) { MockInputPolicy: func() model.InputPolicy { return model.InputNone }, + MockSetOptionsJSON: func(value json.RawMessage) error { + return nil + }, MockSetOptionsAny: func(options map[string]any) error { return nil }, @@ -426,7 +428,7 @@ func TestV2MeasureDescriptor(t *testing.T) { Author: "", Nettests: []V2Nettest{{ Inputs: []string{}, - Options: map[string]any{}, + Options: json.RawMessage(`{}`), TestName: "example", }}, } @@ -532,5 +534,4 @@ func TestV2DescriptorCacheLoad(t *testing.T) { t.Fatal("expected nil cache") } }) - } diff --git a/internal/registry/factory.go b/internal/registry/factory.go index ee73e95a8..9c5ffac30 100644 --- a/internal/registry/factory.go +++ b/internal/registry/factory.go @@ -5,6 +5,7 @@ package registry // import ( + "encoding/json" "errors" "fmt" "math" @@ -228,6 +229,20 @@ func (b *Factory) SetOptionsAny(options map[string]any) error { return nil } +// SetOptionsJSON unmarshals the given [json.RawMessage] inside +// the experiment specific configuration. +func (b *Factory) SetOptionsJSON(value json.RawMessage) error { + // handle the case where the options are empty + if len(value) <= 0 { + return nil + } + + // otherwise unmarshal into the configuration, which we assume + // to be a pointer to a structure. + // TODO(bassosimone): make sure with testing that b.config is always a pointer. + return json.Unmarshal(value, b.config) +} + // fieldbyname return v's field whose name is equal to the given key. func (b *Factory) fieldbyname(v interface{}, key string) (reflect.Value, error) { // See https://stackoverflow.com/a/6396678/4354461 diff --git a/internal/registry/portfiltering.go b/internal/registry/portfiltering.go index 8c6a857df..2d7c67f25 100644 --- a/internal/registry/portfiltering.go +++ b/internal/registry/portfiltering.go @@ -15,11 +15,11 @@ func init() { return &Factory{ build: func(config any) model.ExperimentMeasurer { return portfiltering.NewExperimentMeasurer( - config.(portfiltering.Config), + *config.(*portfiltering.Config), ) }, canonicalName: canonicalName, - config: portfiltering.Config{}, + config: &portfiltering.Config{}, enabledByDefault: true, interruptible: false, inputPolicy: model.InputNone, diff --git a/internal/registry/telegram.go b/internal/registry/telegram.go index 987e64093..bf4f71aed 100644 --- a/internal/registry/telegram.go +++ b/internal/registry/telegram.go @@ -15,11 +15,11 @@ func init() { return &Factory{ build: func(config any) model.ExperimentMeasurer { return telegram.NewExperimentMeasurer( - config.(telegram.Config), + *config.(*telegram.Config), ) }, canonicalName: canonicalName, - config: telegram.Config{}, + config: &telegram.Config{}, enabledByDefault: true, interruptible: false, inputPolicy: model.InputNone,