Add support for returning measurement_uid as part of oonimkall API (#1673)

hellais · web-flow · commit a0e34565f36b · 2024-12-12T02:42:14.000+05:30
Implements: ooni/probe#2521
diff --git a/cmd/ooniprobe/internal/nettests/nettests.go b/cmd/ooniprobe/internal/nettests/nettests.go
@@ -215,7 +215,7 @@ func (c *Controller) Run(builder model.ExperimentBuilder, inputs []model.Experim
 			// Implementation note: SubmitMeasurement will fail here if we did fail
 			// to open the report but we still want to continue. There will be a
 			// bit of a spew in the logs, perhaps, but stopping seems less efficient.
-			if err := exp.SubmitAndUpdateMeasurementContext(context.Background(), measurement); err != nil {
+			if _, err := exp.SubmitAndUpdateMeasurementContext(context.Background(), measurement); err != nil {
 				log.Debug(color.RedString("failure.measurement_submission"))
 				if err := db.UploadFailed(c.msmts[idx64], err.Error()); err != nil {
 					return errors.Wrap(err, "failed to mark upload as failed")
diff --git a/internal/cmd/oonireport/oonireport.go b/internal/cmd/oonireport/oonireport.go
@@ -106,7 +106,7 @@ func submitAll(ctx context.Context, lines []string, subm model.Submitter) (int,
 	for _, line := range lines {
 		mm := toMeasurement(line)
 		// submit the measurement
-		err := subm.Submit(ctx, mm)
+		_, err := subm.Submit(ctx, mm)
 		if err != nil {
 			return submitted, err
 		}
diff --git a/internal/engine/experiment.go b/internal/engine/experiment.go
@@ -104,10 +104,10 @@ func (e *experiment) ReportID() string {
 
 // SubmitAndUpdateMeasurementContext implements [model.Experiment].
 func (e *experiment) SubmitAndUpdateMeasurementContext(
-	ctx context.Context, measurement *model.Measurement) error {
+	ctx context.Context, measurement *model.Measurement) (string, error) {
 	report := e.mrep.Get()
 	if report == nil {
-		return errors.New("report is not open")
+		return "", errors.New("report is not open")
 	}
 	return report.SubmitMeasurement(ctx, measurement)
 }
diff --git a/internal/engine/experiment_integration_test.go b/internal/engine/experiment_integration_test.go
@@ -280,7 +280,7 @@ func runexperimentflow(t *testing.T, experiment model.Experiment, input string)
 	}
 	filename := tempfile.Name()
 	tempfile.Close()
-	err = experiment.SubmitAndUpdateMeasurementContext(ctx, measurement)
+	_, err = experiment.SubmitAndUpdateMeasurementContext(ctx, measurement)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -323,7 +323,7 @@ func TestOpenReportIdempotent(t *testing.T) {
 		t.Fatal("unexpected initial report ID")
 	}
 	ctx := context.Background()
-	if err := exp.SubmitAndUpdateMeasurementContext(ctx, &model.Measurement{}); err == nil {
+	if _, err := exp.SubmitAndUpdateMeasurementContext(ctx, &model.Measurement{}); err == nil {
 		t.Fatal("we should not be able to submit before OpenReport")
 	}
 	err = exp.OpenReportContext(ctx)
@@ -403,7 +403,7 @@ func TestSubmitAndUpdateMeasurementWithClosedReport(t *testing.T) {
 	}
 	exp := builder.NewExperiment()
 	m := new(model.Measurement)
-	err = exp.SubmitAndUpdateMeasurementContext(context.Background(), m)
+	_, err = exp.SubmitAndUpdateMeasurementContext(context.Background(), m)
 	if err == nil {
 		t.Fatal("expected an error here")
 	}
diff --git a/internal/mocks/experiment.go b/internal/mocks/experiment.go
@@ -22,7 +22,7 @@ type Experiment struct {
 	MockSaveMeasurement func(measurement *model.Measurement, filePath string) error
 
 	MockSubmitAndUpdateMeasurementContext func(
-		ctx context.Context, measurement *model.Measurement) error
+		ctx context.Context, measurement *model.Measurement) (string, error)
 
 	MockOpenReportContext func(ctx context.Context) error
 }
@@ -53,7 +53,7 @@ func (e *Experiment) SaveMeasurement(measurement *model.Measurement, filePath st
 }
 
 func (e *Experiment) SubmitAndUpdateMeasurementContext(
-	ctx context.Context, measurement *model.Measurement) error {
+	ctx context.Context, measurement *model.Measurement) (string, error) {
 	return e.MockSubmitAndUpdateMeasurementContext(ctx, measurement)
 }
 
diff --git a/internal/mocks/experiment_test.go b/internal/mocks/experiment_test.go
@@ -93,11 +93,11 @@ func TestExperiment(t *testing.T) {
 	t.Run("SubmitAndUpdateMeasurementContext", func(t *testing.T) {
 		expected := errors.New("mocked err")
 		e := &Experiment{
-			MockSubmitAndUpdateMeasurementContext: func(ctx context.Context, measurement *model.Measurement) error {
-				return expected
+			MockSubmitAndUpdateMeasurementContext: func(ctx context.Context, measurement *model.Measurement) (string, error) {
+				return "", expected
 			},
 		}
-		err := e.SubmitAndUpdateMeasurementContext(context.Background(), &model.Measurement{})
+		_, err := e.SubmitAndUpdateMeasurementContext(context.Background(), &model.Measurement{})
 		if !errors.Is(err, expected) {
 			t.Fatal("unexpected err", err)
 		}
diff --git a/internal/mocks/submitter.go b/internal/mocks/submitter.go
@@ -8,10 +8,10 @@ import (
 
 // Submitter mocks model.Submitter.
 type Submitter struct {
-	MockSubmit func(ctx context.Context, m *model.Measurement) error
+	MockSubmit func(ctx context.Context, m *model.Measurement) (string, error)
 }
 
 // Submit calls MockSubmit
-func (s *Submitter) Submit(ctx context.Context, m *model.Measurement) error {
+func (s *Submitter) Submit(ctx context.Context, m *model.Measurement) (string, error) {
 	return s.MockSubmit(ctx, m)
 }
diff --git a/internal/mocks/submitter_test.go b/internal/mocks/submitter_test.go
@@ -12,11 +12,11 @@ func TestSubmitter(t *testing.T) {
 	t.Run("Submit", func(t *testing.T) {
 		expect := errors.New("mocked error")
 		s := &Submitter{
-			MockSubmit: func(ctx context.Context, m *model.Measurement) error {
-				return expect
+			MockSubmit: func(ctx context.Context, m *model.Measurement) (string, error) {
+				return "", expect
 			},
 		}
-		err := s.Submit(context.Background(), &model.Measurement{})
+		_, err := s.Submit(context.Background(), &model.Measurement{})
 		if !errors.Is(err, expect) {
 			t.Fatal("unexpected err", err)
 		}
diff --git a/internal/model/experiment.go b/internal/model/experiment.go
@@ -186,7 +186,7 @@ type Experiment interface {
 	// SubmitAndUpdateMeasurementContext submits a measurement and updates the
 	// fields whose value has changed as part of the submission.
 	SubmitAndUpdateMeasurementContext(
-		ctx context.Context, measurement *Measurement) error
+		ctx context.Context, measurement *Measurement) (string, error)
 
 	// OpenReportContext will open a report using the given context
 	// to possibly limit the lifetime of this operation.
@@ -322,7 +322,7 @@ type ExperimentTargetLoader interface {
 type Submitter interface {
 	// Submit submits the measurement and updates its
 	// report ID field in case of success.
-	Submit(ctx context.Context, m *Measurement) error
+	Submit(ctx context.Context, m *Measurement) (string, error)
 }
 
 // Saver saves a measurement on some persistent storage.
diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go
@@ -263,10 +263,11 @@ type experimentSubmitterWrapper struct {
 	logger model.Logger
 }
 
-func (sw *experimentSubmitterWrapper) Submit(ctx context.Context, idx int, m *model.Measurement) error {
-	if err := sw.child.Submit(ctx, idx, m); err != nil {
+func (sw *experimentSubmitterWrapper) Submit(ctx context.Context, idx int, m *model.Measurement) (string, error) {
+	mstUID, err := sw.child.Submit(ctx, idx, m)
+	if err != nil {
 		sw.logger.Warnf("submitting measurement failed: %s", err.Error())
 	}
 	// policy: we do not stop the loop if measurement submission fails
-	return nil
+	return mstUID, nil
 }
diff --git a/internal/oonirun/experiment_test.go b/internal/oonirun/experiment_test.go
@@ -93,9 +93,9 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) {
 		newTargetLoaderFn:      nil,
 		newSubmitterFn: func(ctx context.Context) (model.Submitter, error) {
 			subm := &mocks.Submitter{
-				MockSubmit: func(ctx context.Context, m *model.Measurement) error {
+				MockSubmit: func(ctx context.Context, m *model.Measurement) (string, error) {
 					failedToSubmit++
-					return errors.New("mocked error")
+					return "", errors.New("mocked error")
 				},
 			}
 			return subm, nil
diff --git a/internal/oonirun/inputprocessor.go b/internal/oonirun/inputprocessor.go
@@ -87,7 +87,7 @@ func (ipsw inputProcessorSaverWrapper) SaveMeasurement(
 // InputProcessorSubmitterWrapper is InputProcessor's
 // wrapper for a Submitter implementation.
 type InputProcessorSubmitterWrapper interface {
-	Submit(ctx context.Context, idx int, m *model.Measurement) error
+	Submit(ctx context.Context, idx int, m *model.Measurement) (string, error)
 }
 
 type inputProcessorSubmitterWrapper struct {
@@ -101,7 +101,7 @@ func NewInputProcessorSubmitterWrapper(submitter Submitter) InputProcessorSubmit
 }
 
 func (ipsw inputProcessorSubmitterWrapper) Submit(
-	ctx context.Context, idx int, m *model.Measurement) error {
+	ctx context.Context, idx int, m *model.Measurement) (string, error) {
 	return ipsw.submitter.Submit(ctx, m)
 }
 
@@ -141,7 +141,7 @@ func (ip *InputProcessor) run(ctx context.Context) (int, error) {
 			return 0, err
 		}
 		meas.AddAnnotations(ip.Annotations)
-		err = ip.Submitter.Submit(ctx, idx, meas)
+		_, err = ip.Submitter.Submit(ctx, idx, meas)
 		if err != nil {
 			// TODO(bassosimone): when re-reading this code, I find it confusing that
 			// we return on error because I am always like "wait, this is not the right
diff --git a/internal/oonirun/inputprocessor_test.go b/internal/oonirun/inputprocessor_test.go
@@ -55,9 +55,9 @@ type FakeInputProcessorSubmitter struct {
 }
 
 func (fips *FakeInputProcessorSubmitter) Submit(
-	ctx context.Context, m *model.Measurement) error {
+	ctx context.Context, m *model.Measurement) (string, error) {
 	fips.M = append(fips.M, m)
-	return fips.Err
+	return "", fips.Err
 }
 
 func TestInputProcessorSubmissionFailed(t *testing.T) {
diff --git a/internal/oonirun/submitter.go b/internal/oonirun/submitter.go
@@ -46,8 +46,8 @@ func NewSubmitter(ctx context.Context, config SubmitterConfig) (Submitter, error
 
 type stubSubmitter struct{}
 
-func (stubSubmitter) Submit(ctx context.Context, m *model.Measurement) error {
-	return nil
+func (stubSubmitter) Submit(ctx context.Context, m *model.Measurement) (string, error) {
+	return "", nil
 }
 
 var _ Submitter = stubSubmitter{}
@@ -57,7 +57,7 @@ type realSubmitter struct {
 	logger model.Logger
 }
 
-func (rs realSubmitter) Submit(ctx context.Context, m *model.Measurement) error {
+func (rs realSubmitter) Submit(ctx context.Context, m *model.Measurement) (string, error) {
 	rs.logger.Info("submitting measurement to OONI collector; please be patient...")
 	return rs.subm.Submit(ctx, m)
 }
diff --git a/internal/oonirun/submitter_test.go b/internal/oonirun/submitter_test.go
@@ -22,7 +22,8 @@ func TestSubmitterNotEnabled(t *testing.T) {
 		t.Fatal("we did not get a stubSubmitter instance")
 	}
 	m := new(model.Measurement)
-	if err := submitter.Submit(ctx, m); err != nil {
+	_, err = submitter.Submit(ctx, m)
+	if err != nil {
 		t.Fatal(err)
 	}
 }
@@ -32,11 +33,11 @@ type FakeSubmitter struct {
 	Error error
 }
 
-func (fs *FakeSubmitter) Submit(ctx context.Context, m *model.Measurement) error {
+func (fs *FakeSubmitter) Submit(ctx context.Context, m *model.Measurement) (string, error) {
 	if fs.Calls != nil {
 		fs.Calls.Add(1)
 	}
-	return fs.Error
+	return "", fs.Error
 }
 
 var _ Submitter = &FakeSubmitter{}
@@ -83,7 +84,7 @@ func TestNewSubmitterWithFailedSubmission(t *testing.T) {
 		t.Fatal(err)
 	}
 	m := new(model.Measurement)
-	err = submitter.Submit(context.Background(), m)
+	_, err = submitter.Submit(context.Background(), m)
 	if !errors.Is(err, expected) {
 		t.Fatalf("not the error we expected: %+v", err)
 	}
diff --git a/internal/oonirun/v2_test.go b/internal/oonirun/v2_test.go
@@ -486,7 +486,7 @@ func TestV2MeasureDescriptor(t *testing.T) {
 		// represents a fundamental failure in setting up the experiment
 		sess.MockNewSubmitter = func(ctx context.Context) (model.Submitter, error) {
 			subm := &mocks.Submitter{
-				MockSubmit: func(ctx context.Context, m *model.Measurement) error {
+				MockSubmit: func(ctx context.Context, m *model.Measurement) (string, error) {
 					panic("should not be called")
 				},
 			}
diff --git a/internal/probeservices/collector.go b/internal/probeservices/collector.go
@@ -101,15 +101,15 @@ func (r reportChan) CanSubmit(m *model.Measurement) bool {
 // such that it contains the report ID for which it has been
 // submitted. Otherwise, we'll set the report ID to the empty
 // string, so that you know which measurements weren't submitted.
-func (r reportChan) SubmitMeasurement(ctx context.Context, m *model.Measurement) error {
+func (r reportChan) SubmitMeasurement(ctx context.Context, m *model.Measurement) (string, error) {
 	// TODO(bassosimone): do we need to prevent measurement submission
 	// if the measurement isn't consistent with the orig template?
 
 	m.ReportID = r.ID
 
 	URL, err := urlx.ResolveReference(r.client.BaseURL, fmt.Sprintf("/report/%s", r.ID), "")
 	if err != nil {
-		return err
+		return "", err
 	}
 
 	apiReq := model.OOAPICollectorUpdateRequest{
@@ -131,13 +131,13 @@ func (r reportChan) SubmitMeasurement(ctx context.Context, m *model.Measurement)
 
 	if err != nil {
 		m.ReportID = ""
-		return err
+		return "", err
 	}
 
 	// TODO(bassosimone): we should use the session logger here but for now this stopgap
 	// solution will allow observing the measurement URL for CLI users.
 	log.Printf("Measurement URL: https://explorer.ooni.org/m/%s", updateResponse.MeasurementUID)
-	return nil
+	return updateResponse.MeasurementUID, nil
 }
 
 // ReportID returns the report ID.
@@ -150,7 +150,7 @@ func (r reportChan) ReportID() string {
 type ReportChannel interface {
 	CanSubmit(m *model.Measurement) bool
 	ReportID() string
-	SubmitMeasurement(ctx context.Context, m *model.Measurement) error
+	SubmitMeasurement(ctx context.Context, m *model.Measurement) (string, error)
 }
 
 var _ ReportChannel = &reportChan{}
@@ -182,14 +182,14 @@ func NewSubmitter(opener ReportOpener, logger model.Logger) *Submitter {
 
 // Submit submits the current measurement to the OONI backend created using
 // the ReportOpener passed to the constructor.
-func (sub *Submitter) Submit(ctx context.Context, m *model.Measurement) error {
+func (sub *Submitter) Submit(ctx context.Context, m *model.Measurement) (string, error) {
 	var err error
 	sub.mu.Lock()
 	defer sub.mu.Unlock()
 	if sub.channel == nil || !sub.channel.CanSubmit(m) {
 		sub.channel, err = sub.opener.OpenReport(ctx, NewReportTemplate(m))
 		if err != nil {
-			return err
+			return "", err
 		}
 		sub.logger.Infof("New reportID: %s", sub.channel.ReportID())
 	}
diff --git a/internal/probeservices/collector_test.go b/internal/probeservices/collector_test.go
diff --git a/pkg/oonimkall/session.go b/pkg/oonimkall/session.go
diff --git a/pkg/oonimkall/taskmodel.go b/pkg/oonimkall/taskmodel.go
diff --git a/pkg/oonimkall/taskrunner.go b/pkg/oonimkall/taskrunner.go
diff --git a/pkg/oonimkall/taskrunner_test.go b/pkg/oonimkall/taskrunner_test.go

Original file line number	Diff line number	Diff line change
`@@ -106,7 +106,7 @@ func submitAll(ctx context.Context, lines []string, subm model.Submitter) (int,`
`106`	`106`	`for _, line := range lines {`
`107`	`107`	`mm := toMeasurement(line)`
`108`	`108`	`// submit the measurement`
`109`		`- err := subm.Submit(ctx, mm)`
	`109`	`+ _, err := subm.Submit(ctx, mm)`
`110`	`110`	`if err != nil {`
`111`	`111`	`return submitted, err`
`112`	`112`	`}`
Original file line number	Diff line number	Diff line change
`@@ -104,10 +104,10 @@ func (e *experiment) ReportID() string {`
`104`	`104`
`105`	`105`	`// SubmitAndUpdateMeasurementContext implements [model.Experiment].`
`106`	`106`	`func (e *experiment) SubmitAndUpdateMeasurementContext(`
`107`		`- ctx context.Context, measurement *model.Measurement) error {`
	`107`	`+ ctx context.Context, measurement *model.Measurement) (string, error) {`
`108`	`108`	`report := e.mrep.Get()`
`109`	`109`	`if report == nil {`
`110`		`- return errors.New("report is not open")`
	`110`	`+ return "", errors.New("report is not open")`
`111`	`111`	`}`
`112`	`112`	`return report.SubmitMeasurement(ctx, measurement)`
`113`	`113`	`}`
Original file line number	Diff line number	Diff line change
`@@ -280,7 +280,7 @@ func runexperimentflow(t *testing.T, experiment model.Experiment, input string)`
`280`	`280`	`}`
`281`	`281`	`filename := tempfile.Name()`
`282`	`282`	`tempfile.Close()`
`283`		`- err = experiment.SubmitAndUpdateMeasurementContext(ctx, measurement)`
	`283`	`+ _, err = experiment.SubmitAndUpdateMeasurementContext(ctx, measurement)`
`284`	`284`	`if err != nil {`
`285`	`285`	`t.Fatal(err)`
`286`	`286`	`}`
`@@ -323,7 +323,7 @@ func TestOpenReportIdempotent(t *testing.T) {`
`323`	`323`	`t.Fatal("unexpected initial report ID")`
`324`	`324`	`}`
`325`	`325`	`ctx := context.Background()`
`326`		`- if err := exp.SubmitAndUpdateMeasurementContext(ctx, &model.Measurement{}); err == nil {`
	`326`	`+ if _, err := exp.SubmitAndUpdateMeasurementContext(ctx, &model.Measurement{}); err == nil {`
`327`	`327`	`t.Fatal("we should not be able to submit before OpenReport")`
`328`	`328`	`}`
`329`	`329`	`err = exp.OpenReportContext(ctx)`
`@@ -403,7 +403,7 @@ func TestSubmitAndUpdateMeasurementWithClosedReport(t *testing.T) {`
`403`	`403`	`}`
`404`	`404`	`exp := builder.NewExperiment()`
`405`	`405`	`m := new(model.Measurement)`
`406`		`- err = exp.SubmitAndUpdateMeasurementContext(context.Background(), m)`
	`406`	`+ _, err = exp.SubmitAndUpdateMeasurementContext(context.Background(), m)`
`407`	`407`	`if err == nil {`
`408`	`408`	`t.Fatal("expected an error here")`
`409`	`409`	`}`
Original file line number	Diff line number	Diff line change
`@@ -22,7 +22,7 @@ type Experiment struct {`
`22`	`22`	`MockSaveMeasurement func(measurement *model.Measurement, filePath string) error`
`23`	`23`
`24`	`24`	`MockSubmitAndUpdateMeasurementContext func(`
`25`		`- ctx context.Context, measurement *model.Measurement) error`
	`25`	`+ ctx context.Context, measurement *model.Measurement) (string, error)`
`26`	`26`
`27`	`27`	`MockOpenReportContext func(ctx context.Context) error`
`28`	`28`	`}`
`@@ -53,7 +53,7 @@ func (e Experiment) SaveMeasurement(measurement model.Measurement, filePath st`
`53`	`53`	`}`
`54`	`54`
`55`	`55`	`func (e *Experiment) SubmitAndUpdateMeasurementContext(`
`56`		`- ctx context.Context, measurement *model.Measurement) error {`
	`56`	`+ ctx context.Context, measurement *model.Measurement) (string, error) {`
`57`	`57`	`return e.MockSubmitAndUpdateMeasurementContext(ctx, measurement)`
`58`	`58`	`}`
`59`	`59`
Original file line number	Diff line number	Diff line change
`@@ -8,10 +8,10 @@ import (`
`8`	`8`
`9`	`9`	`// Submitter mocks model.Submitter.`
`10`	`10`	`type Submitter struct {`
`11`		`- MockSubmit func(ctx context.Context, m *model.Measurement) error`
	`11`	`+ MockSubmit func(ctx context.Context, m *model.Measurement) (string, error)`
`12`	`12`	`}`
`13`	`13`
`14`	`14`	`// Submit calls MockSubmit`
`15`		`-func (s Submitter) Submit(ctx context.Context, m model.Measurement) error {`
	`15`	`+func (s Submitter) Submit(ctx context.Context, m model.Measurement) (string, error) {`
`16`	`16`	`return s.MockSubmit(ctx, m)`
`17`	`17`	`}`
Original file line number	Diff line number	Diff line change
`@@ -263,10 +263,11 @@ type experimentSubmitterWrapper struct {`
`263`	`263`	`logger model.Logger`
`264`	`264`	`}`
`265`	`265`
`266`		`-func (sw experimentSubmitterWrapper) Submit(ctx context.Context, idx int, m model.Measurement) error {`
`267`		`- if err := sw.child.Submit(ctx, idx, m); err != nil {`
	`266`	`+func (sw experimentSubmitterWrapper) Submit(ctx context.Context, idx int, m model.Measurement) (string, error) {`
	`267`	`+ mstUID, err := sw.child.Submit(ctx, idx, m)`
	`268`	`+ if err != nil {`
`268`	`269`	`sw.logger.Warnf("submitting measurement failed: %s", err.Error())`
`269`	`270`	`}`
`270`	`271`	`// policy: we do not stop the loop if measurement submission fails`
`271`		`- return nil`
	`272`	`+ return mstUID, nil`
`272`	`273`	`}`