cleanup: move ArchivalMaybeBinaryData to legacymodel

internal/experiment/hirl/hirl.go

@@ -11,6 +11,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/ooni/probe-cli/v3/internal/legacy/legacymodel"
 	"github.com/ooni/probe-cli/v3/internal/legacy/netx"
 	"github.com/ooni/probe-cli/v3/internal/legacy/tracex"
 	"github.com/ooni/probe-cli/v3/internal/model"
@@ -29,11 +30,11 @@ type Config struct{}
 
 // TestKeys contains the experiment test keys.
 type TestKeys struct {
-	FailureList   []*string                 `json:"failure_list"`
-	Received      []tracex.MaybeBinaryValue `json:"received"`
-	Sent          []string                  `json:"sent"`
-	TamperingList []bool                    `json:"tampering_list"`
-	Tampering     bool                      `json:"tampering"`
+	FailureList   []*string                             `json:"failure_list"`
+	Received      []legacymodel.ArchivalMaybeBinaryData `json:"received"`
+	Sent          []string                              `json:"sent"`
+	TamperingList []bool                                `json:"tampering_list"`
+	Tampering     bool                                  `json:"tampering"`
 }
 
 // NewExperimentMeasurer creates a new ExperimentMeasurer.
@@ -151,7 +152,7 @@ type MethodConfig struct {
 type MethodResult struct {
 	Err       error
 	Name      string
-	Received  tracex.MaybeBinaryValue
+	Received  legacymodel.ArchivalMaybeBinaryData
 	Sent      string
 	Tampering bool
 }

internal/experiment/hirl/hirl_test.go

@@ -8,9 +8,9 @@ import (
 
 	"github.com/apex/log"
 	"github.com/ooni/probe-cli/v3/internal/experiment/hirl"
+	"github.com/ooni/probe-cli/v3/internal/legacy/legacymodel"
 	"github.com/ooni/probe-cli/v3/internal/legacy/mockable"
 	"github.com/ooni/probe-cli/v3/internal/legacy/netx"
-	"github.com/ooni/probe-cli/v3/internal/legacy/tracex"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
 )
@@ -159,7 +159,7 @@ func (FakeMethodSuccessful) Name() string {
 func (meth FakeMethodSuccessful) Run(ctx context.Context, config hirl.MethodConfig) {
 	config.Out <- hirl.MethodResult{
 		Name:      meth.Name(),
-		Received:  tracex.MaybeBinaryValue{Value: "antani"},
+		Received:  legacymodel.ArchivalMaybeBinaryData{Value: "antani"},
 		Sent:      "antani",
 		Tampering: false,
 	}
@@ -174,7 +174,7 @@ func (FakeMethodFailure) Name() string {
 func (meth FakeMethodFailure) Run(ctx context.Context, config hirl.MethodConfig) {
 	config.Out <- hirl.MethodResult{
 		Name:      meth.Name(),
-		Received:  tracex.MaybeBinaryValue{Value: "antani"},
+		Received:  legacymodel.ArchivalMaybeBinaryData{Value: "antani"},
 		Sent:      "melandri",
 		Tampering: true,
 	}

internal/experiment/quicping/quicping.go

@@ -17,6 +17,7 @@ import (
 
 	_ "crypto/sha256"
 
+	"github.com/ooni/probe-cli/v3/internal/legacy/legacymodel"
 	"github.com/ooni/probe-cli/v3/internal/legacy/tracex"
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
@@ -78,26 +79,26 @@ type TestKeys struct {
 
 // SinglePing is a result of a single ping operation.
 type SinglePing struct {
-	ConnIdDst string                         `json:"conn_id_dst"`
-	ConnIdSrc string                         `json:"conn_id_src"`
-	Failure   *string                        `json:"failure"`
-	Request   *model.ArchivalMaybeBinaryData `json:"request"`
-	T         float64                        `json:"t"`
-	Responses []*SinglePingResponse          `json:"responses"`
+	ConnIdDst string                               `json:"conn_id_dst"`
+	ConnIdSrc string                               `json:"conn_id_src"`
+	Failure   *string                              `json:"failure"`
+	Request   *legacymodel.ArchivalMaybeBinaryData `json:"request"`
+	T         float64                              `json:"t"`
+	Responses []*SinglePingResponse                `json:"responses"`
 }
 
 type SinglePingResponse struct {
-	Data              *model.ArchivalMaybeBinaryData `json:"response_data"`
-	Failure           *string                        `json:"failure"`
-	T                 float64                        `json:"t"`
-	SupportedVersions []uint32                       `json:"supported_versions"`
+	Data              *legacymodel.ArchivalMaybeBinaryData `json:"response_data"`
+	Failure           *string                              `json:"failure"`
+	T                 float64                              `json:"t"`
+	SupportedVersions []uint32                             `json:"supported_versions"`
 }
 
 // makeResponse is a utility function to create a SinglePingResponse
 func makeResponse(resp *responseInfo) *SinglePingResponse {
-	var data *model.ArchivalMaybeBinaryData
+	var data *legacymodel.ArchivalMaybeBinaryData
 	if resp.raw != nil {
-		data = &model.ArchivalMaybeBinaryData{Value: string(resp.raw)}
+		data = &legacymodel.ArchivalMaybeBinaryData{Value: string(resp.raw)}
 	}
 	return &SinglePingResponse{
 		Data:              data,
@@ -272,7 +273,7 @@ L:
 					ConnIdDst: req.dstID,
 					ConnIdSrc: req.srcID,
 					Failure:   tracex.NewFailure(req.err),
-					Request:   &model.ArchivalMaybeBinaryData{Value: string(req.raw)},
+					Request:   &legacymodel.ArchivalMaybeBinaryData{Value: string(req.raw)},
 					T:         req.t,
 				})
 				continue
@@ -312,7 +313,7 @@ L:
 				ConnIdDst: ping.request.dstID,
 				ConnIdSrc: ping.request.srcID,
 				Failure:   tracex.NewFailure(timeoutErr),
-				Request:   &model.ArchivalMaybeBinaryData{Value: string(ping.request.raw)},
+				Request:   &legacymodel.ArchivalMaybeBinaryData{Value: string(ping.request.raw)},
 				T:         ping.request.t,
 			})
 			continue
@@ -325,7 +326,7 @@ L:
 			ConnIdDst: ping.request.dstID,
 			ConnIdSrc: ping.request.srcID,
 			Failure:   nil,
-			Request:   &model.ArchivalMaybeBinaryData{Value: string(ping.request.raw)},
+			Request:   &legacymodel.ArchivalMaybeBinaryData{Value: string(ping.request.raw)},
 			T:         ping.request.t,
 			Responses: responses,
 		})

internal/legacy/legacymodel/archival.go

@@ -0,0 +1,60 @@
+package legacymodel
+
+import (
+	"encoding/base64"
+	"encoding/json"
+	"errors"
+	"unicode/utf8"
+)
+
+// ArchivalMaybeBinaryData is a possibly binary string. We use this helper class
+// to define a custom JSON encoder that allows us to choose the proper
+// representation depending on whether the Value field is valid UTF-8 or not.
+//
+// See https://github.com/ooni/spec/blob/master/data-formats/df-001-httpt.md#maybebinarydata
+//
+// Deprecated: do not use this type in new code.
+//
+// Removing this struct is TODO(https://github.com/ooni/probe/issues/2543).
+type ArchivalMaybeBinaryData struct {
+	Value string
+}
+
+// MarshalJSON marshals a string-like to JSON following the OONI spec that
+// says that UTF-8 content is represented as string and non-UTF-8 content is
+// instead represented using `{"format":"base64","data":"..."}`.
+func (hb ArchivalMaybeBinaryData) MarshalJSON() ([]byte, error) {
+	// if we can serialize as UTF-8 string, do that
+	if utf8.ValidString(hb.Value) {
+		return json.Marshal(hb.Value)
+	}
+
+	// otherwise fallback to the ooni/spec representation for binary data
+	er := make(map[string]string)
+	er["format"] = "base64"
+	er["data"] = base64.StdEncoding.EncodeToString([]byte(hb.Value))
+	return json.Marshal(er)
+}
+
+// UnmarshalJSON is the opposite of MarshalJSON.
+func (hb *ArchivalMaybeBinaryData) UnmarshalJSON(d []byte) error {
+	if err := json.Unmarshal(d, &hb.Value); err == nil {
+		return nil
+	}
+	er := make(map[string]string)
+	if err := json.Unmarshal(d, &er); err != nil {
+		return err
+	}
+	if v, ok := er["format"]; !ok || v != "base64" {
+		return errors.New("missing or invalid format field")
+	}
+	if _, ok := er["data"]; !ok {
+		return errors.New("missing data field")
+	}
+	b64, err := base64.StdEncoding.DecodeString(er["data"])
+	if err != nil {
+		return err
+	}
+	hb.Value = string(b64)
+	return nil
+}

internal/legacy/legacymodel/archival_test.go

@@ -0,0 +1,111 @@
+package legacymodel_test
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/ooni/probe-cli/v3/internal/legacy/legacymodel"
+)
+
+// we use this value below to test we can handle binary data
+var archivalBinaryInput = []uint8{
+	0x57, 0xe5, 0x79, 0xfb, 0xa6, 0xbb, 0x0d, 0xbc, 0xce, 0xbd, 0xa7, 0xa0,
+	0xba, 0xa4, 0x78, 0x78, 0x12, 0x59, 0xee, 0x68, 0x39, 0xa4, 0x07, 0x98,
+	0xc5, 0x3e, 0xbc, 0x55, 0xcb, 0xfe, 0x34, 0x3c, 0x7e, 0x1b, 0x5a, 0xb3,
+	0x22, 0x9d, 0xc1, 0x2d, 0x6e, 0xca, 0x5b, 0xf1, 0x10, 0x25, 0x47, 0x1e,
+	0x44, 0xe2, 0x2d, 0x60, 0x08, 0xea, 0xb0, 0x0a, 0xcc, 0x05, 0x48, 0xa0,
+	0xf5, 0x78, 0x38, 0xf0, 0xdb, 0x3f, 0x9d, 0x9f, 0x25, 0x6f, 0x89, 0x00,
+	0x96, 0x93, 0xaf, 0x43, 0xac, 0x4d, 0xc9, 0xac, 0x13, 0xdb, 0x22, 0xbe,
+	0x7a, 0x7d, 0xd9, 0x24, 0xa2, 0x52, 0x69, 0xd8, 0x89, 0xc1, 0xd1, 0x57,
+	0xaa, 0x04, 0x2b, 0xa2, 0xd8, 0xb1, 0x19, 0xf6, 0xd5, 0x11, 0x39, 0xbb,
+	0x80, 0xcf, 0x86, 0xf9, 0x5f, 0x9d, 0x8c, 0xab, 0xf5, 0xc5, 0x74, 0x24,
+	0x3a, 0xa2, 0xd4, 0x40, 0x4e, 0xd7, 0x10, 0x1f,
+}
+
+// we use this value below to test we can handle binary data
+var archivalEncodedBinaryInput = []byte(`{"data":"V+V5+6a7DbzOvaeguqR4eBJZ7mg5pAeYxT68Vcv+NDx+G1qzIp3BLW7KW/EQJUceROItYAjqsArMBUig9Xg48Ns/nZ8lb4kAlpOvQ6xNyawT2yK+en3ZJKJSadiJwdFXqgQrotixGfbVETm7gM+G+V+djKv1xXQkOqLUQE7XEB8=","format":"base64"}`)
+
+func TestArchivalMaybeBinaryData(t *testing.T) {
+	t.Run("MarshalJSON", func(t *testing.T) {
+		tests := []struct {
+			name    string // test name
+			input   string // value to marshal
+			want    []byte // expected result
+			wantErr bool   // whether we expect an error
+		}{{
+			name:    "with string input",
+			input:   "antani",
+			want:    []byte(`"antani"`),
+			wantErr: false,
+		}, {
+			name:    "with binary input",
+			input:   string(archivalBinaryInput),
+			want:    archivalEncodedBinaryInput,
+			wantErr: false,
+		}}
+		for _, tt := range tests {
+			t.Run(tt.name, func(t *testing.T) {
+				hb := legacymodel.ArchivalMaybeBinaryData{
+					Value: tt.input,
+				}
+				got, err := hb.MarshalJSON()
+				if (err != nil) != tt.wantErr {
+					t.Fatalf("ArchivalMaybeBinaryData.MarshalJSON() error = %v, wantErr %v", err, tt.wantErr)
+				}
+				if diff := cmp.Diff(tt.want, got); diff != "" {
+					t.Fatal(diff)
+				}
+			})
+		}
+	})
+
+	t.Run("UnmarshalJSON", func(t *testing.T) {
+		tests := []struct {
+			name    string // test name
+			input   []byte // value to unmarshal
+			want    string // expected result
+			wantErr bool   // whether we want an error
+		}{{
+			name:    "with string input",
+			input:   []byte(`"xo"`),
+			want:    "xo",
+			wantErr: false,
+		}, {
+			name:    "with nil input",
+			input:   nil,
+			want:    "",
+			wantErr: true,
+		}, {
+			name:    "with missing/invalid format",
+			input:   []byte(`{"format": "foo"}`),
+			want:    "",
+			wantErr: true,
+		}, {
+			name:    "with missing data",
+			input:   []byte(`{"format": "base64"}`),
+			want:    "",
+			wantErr: true,
+		}, {
+			name:    "with invalid base64 data",
+			input:   []byte(`{"format": "base64", "data": "x"}`),
+			want:    "",
+			wantErr: true,
+		}, {
+			name:    "with valid base64 data",
+			input:   archivalEncodedBinaryInput,
+			want:    string(archivalBinaryInput),
+			wantErr: false,
+		}}
+		for _, tt := range tests {
+			t.Run(tt.name, func(t *testing.T) {
+				hb := &legacymodel.ArchivalMaybeBinaryData{}
+				if err := hb.UnmarshalJSON(tt.input); (err != nil) != tt.wantErr {
+					t.Fatalf("ArchivalMaybeBinaryData.UnmarshalJSON() error = %v, wantErr %v", err, tt.wantErr)
+				}
+				if d := cmp.Diff(tt.want, hb.Value); d != "" {
+					t.Fatal(d)
+				}
+			})
+		}
+	})
+}

internal/legacy/legacymodel/doc.go

@@ -0,0 +1,2 @@
+// Package legacymodel contains legacy content that used to be in internal/model
+package legacymodel

internal/legacy/tracex/archival.go

@@ -21,7 +21,6 @@ type (
 	ExtSpec          = model.ArchivalExtSpec
 	TCPConnectEntry  = model.ArchivalTCPConnectResult
 	TCPConnectStatus = model.ArchivalTCPConnectStatus
-	MaybeBinaryValue = model.ArchivalMaybeBinaryData
 	DNSQueryEntry    = model.ArchivalDNSLookupResult
 	DNSAnswerEntry   = model.ArchivalDNSAnswer
 	TLSHandshake     = model.ArchivalTLSOrQUICHandshakeResult

internal/model/archival.go

@@ -12,7 +12,6 @@ package model
 
 import (
 	"bytes"
-	"encoding/base64"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -172,58 +171,6 @@ func (value *ArchivalScrubbedMaybeBinaryString) UnmarshalJSON(rawData []byte) er
 	return nil
 }
 
-// ArchivalMaybeBinaryData is a possibly binary string. We use this helper class
-// to define a custom JSON encoder that allows us to choose the proper
-// representation depending on whether the Value field is valid UTF-8 or not.
-//
-// See https://github.com/ooni/spec/blob/master/data-formats/df-001-httpt.md#maybebinarydata
-//
-// Deprecated: do not use this type in new code.
-//
-// Removing this struct is TODO(https://github.com/ooni/probe/issues/2543).
-type ArchivalMaybeBinaryData struct {
-	Value string
-}
-
-// MarshalJSON marshals a string-like to JSON following the OONI spec that
-// says that UTF-8 content is represented as string and non-UTF-8 content is
-// instead represented using `{"format":"base64","data":"..."}`.
-func (hb ArchivalMaybeBinaryData) MarshalJSON() ([]byte, error) {
-	// if we can serialize as UTF-8 string, do that
-	if utf8.ValidString(hb.Value) {
-		return json.Marshal(hb.Value)
-	}
-
-	// otherwise fallback to the ooni/spec representation for binary data
-	er := make(map[string]string)
-	er["format"] = "base64"
-	er["data"] = base64.StdEncoding.EncodeToString([]byte(hb.Value))
-	return json.Marshal(er)
-}
-
-// UnmarshalJSON is the opposite of MarshalJSON.
-func (hb *ArchivalMaybeBinaryData) UnmarshalJSON(d []byte) error {
-	if err := json.Unmarshal(d, &hb.Value); err == nil {
-		return nil
-	}
-	er := make(map[string]string)
-	if err := json.Unmarshal(d, &er); err != nil {
-		return err
-	}
-	if v, ok := er["format"]; !ok || v != "base64" {
-		return errors.New("missing or invalid format field")
-	}
-	if _, ok := er["data"]; !ok {
-		return errors.New("missing data field")
-	}
-	b64, err := base64.StdEncoding.DecodeString(er["data"])
-	if err != nil {
-		return err
-	}
-	hb.Value = string(b64)
-	return nil
-}
-
 //
 // DNS lookup
 //