Make the testKeys extractions methods more robust

nettests/im/facebook_messenger.go

@@ -24,7 +24,7 @@ type FacebookMessengerTestKeys struct {
 }
 
 // GetTestKeys generates a summary for a test run
-func (h FacebookMessenger) GetTestKeys(tk map[string]interface{}) interface{} {
+func (h FacebookMessenger) GetTestKeys(tk map[string]interface{}) (interface{}, error) {
 	var (
 		dnsBlocking bool
 		tcpBlocking bool
@@ -45,7 +45,7 @@ func (h FacebookMessenger) GetTestKeys(tk map[string]interface{}) interface{} {
 		DNSBlocking: dnsBlocking,
 		TCPBlocking: tcpBlocking,
 		IsAnomaly:   dnsBlocking || tcpBlocking,
-	}
+	}, nil
 }
 
 // LogSummary writes the summary to the standard output

nettests/im/telegram.go

@@ -25,7 +25,7 @@ type TelegramTestKeys struct {
 }
 
 // GetTestKeys generates a summary for a test run
-func (h Telegram) GetTestKeys(tk map[string]interface{}) interface{} {
+func (h Telegram) GetTestKeys(tk map[string]interface{}) (interface{}, error) {
 	var (
 		tcpBlocking  bool
 		httpBlocking bool
@@ -53,7 +53,7 @@ func (h Telegram) GetTestKeys(tk map[string]interface{}) interface{} {
 		HTTPBlocking: httpBlocking,
 		WebBlocking:  webBlocking,
 		IsAnomaly:    webBlocking || httpBlocking || tcpBlocking,
-	}
+	}, nil
 }
 
 // LogSummary writes the summary to the standard output

nettests/im/whatsapp.go

@@ -25,7 +25,7 @@ type WhatsAppTestKeys struct {
 }
 
 // GetTestKeys generates a summary for a test run
-func (h WhatsApp) GetTestKeys(tk map[string]interface{}) interface{} {
+func (h WhatsApp) GetTestKeys(tk map[string]interface{}) (interface{}, error) {
 	var (
 		webBlocking          bool
 		registrationBlocking bool
@@ -51,7 +51,7 @@ func (h WhatsApp) GetTestKeys(tk map[string]interface{}) interface{} {
 		WebBlocking:                webBlocking,
 		EndpointsBlocking:          endpointsBlocking,
 		IsAnomaly:                  registrationBlocking || webBlocking || endpointsBlocking,
-	}
+	}, nil
 }
 
 // LogSummary writes the summary to the standard output

nettests/middlebox/http_header_field_manipulation.go

@@ -1,6 +1,8 @@
 package middlebox
 
 import (
+	"errors"
+
 	"github.com/measurement-kit/go-measurement-kit"
 	"github.com/ooni/probe-cli/nettests"
 )
@@ -22,19 +24,21 @@ type HTTPHeaderFieldManipulationTestKeys struct {
 }
 
 // GetTestKeys returns a projection of the tests keys needed for the views
-func (h HTTPHeaderFieldManipulation) GetTestKeys(tk map[string]interface{}) interface{} {
+func (h HTTPHeaderFieldManipulation) GetTestKeys(tk map[string]interface{}) (interface{}, error) {
-	tampering := false
+	testKeys := HTTPHeaderFieldManipulationTestKeys{IsAnomaly: false}
-	for _, v := range tk["tampering"].(map[string]interface{}) {
+	tampering, ok := tk["tampering"].(map[string]interface{})
+	if !ok {
+		return testKeys, errors.New("tampering testkey is invalid")
+	}
+	for _, v := range tampering {
 		t, ok := v.(bool)
 		// Ignore non booleans in the tampering map
 		if ok && t == true {
-			tampering = true
+			testKeys.IsAnomaly = true
 		}
 	}
 
-	return HTTPHeaderFieldManipulationTestKeys{
+	return testKeys, nil
-		IsAnomaly: tampering,
-	}
 }
 
 // LogSummary writes the summary to the standard output

nettests/middlebox/http_invalid_request_line.go

@@ -1,6 +1,8 @@
 package middlebox
 
 import (
+	"errors"
+
 	"github.com/measurement-kit/go-measurement-kit"
 	"github.com/ooni/probe-cli/nettests"
 )
@@ -22,12 +24,16 @@ type HTTPInvalidRequestLineTestKeys struct {
 }
 
 // GetTestKeys generates a summary for a test run
-func (h HTTPInvalidRequestLine) GetTestKeys(tk map[string]interface{}) interface{} {
+func (h HTTPInvalidRequestLine) GetTestKeys(tk map[string]interface{}) (interface{}, error) {
-	tampering := tk["tampering"].(bool)
+	testKeys := HTTPInvalidRequestLineTestKeys{IsAnomaly: false}
 
-	return HTTPInvalidRequestLineTestKeys{
+	tampering, ok := tk["tampering"].(bool)
-		IsAnomaly: tampering,
+	if !ok {
+		return testKeys, errors.New("tampering is not bool")
 	}
+	testKeys.IsAnomaly = tampering
+
+	return testKeys, nil
 }
 
 // LogSummary writes the summary to the standard output

nettests/nettests.go

@@ -22,7 +22,7 @@ import (
 // Nettest interface. Every Nettest should implement this.
 type Nettest interface {
 	Run(*Controller) error
-	GetTestKeys(map[string]interface{}) interface{}
+	GetTestKeys(map[string]interface{}) (interface{}, error)
 	LogSummary(string) error
 }
 
@@ -299,10 +299,15 @@ func (c *Controller) OnEntry(idx int64, jsonStr string) {
 		log.WithError(err).Error("failed to parse onEntry")
 		return
 	}
-	tk := c.nt.GetTestKeys(entry.TestKeys)
+	// XXX is it correct to just log the error instead of marking the whole
+	// measurement as failed?
+	tk, err := c.nt.GetTestKeys(entry.TestKeys)
+	if err != nil {
+		log.WithError(err).Error("failed to obtain testKeys")
+	}
 
 	log.Debugf("Fetching: %s %v", idx, c.msmts[idx])
-	err := database.AddTestKeys(c.Ctx.DB, c.msmts[idx], tk)
+	err = database.AddTestKeys(c.Ctx.DB, c.msmts[idx], tk)
 	if err != nil {
 		log.WithError(err).Error("failed to add test keys to summary")
 	}

nettests/performance/dash.go

@@ -1,6 +1,8 @@
 package performance
 
 import (
+	"github.com/pkg/errors"
+
 	"github.com/measurement-kit/go-measurement-kit"
 	"github.com/ooni/probe-cli/nettests"
 )
@@ -20,21 +22,40 @@ func (d Dash) Run(ctl *nettests.Controller) error {
 // TODO: process 'receiver_data' to provide an array of performance for a chart.
 type DashTestKeys struct {
 	Latency   float64 `json:"connect_latency"`
-	Bitrate   int64   `json:"median_bitrate"`
+	Bitrate   float64 `json:"median_bitrate"`
 	Delay     float64 `json:"min_playout_delay"`
 	IsAnomaly bool    `json:"-"`
 }
 
 // GetTestKeys generates a summary for a test run
-func (d Dash) GetTestKeys(tk map[string]interface{}) interface{} {
+func (d Dash) GetTestKeys(tk map[string]interface{}) (interface{}, error) {
-	simple := tk["simple"].(map[string]interface{})
+	var err error
 
-	return DashTestKeys{
+	testKeys := DashTestKeys{IsAnomaly: false}
-		IsAnomaly: false,
+
-		Latency:   simple["connect_latency"].(float64),
+	simple, ok := tk["simple"].(map[string]interface{})
-		Bitrate:   int64(simple["median_bitrate"].(float64)),
+	if !ok {
-		Delay:     simple["min_playout_delay"].(float64),
+		return testKeys, errors.New("simple key is not of the expected type")
 	}
+
+	latency, ok := simple["connect_latency"].(float64)
+	if !ok {
+		err = errors.Wrap(err, "connect_latency is invalid")
+	}
+	testKeys.Latency = latency
+
+	bitrate, ok := simple["median_bitrate"].(float64)
+	if !ok {
+		err = errors.Wrap(err, "median_bitrate is invalid")
+	}
+	testKeys.Bitrate = bitrate
+
+	delay, ok := simple["min_playout_delay"].(float64)
+	if !ok {
+		err = errors.Wrap(err, "min_playout_delay is invalid")
+	}
+	testKeys.Delay = delay
+	return testKeys, err
 }
 
 // LogSummary writes the summary to the standard output

nettests/performance/ndt.go

@@ -3,6 +3,7 @@ package performance
 import (
 	"github.com/measurement-kit/go-measurement-kit"
 	"github.com/ooni/probe-cli/nettests"
+	"github.com/pkg/errors"
 )
 
 // NDT test implementation
@@ -18,36 +19,75 @@ func (n NDT) Run(ctl *nettests.Controller) error {
 
 // NDTTestKeys for the test
 type NDTTestKeys struct {
-	Upload     int64   `json:"upload"`
+	Upload     float64 `json:"upload"`
-	Download   int64   `json:"download"`
+	Download   float64 `json:"download"`
-	Ping       int64   `json:"ping"`
+	Ping       float64 `json:"ping"`
 	MaxRTT     float64 `json:"max_rtt"`
 	AvgRTT     float64 `json:"avg_rtt"`
 	MinRTT     float64 `json:"min_rtt"`
-	MSS        int64   `json:"mss"`
+	MSS        float64 `json:"mss"`
-	OutOfOrder int64   `json:"out_of_order"`
+	OutOfOrder float64 `json:"out_of_order"`
 	PacketLoss float64 `json:"packet_loss"`
-	Timeouts   int64   `json:"timeouts"`
+	Timeouts   float64 `json:"timeouts"`
 	IsAnomaly  bool    `json:"-"`
 }
 
 // GetTestKeys generates a summary for a test run
-func (n NDT) GetTestKeys(tk map[string]interface{}) interface{} {
+func (n NDT) GetTestKeys(tk map[string]interface{}) (interface{}, error) {
-	simple := tk["simple"].(map[string]interface{})
+	var err error
-	advanced := tk["advanced"].(map[string]interface{})
+	testKeys := NDTTestKeys{IsAnomaly: false}
 
-	return NDTTestKeys{
+	simple, ok := tk["simple"].(map[string]interface{})
-		Upload:     int64(simple["upload"].(float64)),
+	if !ok {
-		Download:   int64(simple["download"].(float64)),
+		return testKeys, errors.New("simple key is invalid")
-		Ping:       int64(simple["ping"].(float64)),
-		MaxRTT:     advanced["max_rtt"].(float64),
-		AvgRTT:     advanced["avg_rtt"].(float64),
-		MinRTT:     advanced["min_rtt"].(float64),
-		MSS:        int64(advanced["mss"].(float64)),
-		OutOfOrder: int64(advanced["out_of_order"].(float64)),
-		PacketLoss: advanced["packet_loss"].(float64),
-		Timeouts:   int64(advanced["timeouts"].(float64)),
 	}
+	advanced, ok := tk["advanced"].(map[string]interface{})
+	if !ok {
+		return testKeys, errors.New("advanced key is invalid")
+	}
+
+	// XXX there is likely a better pattern for this
+	testKeys.Upload, ok = simple["upload"].(float64)
+	if !ok {
+		err = errors.Wrap(err, "upload key invalid")
+	}
+	testKeys.Download, ok = simple["download"].(float64)
+	if !ok {
+		err = errors.Wrap(err, "download key invalid")
+	}
+	testKeys.Ping, ok = simple["ping"].(float64)
+	if !ok {
+		err = errors.Wrap(err, "ping key invalid")
+	}
+	testKeys.MaxRTT, ok = advanced["max_rtt"].(float64)
+	if !ok {
+		err = errors.Wrap(err, "max_rtt key invalid")
+	}
+	testKeys.AvgRTT, ok = advanced["avg_rtt"].(float64)
+	if !ok {
+		err = errors.Wrap(err, "avg_rtt key invalid")
+	}
+	testKeys.MinRTT, ok = advanced["min_rtt"].(float64)
+	if !ok {
+		err = errors.Wrap(err, "min_rtt key invalid")
+	}
+	testKeys.MSS, ok = advanced["mss"].(float64)
+	if !ok {
+		err = errors.Wrap(err, "mss key invalid")
+	}
+	testKeys.OutOfOrder, ok = advanced["out_of_order"].(float64)
+	if !ok {
+		err = errors.Wrap(err, "out_of_order key invalid")
+	}
+	testKeys.PacketLoss, ok = advanced["packet_loss"].(float64)
+	if !ok {
+		err = errors.Wrap(err, "packet_loss key invalid")
+	}
+	testKeys.Timeouts, ok = advanced["timeouts"].(float64)
+	if !ok {
+		err = errors.Wrap(err, "timeouts key invalid")
+	}
+	return testKeys, err
 }
 
 // LogSummary writes the summary to the standard output

nettests/websites/web_connectivity.go

@@ -93,7 +93,7 @@ type WebConnectivityTestKeys struct {
 }
 
 // GetTestKeys generates a summary for a test run
-func (n WebConnectivity) GetTestKeys(tk map[string]interface{}) interface{} {
+func (n WebConnectivity) GetTestKeys(tk map[string]interface{}) (interface{}, error) {
 	var (
 		blocked    bool
 		blocking   string
@@ -124,7 +124,7 @@ func (n WebConnectivity) GetTestKeys(tk map[string]interface{}) interface{} {
 		Accessible: accessible,
 		Blocking:   blocking,
 		IsAnomaly:  blocked,
-	}
+	}, nil
 }
 
 // LogSummary writes the summary to the standard output